-
Notifications
You must be signed in to change notification settings - Fork 82
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.
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
feat(sequencer): rewrite memool to have per-account transaction storage and maintenance #1323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lilyjjo
changed the title
mempool rewrite
ENG-51/sequencer: rewrite memool to have per-account transaction storage and maintenance
Jul 31, 2024
Lilyjjo
force-pushed
the
lilyjjo/sequencer_app_mempool_pending
branch
from
July 31, 2024 20:02
c6d7b00
to
ce4f642
Compare
joroshiba
changed the title
ENG-51/sequencer: rewrite memool to have per-account transaction storage and maintenance
feat(sequencer): rewrite memool to have per-account transaction storage and maintenance
Aug 1, 2024
Lilyjjo
force-pushed
the
lilyjjo/sequencer_app_mempool_pending
branch
from
August 1, 2024 21:32
665f8cd
to
038b94d
Compare
Lilyjjo
pushed a commit
that referenced
this pull request
Aug 13, 2024
## Summary This is effectively an update to the existing PR #1323. ## Background While reviewing #1323, it seemed more expedient to open a PR against it rather than adding many GH comments (this was discussed with @Lilyjjo). As always, there are more changes here than I initially expected. The main thrust of the changes was to try and use the type system to avoid certain classes of errors, and to make things a little more canonical where possible. ## Changes - At a high level, there are now separate types dealing with pending txs and parked txs: `PendingTransactionsForAccount`, `PendingTransactions`, `ParkedTransactionsForAccount` and `ParkedTransactions`. For the ones dealing with a single account, much of their logic is common, so a trait `TransactionsForAccount` was introduced to express that shared logic. For the ones dealing with collection of accounts' txs, a struct was introduced: `TransactionsContainer` generic over the type of the account-specific container `T: TransactionsForAccount`. This allowed for the shared logic to be expressed on the generic impl, with methods which are only relevant to pending being added to only that specialization, and similarly for the parked impl. - Extraneous fields were removed to avoid potential synchronization issues (e.g. `Mempool::all` and `TransactionContainer::all`) - Reduced scope of several of the locks made on the `RwLock`s - Various other smaller changes ## Testing - No additional tests were added, the existing tests were modified. - Benchmarks were run and reveal very promising results. These are a reflection of the changes made in the original PR rather than this one. Example output is included below, and for reference, the previous figures are available in the description of #1238. Summary is that for a mempool with 100 txs, we're seeing roughly a two times speed increase, and at 100,000 txs, we're seeing roughly a ten times increase. <details> <summary>Example of running <code>cargo bench -qp astria-sequencer</code> on my Ryzen 7900X</summary> ``` Timer precision: 10 ns astria_sequencer fastest │ slowest │ median │ mean │ samples │ iters ╰─ mempool │ │ │ │ │ ╰─ benchmarks │ │ │ │ │ ├─ check_removed_comet_bft 41.1 µs │ 47.7 µs │ 41.96 µs │ 42.81 µs │ 100 │ 100 ├─ builder_queue │ │ │ │ │ │ ├─ mempool_with_100000_txs 9.578 ms │ 13.59 ms │ 10.09 ms │ 10.17 ms │ 100 │ 100 │ ├─ mempool_with_10000_txs 613 µs │ 821.4 µs │ 734.7 µs │ 723.7 µs │ 100 │ 100 │ ├─ mempool_with_1000_txs 55.37 µs │ 68.09 µs │ 59.3 µs │ 59.44 µs │ 100 │ 100 │ ╰─ mempool_with_100_txs 9.377 µs │ 14.2 µs │ 9.913 µs │ 10.21 µs │ 100 │ 100 ├─ insert │ │ │ │ │ │ ├─ mempool_with_100000_txs 2.79 ms │ 3.327 ms │ 2.944 ms │ 2.968 ms │ 100 │ 100 │ ├─ mempool_with_10000_txs 137.9 µs │ 277.3 µs │ 201.4 µs │ 199.2 µs │ 100 │ 100 │ ├─ mempool_with_1000_txs 15.38 µs │ 24.64 µs │ 18.73 µs │ 18.78 µs │ 100 │ 100 │ ╰─ mempool_with_100_txs 7.303 µs │ 11.24 µs │ 7.579 µs │ 7.686 µs │ 100 │ 100 ├─ pending_nonce │ │ │ │ │ │ ├─ mempool_with_100000_txs 2.727 ms │ 3.524 ms │ 2.896 ms │ 2.92 ms │ 100 │ 100 │ ├─ mempool_with_10000_txs 124.4 µs │ 275.7 µs │ 198.2 µs │ 194.4 µs │ 100 │ 100 │ ├─ mempool_with_1000_txs 12.56 µs │ 22.14 µs │ 17.16 µs │ 16.97 µs │ 100 │ 100 │ ╰─ mempool_with_100_txs 6.001 µs │ 11.11 µs │ 6.176 µs │ 6.262 µs │ 100 │ 100 ├─ remove_tx_invalid │ │ │ │ │ │ ├─ mempool_with_100000_txs 2.76 ms │ 3.832 ms │ 2.945 ms │ 3.02 ms │ 100 │ 100 │ ├─ mempool_with_10000_txs 120.5 µs │ 288.1 µs │ 199.2 µs │ 201 µs │ 100 │ 100 │ ├─ mempool_with_1000_txs 14.76 µs │ 31.87 µs │ 18.22 µs │ 18.35 µs │ 100 │ 100 │ ╰─ mempool_with_100_txs 7.593 µs │ 18.72 µs │ 7.824 µs │ 8.123 µs │ 100 │ 100 ╰─ run_maintenance │ │ │ │ │ ├─ mempool_with_100000_txs 3.209 ms │ 3.877 ms │ 3.424 ms │ 3.456 ms │ 100 │ 100 ├─ mempool_with_10000_txs 423.5 µs │ 765.5 µs │ 497.7 µs │ 505.3 µs │ 100 │ 100 ├─ mempool_with_1000_txs 81 µs │ 125.8 µs │ 88.26 µs │ 88.84 µs │ 100 │ 100 ╰─ mempool_with_100_txs 14.14 µs │ 24.13 µs │ 14.64 µs │ 15.04 µs │ 100 │ 100 ``` </details>
Fraser999
approved these changes
Aug 13, 2024
Lilyjjo
force-pushed
the
lilyjjo/sequencer_app_mempool_pending
branch
from
August 20, 2024 15:05
6319330
to
4b17484
Compare
Lilyjjo
force-pushed
the
lilyjjo/sequencer_app_mempool_pending
branch
from
August 20, 2024 15:06
4b17484
to
be99377
Compare
steezeburger
added a commit
that referenced
this pull request
Aug 22, 2024
* main: refactor(core, proto)!: define app genesis state in proto (#1346) fix(sequencer): bump penumbra dep to fix ibc state access bug (#1389) feat(conductor)!: support disabled celestia auth (#1372) fix(sequencer)!: fix block fees (#1343) perf(sequencer): add benchmark for prepare_proposal (ENG-660) (#1337) fix(proto): fix import name mismatch (#1380) fix(ci): enable bridge withdrawer building with tag (#1374) feat(sequencer): rewrite memool to have per-account transaction storage and maintenance (#1323) refactor(core, sequencer)!: require that bridge unlock address always be set (#1339) fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344) fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332) fix: abci error code (#1280) refactor(core): shorten `try_from_block_info_and_data()` (#1371) fix(relayer): change `reqwest` for `isahc` in relayer blackbox tests (ENG-699) (#1366) fix(conductor): update for celestia-node v0.15.0 (#1367) Chore: Upgrade celestia-node to v0.14.1 (#1360) chore(charts): fix charts production templates (#1359) chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
This was referenced Aug 28, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 4, 2024
…sion (#1515) ## Summary Previously `handle_check_tx()`'s would always run all stateless checks and attempt insertion. This PR modified the function to only run the checks on transactions that have not been removed or are not contained inside of the app side mempool. This PR additionally adds a HashSet of transaction hashes that are contained in the app's Mempool to the state of the app Mempool. This is used to enable seeing if a transaction is contained in the Mempool without having to decode it into a `SignedTransaction`. This will be kept in sync with unit tests which check if the length of the mempool (which is now calculated with the HashSet) matches the expected state of mutating the underlying containers. We considered using just a toggle on `CheckTX::New` and `CheckTX::Recheck` instead of the HashSet, but there is the possibility of CometBFT persisting transactions on restart which could make use of toggling on the app mempool knowing the transaction instead. TBD, but it's also useful to be able to query the mempool if it contains a transaction. This PR also restores the original behavior of `CheckTx` to not kick out transaction needlessly on rechecks, this regression was added when upgrading the mempool in #1323. Pre #1323, re-inserting the same transaction into the mempool would not cause an error. Post #1323, the re-insertion would cause an error. We now check to make sure that the transaction is not inside of the mempool before attempting to re-insert it. ## Background A lot of the functionality from the original `handle_check_tx()` has moved into the app side mempool itself, including the balance and nonce checks. All of the remaining checks will not change on `CheckTx::Recheck` and only need to be ran when considering a transaction for insertion into the appside mempool. ## Testing Ran locally and added unit tests to ensure future nonces can be added and rechecks do not remove transactions needlessly. ## Metrics - Deleted `check_tx_removed_stale_nonce` - Added `check_tx_duration_seconds_check_tracked`, `mempool_tx_logic_error` - Changed `check_tx_duration_seconds_check_nonce` to `check_tx_duration_seconds_fetch_nonce` ## Breaking Changes Added new ABCI error codes for transaction insertion errors that would be useful to surface to the end user: `ALREADY_PRESENT`, `NONCE_TAKEN`, `ACCOUNT_SIZE_LIMIT`. ## Related Issues closes #1481 --------- Co-authored-by: Fraser Hutchison <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 4, 2024
…sion (#1515) ## Summary Previously `handle_check_tx()`'s would always run all stateless checks and attempt insertion. This PR modified the function to only run the checks on transactions that have not been removed or are not contained inside of the app side mempool. This PR additionally adds a HashSet of transaction hashes that are contained in the app's Mempool to the state of the app Mempool. This is used to enable seeing if a transaction is contained in the Mempool without having to decode it into a `SignedTransaction`. This will be kept in sync with unit tests which check if the length of the mempool (which is now calculated with the HashSet) matches the expected state of mutating the underlying containers. We considered using just a toggle on `CheckTX::New` and `CheckTX::Recheck` instead of the HashSet, but there is the possibility of CometBFT persisting transactions on restart which could make use of toggling on the app mempool knowing the transaction instead. TBD, but it's also useful to be able to query the mempool if it contains a transaction. This PR also restores the original behavior of `CheckTx` to not kick out transaction needlessly on rechecks, this regression was added when upgrading the mempool in #1323. Pre #1323, re-inserting the same transaction into the mempool would not cause an error. Post #1323, the re-insertion would cause an error. We now check to make sure that the transaction is not inside of the mempool before attempting to re-insert it. ## Background A lot of the functionality from the original `handle_check_tx()` has moved into the app side mempool itself, including the balance and nonce checks. All of the remaining checks will not change on `CheckTx::Recheck` and only need to be ran when considering a transaction for insertion into the appside mempool. ## Testing Ran locally and added unit tests to ensure future nonces can be added and rechecks do not remove transactions needlessly. ## Metrics - Deleted `check_tx_removed_stale_nonce` - Added `check_tx_duration_seconds_check_tracked`, `mempool_tx_logic_error` - Changed `check_tx_duration_seconds_check_nonce` to `check_tx_duration_seconds_fetch_nonce` ## Breaking Changes Added new ABCI error codes for transaction insertion errors that would be useful to surface to the end user: `ALREADY_PRESENT`, `NONCE_TAKEN`, `ACCOUNT_SIZE_LIMIT`. ## Related Issues closes #1481 --------- Co-authored-by: Fraser Hutchison <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This rewrite makes our mempool similar to Geth's/Reth's with per-account transaction storage and maintenance. This rewrite tracks 'pending' transactions that are ready to execute, and 'parked' transactions that could be ready for execution in the future on a per-account basis.
This rewrite adds minor new functionality from the previous mempool and changes the mempool interface. Was co-written with @Fraser999.
Background
Prior to this rewrite, the mempool was not in a state to be improved upon. All transactions, ready to execute or not, were being stored together in a giant queue. This structure didn't allow for mempool optimizations such as performing mempool upkeep on a per account basis-- instead the only option was to loop through all transactions on the per-block maintenance upkeep.
mempool/mod.rs
ChangesThis file contains the rewrite of the mempool interface. Instead of returning a modifiable queue of all transactions to the builder in
prepare_proposal()
, now we return a copy of the transactions. Instead of removing transactions as we process them, we only remove transactions during thefinalize_block()
logic, updating accounts to reflect their new nonces.The main app-exposed mempool calls are:
insert()
, which will only insert a transaction if it meets the mempool's transaction policies.remove_tx_invalid()
, to be used byprepare_proposal()
to remove transactions that fail execution from the mempool.builder_queue()
, returns a copy of the ready-to-execute transactions sorted by priority order.run_maintenance()
, updates the stored transactions post new block height increases.The mempool implements the following transaction policies:
PARKED_SIZE_LIMIT
transactions in their parked queues (currently set to 15).TX_TTL
time (currently set to 4 minutes).mempool/transactions_container.rs
This is a new file containing the data structures for the per-account transaction logic. Several new code level constructs are added:
TimemarkedTransaction
: aSignedTransaction
wrapper that also stores the time that the transaction was first seen by the mempool. This is used for implementing the transaction expiry policy.TransactionsForAccount
with typesPendingTransactionsForAccount
andParkedTransactionForAccount
. This is used to house the per-account logic of storing pending and parked transactions, respectively.TransactionsContainer<T>
: a struct with a generic overTransactionsForAccount
. TypesPendingTransactions
andParkedTransactions
are defined over this as the mempool level containers for pending and parked transactions.Other notable changes
CheckTx()
CometBFT mempool logic. TBD on how to surface this to the end user.finalize_block()
logic which was updating the mempool with the previous block's state.Mempool Overview
The overall mempool structure as of this PR for Astria is shown in the below diagram:
Testing
TODO still
Related Issues
closes #1154, #1150, #1334