Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

testing: Add integration test for tx prioritization #513

Closed
Tracked by #514
, #1256
...
evan-forbes opened this issue Jul 1, 2022 · 4 comments · Fixed by #2394
Closed
Tracked by #514
, #1256
...

testing: Add integration test for tx prioritization #513

evan-forbes opened this issue Jul 1, 2022 · 4 comments · Fixed by #2394
Assignees
@evan-forbes
Labels
testing items that are strictly related to adding or extending test coverage

Comments

@evan-forbes
Copy link
Member

When we upgraded to cosmos-sdk v0.46, we gain the ability prioritized txs in the mempool. Since this feature is so critical, we need to test this in an integration test.
@evan-forbes evan-forbes added the testing items that are strictly related to adding or extending test coverage label Jul 1, 2022
@evan-forbes evan-forbes moved this to TODO in Celestia Node Jul 1, 2022
This was referenced Jul 1, 2022
Closed
Closed
Closed
@rootulp rootulp self-assigned this Aug 12, 2022
@rootulp rootulp moved this from TODO to In Progress in Celestia Node Aug 12, 2022
@rootulp rootulp removed their assignment Aug 15, 2022
@rootulp
Copy link
Collaborator

rootulp commented Aug 24, 2022

Note from synchronous discussion @evan-forbes @musalbas :

  • We may want a separate test (not end-to-end) on testground or our testnet that spams transactions and verifies transaction prioritization

@evan-forbes evan-forbes moved this from In Progress to TODO in Celestia Node Sep 29, 2022
@evan-forbes
Copy link
Member Author

comment

comment

as discussed in the linked comments, it looks like we should just just the v0 mempool and sort the tx by fee ourselves, or wait and use the default app side mempool coming in v0.47.0 of the sdk

@evan-forbes
Copy link
Member Author

we should still create a test to ensure that we don't accidentally break fee prioritization by default.

@rootulp rootulp mentioned this issue Oct 20, 2022
Closed
@evan-forbes
Copy link
Member Author

to update this, we are just planning on using the v1 mempool now. It will not be deprecated until at least v0.38.0 of tendermint. We should have a custom mempool by then anyway.

We can leave this open as its a nice to have, but having a test for the prioritized mempool is a bit redundant so its not a priority

@evan-forbes evan-forbes mentioned this issue Jan 18, 2023
Closed
17 tasks
@evan-forbes evan-forbes self-assigned this Aug 29, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Aug 29, 2023
@evan-forbes evan-forbes mentioned this issue Aug 31, 2023
Merged
5 tasks
evan-forbes added a commit that referenced this issue Aug 31, 2023
@evan-forbes @rootulp
chore: add test for tx priority (#2394)
58c1e4e 
## Overview

adds a simple integration test for tx priority in the mempool

closes #513

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Rootul Patel <rootulp@gmail.com>
@github-project-automation github-project-automation bot moved this from TODO to Done in Celestia Node Aug 31, 2023
mergify bot pushed a commit that referenced this issue Aug 31, 2023
@evan-forbes @mergify
chore: add test for tx priority (#2394)
f5db4f2 
## Overview

adds a simple integration test for tx priority in the mempool

closes #513

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Rootul Patel <rootulp@gmail.com>
(cherry picked from commit 58c1e4e)

# Conflicts:
#	Makefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evan-forbes evan-forbes

Labels
testing items that are strictly related to adding or extending test coverage
Projects
No open projects
Celestia Node
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: add test for tx priority celestiaorg/celestia-app
2 participants
@rootulp @evan-forbes