fix(sequencer): run only prepare_proposal if proposer
#558
Conversation
prepare_proposal if proposer
| sequencer_block_hash = ?block.block_hash, | ||
| sequencer_block_height = block.header.height.value(), | ||
| execution_block_hash = hex::encode(&executed_block.hash), | ||
| tx_count, |
There was a problem hiding this comment.
This is getting messy. Can you instrument execute_block and move the tx count and the other 2 fields derived from the block into the span?
crates/astria-sequencer/src/app.rs
Outdated
|
|
||
| // set to true when `prepare_proposal` is called, indicating we are the proposer for this slot. | ||
| // set to false when `commit` is called, finalizing the block. | ||
| // when this is se to true, `process_proposal` is not executed, as we have already executed |
There was a problem hiding this comment.
| // when this is se to true, `process_proposal` is not executed, as we have already executed | |
| // if true, `process_proposal` is not executed, as we have already executed |
| process_proposal: abci::request::ProcessProposal, | ||
| ) -> anyhow::Result<()> { | ||
| if self.is_proposer { | ||
| return Ok(()); |
crates/astria-sequencer/src/app.rs
Outdated
|
|
||
| // Get the latest version of the state, now that we've committed it. | ||
| self.state = Arc::new(StateDelta::new(storage.latest_snapshot())); | ||
| self.is_proposer = false; |
There was a problem hiding this comment.
Add a comment for why this is necessary to if the reader forgot what is_propser is.
SuperFluffy
left a comment
There was a problem hiding this comment.
Just a few comments.
| &mut self, | ||
| process_proposal: abci::request::ProcessProposal, | ||
| ) -> anyhow::Result<()> { | ||
| if self.is_proposer { |
There was a problem hiding this comment.
Should we block out only specific tasks within here when running? IE only avoid the re-execution?
There was a problem hiding this comment.
basically everything is here is verification and re-execution, don't think we need to re-do any of it
| // when this is se to true, `process_proposal` is not executed, as we have already executed | ||
| // the transactions for the block during `prepare_proposal`, and re-executing them would | ||
| // cause failure. | ||
| is_proposer: bool, |
There was a problem hiding this comment.
I'm a bit concerned about this mechanism in adversarial or dysfunctional scenarios?
What if we can't get a commit to the block in time our proposer gets skipped/slashed but still thinks it's in "proposer" mode? Could we instead check in execute_block_data somehow if that block has already been executed and just skip it if it has?
There was a problem hiding this comment.
yeah i agree, this wouldn't work in that case. i updated this to always set is_proposer = false when process_proposal is called, so process_proposal will skip only in the case where prepare_proposal is called directly before it. so if we propose a block, but it's not accepted, and then process_proposal is called for the next proposed block, it will execute as normal
turns out both `prepare_proposal` and `process_proposal` are called for a proposer node. since we moved transaction execution to prepare/process_proposal, when the proposer called both these functions, execution would fail during `process_proposal` since the state was already updated for post-execution. see summary. - track if we're the proposer during the block lifecycle in the `App` struct, and if we are, don't execute transactions during `process_proposal` local testing
turns out both `prepare_proposal` and `process_proposal` are called for a proposer node. since we moved transaction execution to prepare/process_proposal, when the proposer called both these functions, execution would fail during `process_proposal` since the state was already updated for post-execution. see summary. - track if we're the proposer during the block lifecycle in the `App` struct, and if we are, don't execute transactions during `process_proposal` local testing
Summary
turns out both
prepare_proposalandprocess_proposalare called for a proposer node. since we moved transaction execution to prepare/process_proposal, when the proposer called both these functions, execution would fail duringprocess_proposalsince the state was already updated for post-execution.Background
see summary.
Changes
Appstruct, and if we are, don't execute transactions duringprocess_proposalTesting
local testing