From 02582ac9ae6d08909f1f389babc5bf002311e598 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Wed, 24 Apr 2024 16:05:05 -0400 Subject: [PATCH 01/11] remove SequencerBlock::try_from_cometbft --- crates/astria-cli/src/commands/rollup.rs | 6 +- crates/astria-cli/src/commands/sequencer.rs | 7 +- .../src/celestia/reporting.rs | 19 --- crates/astria-conductor/src/executor/tests.rs | 65 +++----- .../src/sequencer/reporting.rs | 13 +- .../tests/blackbox/helpers/macros.rs | 7 +- .../tests/blackbox/helpers/mod.rs | 22 ++- crates/astria-core/src/protocol/test_utils.rs | 152 ++++++------------ .../src/sequencerblock/v1alpha1/block.rs | 74 --------- .../src/sequencerblock/v1alpha1/tests.rs | 8 +- .../src/extension_trait.rs | 74 --------- .../tests/blackbox/helper.rs | 32 ++-- crates/astria-sequencer/Cargo.toml | 1 + crates/astria-sequencer/src/api_state_ext.rs | 62 ++----- crates/astria-sequencer/src/grpc/sequencer.rs | 54 ++----- 15 files changed, 140 insertions(+), 456 deletions(-) diff --git a/crates/astria-cli/src/commands/rollup.rs b/crates/astria-cli/src/commands/rollup.rs index 16d72993e0..447f2f085e 100644 --- a/crates/astria-cli/src/commands/rollup.rs +++ b/crates/astria-cli/src/commands/rollup.rs @@ -13,8 +13,8 @@ use std::{ }; use astria_sequencer_client::{ + Client, HttpClient, - SequencerClientExt, }; use color_eyre::{ eyre, @@ -112,11 +112,11 @@ pub(crate) async fn create_config(args: &ConfigCreateArgs) -> eyre::Result<()> { let sequencer_client = HttpClient::new(conf.sequencer_rpc.as_str()) .wrap_err("failed constructing http sequencer client")?; let res = sequencer_client - .latest_sequencer_block() + .latest_block() .await .wrap_err("failed to get sequencer block for initial sequencer height")?; - let new_height: u64 = res.height().into(); + let new_height: u64 = res.block.header.height.into(); conf.sequencer_initial_block_height = Some(new_height); } diff --git a/crates/astria-cli/src/commands/sequencer.rs b/crates/astria-cli/src/commands/sequencer.rs index 2920a300ac..532bad8118 100644 --- a/crates/astria-cli/src/commands/sequencer.rs +++ b/crates/astria-cli/src/commands/sequencer.rs @@ -19,6 +19,7 @@ use astria_sequencer_client::{ tendermint, tendermint_rpc::endpoint, Address, + Client, HttpClient, SequencerClientExt, }; @@ -157,12 +158,12 @@ pub(crate) async fn get_block_height(args: &BlockHeightGetArgs) -> eyre::Result< .wrap_err("failed constructing http sequencer client")?; let res = sequencer_client - .latest_sequencer_block() + .latest_block() .await - .wrap_err("failed to get sequencer block")?; + .wrap_err("failed to get cometbft block")?; println!("Block Height:"); - println!(" {}", res.height()); + println!(" {}", res.block.header.height); Ok(()) } diff --git a/crates/astria-conductor/src/celestia/reporting.rs b/crates/astria-conductor/src/celestia/reporting.rs index 78a2ab2dea..cd16454b6f 100644 --- a/crates/astria-conductor/src/celestia/reporting.rs +++ b/crates/astria-conductor/src/celestia/reporting.rs @@ -10,25 +10,6 @@ use super::{ ReconstructedBlock, ReconstructedBlocks, }; -use crate::block_cache::GetSequencerHeight; - -pub(super) struct ReportSequencerHeights<'a, T>(pub(super) &'a [T]); - -impl<'a, T> Serialize for ReportSequencerHeights<'a, T> -where - T: GetSequencerHeight, -{ - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - let mut seq = serializer.serialize_seq(Some(self.0.len()))?; - for elem in self.0 { - seq.serialize_element(&elem.get_height())?; - } - seq.end() - } -} pub(super) struct ReportReconstructedBlocks<'a>(pub(super) &'a ReconstructedBlocks); impl<'a> Serialize for ReportReconstructedBlocks<'a> { diff --git a/crates/astria-conductor/src/executor/tests.rs b/crates/astria-conductor/src/executor/tests.rs index c035f61205..f0756fcb88 100644 --- a/crates/astria-conductor/src/executor/tests.rs +++ b/crates/astria-conductor/src/executor/tests.rs @@ -30,14 +30,8 @@ use astria_core::{ }, sequencerblock::v1alpha1::RollupData as RawRollupData, }, - protocol::test_utils::{ - make_cometbft_block, - ConfigureCometBftBlock, - }, - sequencerblock::v1alpha1::{ - block::RollupData, - SequencerBlock, - }, + protocol::test_utils::ConfigureSequencerBlock, + sequencerblock::v1alpha1::block::RollupData, Protobuf as _, }; use bytes::Bytes; @@ -215,12 +209,11 @@ fn get_expected_execution_hash( } fn make_reconstructed_block(height: u32) -> ReconstructedBlock { - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height, ..Default::default() } .make(); - let block = SequencerBlock::try_from_cometbft(block).unwrap(); ReconstructedBlock { block_hash: block.block_hash(), @@ -316,11 +309,12 @@ async fn firm_blocks_at_expected_heights_are_executed() { async fn soft_blocks_at_expected_heights_are_executed() { let mut mock = start_mock().await; - let mut block = make_cometbft_block(); - block.header.height = SequencerHeight::from(100u32); - let block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let block = ConfigureSequencerBlock { + height: 100, + ..Default::default() + } + .make(); + let block = block.into_filtered_block([ROLLUP_ID]); assert!( mock.executor .execute_soft(mock.client.clone(), block) @@ -328,11 +322,12 @@ async fn soft_blocks_at_expected_heights_are_executed() { .is_ok() ); - let mut block = make_cometbft_block(); - block.header.height = SequencerHeight::from(101u32); - let block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let block = ConfigureSequencerBlock { + height: 101, + ..Default::default() + } + .make(); + let block = block.into_filtered_block([ROLLUP_ID]); mock.executor .execute_soft(mock.client.clone(), block) .await @@ -347,15 +342,13 @@ async fn soft_blocks_at_expected_heights_are_executed() { async fn first_firm_then_soft_leads_to_soft_being_dropped() { let mut mock = start_mock().await; - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 100, rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); - let soft_block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let soft_block = block.into_filtered_block([ROLLUP_ID]); let block_hash = soft_block.block_hash(); @@ -402,15 +395,13 @@ async fn first_firm_then_soft_leads_to_soft_being_dropped() { async fn first_soft_then_firm_update_state_correctly() { let mut mock = start_mock().await; - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 100, rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); - let soft_block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let soft_block = block.into_filtered_block([ROLLUP_ID]); let block_hash = soft_block.block_hash(); @@ -454,15 +445,13 @@ async fn first_soft_then_firm_update_state_correctly() { #[tokio::test] async fn old_soft_blocks_are_ignored() { let mut mock = start_mock().await; - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 99, rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); - let sequencer_block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let sequencer_block = block.into_filtered_block([ROLLUP_ID]); let firm = mock.executor.state.borrow().firm().clone(); let soft = mock.executor.state.borrow().soft().clone(); @@ -488,15 +477,13 @@ async fn old_soft_blocks_are_ignored() { async fn non_sequential_future_soft_blocks_give_error() { let mut mock = start_mock().await; - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 101, rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); - let sequencer_block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let sequencer_block = block.into_filtered_block([ROLLUP_ID]); assert!( mock.executor .execute_soft(mock.client.clone(), sequencer_block) @@ -504,15 +491,13 @@ async fn non_sequential_future_soft_blocks_give_error() { .is_err() ); - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 100, rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); - let sequencer_block = SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_ID]); + let sequencer_block = block.into_filtered_block([ROLLUP_ID]); assert!( mock.executor .execute_soft(mock.client.clone(), sequencer_block) diff --git a/crates/astria-conductor/src/sequencer/reporting.rs b/crates/astria-conductor/src/sequencer/reporting.rs index eac1e408c3..466e7504f2 100644 --- a/crates/astria-conductor/src/sequencer/reporting.rs +++ b/crates/astria-conductor/src/sequencer/reporting.rs @@ -44,11 +44,8 @@ impl<'a> Serialize for ReportRollups<'a> { mod tests { use astria_core::{ primitive::v1::RollupId, - protocol::test_utils::ConfigureCometBftBlock, - sequencerblock::v1alpha1::block::{ - FilteredSequencerBlock, - SequencerBlock, - }, + protocol::test_utils::ConfigureSequencerBlock, + sequencerblock::v1alpha1::block::FilteredSequencerBlock, }; use insta::assert_json_snapshot; @@ -61,7 +58,7 @@ mod tests { const ROLLUP_69: RollupId = RollupId::new([69u8; 32]); fn snapshot_block() -> FilteredSequencerBlock { - let block = ConfigureCometBftBlock { + let block = ConfigureSequencerBlock { height: 100, rollup_transactions: vec![ (ROLLUP_42, b"hello".to_vec()), @@ -72,9 +69,7 @@ mod tests { } .make(); - SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([ROLLUP_42, ROLLUP_69]) + block.into_filtered_block([ROLLUP_42, ROLLUP_69]) } #[test] diff --git a/crates/astria-conductor/tests/blackbox/helpers/macros.rs b/crates/astria-conductor/tests/blackbox/helpers/macros.rs index 20500d516c..386f294093 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/macros.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/macros.rs @@ -77,16 +77,13 @@ macro_rules! commitment_state { #[macro_export] macro_rules! filtered_sequencer_block { (sequencer_height: $height:expr) => {{ - let block = ::astria_core::protocol::test_utils::ConfigureCometBftBlock { + let block = ::astria_core::protocol::test_utils::ConfigureSequencerBlock { height: $height, rollup_transactions: vec![($crate::ROLLUP_ID, $crate::helpers::data())], ..Default::default() } .make(); - ::astria_core::sequencerblock::v1alpha1::SequencerBlock::try_from_cometbft(block) - .unwrap() - .into_filtered_block([$crate::ROLLUP_ID]) - .into_raw() + block.into_filtered_block([$crate::ROLLUP_ID]).into_raw() }}; } diff --git a/crates/astria-conductor/tests/blackbox/helpers/mod.rs b/crates/astria-conductor/tests/blackbox/helpers/mod.rs index 3173bad497..5243a9780b 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/mod.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/mod.rs @@ -426,18 +426,16 @@ fn make_config() -> Config { #[must_use] pub fn make_sequencer_block(height: u32) -> astria_core::sequencerblock::v1alpha1::SequencerBlock { - astria_core::sequencerblock::v1alpha1::SequencerBlock::try_from_cometbft( - astria_core::protocol::test_utils::ConfigureCometBftBlock { - chain_id: Some(crate::SEQUENCER_CHAIN_ID.to_string()), - height, - rollup_transactions: vec![(crate::ROLLUP_ID, data())], - unix_timestamp: (1i64, 1u32).into(), - signing_key: Some(signing_key()), - proposer_address: None, - } - .make(), - ) - .unwrap() + astria_core::protocol::test_utils::ConfigureSequencerBlock { + chain_id: Some(crate::SEQUENCER_CHAIN_ID.to_string()), + height, + rollup_transactions: vec![(crate::ROLLUP_ID, data())], + unix_timestamp: (1i64, 1u32).into(), + signing_key: Some(signing_key()), + proposer_address: None, + ..Default::default() + } + .make() } pub struct Blobs { diff --git a/crates/astria-core/src/protocol/test_utils.rs b/crates/astria-core/src/protocol/test_utils.rs index 736c2bf0b8..5cd0c59c8a 100644 --- a/crates/astria-core/src/protocol/test_utils.rs +++ b/crates/astria-core/src/protocol/test_utils.rs @@ -1,5 +1,7 @@ //! Utilities to create objects used in various tests of the Astria codebase. +use std::collections::HashMap; + use prost::Message as _; use super::{ @@ -10,28 +12,18 @@ use super::{ UnsignedTransaction, }, }; -use crate::primitive::v1::{ - asset::default_native_asset_id, - derive_merkle_tree_from_rollup_txs, - RollupId, +use crate::{ + primitive::v1::{ + asset::default_native_asset_id, + derive_merkle_tree_from_rollup_txs, + RollupId, + }, + sequencerblock::v1alpha1::{ + block::Deposit, + SequencerBlock, + }, }; -/// Create a Comet BFT block. -/// -/// If you don't really care what's in the block, you just need it to be a valid block. -#[must_use] -pub fn make_cometbft_block() -> tendermint::Block { - let height = 1; - let rollup_id = RollupId::from_unhashed_bytes(b"test_chain_id_1"); - let data = b"hello_world_id_1".to_vec(); - ConfigureCometBftBlock { - height, - rollup_transactions: vec![(rollup_id, data)], - ..Default::default() - } - .make() -} - #[derive(Default)] pub struct UnixTimeStamp { pub secs: i64, @@ -52,38 +44,40 @@ impl From<(i64, u32)> for UnixTimeStamp { /// /// If the proposer address is not set it will be generated from the signing key. #[derive(Default)] -pub struct ConfigureCometBftBlock { +pub struct ConfigureSequencerBlock { + pub block_hash: Option<[u8; 32]>, pub chain_id: Option, pub height: u32, pub proposer_address: Option, pub signing_key: Option, pub rollup_transactions: Vec<(RollupId, Vec)>, + pub deposits: Vec, pub unix_timestamp: UnixTimeStamp, } -impl ConfigureCometBftBlock { +impl ConfigureSequencerBlock { /// Construct a Comet BFT block with the configured parameters. #[must_use] #[allow(clippy::missing_panics_doc)] // This should only be used in tests, so everything here is unwrapped - pub fn make(self) -> tendermint::Block { - use sha2::Digest as _; - use tendermint::{ - block, - evidence, - hash::AppHash, - merkle::simple_hash_from_byte_vectors, - Hash, - Time, - }; + pub fn make(self) -> SequencerBlock { + use tendermint::Time; + + use crate::sequencerblock::v1alpha1::block::RollupData; + let Self { + block_hash, chain_id, height, signing_key, proposer_address, rollup_transactions, unix_timestamp, + deposits, } = self; + let block_hash = block_hash.unwrap_or_default(); + let chain_id = chain_id.unwrap_or_else(|| "test".to_string()); + let signing_key = signing_key.unwrap_or_else(|| ed25519_consensus::SigningKey::new(rand::rngs::OsRng)); @@ -112,11 +106,28 @@ impl ConfigureCometBftBlock { }, }; + let mut deposits_map: HashMap> = HashMap::new(); + deposits.into_iter().for_each(|deposit| { + if let Some(entry) = deposits_map.get_mut(deposit.rollup_id()) { + entry.push(deposit); + } else { + deposits_map.insert(deposit.rollup_id().clone(), vec![deposit]); + } + }); + let signed_transaction = unsigned_transaction.into_signed(&signing_key); - let rollup_transactions = + let mut rollup_transactions = group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(&[ signed_transaction.clone(), ]); + for (rollup_id, deposit) in deposits_map.clone() { + rollup_transactions.entry(rollup_id).or_default().extend( + deposit + .into_iter() + .map(|deposit| RollupData::Deposit(deposit).into_raw().encode_to_vec()), + ); + } + rollup_transactions.sort_unstable_keys(); let rollup_transactions_tree = derive_merkle_tree_from_rollup_txs(&rollup_transactions); let rollup_ids_root = merkle::Tree::from_leaves( @@ -130,75 +141,16 @@ impl ConfigureCometBftBlock { rollup_ids_root.to_vec(), signed_transaction.into_raw().encode_to_vec(), ]; - let data_hash = Some(Hash::Sha256(simple_hash_from_byte_vectors::( - &data.iter().map(sha2::Sha256::digest).collect::>(), - ))); - - let (last_commit_hash, last_commit) = make_test_commit_and_hash(); - - tendermint::Block::new( - block::Header { - version: block::header::Version { - block: 0, - app: 0, - }, - chain_id: chain_id - .unwrap_or_else(|| "test".to_string()) - .try_into() - .unwrap(), - height: block::Height::from(height), - time: Time::from_unix_timestamp(unix_timestamp.secs, unix_timestamp.nanos).unwrap(), - last_block_id: None, - last_commit_hash: (height > 1).then_some(last_commit_hash), - data_hash, - validators_hash: Hash::Sha256([0; 32]), - next_validators_hash: Hash::Sha256([0; 32]), - consensus_hash: Hash::Sha256([0; 32]), - app_hash: AppHash::try_from([0; 32].to_vec()).unwrap(), - last_results_hash: None, - evidence_hash: None, - proposer_address, - }, + + SequencerBlock::try_from_block_info_and_data( + block_hash, + chain_id.try_into().unwrap(), + height.into(), + Time::from_unix_timestamp(unix_timestamp.secs, unix_timestamp.nanos).unwrap(), + proposer_address, data, - evidence::List::default(), - // The first height must not, every height after must contain a last commit - (height > 1).then_some(last_commit), + deposits_map, ) .unwrap() } } - -// Returns a tendermint commit and hash for testing purposes. -#[must_use] -pub fn make_test_commit_and_hash() -> (tendermint::Hash, tendermint::block::Commit) { - let commit = tendermint::block::Commit { - height: 1u32.into(), - ..Default::default() - }; - (calculate_last_commit_hash(&commit), commit) -} - -// Calculates the `last_commit_hash` given a Tendermint [`Commit`]. -// -// It merkleizes the commit and returns the root. The leaves of the merkle tree -// are the protobuf-encoded [`CommitSig`]s; ie. the signatures that the commit consist of. -// -// See https://github.com/cometbft/cometbft/blob/539985efc7d461668ffb46dff88b3f7bb9275e5a/types/block.go#L922 -#[must_use] -fn calculate_last_commit_hash(commit: &tendermint::block::Commit) -> tendermint::Hash { - use prost::Message as _; - use tendermint::{ - crypto, - merkle, - }; - use tendermint_proto::types::CommitSig; - - let signatures = commit - .signatures - .iter() - .map(|commit_sig| CommitSig::from(commit_sig.clone()).encode_to_vec()) - .collect::>(); - tendermint::Hash::Sha256(merkle::simple_hash_from_byte_vectors::< - crypto::default::Sha256, - >(&signatures)) -} diff --git a/crates/astria-core/src/sequencerblock/v1alpha1/block.rs b/crates/astria-core/src/sequencerblock/v1alpha1/block.rs index 687e197f41..60f0c41afd 100644 --- a/crates/astria-core/src/sequencerblock/v1alpha1/block.rs +++ b/crates/astria-core/src/sequencerblock/v1alpha1/block.rs @@ -172,14 +172,6 @@ impl SequencerBlockError { Self(SequencerBlockErrorKind::InvalidBlockHash(length)) } - fn comet_bft_data_hash_does_not_match_reconstructed() -> Self { - Self(SequencerBlockErrorKind::CometBftDataHashDoesNotMatchReconstructed) - } - - fn comet_bft_block_hash_is_none() -> Self { - Self(SequencerBlockErrorKind::CometBftBlockHashIsNone) - } - fn field_not_set(field: &'static str) -> Self { Self(SequencerBlockErrorKind::FieldNotSet(field)) } @@ -257,13 +249,6 @@ impl SequencerBlockError { enum SequencerBlockErrorKind { #[error("the block hash was expected to be 32 bytes long, but was actually `{0}`")] InvalidBlockHash(usize), - #[error( - "the CometBFT block.header.data_hash does not match the Merkle Tree Hash derived from \ - block.data" - )] - CometBftDataHashDoesNotMatchReconstructed, - #[error("hashing the CometBFT block.header returned an empty hash which is not permitted")] - CometBftBlockHashIsNone, #[error("the expected field in the raw source type was not set: `{0}`")] FieldNotSet(&'static str), #[error("failed constructing a sequencer block header from the raw protobuf header")] @@ -706,65 +691,6 @@ impl SequencerBlock { celestia::CelestiaBlobBundle::from_sequencer_block(self).into_parts() } - /// Converts from a [`tendermint::Block`]. - /// - /// # Errors - /// TODO(https://github.com/astriaorg/astria/issues/612) - #[allow(clippy::missing_panics_doc)] // the panic sources are checked before hand; revisit if refactoring - pub fn try_from_cometbft(block: tendermint::Block) -> Result { - let tendermint::Block { - header, - data, - .. - } = block; - - // TODO: see https://github.com/astriaorg/astria/issues/774#issuecomment-1981584681 - // deposits are not included in a block pulled from cometbft, so they don't match what's - // stored in the sequencer any more. - // this function can be removed after relayer/conductor are updated to use the sequencer - // API. - Self::try_from_cometbft_header_and_data(header, data, HashMap::new()) - } - - /// Converts from a [`tendermint::block::Header`] and the block data. - /// - /// # Errors - /// TODO(https://github.com/astriaorg/astria/issues/612) - /// - /// # Panics - /// - /// - if a rollup data merkle proof cannot be constructed. - pub fn try_from_cometbft_header_and_data( - cometbft_header: tendermint::block::Header, - data: Vec>, - deposits: HashMap>, - ) -> Result { - let Some(tendermint::Hash::Sha256(data_hash)) = cometbft_header.data_hash else { - // header.data_hash is Option and Hash itself has - // variants Sha256([u8; 32]) or None. - return Err(SequencerBlockError::field_not_set("header.data_hash")); - }; - - let tendermint::Hash::Sha256(block_hash) = cometbft_header.hash() else { - return Err(SequencerBlockError::comet_bft_block_hash_is_none()); - }; - - let tree = merkle_tree_from_data(&data); - if tree.root() != data_hash { - return Err(SequencerBlockError::comet_bft_data_hash_does_not_match_reconstructed()); - } - - Self::try_from_block_info_and_data( - block_hash, - cometbft_header.chain_id, - cometbft_header.height, - cometbft_header.time, - cometbft_header.proposer_address, - data, - deposits, - ) - } - /// Converts from relevant header fields and the block data. /// /// # Errors diff --git a/crates/astria-core/src/sequencerblock/v1alpha1/tests.rs b/crates/astria-core/src/sequencerblock/v1alpha1/tests.rs index ae0db78694..eb9a16c156 100644 --- a/crates/astria-core/src/sequencerblock/v1alpha1/tests.rs +++ b/crates/astria-core/src/sequencerblock/v1alpha1/tests.rs @@ -1,12 +1,11 @@ use sha2::Digest as _; use super::*; -use crate::protocol::transaction::test_utils::make_cometbft_block; +use crate::protocol::transaction::test_utils::ConfigureSequencerBlock; #[test] fn sequencer_block_from_cometbft_block_gives_expected_merkle_proofs() { - let block = make_cometbft_block(); - let sequencer_block = SequencerBlock::try_from_cometbft(block).unwrap(); + let sequencer_block = ConfigureSequencerBlock::default().make(); let rollup_ids_root = merkle::Tree::from_leaves(sequencer_block.rollup_transactions.keys()).root(); @@ -50,8 +49,7 @@ fn sequencer_block_from_cometbft_block_gives_expected_merkle_proofs() { #[test] fn block_to_filtered_roundtrip() { - let block = make_cometbft_block(); - let sequencer_block = SequencerBlock::try_from_cometbft(block).unwrap(); + let sequencer_block = ConfigureSequencerBlock::default().make(); let rollup_ids = sequencer_block.rollup_transactions.keys(); let filtered_sequencer_block = sequencer_block.to_filtered_block(rollup_ids); diff --git a/crates/astria-sequencer-client/src/extension_trait.rs b/crates/astria-sequencer-client/src/extension_trait.rs index 0a17915560..d55670d94b 100644 --- a/crates/astria-sequencer-client/src/extension_trait.rs +++ b/crates/astria-sequencer-client/src/extension_trait.rs @@ -92,7 +92,6 @@ impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self.inner { ErrorKind::AbciQueryDeserialization(e) => Some(e), - ErrorKind::CometBftConversion(e) => Some(e), ErrorKind::TendermintRpc(e) => Some(e), } } @@ -124,12 +123,6 @@ impl Error { } } - fn cometbft_conversion(e: SequencerBlockError) -> Self { - Self { - inner: ErrorKind::CometBftConversion(e), - } - } - /// Convenience function to construct `Error` containing a `TendermintRpcError`. fn tendermint_rpc(rpc: &'static str, inner: tendermint_rpc::error::Error) -> Self { Self { @@ -254,7 +247,6 @@ impl std::error::Error for DeserializationError { #[derive(Debug)] pub enum ErrorKind { AbciQueryDeserialization(AbciQueryDeserializationError), - CometBftConversion(SequencerBlockError), TendermintRpc(TendermintRpcError), } @@ -354,45 +346,6 @@ impl Stream for LatestHeightStream { #[async_trait] pub trait SequencerSubscriptionClientExt: SubscriptionClient { - /// Subscribe to sequencer blocks from cometbft, returning a stream. - /// - /// This trait method calls the cometbft `/subscribe` endpoint with a - /// `tm.event = 'NewBlock'` argument, and then attempts to convert each - /// cometbft block to a [`SequencerBlock`] type. - async fn subscribe_new_block_data(&self) -> Result { - use futures::stream::{ - StreamExt as _, - TryStreamExt as _, - }; - use tendermint_rpc::query::{ - EventType, - Query, - }; - let stream = self - .subscribe(Query::from(EventType::NewBlock)) - .await? - .map_err(NewBlockStreamError::Rpc) - .and_then(|event| { - future::ready(match event.data { - EventData::LegacyNewBlock { - block: Some(block), - .. - } => SequencerBlock::try_from_cometbft(*block) - .map_err(NewBlockStreamError::CometBftConversion), - - EventData::LegacyNewBlock { - block: None, .. - } => Err(NewBlockStreamError::NoBlock), - - other => Err(NewBlockStreamError::unexpected_event(&other)), - }) - }) - .boxed(); - Ok(NewBlocksStream { - inner: stream, - }) - } - async fn subscribe_latest_height(&self) -> Result { use futures::stream::{ StreamExt as _, @@ -532,33 +485,6 @@ pub trait SequencerClientExt: Client { self.get_nonce(address, 0u32).await } - /// Get the latest sequencer block. - /// - /// This is a convenience method that converts the result [`Client::latest_block`] - /// to [`SequencerBlock`]. - async fn latest_sequencer_block(&self) -> Result { - let rsp = self - .latest_block() - .await - .map_err(|e| Error::tendermint_rpc("latest_block", e))?; - SequencerBlock::try_from_cometbft(rsp.block).map_err(Error::cometbft_conversion) - } - - /// Get the sequencer block at the provided height. - /// - /// This is a convenience method that converts the result of calling [`Client::block`] - /// to an Astria [`SequencerBlock`]. - async fn sequencer_block(&self, height: HeightT) -> Result - where - HeightT: Into + Send, - { - let rsp = self - .block(height.into()) - .await - .map_err(|e| Error::tendermint_rpc("block", e))?; - SequencerBlock::try_from_cometbft(rsp.block).map_err(Error::cometbft_conversion) - } - /// Submits the given transaction to the Sequencer node. /// /// This method blocks until the transaction is checked, but not until it's committed. diff --git a/crates/astria-sequencer-relayer/tests/blackbox/helper.rs b/crates/astria-sequencer-relayer/tests/blackbox/helper.rs index 37f2562610..e0c92784b2 100644 --- a/crates/astria-sequencer-relayer/tests/blackbox/helper.rs +++ b/crates/astria-sequencer-relayer/tests/blackbox/helper.rs @@ -21,8 +21,7 @@ use astria_core::{ GetSequencerBlockRequest, SequencerBlock as RawSequencerBlock, }, - protocol::test_utils::make_cometbft_block, - sequencerblock::v1alpha1::SequencerBlock, + protocol::test_utils::ConfigureSequencerBlock, }; use astria_sequencer_relayer::{ config::Config, @@ -237,13 +236,14 @@ impl TestSequencerRelayer { tendermint::account::Id::try_from(vec![0u8; 20]).unwrap() }; - let mut cometbft_block = make_cometbft_block(); - cometbft_block.header.height = height.into(); - cometbft_block.header.proposer_address = proposer; - let (tx, rx) = oneshot::channel(); - let block = SequencerBlock::try_from_cometbft(cometbft_block).unwrap(); + let block = ConfigureSequencerBlock { + height, + proposer_address: Some(proposer), + ..Default::default() + } + .make(); let mut blocks = self.sequencer_server_blocks.lock().unwrap(); blocks.push_back((tx, block.into_raw())); BlockGuard { @@ -258,19 +258,19 @@ impl TestSequencerRelayer { tendermint::account::Id::try_from(vec![0u8; 20]).unwrap() }; - let mut cometbft_block = make_cometbft_block(); - cometbft_block.header.height = height.into(); - cometbft_block.header.proposer_address = proposer; - let (tx, rx) = oneshot::channel(); - let mut block = SequencerBlock::try_from_cometbft(cometbft_block) - .unwrap() - .into_raw(); + let block = ConfigureSequencerBlock { + height, + proposer_address: Some(proposer), + ..Default::default() + } + .make(); + let mut block = block.into_raw(); // make the block bad!! - let cometbft_header = block.header.as_mut().unwrap(); - cometbft_header.data_hash = [0; 32].to_vec(); + let header = block.header.as_mut().unwrap(); + header.data_hash = [0; 32].to_vec(); let mut blocks = self.sequencer_server_blocks.lock().unwrap(); blocks.push_back((tx, block)); diff --git a/crates/astria-sequencer/Cargo.toml b/crates/astria-sequencer/Cargo.toml index 941440dbdd..6f1de24a65 100644 --- a/crates/astria-sequencer/Cargo.toml +++ b/crates/astria-sequencer/Cargo.toml @@ -59,6 +59,7 @@ ibc-proto = { version = "0.41.0", features = ["server"] } tower-http = { version = "0.4", features = ["cors"] } [dev-dependencies] +astria-core = { path = "../astria-core", features = ["server", "serde", "test-utils"] } config = { package = "astria-config", path = "../astria-config", features = [ "tests", ] } diff --git a/crates/astria-sequencer/src/api_state_ext.rs b/crates/astria-sequencer/src/api_state_ext.rs index f569a46b1e..e9cabb1f2e 100644 --- a/crates/astria-sequencer/src/api_state_ext.rs +++ b/crates/astria-sequencer/src/api_state_ext.rs @@ -382,63 +382,26 @@ impl StateWriteExt for T {} #[cfg(test)] mod test { - - use std::collections::HashMap; - use astria_core::{ primitive::v1::{ asset::Id, Address, }, + protocol::test_utils::ConfigureSequencerBlock, sequencerblock::v1alpha1::block::Deposit, }; use cnidarium::StateDelta; use rand::Rng; - use sha2::{ - Digest as _, - Sha256, - }; - use tendermint::{ - account, - block::{ - header::Version, - Header, - }, - AppHash, - Hash, - Time, - }; use super::*; - use crate::proposal::commitment::generate_rollup_datas_commitment; // creates new sequencer block, optionally shifting all values except the height by 1 fn make_test_sequencer_block(height: u32) -> SequencerBlock { let mut rng = rand::thread_rng(); - - // create cometbft header - let mut header = Header { - app_hash: AppHash::default(), - chain_id: "test".to_string().try_into().unwrap(), - consensus_hash: Hash::default(), - data_hash: Some(Hash::default()), - evidence_hash: None, - height: height.into(), - last_block_id: None, - last_commit_hash: None, - last_results_hash: None, - 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, - }, - }; + let block_hash: [u8; 32] = rng.gen(); // create inner rollup id/tx data - let mut deposits = HashMap::new(); + let mut deposits = vec![]; for _ in 0..2 { let rollup_id = RollupId::new(rng.gen()); let bridge_address = Address::try_from_slice(&[rng.gen(); 20]).unwrap(); @@ -452,19 +415,16 @@ mod test { asset_id, destination_chain_address, ); - deposits.insert(rollup_id, vec![deposit]); + deposits.push(deposit); } - // create block's other data - let commitments = generate_rollup_datas_commitment(&[], deposits.clone()); - let block_data = vec![ - commitments.rollup_datas_root.to_vec(), - commitments.rollup_ids_root.to_vec(), - ]; - let data_hash = merkle::Tree::from_leaves(block_data.iter().map(Sha256::digest)).root(); - header.data_hash = Some(Hash::try_from(data_hash.to_vec()).unwrap()); - header.height = height.into(); - SequencerBlock::try_from_cometbft_header_and_data(header, block_data, deposits).unwrap() + ConfigureSequencerBlock { + block_hash: Some(block_hash), + height: height.into(), + deposits, + ..Default::default() + } + .make() } #[tokio::test] diff --git a/crates/astria-sequencer/src/grpc/sequencer.rs b/crates/astria-sequencer/src/grpc/sequencer.rs index 0ae42efd4c..93469cb88c 100644 --- a/crates/astria-sequencer/src/grpc/sequencer.rs +++ b/crates/astria-sequencer/src/grpc/sequencer.rs @@ -156,25 +156,11 @@ impl SequencerService for SequencerServer { #[cfg(test)] mod test { - use std::collections::HashMap; - - use astria_core::sequencerblock::v1alpha1::SequencerBlock; - use cnidarium::StateDelta; - use sha2::{ - Digest as _, - Sha256, - }; - use tendermint::{ - account, - block::{ - header::Version, - Header, - Height, - }, - AppHash, - Hash, - Time, + use astria_core::{ + protocol::test_utils::ConfigureSequencerBlock, + sequencerblock::v1alpha1::SequencerBlock, }; + use cnidarium::StateDelta; use super::*; use crate::{ @@ -183,33 +169,11 @@ mod test { }; fn make_test_sequencer_block(height: u32) -> SequencerBlock { - let mut 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, - }, - }; - - let empty_hash = merkle::Tree::from_leaves(Vec::>::new()).root(); - let block_data = vec![empty_hash.to_vec(), empty_hash.to_vec()]; - let data_hash = merkle::Tree::from_leaves(block_data.iter().map(Sha256::digest)).root(); - header.data_hash = Some(Hash::try_from(data_hash.to_vec()).unwrap()); - header.height = height.into(); - SequencerBlock::try_from_cometbft_header_and_data(header, block_data, HashMap::new()) - .unwrap() + ConfigureSequencerBlock { + height, + ..Default::default() + } + .make() } #[tokio::test] From 850b8f1d617677a4d33b76ff425e7f030e15d4b4 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Wed, 24 Apr 2024 16:26:41 -0400 Subject: [PATCH 02/11] fix relayer tests --- Cargo.lock | 1 + crates/astria-conductor/src/executor/tests.rs | 10 ++--- .../src/sequencer/reporting.rs | 2 +- .../tests/blackbox/helpers/macros.rs | 2 +- .../tests/blackbox/helpers/mod.rs | 2 +- crates/astria-core/src/protocol/test_utils.rs | 37 +++++++++++-------- crates/astria-sequencer-relayer/Cargo.toml | 1 + .../tests/blackbox/helper.rs | 11 ++++++ 8 files changed, 42 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 352d47b3c9..cda2bcf5ba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -835,6 +835,7 @@ dependencies = [ "once_cell", "pin-project-lite", "prost", + "rand 0.8.5", "rand_core 0.6.4", "reqwest", "serde", diff --git a/crates/astria-conductor/src/executor/tests.rs b/crates/astria-conductor/src/executor/tests.rs index f0756fcb88..88e96350d0 100644 --- a/crates/astria-conductor/src/executor/tests.rs +++ b/crates/astria-conductor/src/executor/tests.rs @@ -344,7 +344,7 @@ async fn first_firm_then_soft_leads_to_soft_being_dropped() { let block = ConfigureSequencerBlock { height: 100, - rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], + sequence_data: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); @@ -397,7 +397,7 @@ async fn first_soft_then_firm_update_state_correctly() { let block = ConfigureSequencerBlock { height: 100, - rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], + sequence_data: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); @@ -447,7 +447,7 @@ async fn old_soft_blocks_are_ignored() { let mut mock = start_mock().await; let block = ConfigureSequencerBlock { height: 99, - rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], + sequence_data: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); @@ -479,7 +479,7 @@ async fn non_sequential_future_soft_blocks_give_error() { let block = ConfigureSequencerBlock { height: 101, - rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], + sequence_data: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); @@ -493,7 +493,7 @@ async fn non_sequential_future_soft_blocks_give_error() { let block = ConfigureSequencerBlock { height: 100, - rollup_transactions: vec![(ROLLUP_ID, b"hello_world".to_vec())], + sequence_data: vec![(ROLLUP_ID, b"hello_world".to_vec())], ..Default::default() } .make(); diff --git a/crates/astria-conductor/src/sequencer/reporting.rs b/crates/astria-conductor/src/sequencer/reporting.rs index 466e7504f2..285d109a05 100644 --- a/crates/astria-conductor/src/sequencer/reporting.rs +++ b/crates/astria-conductor/src/sequencer/reporting.rs @@ -60,7 +60,7 @@ mod tests { fn snapshot_block() -> FilteredSequencerBlock { let block = ConfigureSequencerBlock { height: 100, - rollup_transactions: vec![ + sequence_data: vec![ (ROLLUP_42, b"hello".to_vec()), (ROLLUP_42, b"hello world".to_vec()), (ROLLUP_69, b"hello world".to_vec()), diff --git a/crates/astria-conductor/tests/blackbox/helpers/macros.rs b/crates/astria-conductor/tests/blackbox/helpers/macros.rs index 386f294093..8d0f306e4a 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/macros.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/macros.rs @@ -79,7 +79,7 @@ macro_rules! filtered_sequencer_block { (sequencer_height: $height:expr) => {{ let block = ::astria_core::protocol::test_utils::ConfigureSequencerBlock { height: $height, - rollup_transactions: vec![($crate::ROLLUP_ID, $crate::helpers::data())], + sequence_data: vec![($crate::ROLLUP_ID, $crate::helpers::data())], ..Default::default() } .make(); diff --git a/crates/astria-conductor/tests/blackbox/helpers/mod.rs b/crates/astria-conductor/tests/blackbox/helpers/mod.rs index 5243a9780b..a477bb8496 100644 --- a/crates/astria-conductor/tests/blackbox/helpers/mod.rs +++ b/crates/astria-conductor/tests/blackbox/helpers/mod.rs @@ -429,7 +429,7 @@ pub fn make_sequencer_block(height: u32) -> astria_core::sequencerblock::v1alpha astria_core::protocol::test_utils::ConfigureSequencerBlock { chain_id: Some(crate::SEQUENCER_CHAIN_ID.to_string()), height, - rollup_transactions: vec![(crate::ROLLUP_ID, data())], + sequence_data: vec![(crate::ROLLUP_ID, data())], unix_timestamp: (1i64, 1u32).into(), signing_key: Some(signing_key()), proposer_address: None, diff --git a/crates/astria-core/src/protocol/test_utils.rs b/crates/astria-core/src/protocol/test_utils.rs index 5cd0c59c8a..0d0b1f83e3 100644 --- a/crates/astria-core/src/protocol/test_utils.rs +++ b/crates/astria-core/src/protocol/test_utils.rs @@ -50,7 +50,7 @@ pub struct ConfigureSequencerBlock { pub height: u32, pub proposer_address: Option, pub signing_key: Option, - pub rollup_transactions: Vec<(RollupId, Vec)>, + pub sequence_data: Vec<(RollupId, Vec)>, pub deposits: Vec, pub unix_timestamp: UnixTimeStamp, } @@ -62,7 +62,10 @@ impl ConfigureSequencerBlock { pub fn make(self) -> SequencerBlock { use tendermint::Time; - use crate::sequencerblock::v1alpha1::block::RollupData; + use crate::{ + protocol::transaction::v1alpha1::Action, + sequencerblock::v1alpha1::block::RollupData, + }; let Self { block_hash, @@ -70,7 +73,7 @@ impl ConfigureSequencerBlock { height, signing_key, proposer_address, - rollup_transactions, + sequence_data, unix_timestamp, deposits, } = self; @@ -87,7 +90,7 @@ impl ConfigureSequencerBlock { tendermint::account::Id::from(public_key) }); - let actions = rollup_transactions + let actions: Vec = sequence_data .into_iter() .map(|(rollup_id, data)| { SequenceAction { @@ -98,12 +101,17 @@ impl ConfigureSequencerBlock { .into() }) .collect(); - let unsigned_transaction = UnsignedTransaction { - actions, - params: TransactionParams { - nonce: 1, - chain_id: "test-1".to_string(), - }, + let txs = if !actions.is_empty() { + let unsigned_transaction = UnsignedTransaction { + actions, + params: TransactionParams { + nonce: 1, + chain_id: chain_id.clone(), + }, + }; + vec![unsigned_transaction.into_signed(&signing_key)] + } else { + vec![] }; let mut deposits_map: HashMap> = HashMap::new(); @@ -115,11 +123,8 @@ impl ConfigureSequencerBlock { } }); - let signed_transaction = unsigned_transaction.into_signed(&signing_key); let mut rollup_transactions = - group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(&[ - signed_transaction.clone(), - ]); + group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(&txs); for (rollup_id, deposit) in deposits_map.clone() { rollup_transactions.entry(rollup_id).or_default().extend( deposit @@ -136,11 +141,11 @@ impl ConfigureSequencerBlock { .map(|rollup_id| rollup_id.as_ref().to_vec()), ) .root(); - let data = vec![ + let mut data = vec![ rollup_transactions_tree.root().to_vec(), rollup_ids_root.to_vec(), - signed_transaction.into_raw().encode_to_vec(), ]; + data.extend(txs.into_iter().map(|tx| tx.into_raw().encode_to_vec())); SequencerBlock::try_from_block_info_and_data( block_hash, diff --git a/crates/astria-sequencer-relayer/Cargo.toml b/crates/astria-sequencer-relayer/Cargo.toml index 0178197ffe..9b1e9e6414 100644 --- a/crates/astria-sequencer-relayer/Cargo.toml +++ b/crates/astria-sequencer-relayer/Cargo.toml @@ -67,6 +67,7 @@ tendermint-rpc = { workspace = true, features = ["http-client"] } jsonrpsee = { workspace = true, features = ["server"] } once_cell = { workspace = true } +rand = { workspace = true } tempfile = { workspace = true } tokio = { workspace = true, features = ["test-util"] } tokio-stream = { workspace = true, features = ["net"] } diff --git a/crates/astria-sequencer-relayer/tests/blackbox/helper.rs b/crates/astria-sequencer-relayer/tests/blackbox/helper.rs index e0c92784b2..9256faf949 100644 --- a/crates/astria-sequencer-relayer/tests/blackbox/helper.rs +++ b/crates/astria-sequencer-relayer/tests/blackbox/helper.rs @@ -230,6 +230,11 @@ impl TestSequencerRelayer { } pub fn mount_block_response(&mut self, height: u32) -> BlockGuard { + use astria_core::primitive::v1::RollupId; + use rand::Rng; + + let mut rng = rand::thread_rng(); + let proposer = if RELAY_SELF { self.account } else { @@ -239,11 +244,17 @@ impl TestSequencerRelayer { let (tx, rx) = oneshot::channel(); let block = ConfigureSequencerBlock { + block_hash: Some(rng.gen::<[u8; 32]>()), height, proposer_address: Some(proposer), + sequence_data: vec![( + RollupId::from_unhashed_bytes(b"some_rollup_id"), + vec![99u8; 32], + )], ..Default::default() } .make(); + let mut blocks = self.sequencer_server_blocks.lock().unwrap(); blocks.push_back((tx, block.into_raw())); BlockGuard { From fa18eaa561e8e576acf3005d3f5edd656917f7be Mon Sep 17 00:00:00 2001 From: elizabeth Date: Wed, 24 Apr 2024 16:31:45 -0400 Subject: [PATCH 03/11] clippy --- crates/astria-core/src/protocol/test_utils.rs | 12 ++++++------ crates/astria-sequencer/Cargo.toml | 6 +++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/crates/astria-core/src/protocol/test_utils.rs b/crates/astria-core/src/protocol/test_utils.rs index 0d0b1f83e3..79f0f6a32c 100644 --- a/crates/astria-core/src/protocol/test_utils.rs +++ b/crates/astria-core/src/protocol/test_utils.rs @@ -101,7 +101,9 @@ impl ConfigureSequencerBlock { .into() }) .collect(); - let txs = if !actions.is_empty() { + let txs = if actions.is_empty() { + vec![] + } else { let unsigned_transaction = UnsignedTransaction { actions, params: TransactionParams { @@ -110,18 +112,16 @@ impl ConfigureSequencerBlock { }, }; vec![unsigned_transaction.into_signed(&signing_key)] - } else { - vec![] }; let mut deposits_map: HashMap> = HashMap::new(); - deposits.into_iter().for_each(|deposit| { + for deposit in deposits { if let Some(entry) = deposits_map.get_mut(deposit.rollup_id()) { entry.push(deposit); } else { - deposits_map.insert(deposit.rollup_id().clone(), vec![deposit]); + deposits_map.insert(*deposit.rollup_id(), vec![deposit]); } - }); + } let mut rollup_transactions = group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(&txs); diff --git a/crates/astria-sequencer/Cargo.toml b/crates/astria-sequencer/Cargo.toml index 6f1de24a65..326199b1c8 100644 --- a/crates/astria-sequencer/Cargo.toml +++ b/crates/astria-sequencer/Cargo.toml @@ -59,7 +59,11 @@ ibc-proto = { version = "0.41.0", features = ["server"] } tower-http = { version = "0.4", features = ["cors"] } [dev-dependencies] -astria-core = { path = "../astria-core", features = ["server", "serde", "test-utils"] } +astria-core = { path = "../astria-core", features = [ + "server", + "serde", + "test-utils", +] } config = { package = "astria-config", path = "../astria-config", features = [ "tests", ] } From 23d39024fc7148b19ddc111b7553f747a1f2727e Mon Sep 17 00:00:00 2001 From: elizabeth Date: Thu, 25 Apr 2024 12:06:30 -0400 Subject: [PATCH 04/11] store transfer fee in db --- .../astria-sequencer/src/accounts/action.rs | 15 +++++------ .../src/accounts/component.rs | 5 ++++ .../src/accounts/state_ext.rs | 26 +++++++++++++++++++ crates/astria-sequencer/src/app.rs | 13 ++++++---- .../astria-sequencer/src/transaction/mod.rs | 21 +++++++++------ 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index bcb4904569..1d74b4467d 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -22,9 +22,6 @@ use crate::{ transaction::action_handler::ActionHandler, }; -/// Fee charged for a `Transfer` action. -pub(crate) const TRANSFER_FEE: u128 = 12; - pub(crate) async fn transfer_check_stateful( action: &TransferAction, state: &S, @@ -38,6 +35,7 @@ pub(crate) async fn transfer_check_stateful( "invalid fee asset", ); + let fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; let transfer_asset_id = action.asset_id; let from_fee_balance = state @@ -50,7 +48,7 @@ pub(crate) async fn transfer_check_stateful( if action.fee_asset_id == transfer_asset_id { let payment_amount = action .amount - .checked_add(TRANSFER_FEE) + .checked_add(fee) .context("transfer amount plus fee overflowed")?; ensure!( @@ -61,7 +59,7 @@ pub(crate) async fn transfer_check_stateful( // otherwise, check the fee asset account has enough to cover the fees, // and the transfer asset account has enough to cover the transfer ensure!( - from_fee_balance >= TRANSFER_FEE, + from_fee_balance >= fee, "insufficient funds for fee payment" ); @@ -109,8 +107,9 @@ impl ActionHandler for TransferAction { ) )] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { + let fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; state - .get_and_increase_block_fees(self.fee_asset_id, TRANSFER_FEE) + .get_and_increase_block_fees(self.fee_asset_id, fee) .await .context("failed to add to block fees")?; @@ -122,7 +121,7 @@ impl ActionHandler for TransferAction { // check_stateful should have already checked this arithmetic let payment_amount = self .amount - .checked_add(TRANSFER_FEE) + .checked_add(fee) .expect("transfer amount plus fee should not overflow"); state @@ -147,7 +146,7 @@ impl ActionHandler for TransferAction { // deduct fee from fee asset balance state - .decrease_balance(from, self.fee_asset_id, TRANSFER_FEE) + .decrease_balance(from, self.fee_asset_id, fee) .await .context("failed decreasing `from` account balance for fee payment")?; } diff --git a/crates/astria-sequencer/src/accounts/component.rs b/crates/astria-sequencer/src/accounts/component.rs index 14e6631bca..d9e5d7cb39 100644 --- a/crates/astria-sequencer/src/accounts/component.rs +++ b/crates/astria-sequencer/src/accounts/component.rs @@ -17,6 +17,10 @@ use crate::{ genesis::GenesisState, }; +/// Default transfer base fee. +/// TODO: put in app genesis state +pub(crate) const DEFAULT_TRANSFER_BASE_FEE: u128 = 12; + #[derive(Default)] pub(crate) struct AccountsComponent; @@ -33,6 +37,7 @@ impl Component for AccountsComponent { .context("failed writing account balance to state")?; } + state.put_transfer_base_fee(DEFAULT_TRANSFER_BASE_FEE).context("failed to put transfer base fee")?; Ok(()) } diff --git a/crates/astria-sequencer/src/accounts/state_ext.rs b/crates/astria-sequencer/src/accounts/state_ext.rs index 2370c99a22..f285125ea2 100644 --- a/crates/astria-sequencer/src/accounts/state_ext.rs +++ b/crates/astria-sequencer/src/accounts/state_ext.rs @@ -30,7 +30,12 @@ struct Nonce(u32); #[derive(BorshSerialize, BorshDeserialize, Debug)] struct Balance(u128); +/// Newtype wrapper to read and write a u128 from rocksdb. +#[derive(BorshSerialize, BorshDeserialize, Debug)] +struct Fee(u128); + const ACCOUNTS_PREFIX: &str = "accounts"; +const TRANSFER_BASE_FEE_STORAGE_KEY: &str = "transferfee"; fn storage_key(address: &str) -> String { format!("{ACCOUNTS_PREFIX}/{address}") @@ -127,6 +132,20 @@ pub(crate) trait StateReadExt: StateRead { let Nonce(nonce) = Nonce::try_from_slice(&bytes).context("invalid nonce bytes")?; Ok(nonce) } + + #[instrument(skip_all)] + async fn get_transfer_base_fee(&self) -> Result { + let bytes = self + .get_raw(TRANSFER_BASE_FEE_STORAGE_KEY) + .await + .context("failed reading raw transfer base fee from state")?; + let Some(bytes) = bytes else { + return Err(anyhow::anyhow!("transfer base fee not set")); + }; + + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } } impl StateReadExt for T {} @@ -195,6 +214,13 @@ pub(crate) trait StateWriteExt: StateWrite { .context("failed to store updated account balance in database")?; Ok(()) } + + #[instrument(skip(self))] + fn put_transfer_base_fee(&mut self, fee: u128) -> Result<()> { + let bytes = borsh::to_vec(&Fee(fee)).context("failed to serialize fee")?; + self.put_raw(TRANSFER_BASE_FEE_STORAGE_KEY.to_string(), bytes); + Ok(()) + } } impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/app.rs b/crates/astria-sequencer/src/app.rs index f44899d8a9..d6e814e484 100644 --- a/crates/astria-sequencer/src/app.rs +++ b/crates/astria-sequencer/src/app.rs @@ -1057,7 +1057,6 @@ mod test { use super::*; use crate::{ - accounts::action::TRANSFER_FEE, app::test_utils::*, asset::get_native_asset, authority::state_ext::ValidatorSet, @@ -1289,12 +1288,13 @@ mod test { .unwrap(), value + 10u128.pow(19) ); + let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); assert_eq!( app.state .get_account_balance(alice_address, native_asset) .await .unwrap(), - 10u128.pow(19) - (value + TRANSFER_FEE), + 10u128.pow(19) - (value + transfer_fee), ); assert_eq!(app.state.get_account_nonce(bob_address).await.unwrap(), 0); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); @@ -1353,12 +1353,13 @@ mod test { value, // transferred amount ); + let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); assert_eq!( app.state .get_account_balance(alice_address, native_asset) .await .unwrap(), - 10u128.pow(19) - TRANSFER_FEE, // genesis balance - fee + 10u128.pow(19) - transfer_fee, // genesis balance - fee ); assert_eq!( app.state @@ -1929,12 +1930,13 @@ mod test { app.execute_transaction(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); + let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); assert_eq!( app.state .get_account_balance(alice_address, asset_id) .await .unwrap(), - alice_before_balance - (amount + TRANSFER_FEE) + alice_before_balance - (amount + transfer_fee) ); assert_eq!( app.state @@ -2328,12 +2330,13 @@ mod test { app.commit(storage).await; // assert that transaction fees were transferred to the block proposer + let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); assert_eq!( app.state .get_account_balance(sequencer_proposer_address, native_asset) .await .unwrap(), - TRANSFER_FEE, + transfer_fee, ); assert_eq!(app.state.get_block_fees().await.unwrap().len(), 0); } diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index 6946b6e691..a14097634b 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -21,7 +21,6 @@ use tracing::instrument; use crate::{ accounts::{ - action::TRANSFER_FEE, state_ext::{ StateReadExt, StateWriteExt, @@ -72,6 +71,8 @@ pub(crate) async fn check_balance_mempool( state: &S, ) -> anyhow::Result<()> { use std::collections::HashMap; + + let transfer_fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; let signer_address = Address::from_verification_key(tx.verification_key()); let mut fees_by_asset = HashMap::new(); @@ -84,8 +85,8 @@ pub(crate) async fn check_balance_mempool( .or_insert(act.amount); fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += TRANSFER_FEE) - .or_insert(TRANSFER_FEE); + .and_modify(|amt| *amt += transfer_fee) + .or_insert(transfer_fee); } Action::Sequence(act) => { let fee = crate::sequence::calculate_fee(&act.data) @@ -118,8 +119,8 @@ pub(crate) async fn check_balance_mempool( .or_insert(act.amount); fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += TRANSFER_FEE) - .or_insert(TRANSFER_FEE); + .and_modify(|amt| *amt += transfer_fee) + .or_insert(transfer_fee); } Action::ValidatorUpdate(_) | Action::SudoAddressChange(_) @@ -467,6 +468,7 @@ mod test { let snapshot = storage.latest_snapshot(); let mut state_tx = StateDelta::new(snapshot); + state_tx.put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE).unwrap(); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); let other_asset = Denom::from_base_denom("other").id(); @@ -474,11 +476,12 @@ mod test { let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); let amount = 100; let data = [0; 32].to_vec(); + let transfer_fee = state_tx.get_transfer_base_fee().await.unwrap(); state_tx .increase_balance( alice_address, native_asset, - TRANSFER_FEE + crate::sequence::calculate_fee(&data).unwrap(), + transfer_fee + crate::sequence::calculate_fee(&data).unwrap(), ) .await .unwrap(); @@ -521,7 +524,8 @@ mod test { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state_tx = StateDelta::new(snapshot); - + + state_tx.put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE).unwrap(); crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); let other_asset = Denom::from_base_denom("other").id(); @@ -529,11 +533,12 @@ mod test { let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); let amount = 100; let data = [0; 32].to_vec(); + let transfer_fee = state_tx.get_transfer_base_fee().await.unwrap(); state_tx .increase_balance( alice_address, native_asset, - TRANSFER_FEE + crate::sequence::calculate_fee(&data).unwrap(), + transfer_fee + crate::sequence::calculate_fee(&data).unwrap(), ) .await .unwrap(); From e084f6a9b12ac7663672e5706a1e454078b55675 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Fri, 26 Apr 2024 16:46:35 -0400 Subject: [PATCH 05/11] store fees for sequence action and bridge --- .../astria-sequencer/src/accounts/action.rs | 10 ++- .../src/accounts/component.rs | 4 +- crates/astria-sequencer/src/app.rs | 55 ++++++++++--- .../src/bridge/bridge_lock_action.rs | 39 +++++++++- .../astria-sequencer/src/bridge/component.rs | 54 +++++++++++++ .../src/bridge/init_bridge_account_action.rs | 11 +-- crates/astria-sequencer/src/bridge/mod.rs | 4 + .../astria-sequencer/src/bridge/state_ext.rs | 44 +++++++++++ crates/astria-sequencer/src/ibc/component.rs | 5 ++ .../src/ibc/ics20_withdrawal.rs | 18 +++-- crates/astria-sequencer/src/ibc/state_ext.rs | 27 +++++++ .../src/{sequence.rs => sequence/action.rs} | 47 +++++++---- .../src/sequence/component.rs | 52 +++++++++++++ crates/astria-sequencer/src/sequence/mod.rs | 5 ++ .../src/sequence/state_ext.rs | 70 +++++++++++++++++ .../astria-sequencer/src/transaction/mod.rs | 77 ++++++++++++++----- 16 files changed, 465 insertions(+), 57 deletions(-) create mode 100644 crates/astria-sequencer/src/bridge/component.rs rename crates/astria-sequencer/src/{sequence.rs => sequence/action.rs} (59%) create mode 100644 crates/astria-sequencer/src/sequence/component.rs create mode 100644 crates/astria-sequencer/src/sequence/mod.rs create mode 100644 crates/astria-sequencer/src/sequence/state_ext.rs diff --git a/crates/astria-sequencer/src/accounts/action.rs b/crates/astria-sequencer/src/accounts/action.rs index 1d74b4467d..f06ed1d178 100644 --- a/crates/astria-sequencer/src/accounts/action.rs +++ b/crates/astria-sequencer/src/accounts/action.rs @@ -35,7 +35,10 @@ pub(crate) async fn transfer_check_stateful( "invalid fee asset", ); - let fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; + let fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; let transfer_asset_id = action.asset_id; let from_fee_balance = state @@ -107,7 +110,10 @@ impl ActionHandler for TransferAction { ) )] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - let fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; + let fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; state .get_and_increase_block_fees(self.fee_asset_id, fee) .await diff --git a/crates/astria-sequencer/src/accounts/component.rs b/crates/astria-sequencer/src/accounts/component.rs index d9e5d7cb39..f50896293f 100644 --- a/crates/astria-sequencer/src/accounts/component.rs +++ b/crates/astria-sequencer/src/accounts/component.rs @@ -37,7 +37,9 @@ impl Component for AccountsComponent { .context("failed writing account balance to state")?; } - state.put_transfer_base_fee(DEFAULT_TRANSFER_BASE_FEE).context("failed to put transfer base fee")?; + state + .put_transfer_base_fee(DEFAULT_TRANSFER_BASE_FEE) + .context("failed to put transfer base fee")?; Ok(()) } diff --git a/crates/astria-sequencer/src/app.rs b/crates/astria-sequencer/src/app.rs index d6e814e484..97399bc800 100644 --- a/crates/astria-sequencer/src/app.rs +++ b/crates/astria-sequencer/src/app.rs @@ -69,9 +69,12 @@ use crate::{ StateWriteExt as _, }, }, - bridge::state_ext::{ - StateReadExt as _, - StateWriteExt, + bridge::{ + component::BridgeComponent, + state_ext::{ + StateReadExt as _, + StateWriteExt, + }, }, component::Component as _, genesis::GenesisState, @@ -84,6 +87,7 @@ use crate::{ GeneratedCommitments, }, }, + sequence::component::SequenceComponent, state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -198,9 +202,15 @@ impl App { ) .await .context("failed to call init_chain on AuthorityComponent")?; + BridgeComponent::init_chain(&mut state_tx, &genesis_state) + .await + .context("failed to call init_chain on BridgeComponent")?; IbcComponent::init_chain(&mut state_tx, &genesis_state) .await .context("failed to call init_chain on IbcComponent")?; + SequenceComponent::init_chain(&mut state_tx, &genesis_state) + .await + .context("failed to call init_chain on SequenceComponent")?; state_tx.apply(); @@ -797,9 +807,15 @@ impl App { AuthorityComponent::begin_block(&mut arc_state_tx, begin_block) .await .context("failed to call begin_block on AuthorityComponent")?; + BridgeComponent::begin_block(&mut arc_state_tx, begin_block) + .await + .context("failed to call begin_block on BridgeComponent")?; IbcComponent::begin_block(&mut arc_state_tx, begin_block) .await .context("failed to call begin_block on IbcComponent")?; + SequenceComponent::begin_block(&mut arc_state_tx, begin_block) + .await + .context("failed to call begin_block on SequenceComponent")?; let state_tx = Arc::try_unwrap(arc_state_tx) .expect("components should not retain copies of shared state"); @@ -870,9 +886,15 @@ impl App { AuthorityComponent::end_block(&mut arc_state_tx, &end_block) .await .context("failed to call end_block on AuthorityComponent")?; + BridgeComponent::end_block(&mut arc_state_tx, &end_block) + .await + .context("failed to call end_block on BridgeComponent")?; IbcComponent::end_block(&mut arc_state_tx, &end_block) .await .context("failed to call end_block on IbcComponent")?; + SequenceComponent::end_block(&mut arc_state_tx, &end_block) + .await + .context("failed to call end_block on SequenceComponent")?; let mut state_tx = Arc::try_unwrap(arc_state_tx) .expect("components should not retain copies of shared state"); @@ -1062,7 +1084,7 @@ mod test { authority::state_ext::ValidatorSet, genesis::Account, ibc::state_ext::StateReadExt as _, - sequence::calculate_fee, + sequence::calculate_fee_from_state, transaction::InvalidChainId, }; @@ -1412,11 +1434,17 @@ mod test { #[tokio::test] async fn app_execute_transaction_sequence() { + use crate::sequence::state_ext::StateWriteExt as _; + let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + state_tx.put_sequence_action_base_fee(0); + state_tx.put_sequence_action_byte_cost_multiplier(1); + app.apply(state_tx); let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); let data = b"hello world".to_vec(); - let fee = calculate_fee(&data).unwrap(); + let fee = calculate_fee_from_state(&data, &app.state).await.unwrap(); let tx = UnsignedTransaction { params: TransactionParams { @@ -1788,10 +1816,12 @@ mod test { async fn app_execute_transaction_init_bridge_account_ok() { use astria_core::protocol::transaction::v1alpha1::action::InitBridgeAccountAction; - use crate::bridge::init_bridge_account_action::INIT_BRIDGE_ACCOUNT_FEE; - let (alice_signing_key, alice_address) = get_alice_signing_key_and_address(); let mut app = initialize_app(None, vec![]).await; + let mut state_tx = StateDelta::new(app.state.clone()); + let fee = 12; // arbitrary + state_tx.put_init_bridge_account_base_fee(fee); + app.apply(state_tx); let rollup_id = RollupId::from_unhashed_bytes(b"testchainid"); let asset_id = get_native_asset().id(); @@ -1837,7 +1867,7 @@ mod test { .get_account_balance(alice_address, asset_id) .await .unwrap(), - before_balance - INIT_BRIDGE_ACCOUNT_FEE + before_balance - fee, ); } @@ -1931,12 +1961,19 @@ mod test { app.execute_transaction(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); + let fee = transfer_fee + + app + .state + .get_bridge_lock_byte_cost_multiplier() + .await + .unwrap() + * crate::bridge::DEPOSIT_BYTE_LEN; assert_eq!( app.state .get_account_balance(alice_address, asset_id) .await .unwrap(), - alice_before_balance - (amount + transfer_fee) + alice_before_balance - (amount + fee) ); assert_eq!( app.state diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 09a192a69c..b3aa18d687 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -14,7 +14,13 @@ use astria_core::{ use tracing::instrument; use crate::{ - accounts::action::transfer_check_stateful, + accounts::{ + action::transfer_check_stateful, + state_ext::{ + StateReadExt as _, + StateWriteExt as _, + }, + }, bridge::state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -26,6 +32,8 @@ use crate::{ transaction::action_handler::ActionHandler, }; +pub(crate) const DEPOSIT_BYTE_LEN: u128 = std::mem::size_of::() as u128; + #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { async fn check_stateful( @@ -58,6 +66,22 @@ impl ActionHandler for BridgeLockAction { "asset ID is not authorized for transfer to bridge account", ); + let from_balance = state + .get_account_balance(from, self.fee_asset_id) + .await + .context("failed to get sender account balance")?; + let transfer_fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + + let byte_cost_multiplier = state + .get_bridge_lock_byte_cost_multiplier() + .await + .context("failed to get byte cost multiplier")?; + let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN + transfer_fee; + ensure!(from_balance >= fee, "insuffient funds for fee payment",); + // this performs the same checks as a normal `TransferAction`, // but without the check that prevents transferring to a bridge account, // as we are explicitly transferring to a bridge account here. @@ -78,6 +102,19 @@ impl ActionHandler for BridgeLockAction { .await .context("failed to execute bridge lock action as transfer action")?; + // the transfer fee is already deducted in `transfer_action.execute()`, + // so we just deduct the bridge lock byte multiplier fee. + let byte_cost_multiplier = state + .get_bridge_lock_byte_cost_multiplier() + .await + .context("failed to get byte cost multiplier")?; + let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN; + + state + .decrease_balance(from, self.fee_asset_id, fee) + .await + .context("failed to deduct fee from account balance")?; + let rollup_id = state .get_bridge_account_rollup_id(&self.to) .await? diff --git a/crates/astria-sequencer/src/bridge/component.rs b/crates/astria-sequencer/src/bridge/component.rs new file mode 100644 index 0000000000..492668367f --- /dev/null +++ b/crates/astria-sequencer/src/bridge/component.rs @@ -0,0 +1,54 @@ +use std::sync::Arc; + +use anyhow::Result; +use tendermint::abci::request::{ + BeginBlock, + EndBlock, +}; +use tracing::instrument; + +use super::state_ext::StateWriteExt; +use crate::{ + component::Component, + genesis::GenesisState, +}; + +/// Default base fee for a [`InitBridgeAccountAction`]. +const DEFAULT_INIT_BRIDGE_ACCOUNT_BASE_FEE: u128 = 48; + +/// Default multiplier for the cost of a byte in a [`BridgeLockAction`]. +/// +/// Note that the base fee for a [`BridgeLockAction`] is the same as the +/// base fee for a [`TransferAction`]. +const DEFAULT_BRIDGE_LOCK_BYTE_COST_MULTIPLIER: u128 = 1; + +#[derive(Default)] +pub(crate) struct BridgeComponent; + +#[async_trait::async_trait] +impl Component for BridgeComponent { + type AppState = GenesisState; + + #[instrument(name = "BridgeComponent::init_chain", skip(state))] + async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { + state.put_init_bridge_account_base_fee(DEFAULT_INIT_BRIDGE_ACCOUNT_BASE_FEE); + state.put_bridge_lock_byte_cost_multiplier(DEFAULT_BRIDGE_LOCK_BYTE_COST_MULTIPLIER); + Ok(()) + } + + #[instrument(name = "BridgeComponent::begin_block", skip(_state))] + async fn begin_block( + _state: &mut Arc, + _begin_block: &BeginBlock, + ) -> Result<()> { + Ok(()) + } + + #[instrument(name = "BridgeComponent::end_block", skip(_state))] + async fn end_block( + _state: &mut Arc, + _end_block: &EndBlock, + ) -> Result<()> { + Ok(()) + } +} diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index e872baff23..2f246d0699 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -26,9 +26,6 @@ use crate::{ transaction::action_handler::ActionHandler, }; -/// Fee charged for a `InitBridgeAccountAction`. -pub(crate) const INIT_BRIDGE_ACCOUNT_FEE: u128 = 48; - #[async_trait::async_trait] impl ActionHandler for InitBridgeAccountAction { async fn check_stateful( @@ -41,6 +38,8 @@ impl ActionHandler for InitBridgeAccountAction { "invalid fee asset", ); + let fee = state.get_init_bridge_account_base_fee().await?; + // this prevents the address from being registered as a bridge account // if it's been previously initialized as a bridge account. // @@ -62,7 +61,7 @@ impl ActionHandler for InitBridgeAccountAction { .context("failed getting `from` account balance for fee payment")?; ensure!( - balance >= INIT_BRIDGE_ACCOUNT_FEE, + balance >= fee, "insufficient funds for bridge account initialization", ); @@ -71,13 +70,15 @@ impl ActionHandler for InitBridgeAccountAction { #[instrument(skip_all)] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { + let fee = state.get_init_bridge_account_base_fee().await?; + state.put_bridge_account_rollup_id(&from, &self.rollup_id); state .put_bridge_account_asset_id(&from, &self.asset_id) .context("failed to put asset ID")?; state - .decrease_balance(from, self.fee_asset_id, INIT_BRIDGE_ACCOUNT_FEE) + .decrease_balance(from, self.fee_asset_id, fee) .await .context("failed to deduct fee from account balance")?; Ok(()) diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index d2204a0077..5dea27f34a 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -1,3 +1,7 @@ mod bridge_lock_action; +pub(crate) mod component; pub(crate) mod init_bridge_account_action; pub(crate) mod state_ext; + +#[cfg(test)] +pub(crate) use bridge_lock_action::DEPOSIT_BYTE_LEN; diff --git a/crates/astria-sequencer/src/bridge/state_ext.rs b/crates/astria-sequencer/src/bridge/state_ext.rs index 9e158f68eb..69491cc193 100644 --- a/crates/astria-sequencer/src/bridge/state_ext.rs +++ b/crates/astria-sequencer/src/bridge/state_ext.rs @@ -52,8 +52,14 @@ impl From<&asset::Id> for AssetId { } } +/// Newtype wrapper to read and write a u128 from rocksdb. +#[derive(BorshSerialize, BorshDeserialize, Debug)] +struct Fee(u128); + const BRIDGE_ACCOUNT_PREFIX: &str = "bridgeacc"; const DEPOSIT_PREFIX: &str = "deposit"; +const INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY: &str = "initbridgeaccfee"; +const BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "bridgelockmultiplier"; fn storage_key(address: &str) -> String { format!("{BRIDGE_ACCOUNT_PREFIX}/{address}") @@ -177,6 +183,28 @@ pub(crate) trait StateReadExt: StateRead { } Ok(deposit_events) } + + #[instrument(skip(self))] + async fn get_init_bridge_account_base_fee(&self) -> Result { + let bytes = self + .get_raw(INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY) + .await + .context("failed reading raw init bridge account base fee from state")? + .ok_or_else(|| anyhow!("init bridge account base fee not found"))?; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } + + #[instrument(skip(self))] + async fn get_bridge_lock_byte_cost_multiplier(&self) -> Result { + let bytes = self + .get_raw(BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY) + .await + .context("failed reading raw bridge lock byte cost multiplier from state")? + .ok_or_else(|| anyhow!("bridge lock byte cost multiplier not found"))?; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } } impl StateReadExt for T {} @@ -245,6 +273,22 @@ pub(crate) trait StateWriteExt: StateWrite { } Ok(()) } + + #[instrument(skip(self))] + fn put_init_bridge_account_base_fee(&mut self, fee: u128) { + self.put_raw( + INIT_BRIDGE_ACCOUNT_BASE_FEE_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), + ); + } + + #[instrument(skip(self))] + fn put_bridge_lock_byte_cost_multiplier(&mut self, fee: u128) { + self.put_raw( + BRIDGE_LOCK_BYTE_COST_MULTIPLIER_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), + ); + } } impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/ibc/component.rs b/crates/astria-sequencer/src/ibc/component.rs index c56644b92f..f19cd785da 100644 --- a/crates/astria-sequencer/src/ibc/component.rs +++ b/crates/astria-sequencer/src/ibc/component.rs @@ -23,6 +23,8 @@ use crate::{ }, }; +pub(crate) const DEFAULT_ICS20_WITHDRAWAL_FEE: u128 = 24; + #[derive(Default)] pub(crate) struct IbcComponent; @@ -48,6 +50,9 @@ impl Component for IbcComponent { state.put_ibc_relayer_address(address); } + state + .put_ics20_withdrawal_base_fee(DEFAULT_ICS20_WITHDRAWAL_FEE) + .context("failed to put ics20 withdrawal base fee")?; Ok(()) } diff --git a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs index fe40199ea9..55377de807 100644 --- a/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs +++ b/crates/astria-sequencer/src/ibc/ics20_withdrawal.rs @@ -35,9 +35,6 @@ use crate::{ transaction::action_handler::ActionHandler, }; -/// Fee charged for a `Ics20Withdrawal` action. -pub(crate) const ICS20_WITHDRAWAL_FEE: u128 = 24; - fn withdrawal_to_unchecked_ibc_packet( withdrawal: &action::Ics20Withdrawal, ) -> IBCPacket { @@ -72,6 +69,11 @@ impl ActionHandler for action::Ics20Withdrawal { state: &S, from: Address, ) -> Result<()> { + let fee = state + .get_ics20_withdrawal_base_fee() + .await + .context("failed to get ics20 withdrawal base fee")?; + let packet: IBCPacket = withdrawal_to_unchecked_ibc_packet(self); state .send_packet_check(packet) @@ -90,7 +92,7 @@ impl ActionHandler for action::Ics20Withdrawal { if self.fee_asset_id() == &transfer_asset_id { let payment_amount = self .amount() - .checked_add(ICS20_WITHDRAWAL_FEE) + .checked_add(fee) .ok_or(anyhow!("transfer amount plus fee overflowed"))?; ensure!( @@ -101,7 +103,7 @@ impl ActionHandler for action::Ics20Withdrawal { // otherwise, check the fee asset account has enough to cover the fees, // and the transfer asset account has enough to cover the transfer ensure!( - from_fee_balance >= ICS20_WITHDRAWAL_FEE, + from_fee_balance >= fee, "insufficient funds for fee payment" ); @@ -120,6 +122,10 @@ impl ActionHandler for action::Ics20Withdrawal { #[instrument(skip(self, state))] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { + let fee = state + .get_ics20_withdrawal_base_fee() + .await + .context("failed to get ics20 withdrawal base fee")?; let checked_packet = withdrawal_to_unchecked_ibc_packet(self).assume_checked(); state @@ -128,7 +134,7 @@ impl ActionHandler for action::Ics20Withdrawal { .context("failed to decrease sender balance")?; state - .decrease_balance(from, *self.fee_asset_id(), ICS20_WITHDRAWAL_FEE) + .decrease_balance(from, *self.fee_asset_id(), fee) .await .context("failed to subtract fee from sender balance")?; diff --git a/crates/astria-sequencer/src/ibc/state_ext.rs b/crates/astria-sequencer/src/ibc/state_ext.rs index 66b81f2af6..948e2ef9ba 100644 --- a/crates/astria-sequencer/src/ibc/state_ext.rs +++ b/crates/astria-sequencer/src/ibc/state_ext.rs @@ -32,7 +32,12 @@ struct Balance(u128); #[derive(BorshSerialize, BorshDeserialize, Debug)] struct SudoAddress([u8; ADDRESS_LEN]); +/// Newtype wrapper to read and write a u128 from rocksdb. +#[derive(BorshSerialize, BorshDeserialize, Debug)] +struct Fee(u128); + const IBC_SUDO_STORAGE_KEY: &str = "ibcsudo"; +const ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY: &str = "ics20withdrawalfee"; fn channel_balance_storage_key(channel: &ChannelId, asset: asset::Id) -> String { format!( @@ -84,6 +89,19 @@ pub(crate) trait StateReadExt: StateRead { .context("failed to read ibc relayer key from state")? .is_some()) } + + #[instrument(skip(self))] + async fn get_ics20_withdrawal_base_fee(&self) -> Result { + let Some(bytes) = self + .get_raw(ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY) + .await + .context("failed reading ics20 withdrawal fee from state")? + else { + bail!("ics20 withdrawal fee not found"); + }; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } } impl StateReadExt for T {} @@ -121,6 +139,15 @@ pub(crate) trait StateWriteExt: StateWrite { fn delete_ibc_relayer_address(&mut self, address: &Address) { self.delete(ibc_relayer_key(address)); } + + #[instrument(skip(self))] + fn put_ics20_withdrawal_base_fee(&mut self, fee: u128) -> Result<()> { + self.put_raw( + ICS20_WITHDRAWAL_BASE_FEE_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).context("failed to serialize fee")?, + ); + Ok(()) + } } impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/sequence.rs b/crates/astria-sequencer/src/sequence/action.rs similarity index 59% rename from crates/astria-sequencer/src/sequence.rs rename to crates/astria-sequencer/src/sequence/action.rs index 62b872e0cc..d646c5f1bf 100644 --- a/crates/astria-sequencer/src/sequence.rs +++ b/crates/astria-sequencer/src/sequence/action.rs @@ -14,6 +14,7 @@ use crate::{ StateReadExt, StateWriteExt, }, + sequence::state_ext::StateReadExt as SequenceStateReadExt, state_ext::{ StateReadExt as _, StateWriteExt as _, @@ -21,9 +22,6 @@ use crate::{ transaction::action_handler::ActionHandler, }; -/// Fee charged for a sequence `Action` per byte of `data` included. -const SEQUENCE_ACTION_FEE_PER_BYTE: u128 = 1; - #[async_trait::async_trait] impl ActionHandler for SequenceAction { async fn check_stateful( @@ -40,7 +38,9 @@ impl ActionHandler for SequenceAction { .get_account_balance(from, self.fee_asset_id) .await .context("failed getting `from` account balance for fee payment")?; - let fee = calculate_fee(&self.data).context("calculated fee overflows u128")?; + let fee = calculate_fee_from_state(&self.data, state) + .await + .context("calculated fee overflows u128")?; ensure!(curr_balance >= fee, "insufficient funds"); Ok(()) } @@ -62,7 +62,9 @@ impl ActionHandler for SequenceAction { ) )] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - let fee = calculate_fee(&self.data).context("failed to calculate fee")?; + let fee = calculate_fee_from_state(&self.data, state) + .await + .context("failed to calculate fee")?; state .get_and_increase_block_fees(self.fee_asset_id, fee) .await @@ -76,13 +78,31 @@ impl ActionHandler for SequenceAction { } } +/// Calculates the fee for a sequence `Action` based on the length of the `data`. +pub(crate) async fn calculate_fee_from_state( + data: &[u8], + state: &S, +) -> Result { + let base_fee = state + .get_sequence_action_base_fee() + .await + .context("failed to get base fee")?; + let fee_per_byte = state + .get_sequence_action_byte_cost_multiplier() + .await + .context("failed to get fee per byte")?; + calculate_fee(data, fee_per_byte, base_fee).context("calculated fee overflows u128") +} + /// Calculates the fee for a sequence `Action` based on the length of the `data`. /// Returns `None` if the fee overflows `u128`. -pub(crate) fn calculate_fee(data: &[u8]) -> Option { - SEQUENCE_ACTION_FEE_PER_BYTE.checked_mul( - data.len() - .try_into() - .expect("a usize should always convert to a u128"), +fn calculate_fee(data: &[u8], fee_per_byte: u128, base_fee: u128) -> Option { + base_fee.checked_add( + fee_per_byte.checked_mul( + data.len() + .try_into() + .expect("a usize should always convert to a u128"), + )?, ) } @@ -92,8 +112,9 @@ mod test { #[test] fn calculate_fee_ok() { - assert_eq!(calculate_fee(&[]), Some(0)); - assert_eq!(calculate_fee(&[0]), Some(1)); - assert_eq!(calculate_fee(&[0u8; 10]), Some(10)); + assert_eq!(calculate_fee(&[], 1, 0), Some(0)); + assert_eq!(calculate_fee(&[0], 1, 0), Some(1)); + assert_eq!(calculate_fee(&[0u8; 10], 1, 0), Some(10)); + assert_eq!(calculate_fee(&[0u8; 10], 1, 100), Some(110)); } } diff --git a/crates/astria-sequencer/src/sequence/component.rs b/crates/astria-sequencer/src/sequence/component.rs new file mode 100644 index 0000000000..d76724f110 --- /dev/null +++ b/crates/astria-sequencer/src/sequence/component.rs @@ -0,0 +1,52 @@ +use std::sync::Arc; + +use anyhow::Result; +use tendermint::abci::request::{ + BeginBlock, + EndBlock, +}; +use tracing::instrument; + +use super::state_ext::StateWriteExt; +use crate::{ + component::Component, + genesis::GenesisState, +}; + +/// Default base fee for a [`SequenceAction`]. +const DEFAULT_SEQUENCE_ACTION_BASE_FEE: u128 = 32; + +/// Default multiplier for the cost of a byte in a [`SequenceAction`]. +const DEFAULT_SEQUENCE_ACTION_BYTE_COST_MULTIPLIER: u128 = 1; + +#[derive(Default)] +pub(crate) struct SequenceComponent; + +#[async_trait::async_trait] +impl Component for SequenceComponent { + type AppState = GenesisState; + + #[instrument(name = "SequenceComponent::init_chain", skip(state))] + async fn init_chain(mut state: S, app_state: &Self::AppState) -> Result<()> { + state.put_sequence_action_base_fee(DEFAULT_SEQUENCE_ACTION_BASE_FEE); + state + .put_sequence_action_byte_cost_multiplier(DEFAULT_SEQUENCE_ACTION_BYTE_COST_MULTIPLIER); + Ok(()) + } + + #[instrument(name = "SequenceComponent::begin_block", skip(_state))] + async fn begin_block( + _state: &mut Arc, + _begin_block: &BeginBlock, + ) -> Result<()> { + Ok(()) + } + + #[instrument(name = "SequenceComponent::end_block", skip(_state))] + async fn end_block( + _state: &mut Arc, + _end_block: &EndBlock, + ) -> Result<()> { + Ok(()) + } +} diff --git a/crates/astria-sequencer/src/sequence/mod.rs b/crates/astria-sequencer/src/sequence/mod.rs new file mode 100644 index 0000000000..4295cd7cf9 --- /dev/null +++ b/crates/astria-sequencer/src/sequence/mod.rs @@ -0,0 +1,5 @@ +pub(crate) mod action; +pub(crate) mod component; +pub(crate) mod state_ext; + +pub(crate) use action::calculate_fee_from_state; diff --git a/crates/astria-sequencer/src/sequence/state_ext.rs b/crates/astria-sequencer/src/sequence/state_ext.rs new file mode 100644 index 0000000000..b17de18466 --- /dev/null +++ b/crates/astria-sequencer/src/sequence/state_ext.rs @@ -0,0 +1,70 @@ +use anyhow::{ + anyhow, + Context, + Result, +}; +use async_trait::async_trait; +use borsh::{ + BorshDeserialize, + BorshSerialize, +}; +use cnidarium::{ + StateRead, + StateWrite, +}; +use tracing::instrument; + +const SEQUENCE_ACTION_BASE_FEE_STORAGE_KEY: &str = "seqbasefee"; +const SEQUENCE_ACTION_BYTE_COST_MULTIPLIER_STORAGE_KEY: &str = "seqmultiplier"; + +/// Newtype wrapper to read and write a u128 from rocksdb. +#[derive(BorshSerialize, BorshDeserialize, Debug)] +struct Fee(u128); + +#[async_trait] +pub(crate) trait StateReadExt: StateRead { + #[instrument(skip(self))] + async fn get_sequence_action_base_fee(&self) -> Result { + let bytes = self + .get_raw(SEQUENCE_ACTION_BASE_FEE_STORAGE_KEY) + .await + .context("failed reading raw sequence action base fee from state")? + .ok_or_else(|| anyhow!("sequence action base fee not found"))?; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } + + #[instrument(skip(self))] + async fn get_sequence_action_byte_cost_multiplier(&self) -> Result { + let bytes = self + .get_raw(SEQUENCE_ACTION_BYTE_COST_MULTIPLIER_STORAGE_KEY) + .await + .context("failed reading raw sequence action byte cost multiplier from state")? + .ok_or_else(|| anyhow!("sequence action byte cost multiplier not found"))?; + let Fee(fee) = Fee::try_from_slice(&bytes).context("invalid fee bytes")?; + Ok(fee) + } +} + +impl StateReadExt for T {} + +#[async_trait] +pub(crate) trait StateWriteExt: StateWrite { + #[instrument(skip(self))] + fn put_sequence_action_base_fee(&mut self, fee: u128) { + self.put_raw( + SEQUENCE_ACTION_BASE_FEE_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), + ); + } + + #[instrument(skip(self))] + fn put_sequence_action_byte_cost_multiplier(&mut self, fee: u128) { + self.put_raw( + SEQUENCE_ACTION_BYTE_COST_MULTIPLIER_STORAGE_KEY.to_string(), + borsh::to_vec(&Fee(fee)).expect("failed to serialize fee"), + ); + } +} + +impl StateWriteExt for T {} diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index a14097634b..c2b924803e 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -20,16 +20,13 @@ use astria_core::{ use tracing::instrument; use crate::{ - accounts::{ - state_ext::{ - StateReadExt, - StateWriteExt, - }, + accounts::state_ext::{ + StateReadExt, + StateWriteExt, }, - bridge::init_bridge_account_action::INIT_BRIDGE_ACCOUNT_FEE, + bridge::state_ext::StateReadExt as _, ibc::{ host_interface::AstriaHost, - ics20_withdrawal::ICS20_WITHDRAWAL_FEE, state_ext::StateReadExt as _, }, state_ext::StateReadExt as _, @@ -71,8 +68,19 @@ pub(crate) async fn check_balance_mempool( state: &S, ) -> anyhow::Result<()> { use std::collections::HashMap; - - let transfer_fee = state.get_transfer_base_fee().await.context("failed to get transfer base fee")?; + + let transfer_fee = state + .get_transfer_base_fee() + .await + .context("failed to get transfer base fee")?; + let ics20_withdrawal_fee = state + .get_ics20_withdrawal_base_fee() + .await + .context("failed to get ics20 withdrawal base fee")?; + let init_bridge_account_fee = state + .get_init_bridge_account_base_fee() + .await + .context("failed to get init bridge account base fee")?; let signer_address = Address::from_verification_key(tx.verification_key()); let mut fees_by_asset = HashMap::new(); @@ -89,7 +97,8 @@ pub(crate) async fn check_balance_mempool( .or_insert(transfer_fee); } Action::Sequence(act) => { - let fee = crate::sequence::calculate_fee(&act.data) + let fee = crate::sequence::calculate_fee_from_state(&act.data, state) + .await .context("fee for sequence action overflowed; data too large")?; fees_by_asset .entry(act.fee_asset_id) @@ -103,14 +112,14 @@ pub(crate) async fn check_balance_mempool( .or_insert(act.amount()); fees_by_asset .entry(*act.fee_asset_id()) - .and_modify(|amt| *amt += ICS20_WITHDRAWAL_FEE) - .or_insert(ICS20_WITHDRAWAL_FEE); + .and_modify(|amt| *amt += ics20_withdrawal_fee) + .or_insert(ics20_withdrawal_fee); } Action::InitBridgeAccount(act) => { fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += INIT_BRIDGE_ACCOUNT_FEE) - .or_insert(INIT_BRIDGE_ACCOUNT_FEE); + .and_modify(|amt| *amt += init_bridge_account_fee) + .or_insert(init_bridge_account_fee); } Action::BridgeLock(act) => { fees_by_asset @@ -460,7 +469,13 @@ mod test { use cnidarium::StateDelta; use super::*; - use crate::app::test_utils::*; + use crate::{ + accounts::state_ext::StateWriteExt as _, + app::test_utils::*, + bridge::state_ext::StateWriteExt, + ibc::state_ext::StateWriteExt as _, + sequence::state_ext::StateWriteExt as _, + }; #[tokio::test] async fn check_balance_mempool_ok() { @@ -468,7 +483,15 @@ mod test { let snapshot = storage.latest_snapshot(); let mut state_tx = StateDelta::new(snapshot); - state_tx.put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE).unwrap(); + state_tx + .put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE) + .unwrap(); + state_tx.put_sequence_action_base_fee(0); + state_tx.put_sequence_action_byte_cost_multiplier(1); + state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); + state_tx.put_init_bridge_account_base_fee(12); + state_tx.put_bridge_lock_byte_cost_multiplier(1); + crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); let other_asset = Denom::from_base_denom("other").id(); @@ -481,7 +504,10 @@ mod test { .increase_balance( alice_address, native_asset, - transfer_fee + crate::sequence::calculate_fee(&data).unwrap(), + transfer_fee + + crate::sequence::calculate_fee_from_state(&data, &state_tx) + .await + .unwrap(), ) .await .unwrap(); @@ -524,8 +550,16 @@ mod test { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state_tx = StateDelta::new(snapshot); - - state_tx.put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE).unwrap(); + + state_tx + .put_transfer_base_fee(crate::accounts::component::DEFAULT_TRANSFER_BASE_FEE) + .unwrap(); + state_tx.put_sequence_action_base_fee(0); + state_tx.put_sequence_action_byte_cost_multiplier(1); + state_tx.put_ics20_withdrawal_base_fee(1).unwrap(); + state_tx.put_init_bridge_account_base_fee(12); + state_tx.put_bridge_lock_byte_cost_multiplier(1); + crate::asset::initialize_native_asset(DEFAULT_NATIVE_ASSET_DENOM); let native_asset = crate::asset::get_native_asset().id(); let other_asset = Denom::from_base_denom("other").id(); @@ -538,7 +572,10 @@ mod test { .increase_balance( alice_address, native_asset, - transfer_fee + crate::sequence::calculate_fee(&data).unwrap(), + transfer_fee + + crate::sequence::calculate_fee_from_state(&data, &state_tx) + .await + .unwrap(), ) .await .unwrap(); From aaee4a43ef5db0bea5cc4eae4f281970e21a4d73 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 29 Apr 2024 12:16:13 -0400 Subject: [PATCH 06/11] fmt --- crates/astria-sequencer/src/app.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/astria-sequencer/src/app.rs b/crates/astria-sequencer/src/app.rs index 6398267714..57b7580fdd 100644 --- a/crates/astria-sequencer/src/app.rs +++ b/crates/astria-sequencer/src/app.rs @@ -1445,7 +1445,9 @@ mod test { // figure out needed fee for a single transfer let data = b"hello world".to_vec(); - let fee = calculate_fee_from_state(&data, &app.state.clone()).await.unwrap(); + let fee = calculate_fee_from_state(&data, &app.state.clone()) + .await + .unwrap(); // transfer just enough to cover single sequence fee with data let signed_tx = UnsignedTransaction { From 98e25d62ad48e6c8442b9e82cd4f42700e05ff20 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 29 Apr 2024 12:36:44 -0400 Subject: [PATCH 07/11] use saturating arithmetic, change deposit size calc to runtime --- crates/astria-sequencer/src/app.rs | 18 +++--- .../src/bridge/bridge_lock_action.rs | 61 ++++++++++++------- crates/astria-sequencer/src/bridge/mod.rs | 2 +- .../astria-sequencer/src/transaction/mod.rs | 16 ++--- 4 files changed, 56 insertions(+), 41 deletions(-) diff --git a/crates/astria-sequencer/src/app.rs b/crates/astria-sequencer/src/app.rs index 57b7580fdd..30be511afd 100644 --- a/crates/astria-sequencer/src/app.rs +++ b/crates/astria-sequencer/src/app.rs @@ -2052,13 +2052,21 @@ mod test { app.execute_transaction(signed_tx).await.unwrap(); assert_eq!(app.state.get_account_nonce(alice_address).await.unwrap(), 1); let transfer_fee = app.state.get_transfer_base_fee().await.unwrap(); + let expected_deposit = Deposit::new( + bridge_address, + rollup_id, + amount, + asset_id, + "nootwashere".to_string(), + ); + let fee = transfer_fee + app .state .get_bridge_lock_byte_cost_multiplier() .await .unwrap() - * crate::bridge::DEPOSIT_BYTE_LEN; + * crate::bridge::get_deposit_byte_len(&expected_deposit); assert_eq!( app.state .get_account_balance(alice_address, asset_id) @@ -2074,14 +2082,6 @@ mod test { bridge_before_balance + amount ); - let expected_deposit = Deposit::new( - bridge_address, - rollup_id, - amount, - asset_id, - "nootwashere".to_string(), - ); - let deposits = app.state.get_deposit_events(&rollup_id).await.unwrap(); assert_eq!(deposits.len(), 1); assert_eq!(deposits[0], expected_deposit); diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index b3aa18d687..c99324805c 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -32,8 +32,6 @@ use crate::{ transaction::action_handler::ActionHandler, }; -pub(crate) const DEPOSIT_BYTE_LEN: u128 = std::mem::size_of::() as u128; - #[async_trait::async_trait] impl ActionHandler for BridgeLockAction { async fn check_stateful( @@ -49,13 +47,11 @@ impl ActionHandler for BridgeLockAction { }; // ensure the recipient is a bridge account. - ensure!( - state - .get_bridge_account_rollup_id(&self.to) - .await? - .is_some(), - "bridge lock must be sent to a bridge account", - ); + let rollup_id = state + .get_bridge_account_rollup_id(&self.to) + .await + .context("failed to get bridge account rollup id")? + .ok_or_else(|| anyhow::anyhow!("bridge lock must be sent to a bridge account"))?; let allowed_asset_id = state .get_bridge_account_asset_ids(&self.to) @@ -75,11 +71,21 @@ impl ActionHandler for BridgeLockAction { .await .context("failed to get transfer base fee")?; + let deposit = Deposit::new( + self.to, + rollup_id, + self.amount, + self.asset_id, + self.destination_chain_address.clone(), + ); + let byte_cost_multiplier = state .get_bridge_lock_byte_cost_multiplier() .await .context("failed to get byte cost multiplier")?; - let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN + transfer_fee; + let fee = byte_cost_multiplier + .saturating_mul(get_deposit_byte_len(&deposit)) + .saturating_add(transfer_fee); ensure!(from_balance >= fee, "insuffient funds for fee payment",); // this performs the same checks as a normal `TransferAction`, @@ -102,31 +108,33 @@ impl ActionHandler for BridgeLockAction { .await .context("failed to execute bridge lock action as transfer action")?; + let rollup_id = state + .get_bridge_account_rollup_id(&self.to) + .await + .context("failed to get bridge account rollup id")? + .expect("recipient must be a bridge account; this is a bug in check_stateful"); + + let deposit = Deposit::new( + self.to, + rollup_id, + self.amount, + self.asset_id, + self.destination_chain_address.clone(), + ); + // the transfer fee is already deducted in `transfer_action.execute()`, // so we just deduct the bridge lock byte multiplier fee. let byte_cost_multiplier = state .get_bridge_lock_byte_cost_multiplier() .await .context("failed to get byte cost multiplier")?; - let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN; + let fee = byte_cost_multiplier.saturating_mul(get_deposit_byte_len(&deposit)); state .decrease_balance(from, self.fee_asset_id, fee) .await .context("failed to deduct fee from account balance")?; - let rollup_id = state - .get_bridge_account_rollup_id(&self.to) - .await? - .expect("recipient must be a bridge account; this is a bug in check_stateful"); - - let deposit = Deposit::new( - self.to, - rollup_id, - self.amount, - self.asset_id, - self.destination_chain_address.clone(), - ); state .put_deposit_event(deposit) .await @@ -134,3 +142,10 @@ impl ActionHandler for BridgeLockAction { Ok(()) } } + +/// returns the length of a serialized `Deposit` message. +pub(crate) fn get_deposit_byte_len(deposit: &Deposit) -> u128 { + use prost::Message as _; + let raw = deposit.clone().into_raw(); + raw.encoded_len() as u128 +} diff --git a/crates/astria-sequencer/src/bridge/mod.rs b/crates/astria-sequencer/src/bridge/mod.rs index 5dea27f34a..09ff1b295e 100644 --- a/crates/astria-sequencer/src/bridge/mod.rs +++ b/crates/astria-sequencer/src/bridge/mod.rs @@ -4,4 +4,4 @@ pub(crate) mod init_bridge_account_action; pub(crate) mod state_ext; #[cfg(test)] -pub(crate) use bridge_lock_action::DEPOSIT_BYTE_LEN; +pub(crate) use bridge_lock_action::get_deposit_byte_len; diff --git a/crates/astria-sequencer/src/transaction/mod.rs b/crates/astria-sequencer/src/transaction/mod.rs index f04dd3a3fa..0dc475c852 100644 --- a/crates/astria-sequencer/src/transaction/mod.rs +++ b/crates/astria-sequencer/src/transaction/mod.rs @@ -100,11 +100,11 @@ pub(crate) async fn check_balance_for_total_fees( Action::Transfer(act) => { fees_by_asset .entry(act.asset_id) - .and_modify(|amt| *amt += act.amount) + .and_modify(|amt: &mut u128| *amt = amt.saturating_add(act.amount)) .or_insert(act.amount); fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += transfer_fee) + .and_modify(|amt| *amt = amt.saturating_add(transfer_fee)) .or_insert(transfer_fee); } Action::Sequence(act) => { @@ -113,33 +113,33 @@ pub(crate) async fn check_balance_for_total_fees( .context("fee for sequence action overflowed; data too large")?; fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += fee) + .and_modify(|amt| *amt = amt.saturating_add(fee)) .or_insert(fee); } Action::Ics20Withdrawal(act) => { fees_by_asset .entry(act.denom().id()) - .and_modify(|amt| *amt += act.amount()) + .and_modify(|amt| *amt = amt.saturating_add(act.amount())) .or_insert(act.amount()); fees_by_asset .entry(*act.fee_asset_id()) - .and_modify(|amt| *amt += ics20_withdrawal_fee) + .and_modify(|amt| *amt = amt.saturating_add(ics20_withdrawal_fee)) .or_insert(ics20_withdrawal_fee); } Action::InitBridgeAccount(act) => { fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += init_bridge_account_fee) + .and_modify(|amt| *amt = amt.saturating_add(init_bridge_account_fee)) .or_insert(init_bridge_account_fee); } Action::BridgeLock(act) => { fees_by_asset .entry(act.asset_id) - .and_modify(|amt| *amt += act.amount) + .and_modify(|amt| *amt = amt.saturating_add(act.amount)) .or_insert(act.amount); fees_by_asset .entry(act.fee_asset_id) - .and_modify(|amt| *amt += transfer_fee) + .and_modify(|amt| *amt = amt.saturating_add(transfer_fee)) .or_insert(transfer_fee); } Action::ValidatorUpdate(_) From 59415e97fb5890cbae7eb81ad700aec960010705 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 29 Apr 2024 15:13:02 -0400 Subject: [PATCH 08/11] add unit tests for bridge lock fee calc --- .../src/bridge/bridge_lock_action.rs | 142 +++++++++++++++++- .../src/sequence/state_ext.rs | 38 +++++ 2 files changed, 179 insertions(+), 1 deletion(-) diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index c99324805c..d15b875ddc 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -86,7 +86,7 @@ impl ActionHandler for BridgeLockAction { let fee = byte_cost_multiplier .saturating_mul(get_deposit_byte_len(&deposit)) .saturating_add(transfer_fee); - ensure!(from_balance >= fee, "insuffient funds for fee payment",); + ensure!(from_balance >= fee, "insufficient funds for fee payment"); // this performs the same checks as a normal `TransferAction`, // but without the check that prevents transferring to a bridge account, @@ -149,3 +149,143 @@ pub(crate) fn get_deposit_byte_len(deposit: &Deposit) -> u128 { let raw = deposit.clone().into_raw(); raw.encoded_len() as u128 } + +#[cfg(test)] +mod test { + use astria_core::primitive::v1::{ + asset, + RollupId, + }; + use cnidarium::StateDelta; + + use super::*; + + #[tokio::test] + async fn bridge_lock_check_stateful_fee_calc() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + state.put_transfer_base_fee(12).unwrap(); + state.put_bridge_lock_byte_cost_multiplier(2); + + let bridge_address = Address::from([1; 20]); + let asset_id = asset::Id::from_denom("test"); + let bridge_lock = BridgeLockAction { + to: bridge_address, + asset_id, + amount: 100, + fee_asset_id: asset_id, + destination_chain_address: "someaddress".to_string(), + }; + + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let from_address = Address::from([2; 20]); + + // not enough balance; should fail + state + .put_account_balance(from_address, asset_id, 100) + .unwrap(); + println!( + "{}", + bridge_lock + .check_stateful(&state, from_address) + .await + .unwrap_err() + .to_string() + ); + assert!( + bridge_lock + .check_stateful(&state, from_address) + .await + .unwrap_err() + .to_string() + .contains("insufficient funds for fee payment") + ); + + // enough balance; should pass + let expected_deposit_fee = 12 + + get_deposit_byte_len(&Deposit::new( + bridge_address, + rollup_id, + 100, + asset_id, + "someaddress".to_string(), + )) * 2; + state + .put_account_balance(from_address, asset_id, 100 + expected_deposit_fee) + .unwrap(); + bridge_lock + .check_stateful(&state, from_address) + .await + .unwrap(); + } + + #[tokio::test] + async fn bridge_lock_execute_fee_calc() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + state.put_transfer_base_fee(12).unwrap(); + state.put_bridge_lock_byte_cost_multiplier(2); + + let bridge_address = Address::from([1; 20]); + let asset_id = asset::Id::from_denom("test"); + let bridge_lock = BridgeLockAction { + to: bridge_address, + asset_id, + amount: 100, + fee_asset_id: asset_id, + destination_chain_address: "someaddress".to_string(), + }; + + let rollup_id = RollupId::from_unhashed_bytes(b"test_rollup_id"); + state.put_bridge_account_rollup_id(&bridge_address, &rollup_id); + state + .put_bridge_account_asset_id(&bridge_address, &asset_id) + .unwrap(); + state.put_allowed_fee_asset(asset_id); + + let from_address = Address::from([2; 20]); + + // not enough balance; should fail + state + .put_account_balance(from_address, asset_id, 100) + .unwrap(); + println!( + "{}", + bridge_lock + .check_stateful(&state, from_address) + .await + .unwrap_err() + .to_string() + ); + assert!( + bridge_lock + .execute(&mut state, from_address) + .await + .unwrap_err() + .to_string() + .contains("failed to deduct fee from account balance") + ); + + // enough balance; should pass + let expected_deposit_fee = 12 + + get_deposit_byte_len(&Deposit::new( + bridge_address, + rollup_id, + 100, + asset_id, + "someaddress".to_string(), + )) * 2; + state + .put_account_balance(from_address, asset_id, 100 + expected_deposit_fee) + .unwrap(); + bridge_lock.execute(&mut state, from_address).await.unwrap(); + } +} diff --git a/crates/astria-sequencer/src/sequence/state_ext.rs b/crates/astria-sequencer/src/sequence/state_ext.rs index b17de18466..50345e42e5 100644 --- a/crates/astria-sequencer/src/sequence/state_ext.rs +++ b/crates/astria-sequencer/src/sequence/state_ext.rs @@ -68,3 +68,41 @@ pub(crate) trait StateWriteExt: StateWrite { } impl StateWriteExt for T {} + +#[cfg(test)] +mod test { + use cnidarium::StateDelta; + + use super::{ + StateReadExt as _, + StateWriteExt as _, + }; + + #[tokio::test] + async fn sequence_action_base_fee() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let fee = 42; + state.put_sequence_action_base_fee(fee); + assert_eq!(state.get_sequence_action_base_fee().await.unwrap(), fee); + } + + #[tokio::test] + async fn sequence_action_byte_cost_multiplier() { + let storage = cnidarium::TempStorage::new().await.unwrap(); + let snapshot = storage.latest_snapshot(); + let mut state = StateDelta::new(snapshot); + + let fee = 42; + state.put_sequence_action_byte_cost_multiplier(fee); + assert_eq!( + state + .get_sequence_action_byte_cost_multiplier() + .await + .unwrap(), + fee + ); + } +} From 4d1ebff5cd6c8f6ba2251b7d61a6bf060c414ea0 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 29 Apr 2024 15:15:29 -0400 Subject: [PATCH 09/11] cleanup --- .../src/bridge/bridge_lock_action.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index d15b875ddc..2a03760589 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -191,14 +191,6 @@ mod test { state .put_account_balance(from_address, asset_id, 100) .unwrap(); - println!( - "{}", - bridge_lock - .check_stateful(&state, from_address) - .await - .unwrap_err() - .to_string() - ); assert!( bridge_lock .check_stateful(&state, from_address) @@ -257,14 +249,6 @@ mod test { state .put_account_balance(from_address, asset_id, 100) .unwrap(); - println!( - "{}", - bridge_lock - .check_stateful(&state, from_address) - .await - .unwrap_err() - .to_string() - ); assert!( bridge_lock .execute(&mut state, from_address) From 1e47bcbf317d019d75aeffcca3f00e2ced99e8f0 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Mon, 29 Apr 2024 15:36:37 -0400 Subject: [PATCH 10/11] fix test --- .../src/bridge/bridge_lock_action.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs index 2a03760589..4be9cb3e5a 100644 --- a/crates/astria-sequencer/src/bridge/bridge_lock_action.rs +++ b/crates/astria-sequencer/src/bridge/bridge_lock_action.rs @@ -165,7 +165,8 @@ mod test { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); - state.put_transfer_base_fee(12).unwrap(); + let transfer_fee = 12; + state.put_transfer_base_fee(transfer_fee).unwrap(); state.put_bridge_lock_byte_cost_multiplier(2); let bridge_address = Address::from([1; 20]); @@ -201,7 +202,7 @@ mod test { ); // enough balance; should pass - let expected_deposit_fee = 12 + let expected_deposit_fee = transfer_fee + get_deposit_byte_len(&Deposit::new( bridge_address, rollup_id, @@ -223,7 +224,8 @@ mod test { let storage = cnidarium::TempStorage::new().await.unwrap(); let snapshot = storage.latest_snapshot(); let mut state = StateDelta::new(snapshot); - state.put_transfer_base_fee(12).unwrap(); + let transfer_fee = 12; + state.put_transfer_base_fee(transfer_fee).unwrap(); state.put_bridge_lock_byte_cost_multiplier(2); let bridge_address = Address::from([1; 20]); @@ -247,7 +249,7 @@ mod test { // not enough balance; should fail state - .put_account_balance(from_address, asset_id, 100) + .put_account_balance(from_address, asset_id, 100 + transfer_fee) .unwrap(); assert!( bridge_lock @@ -255,11 +257,11 @@ mod test { .await .unwrap_err() .to_string() - .contains("failed to deduct fee from account balance") + .eq("failed to deduct fee from account balance") ); // enough balance; should pass - let expected_deposit_fee = 12 + let expected_deposit_fee = transfer_fee + get_deposit_byte_len(&Deposit::new( bridge_address, rollup_id, From 14fff865c7b63168dcac73ab13b0de8dd7c25600 Mon Sep 17 00:00:00 2001 From: elizabeth Date: Tue, 30 Apr 2024 13:32:10 -0400 Subject: [PATCH 11/11] context --- .../src/bridge/init_bridge_account_action.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs index 2f246d0699..86cd28a561 100644 --- a/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs +++ b/crates/astria-sequencer/src/bridge/init_bridge_account_action.rs @@ -38,7 +38,10 @@ impl ActionHandler for InitBridgeAccountAction { "invalid fee asset", ); - let fee = state.get_init_bridge_account_base_fee().await?; + let fee = state + .get_init_bridge_account_base_fee() + .await + .context("failed to get base fee for initializing bridge account")?; // this prevents the address from being registered as a bridge account // if it's been previously initialized as a bridge account. @@ -70,7 +73,10 @@ impl ActionHandler for InitBridgeAccountAction { #[instrument(skip_all)] async fn execute(&self, state: &mut S, from: Address) -> Result<()> { - let fee = state.get_init_bridge_account_base_fee().await?; + let fee = state + .get_init_bridge_account_base_fee() + .await + .context("failed to get base fee for initializing bridge account")?; state.put_bridge_account_rollup_id(&from, &self.rollup_id); state