fix(sequencer): remove unwrap from app utilized mempool logic #1772
Conversation
Lilyjjo
changed the title
| .get_account_nonce(address) | ||
| .await | ||
| .wrap_err("failed to fetch account nonce for builder queue")?; | ||
| let current_account_nonce = match state.get_account_nonce(address).await { |
There was a problem hiding this comment.
if this function returns an error, it's pretty severe (either we can't read from state, and the app will crash soon anyways, or we can't deserialize the storage value which would have caused an error earlier) so i think expect()ing here is fine
There was a problem hiding this comment.
expecting is not a replacement for error handling. expecting should only be used for when system invariants are so badly violated that you can't recover from them at all, which is not the case here.
| .builder_queue(&self.state) | ||
| .await | ||
| .expect("failed to fetch pending transactions"); | ||
| let pending_txs = self.mempool.builder_queue(&self.state).await; |
There was a problem hiding this comment.
Rereading the surrounding code, I would have honestly been fine with just wrap_err'oring this (without the other changes).
Tracing the error back up it eventually still panics (execute_transactions_prepare_proposal -> prepare_proposal -> handle_prepare_proposal -> handle_request -> run). But we could fix that by returning the error back up to the server, which would be cleaner (not this PR but followup).
| // use the lowest nonce in pending as the current nonce, this should be the | ||
| // same value as the nonce returned by the state. the pending queue shouldn't | ||
| // be empty, if it is we can continue to the next account |
There was a problem hiding this comment.
This is a good point. I think we should actually just use this technique rather than passing in the state. That would simplify this method and allow it to not be async.
We could change constructing the current account nonce to:
let Some(current_account_nonce) = account_txs.current_account_nonce() else {
error!(
address = %telemetry::display::base64(address),
"pending queue is empty during builder queue step"
);
continue;
};and add this to the impl for PendingTransactionsForAccount:
fn current_account_nonce(&self) -> Option<u32> {
self.txs.first_key_value().map(|(nonce, _)| *nonce)
}
Summary
This PR changes the mempool's
builder_queue()to be infallible. The code previously would return an error if the nonce fetch from the database failed. Now it handles the error case by using the pending's lowest nonce for the account as an educated guess of what nonce to use for transaction priority construction. This is okay sinceprepare_proposal()'s logic will just reject invalid nonces if the mempool's state is out of date due to a bug.Background
We shouldn't have non-existential issues in the mempool cause failures in the sequencer.
Changes
builder_queue()to be infallible with reasonable fallbacks.Testing
Manually changed the code path to only use the new logic and watched all tests pass except for those explicitly testing the jitter between the mempool and database (which shouldn't happen if the rest of the code works). The tests in those scenarios only fail due to unmet assertions and not because of panics.
Changelogs
No updates required.
Related Issues
closes #1769