diff --git a/crates/astria-sequencer/src/accounts/component.rs b/crates/astria-sequencer/src/accounts/component.rs index c9d515ec45..161ac938e2 100644 --- a/crates/astria-sequencer/src/accounts/component.rs +++ b/crates/astria-sequencer/src/accounts/component.rs @@ -6,16 +6,15 @@ use astria_eyre::eyre::{ Result, WrapErr as _, }; -use tendermint::abci::request::{ - BeginBlock, - EndBlock, -}; use tracing::instrument; use crate::{ accounts, assets, - component::Component, + component::{ + Component, + PrepareStateInfo, + }, }; #[derive(Default)] @@ -48,18 +47,17 @@ impl Component for AccountsComponent { Ok(()) } - #[instrument(name = "AccountsComponent::begin_block", skip_all)] - async fn begin_block( + #[instrument(name = "AccountsComponent::prepare_state_for_tx_execution", skip_all)] + async fn prepare_state_for_tx_execution( _state: &mut Arc, - _begin_block: &BeginBlock, + _prepare_state_for_tx_execution: &PrepareStateInfo, ) -> Result<()> { Ok(()) } - #[instrument(name = "AccountsComponent::end_block", skip_all)] - async fn end_block( + #[instrument(name = "AccountsComponent::handle_post_tx_execution", skip_all)] + async fn handle_post_tx_execution( _state: &mut Arc, - _end_block: &EndBlock, ) -> Result<()> { Ok(()) } diff --git a/crates/astria-sequencer/src/app/mod.rs b/crates/astria-sequencer/src/app/mod.rs index 6baf4447b9..822f585b53 100644 --- a/crates/astria-sequencer/src/app/mod.rs +++ b/crates/astria-sequencer/src/app/mod.rs @@ -73,7 +73,6 @@ use tendermint::{ Event, }, account, - block::Header, AppHash, Hash, }; @@ -109,7 +108,10 @@ use crate::{ StateReadExt as _, StateWriteExt as _, }, - component::Component as _, + component::{ + Component as _, + PrepareStateInfo, + }, fees::{ component::FeesComponent, StateReadExt as _, @@ -207,8 +209,8 @@ pub(crate) struct App { // This is set to the executed hash of the proposal during `process_proposal` // - // If it does not match the hash given during `begin_block`, then we clear and - // reset the execution results cache + state delta. Transactions are re-executed. + // If it does not match the hash given during `prepare_state_for_tx_execution`, then we clear + // and reset the execution results cache + state delta. Transactions are re-executed. // If it does match, we utilize cached results to reduce computation. // // Resets to default hash at the beginning of `prepare_proposal`, and `process_proposal` if @@ -682,7 +684,8 @@ impl App { } /// sets up the state for execution of the block's transactions. - /// set the current height and timestamp, and calls `begin_block` on all components. + /// set the current height and timestamp, and calls `prepare_state_for_tx_execution` on all + /// components. /// /// this *must* be called anytime before a block's txs are executed, whether it's /// during the proposal phase, or finalize_block phase. @@ -697,42 +700,19 @@ impl App { // reset recost flag self.recost_mempool = false; - // call begin_block on all components - // NOTE: the fields marked `unused` are not used by any of the components; - // however, we need to still construct a `BeginBlock` type for now as - // the penumbra IBC implementation still requires it as a parameter. - let begin_block: abci::request::BeginBlock = abci::request::BeginBlock { - hash: Hash::default(), // unused - byzantine_validators: block_data.misbehavior.clone(), - header: Header { - app_hash: self.app_hash.clone(), - chain_id: chain_id.clone(), - consensus_hash: Hash::default(), // unused - data_hash: Some(Hash::default()), // unused - evidence_hash: Some(Hash::default()), // unused - height: block_data.height, - last_block_id: None, // unused - last_commit_hash: Some(Hash::default()), // unused - last_results_hash: Some(Hash::default()), // unused - next_validators_hash: block_data.next_validators_hash, - proposer_address: block_data.proposer_address, - time: block_data.time, - validators_hash: Hash::default(), // unused - version: tendermint::block::header::Version { - // unused - app: 0, - block: 0, - }, - }, - last_commit_info: tendermint::abci::types::CommitInfo { - round: 0u16.into(), // unused - votes: vec![], - }, // unused + let prepare_state_info = PrepareStateInfo { + app_hash: self.app_hash.clone(), + byzantine_validators: block_data.misbehavior, + chain_id, + height: block_data.height, + next_validators_hash: block_data.next_validators_hash, + proposer_address: block_data.proposer_address, + time: block_data.time, }; - self.begin_block(&begin_block) + self.start_block(&prepare_state_info) .await - .wrap_err("begin_block failed")?; + .wrap_err("prepare_state_for_tx_execution failed")?; Ok(()) } @@ -766,7 +746,9 @@ impl App { .await .wrap_err("failed to get sudo address from state")?; - let end_block = self.end_block(height.value(), &sudo_address).await?; + let (validator_updates, events) = self + .component_post_execution_state_updates(&sudo_address) + .await?; // get deposits for this block from state's ephemeral cache and put them to storage. let mut state_tx = StateDelta::new(self.state.clone()); @@ -804,15 +786,14 @@ impl App { .wrap_err("failed to write sequencer block to state")?; let result = PostTransactionExecutionResult { - events: end_block.events, - validator_updates: end_block.validator_updates, - consensus_param_updates: end_block.consensus_param_updates, + events, + validator_updates, tx_results: finalize_block_tx_results, }; state_tx.object_put(POST_TRANSACTION_EXECUTION_RESULT_KEY, result); - // events that occur after end_block are ignored here; + // events that occur after handle_post_tx_execution are ignored here; // there should be none anyways. let _ = self.apply(state_tx); @@ -931,7 +912,7 @@ impl App { let finalize_block = abci::response::FinalizeBlock { events: post_transaction_execution_result.events, validator_updates: post_transaction_execution_result.validator_updates, - consensus_param_updates: post_transaction_execution_result.consensus_param_updates, + consensus_param_updates: None, app_hash, tx_results: post_transaction_execution_result.tx_results, }; @@ -976,34 +957,34 @@ impl App { Ok(app_hash) } - #[instrument(name = "App::begin_block", skip_all)] - async fn begin_block( + #[instrument(name = "App::start_block", skip_all)] + async fn start_block( &mut self, - begin_block: &abci::request::BeginBlock, + prepare_state_info: &PrepareStateInfo, ) -> Result> { let mut state_tx = StateDelta::new(self.state.clone()); state_tx - .put_block_height(begin_block.header.height.into()) + .put_block_height(prepare_state_info.height.into()) .wrap_err("failed to put block height")?; state_tx - .put_block_timestamp(begin_block.header.time) + .put_block_timestamp(prepare_state_info.time) .wrap_err("failed to put block timestamp")?; - // call begin_block on all components + // call prepare_state_for_tx_execution on all components let mut arc_state_tx = Arc::new(state_tx); - AccountsComponent::begin_block(&mut arc_state_tx, begin_block) + AccountsComponent::prepare_state_for_tx_execution(&mut arc_state_tx, prepare_state_info) .await - .wrap_err("begin_block failed on AccountsComponent")?; - AuthorityComponent::begin_block(&mut arc_state_tx, begin_block) + .wrap_err("prepare_state_for_tx_execution failed on AccountsComponent")?; + AuthorityComponent::prepare_state_for_tx_execution(&mut arc_state_tx, prepare_state_info) .await - .wrap_err("begin_block failed on AuthorityComponent")?; - IbcComponent::begin_block(&mut arc_state_tx, begin_block) + .wrap_err("prepare_state_for_tx_execution failed on AuthorityComponent")?; + IbcComponent::prepare_state_for_tx_execution(&mut arc_state_tx, prepare_state_info) .await - .wrap_err("begin_block failed on IbcComponent")?; - FeesComponent::begin_block(&mut arc_state_tx, begin_block) + .wrap_err("prepare_state_for_tx_execution failed on IbcComponent")?; + FeesComponent::prepare_state_for_tx_execution(&mut arc_state_tx, prepare_state_info) .await - .wrap_err("begin_block failed on FeesComponent")?; + .wrap_err("prepare_state_for_tx_execution failed on FeesComponent")?; let state_tx = Arc::try_unwrap(arc_state_tx) .expect("components should not retain copies of shared state"); @@ -1049,34 +1030,27 @@ impl App { Ok(events) } - #[instrument(name = "App::end_block", skip_all)] - async fn end_block( + #[instrument(name = "App::component_post_execution_state_updates", skip_all)] + async fn component_post_execution_state_updates( &mut self, - height: u64, fee_recipient: &[u8; 20], - ) -> Result { + ) -> Result<(Vec, Vec)> { let state_tx = StateDelta::new(self.state.clone()); let mut arc_state_tx = Arc::new(state_tx); - let end_block = abci::request::EndBlock { - height: height - .try_into() - .expect("a block height should be able to fit in an i64"), - }; - - // call end_block on all components - AccountsComponent::end_block(&mut arc_state_tx, &end_block) + // call handle_post_tx_execution on all components + AccountsComponent::handle_post_tx_execution(&mut arc_state_tx) .await - .wrap_err("end_block failed on AccountsComponent")?; - AuthorityComponent::end_block(&mut arc_state_tx, &end_block) + .wrap_err("handle_post_tx_execution failed on AccountsComponent")?; + AuthorityComponent::handle_post_tx_execution(&mut arc_state_tx) .await - .wrap_err("end_block failed on AuthorityComponent")?; - FeesComponent::end_block(&mut arc_state_tx, &end_block) + .wrap_err("handle_post_tx_execution failed on AuthorityComponent")?; + FeesComponent::handle_post_tx_execution(&mut arc_state_tx) .await - .wrap_err("end_block failed on FeesComponent")?; - IbcComponent::end_block(&mut arc_state_tx, &end_block) + .wrap_err("handle_post_tx_execution failed on FeesComponent")?; + IbcComponent::handle_post_tx_execution(&mut arc_state_tx) .await - .wrap_err("end_block failed on IbcComponent")?; + .wrap_err("handle_post_tx_execution failed on IbcComponent")?; let mut state_tx = Arc::try_unwrap(arc_state_tx) .expect("components should not retain copies of shared state"); @@ -1102,13 +1076,12 @@ impl App { } let events = self.apply(state_tx); - Ok(abci::response::EndBlock { - validator_updates: validator_updates + Ok(( + validator_updates .try_into_cometbft() .wrap_err("failed converting astria validators to cometbft compatible type")?, events, - ..Default::default() - }) + )) } #[instrument(name = "App::commit", skip_all)] @@ -1194,7 +1167,6 @@ struct PostTransactionExecutionResult { events: Vec, tx_results: Vec, validator_updates: Vec, - consensus_param_updates: Option, } #[derive(PartialEq)] diff --git a/crates/astria-sequencer/src/app/tests_app/mod.rs b/crates/astria-sequencer/src/app/tests_app/mod.rs index 1ca46cec4d..efcb9153ec 100644 --- a/crates/astria-sequencer/src/app/tests_app/mod.rs +++ b/crates/astria-sequencer/src/app/tests_app/mod.rs @@ -46,8 +46,6 @@ use tendermint::{ }, account, block::{ - header::Version, - Header, Height, Round, }, @@ -77,28 +75,6 @@ use crate::{ proposal::commitment::generate_rollup_datas_commitment, }; -fn default_tendermint_header() -> Header { - Header { - app_hash: AppHash::try_from(vec![]).unwrap(), - chain_id: "test".to_string().try_into().unwrap(), - consensus_hash: Hash::default(), - data_hash: Some(Hash::try_from([0u8; 32].to_vec()).unwrap()), - evidence_hash: Some(Hash::default()), - height: Height::default(), - last_block_id: None, - last_commit_hash: Some(Hash::default()), - last_results_hash: Some(Hash::default()), - next_validators_hash: Hash::default(), - proposer_address: account::Id::try_from([0u8; 20].to_vec()).unwrap(), - time: Time::now(), - validators_hash: Hash::default(), - version: Version { - app: 0, - block: 0, - }, - } -} - #[tokio::test] async fn app_genesis_and_init_chain() { let app = initialize_app(None, vec![]).await; @@ -147,7 +123,7 @@ async fn app_pre_execute_transactions() { } #[tokio::test] -async fn app_begin_block_remove_byzantine_validators() { +async fn app_prepare_state_for_tx_execution_remove_byzantine_validators() { use tendermint::abci::types; let initial_validator_set = vec![ @@ -174,18 +150,17 @@ async fn app_begin_block_remove_byzantine_validators() { total_voting_power: 101u32.into(), }; - let mut begin_block = abci::request::BeginBlock { - header: default_tendermint_header(), - hash: Hash::default(), - last_commit_info: CommitInfo { - votes: vec![], - round: Round::default(), - }, + let prepare_state_info = PrepareStateInfo { + app_hash: AppHash::try_from(vec![]).unwrap(), byzantine_validators: vec![misbehavior], + chain_id: "test".to_string().try_into().unwrap(), + height: 1u8.into(), + next_validators_hash: Hash::default(), + proposer_address: account::Id::try_from([0u8; 20].to_vec()).unwrap(), + time: Time::now(), }; - begin_block.header.height = 1u8.into(); - app.begin_block(&begin_block).await.unwrap(); + app.start_block(&prepare_state_info).await.unwrap(); // assert that validator with pubkey_a is removed let validator_set = app.state.get_validator_set().await.unwrap(); @@ -876,7 +851,7 @@ async fn app_process_proposal_transaction_fails_to_execute_fails() { } #[tokio::test] -async fn app_end_block_validator_updates() { +async fn app_handle_post_tx_execution_validator_updates() { let initial_validator_set = vec![ ValidatorUpdate { power: 100, @@ -912,10 +887,13 @@ async fn app_end_block_validator_updates() { .unwrap(); app.apply(state_tx); - let resp = app.end_block(1, &proposer_address).await.unwrap(); + let (validator_updates, _) = app + .component_post_execution_state_updates(&proposer_address) + .await + .unwrap(); // we only assert length here as the ordering of the updates is not guaranteed // and validator::Update does not implement Ord - assert_eq!(resp.validator_updates.len(), validator_updates.len()); + assert_eq!(validator_updates.len(), validator_updates.len()); // validator with pubkey_a should be removed (power set to 0) // validator with pubkey_b should be updated diff --git a/crates/astria-sequencer/src/app/tests_breaking_changes.rs b/crates/astria-sequencer/src/app/tests_breaking_changes.rs index 031538a057..ec15fd9d56 100644 --- a/crates/astria-sequencer/src/app/tests_breaking_changes.rs +++ b/crates/astria-sequencer/src/app/tests_breaking_changes.rs @@ -346,7 +346,9 @@ async fn app_execute_transaction_with_every_action_snapshot() { app.execute_transaction(signed_tx).await.unwrap(); let sudo_address = app.state.get_sudo_address().await.unwrap(); - app.end_block(1, &sudo_address).await.unwrap(); + app.component_post_execution_state_updates(&sudo_address) + .await + .unwrap(); app.prepare_commit(storage.clone()).await.unwrap(); app.commit(storage.clone()).await; diff --git a/crates/astria-sequencer/src/authority/action.rs b/crates/astria-sequencer/src/authority/action.rs index 78d69733eb..5082134fac 100644 --- a/crates/astria-sequencer/src/authority/action.rs +++ b/crates/astria-sequencer/src/authority/action.rs @@ -58,7 +58,7 @@ impl ActionHandler for ValidatorUpdate { ensure!(validator_set.len() != 1, "cannot remove the last validator"); } - // add validator update in non-consensus state to be used in end_block + // add validator update in non-consensus state to be used in handle_post_tx_execution let mut validator_updates = state .get_validator_updates() .await diff --git a/crates/astria-sequencer/src/authority/component.rs b/crates/astria-sequencer/src/authority/component.rs index f6347dcf0c..eb2426bdda 100644 --- a/crates/astria-sequencer/src/authority/component.rs +++ b/crates/astria-sequencer/src/authority/component.rs @@ -9,10 +9,6 @@ use astria_eyre::eyre::{ Result, WrapErr as _, }; -use tendermint::abci::request::{ - BeginBlock, - EndBlock, -}; use tracing::instrument; use super::{ @@ -20,7 +16,10 @@ use super::{ StateWriteExt, ValidatorSet, }; -use crate::component::Component; +use crate::component::{ + Component, + PrepareStateInfo, +}; #[derive(Default)] pub(crate) struct AuthorityComponent; @@ -48,17 +47,17 @@ impl Component for AuthorityComponent { Ok(()) } - #[instrument(name = "AuthorityComponent::begin_block", skip_all)] - async fn begin_block( + #[instrument(name = "AuthorityComponent::prepare_state_for_tx_execution", skip_all)] + async fn prepare_state_for_tx_execution( state: &mut Arc, - begin_block: &BeginBlock, + prepare_state_for_tx_execution: &PrepareStateInfo, ) -> Result<()> { let mut current_set = state .get_validator_set() .await .wrap_err("failed getting validator set")?; - for misbehaviour in &begin_block.byzantine_validators { + for misbehaviour in &prepare_state_for_tx_execution.byzantine_validators { current_set.remove(&misbehaviour.validator.address); } @@ -70,10 +69,9 @@ impl Component for AuthorityComponent { Ok(()) } - #[instrument(name = "AuthorityComponent::end_block", skip_all)] - async fn end_block( + #[instrument(name = "AuthorityComponent::handle_post_tx_execution", skip_all)] + async fn handle_post_tx_execution( state: &mut Arc, - _end_block: &EndBlock, ) -> Result<()> { // update validator set let validator_updates = state diff --git a/crates/astria-sequencer/src/component.rs b/crates/astria-sequencer/src/component.rs index f5e79beafc..e45907237f 100644 --- a/crates/astria-sequencer/src/component.rs +++ b/crates/astria-sequencer/src/component.rs @@ -3,7 +3,25 @@ use std::sync::Arc; use astria_eyre::eyre::Result; use async_trait::async_trait; use cnidarium::StateWrite; -use tendermint::abci; +use tendermint::{ + abci::types, + account, + block, + chain, + AppHash, + Hash, + Time, +}; + +pub(crate) struct PrepareStateInfo { + pub(crate) app_hash: AppHash, + pub(crate) byzantine_validators: Vec, + pub(crate) chain_id: chain::Id, + pub(crate) height: block::Height, + pub(crate) next_validators_hash: Hash, + pub(crate) proposer_address: account::Id, + pub(crate) time: Time, +} /// A component of the Sequencer application. /// Based off Penumbra's [`Component`], but with modifications. @@ -20,8 +38,7 @@ pub(crate) trait Component { /// be empty. async fn init_chain(state: S, app_state: &Self::AppState) -> Result<()>; - /// Begins a new block, optionally inspecting the ABCI - /// [`BeginBlock`](abci::request::BeginBlock) request. + /// Makes necessary state changes for the given component at the start of the block. /// /// # Invariants /// @@ -30,18 +47,17 @@ pub(crate) trait Component { /// called, `state.get_mut().is_some()`, i.e., the `Arc` is not shared. The /// implementor MUST ensure that any clones of the `Arc` are dropped before /// it returns, so that `state.get_mut().is_some()` on completion. - async fn begin_block( + async fn prepare_state_for_tx_execution( state: &mut Arc, - begin_block: &abci::request::BeginBlock, + prepare_state_for_tx_execution: &PrepareStateInfo, ) -> Result<()>; - /// Ends the block, optionally inspecting the ABCI - /// [`EndBlock`](abci::request::EndBlock) request, and performing any batch - /// processing. + /// Handles necessary state changes for the given component after transaction execution, ending + /// the block. /// /// # Invariants /// - /// This method should only be called after [`Component::begin_block`]. + /// This method should only be called after [`Component::prepare_state_for_tx_execution`]. /// No methods should be called following this method. /// /// The `&mut Arc` allows the implementor to optionally share state with @@ -49,8 +65,5 @@ pub(crate) trait Component { /// called, `state.get_mut().is_some()`, i.e., the `Arc` is not shared. The /// implementor MUST ensure that any clones of the `Arc` are dropped before /// it returns, so that `state.get_mut().is_some()` on completion. - async fn end_block( - state: &mut Arc, - end_block: &abci::request::EndBlock, - ) -> Result<()>; + async fn handle_post_tx_execution(state: &mut Arc) -> Result<()>; } diff --git a/crates/astria-sequencer/src/fees/component.rs b/crates/astria-sequencer/src/fees/component.rs index 03ed7cb36c..1c833dc47f 100644 --- a/crates/astria-sequencer/src/fees/component.rs +++ b/crates/astria-sequencer/src/fees/component.rs @@ -5,14 +5,13 @@ use astria_eyre::eyre::{ Result, WrapErr as _, }; -use tendermint::abci::request::{ - BeginBlock, - EndBlock, -}; use tracing::instrument; use crate::{ - component::Component, + component::{ + Component, + PrepareStateInfo, + }, fees, }; @@ -133,18 +132,17 @@ impl Component for FeesComponent { Ok(()) } - #[instrument(name = "FeesComponent::begin_block", skip_all)] - async fn begin_block( + #[instrument(name = "FeesComponent::prepare_state_for_tx_execution", skip_all)] + async fn prepare_state_for_tx_execution( _state: &mut Arc, - _begin_block: &BeginBlock, + _prepare_state_for_tx_execution: &PrepareStateInfo, ) -> Result<()> { Ok(()) } - #[instrument(name = "FeesComponent::end_block", skip_all)] - async fn end_block( + #[instrument(name = "FeesComponent::handle_post_tx_execution", skip_all)] + async fn handle_post_tx_execution( _state: &mut Arc, - _end_block: &EndBlock, ) -> Result<()> { Ok(()) } diff --git a/crates/astria-sequencer/src/ibc/component.rs b/crates/astria-sequencer/src/ibc/component.rs index f8bc969e39..49123881d6 100644 --- a/crates/astria-sequencer/src/ibc/component.rs +++ b/crates/astria-sequencer/src/ibc/component.rs @@ -9,14 +9,20 @@ use penumbra_ibc::{ component::Ibc, genesis::Content, }; -use tendermint::abci::request::{ - BeginBlock, - EndBlock, +use tendermint::{ + abci::{ + self, + }, + block::Header, + Hash, }; use tracing::instrument; use crate::{ - component::Component, + component::{ + Component, + PrepareStateInfo, + }, ibc::{ host_interface::AstriaHost, state_ext::StateWriteExt, @@ -53,21 +59,47 @@ impl Component for IbcComponent { Ok(()) } - #[instrument(name = "IbcComponent::begin_block", skip_all)] - async fn begin_block( + #[instrument(name = "IbcComponent::prepare_state_for_tx_execution", skip_all)] + async fn prepare_state_for_tx_execution( state: &mut Arc, - begin_block: &BeginBlock, + prepare_state_info: &PrepareStateInfo, ) -> Result<()> { - Ibc::begin_block::(state, begin_block).await; + let begin_block: abci::request::BeginBlock = abci::request::BeginBlock { + hash: Hash::default(), + byzantine_validators: prepare_state_info.byzantine_validators.clone(), + header: Header { + app_hash: prepare_state_info.app_hash.clone(), + chain_id: prepare_state_info.chain_id.clone(), + consensus_hash: Hash::default(), + data_hash: Some(Hash::default()), + evidence_hash: Some(Hash::default()), + height: prepare_state_info.height, + last_block_id: None, + last_commit_hash: Some(Hash::default()), + last_results_hash: Some(Hash::default()), + next_validators_hash: prepare_state_info.next_validators_hash, + proposer_address: prepare_state_info.proposer_address, + time: prepare_state_info.time, + validators_hash: Hash::default(), + version: tendermint::block::header::Version { + app: 0, + block: 0, + }, + }, + last_commit_info: tendermint::abci::types::CommitInfo { + round: 0u16.into(), + votes: vec![], + }, + }; + Ibc::begin_block::(state, &begin_block).await; Ok(()) } - #[instrument(name = "IbcComponent::end_block", skip_all)] - async fn end_block( - state: &mut Arc, - end_block: &EndBlock, + #[instrument(name = "IbcComponent::handle_post_tx_execution", skip_all)] + async fn handle_post_tx_execution( + _state: &mut Arc, ) -> Result<()> { - Ibc::end_block(state, end_block).await; + // There is no need to call `Ibc::end_block`. It is a no-op. Ok(()) } }