diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 688c6c1ece..32393321bb 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -136,6 +136,40 @@ const POST_TRANSACTION_EXECUTION_RESULT_KEY: &str = "post_transaction_execution_ /// The inter-block state being written to by the application. type InterBlockState = Arc>; +/// This is used to identify a proposal constructed by the app instance +/// in `prepare_proposal` during a `process_proposal` call. +/// +/// The fields are not exhaustive, in most instances just the validator address +/// is adequate. When running a third party signer such as horcrux however it is +/// possible that multiple nodes are preparing proposals as the same validator +/// address, in these instances the timestamp is used as a unique identifier for +/// the proposal from that node. This is not a perfect solution, but it only +/// impacts sentry nodes does not halt the network and is cheaper computationally +/// than an exhaustive comparison. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub(crate) struct ProposalFingerprint { + validator_address: account::Id, + timestamp: tendermint::Time, +} + +impl From for ProposalFingerprint { + fn from(proposal: abci::request::PrepareProposal) -> Self { + Self { + validator_address: proposal.proposer_address, + timestamp: proposal.time, + } + } +} + +impl From for ProposalFingerprint { + fn from(proposal: abci::request::ProcessProposal) -> Self { + Self { + validator_address: proposal.proposer_address, + timestamp: proposal.time, + } + } +} + /// The Sequencer application, written as a bundle of [`Component`]s. /// /// Note: this is called `App` because this is a Tendermint ABCI application, @@ -152,14 +186,18 @@ pub(crate) struct App { // Transactions are pulled from this mempool during `prepare_proposal`. mempool: Mempool, - // The validator address in cometbft being used to sign votes. + // TODO(https://github.com/astriaorg/astria/issues/1660): The executed_proposal_fingerprint and + // executed_proposal_hash fields should be stored in the ephemeral storage instead of on the + // app struct, to avoid any issues with forgetting to reset them. + + // An identifier for a given proposal constructed by this app. // // Used to avoid executing a block in both `prepare_proposal` and `process_proposal`. It // is set in `prepare_proposal` from information sent in from cometbft and can potentially // change round-to-round. In `process_proposal` we check if we prepared the proposal, and - // if so, we clear the value and we skip re-execution of the block's transactions to avoid + // if so, we clear the value, and we skip re-execution of the block's transactions to avoid // failures caused by re-execution. - validator_address: Option, + executed_proposal_fingerprint: Option, // This is set to the executed hash of the proposal during `process_proposal` // @@ -217,7 +255,7 @@ impl App { Ok(Self { state, mempool, - validator_address: None, + executed_proposal_fingerprint: None, executed_proposal_hash: Hash::default(), recost_mempool: false, write_batch: None, @@ -327,7 +365,7 @@ impl App { prepare_proposal: abci::request::PrepareProposal, storage: Storage, ) -> Result { - self.validator_address = Some(prepare_proposal.proposer_address); + self.executed_proposal_fingerprint = Some(prepare_proposal.clone().into()); self.update_state_for_new_round(&storage); let mut block_size_constraints = BlockSizeConstraints::new( @@ -381,11 +419,12 @@ impl App { // we skip execution for this `process_proposal` call. // // if we didn't propose this block, `self.validator_address` will be None or a different - // value, so we will execute the block as normal. - if let Some(id) = self.validator_address { - if id == process_proposal.proposer_address { + // value, so we will execute block as normal. + if let Some(constructed_id) = self.executed_proposal_fingerprint { + let proposal_id = process_proposal.clone().into(); + if constructed_id == proposal_id { debug!("skipping process_proposal as we are the proposer for this block"); - self.validator_address = None; + self.executed_proposal_fingerprint = None; self.executed_proposal_hash = process_proposal.hash; // if we're the proposer, we should have the execution results from @@ -415,7 +454,7 @@ impl App { "our validator address was set but we're not the proposer, so our previous \ proposal was skipped, executing block" ); - self.validator_address = None; + self.executed_proposal_fingerprint = None; } self.update_state_for_new_round(&storage); diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index 6e5ddea619..f46dca54c5 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -497,6 +497,7 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { local_last_commit: None, misbehavior: vec![], }; + let proposal_fingerprint = prepare_proposal.clone().into(); let prepare_proposal_result = app .prepare_proposal(prepare_proposal, storage.clone()) @@ -504,7 +505,10 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { .unwrap(); assert_eq!(prepare_proposal_result.txs, finalize_block.txs); assert_eq!(app.executed_proposal_hash, Hash::default()); - assert_eq!(app.validator_address.unwrap(), proposer_address); + assert_eq!( + app.executed_proposal_fingerprint, + Some(proposal_fingerprint) + ); app.mempool.run_maintenance(&app.state, false).await; @@ -526,7 +530,7 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { .await .unwrap(); assert_eq!(app.executed_proposal_hash, block_hash); - assert!(app.validator_address.is_none()); + assert!(app.executed_proposal_fingerprint.is_none()); let finalize_block_after_prepare_proposal_result = app .finalize_block(finalize_block.clone(), storage.clone()) @@ -545,7 +549,7 @@ async fn app_execution_results_match_proposal_vs_after_proposal() { .await .unwrap(); assert_eq!(app.executed_proposal_hash, block_hash); - assert!(app.validator_address.is_none()); + assert!(app.executed_proposal_fingerprint.is_none()); let finalize_block_after_process_proposal_result = app .finalize_block(finalize_block, storage.clone()) .await