feat(sequencer): make mempool balance aware#1408
Conversation
Fraser999
left a comment
There was a problem hiding this comment.
Partial review with some minor suggestions.
| .chain(parked.addresses()) | ||
| .collect(); |
There was a problem hiding this comment.
there could be duplicates of addresses here if it's in both pending and parked? maybe sort and then dedup?
There was a problem hiding this comment.
They're collected into a HashSet which does the dedup by nature of the container. Happy to change into something else if you'd prefer
| if demotion_txs.is_empty() { | ||
| // nothing to demote, check for transactions to promote |
There was a problem hiding this comment.
why do we only promote txs if there's nothing to demote? shouldn't both occur either way?
There was a problem hiding this comment.
If we demoted something from an account's pending queue that means there shouldn't be anything to promote from the account's parked queue. The pending queue's enforcement of sequential nonces is the main reason for this.
For example, if we have nonces [0,1,2] in pending and nonces [3,4] in parked , and [2] gets demoted from pending because of lack of account funds, then parked would be [2,3,4]. During the promotion loop 2 would be ineligible for promotion due to lack of account funds, also making 3,4 ineligible because of the sequential nonce requirement.
I should write up docs on how the different queues work.
| /// Note: assumes that the balances in `current_account_balance` are large enough | ||
| /// to cover costs for contained transactions. Will log an error if this is not true | ||
| /// but will not fail. |
There was a problem hiding this comment.
why not return an error? what cases would that error happen, developer error or other?
There was a problem hiding this comment.
It'd be a code logic error. It is possible to have too high costs in pending after transaction re-costing or a different node including a different transaction for the same account's nonce in a block, but we only call get_remaining_balances() after we run the demotion logic on pending which should make the transaction costs covered.
Fraser and I thought it would be better to let the mempool continue running if somewhere else in the code there was a bug that would make the above assumptions untrue instead of having the mempool crash. The idea was that someone would see the error logs and we'd know we need to go fix something 😬
| /// Note: this function only operates on the front of the queue. If the target nonce is not at | ||
| /// the front, an error will be logged and nothing will be returned. | ||
| fn pop_front_contiguous( | ||
| /// the front, nothing will be returned. |
There was a problem hiding this comment.
why pass in the target nonce if the function only works at the front of the queue?
There was a problem hiding this comment.
The target nonce is the next nonce that pending is expecting, I can add that to the documentation. It's not guaranteed that parked will contain a transaction with the target nonce and I was letting this function internalize that check instead of doing it explicitly. I can make it explicit if you'd prefer.
There shouldn't be the scenario where parked and pending have overlapping nonces as we only add things to parked if their nonce isn't already in pending.
| ) -> bool; | ||
|
|
||
| /// Returns `Ok` if adding `ttx` would not break the balance precondition, i.e. enough | ||
| /// balance to cover all transactions in `CostsCovered` mode. |
There was a problem hiding this comment.
Do we have such a thing as CostsCovered mode?
There was a problem hiding this comment.
I was trying to do the naming similar to the SequentialNonces mode we have but for saying the container only contains transactions that are covered by the account's balances. On reflection CostsCovered isn't the same as the SequentialNonces case because we have to reclean to stay in CostsCovered when SequentialNonces is always true.
Happy to remove the comments if you think they are misleading.
There was a problem hiding this comment.
I guess if you're ok to remove just the " in CostsCovered mode" part, that would make more sense to me.
|
Addressed all comments I didn't comment responses to, marking this ready for review again |
SuperFluffy
left a comment
There was a problem hiding this comment.
few comments on the merged asset/asset-balance stream
Fraser999
left a comment
There was a problem hiding this comment.
LGTM now - optimistic approval assuming @SuperFluffy's latest comments are addressed.
4014969 to
a0d157c
Compare
| ttx: &TimemarkedTransaction, | ||
| current_account_balances: &HashMap<IbcPrefixed, u128>, | ||
| ) -> bool { | ||
| let mut current_account_balances = current_account_balances.clone(); |
There was a problem hiding this comment.
This is a pretty innocently named functions where I would not expect a deep copy to happen.
I think this could already be addressed by changing the type signature (to make the clone explicit at the call site) and adding a comment:
/// Tests if `account_balances` has enough funds to cover `self` and `ttx`.
fn has_balance_to_cover(
&self,
ttx: &TimeMarkedTransaction,
mut account_balances: HashMap<IbcPrefixed, u128,
) -> boolThere was a problem hiding this comment.
Ah, good catch. Will change!
There was a problem hiding this comment.
Actually, this was done to prevent doing copies of the account_balances for the parked transactions container, which is a noop for the has_balance_to_cover fn. We can avoid making unnecessary copies if we do the clone on the inside for the pending container
751c709 to
588ee6b
Compare
|
Given this being a breaking charge, please mark this patch as breaking using Also, please add a section
|
…e keys (#1487) ## Summary Updated public addresses in `app/test_util.rs` to addresses with known private keys. ## Background Needed for PR #1408. We want to separate out the 'breaking change' into a different PR since it is unrelated. ## Breaking Changes While the sequencer app hash snapshots are changed, this patch is not breaking as not business logic was updated.
…1411) This PR builds on top of the mempool balance aware logic to recost the mempool when a `FeeAssetChange` or `FeeChange` action are seen in a finalized block. There needs to be a follow up PR to ensure that the `FeeChange` and `FeeAssetChange` actions are only ran at the end of the block to ensure that transactions are invalidated mid-block due to fee changes. The mempool needs to recost transactions if the fees are changed, as the original placement of the transaction in the `parked` or `pending` structure may be out of date. - Added logic to flag when `FeeAssetChange` or `FeeChange` actions are included in a block. - The mempool will now re-cost transactions when this happens. - Changed the mempool to use the app's state directly instead of using getters Unit tests for the mempool and the app logic. This doesn't have breaking changes, but the snapshot files were re-generated because I changed the public addresses for `CAROL_ADDRESS`, `BOB_ADDRESS`, and `JUDY_ADDRESS` in the `test_utils.rs` file. I needed access to their private keys to sign transactions for testing promotion flows on the app level. Added `MEMPOOL_RECOSTED` counter to provide metrics on how often fees are being changed. --------- Co-authored-by: Jordan Oroshiba <jordan@astria.org>
43b2186 to
420fc3a
Compare
* main: feat(sequencer): make mempool balance aware (#1408) chore(sequencer): change test addresses to versions with known private keys (#1487) chore(chart): update geth tag (#1485) feat(sequencer): report deposit events (#1447) feat(proto, core, sequencer)!: add traceability to rollup deposits (#1410) fix(bridge-withdrawer, cli, sequencer-client): migrate from `broadcast_tx_commit` to `broadcast_tx_sync` (#1376) fix(sequencer): add `end_block` to `app_execute_transaction_with_every_action_snapshot` (#1455) release: end of iteration release cuts (#1456) chore(charts): rollupName templates (#1458) chore(sequencer-relayer): Add instrumentation (#1375) feat(proto, core, sequencer)!: permit bech32 compatible addresses (#1425) chore: memoize `address_bytes` of verification key (#1444) chore(ci): include `ibc-bridge-test` in `docker` CI target (#1438) chore(charts): bump celestia versions (#1431)
Summary
Added logic to the mempool to make its pending vs parked logic balance aware. Transactions will only be added to the pending queue if the signing account has enough balance to cover the cost of all of the transactions in the pending plus the new transaction. Transactions that exceed the account's available balance are put into parked.
This PR also contains the logic needed to recost the transactions in the mempool when a
FeeAssetChangeorFeeChangeactions are ran.Background
Previously the mempool did not have sufficient logic to deal with transaction costs.
CheckTxwould kick out any transaction that the account did not have enough balance to cover. This didn't account for the edge cases of:prepare_proposal()'s block construction.Changes
costfield to theTimemarkedTransactionstruct.PendingTransactionsForAccounttype'sadd()functionality reject transactions that would exceed an account's balance.PendingTransactionsForAccounttype and promotion logic to theParkedTransactionsForAccounttype which is invoked in the mempool'srun_maintenance()function.FeeAssetChangeorFeeChangeactions is executed and proceeds to recost the mempool in therun_maintenance()function.Testing
Wrote unit tests and ran locally.
Metrics
Added timing metrics to CheckTx to track how long the transaction costing logic takes:
check_tx_duration_seconds_convert_addresscheck_tx_duration_seconds_fetch_balancescheck_tx_duration_seconds_fetch_tx_costAdded metrics to count how many times the mempool is recosted:
mempool_recostedRelated Issues
closes #1336