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).
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