Merge branch 'tiago/rebased-validate-tx-bytes-len' (#2187)

* tiago/rebased-validate-tx-bytes-len:
  Changelog
  Update masp proofs
  New unit tests
  Validate tx bytes in CheckTx and ProcessProposal
  Add too large tx error
  Validate tx sizes storage api
  Add max tx bytes to genesis template
  Fix docstr
  Add max tx bytes protocol param
  Tune block and mempool CometBFT params

Author: adrianbrink

.changelog/unreleased/improvements/2131-validate-tx-bytes-len.md

@@ -0,0 +1,2 @@
+- Tighten security around potential P2P issues
+  ([\#2131](https://github.com/anoma/namada/pull/2131))

apps/src/lib/config/genesis.rs

@@ -153,7 +153,7 @@ pub struct ImplicitAccount {
     BorshDeserialize,
 )]
 pub struct Parameters {
-    // Max payload size, in bytes, for a tx batch proposal.
+    /// Max payload size, in bytes, for a tx batch proposal.
     pub max_proposal_bytes: ProposalBytes,
     /// Max block gas
     pub max_block_gas: u64,

apps/src/lib/config/genesis/chain.rs

@@ -262,6 +262,7 @@ impl Finalized {
             fee_unshielding_descriptions_limit,
             max_block_gas,
             minimum_gas_price,
+            max_tx_bytes,
             ..
         } = self.parameters.parameters.clone();
 
@@ -291,6 +292,7 @@ impl Finalized {
         let pos_inflation_amount = 0;
 
         namada::ledger::parameters::Parameters {
+            max_tx_bytes,
             epoch_duration,
             max_expected_time_per_block,
             vp_whitelist,

apps/src/lib/config/genesis/templates.rs

@@ -242,6 +242,9 @@ pub struct Parameters<T: TemplateValidation> {
     Eq,
 )]
 pub struct ChainParams<T: TemplateValidation> {
+    /// Max payload size, in bytes, for a tx decided through
+    /// the consensus protocol.
+    pub max_tx_bytes: u32,
     /// Name of the native token - this must one of the tokens from
     /// `tokens.toml` file
     pub native_token: Alias,
@@ -296,6 +299,7 @@ impl ChainParams<Unvalidated> {
         tokens: &Tokens,
     ) -> eyre::Result<ChainParams<Validated>> {
         let ChainParams {
+            max_tx_bytes,
             native_token,
             min_num_of_blocks,
             max_expected_time_per_block,
@@ -342,6 +346,7 @@ impl ChainParams<Unvalidated> {
         }
 
         Ok(ChainParams {
+            max_tx_bytes,
             native_token,
             min_num_of_blocks,
             max_expected_time_per_block,

apps/src/lib/node/ledger/shell/mod.rs

@@ -48,6 +48,7 @@ use namada::ledger::storage::{
     DBIter, Sha256Hasher, Storage, StorageHasher, TempWlStorage, WlStorage, DB,
     EPOCH_SWITCH_BLOCKS_DELAY,
 };
+use namada::ledger::storage_api::tx::validate_tx_bytes;
 use namada::ledger::storage_api::{self, StorageRead};
 use namada::ledger::{parameters, pos, protocol};
 use namada::proof_of_stake::{self, process_slashes, read_pos_params, slash};
@@ -154,6 +155,7 @@ pub enum ErrorCodes {
     TxGasLimit = 11,
     FeeError = 12,
     InvalidVoteExtension = 13,
+    TooLarge = 14,
 }
 
 impl ErrorCodes {
@@ -167,7 +169,8 @@ impl ErrorCodes {
             Ok | WasmRuntimeError => true,
             InvalidTx | InvalidSig | InvalidOrder | ExtraTxs
             | Undecryptable | AllocationError | ReplayTx | InvalidChainId
-            | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension => false,
+            | ExpiredTx | TxGasLimit | FeeError | InvalidVoteExtension
+            | TooLarge => false,
         }
     }
 }
@@ -1073,6 +1076,18 @@ where
         const VALID_MSG: &str = "Mempool validation passed";
         const INVALID_MSG: &str = "Mempool validation failed";
 
+        // check tx bytes
+        //
+        // NB: always keep this as the first tx check,
+        // as it is a pretty cheap one
+        if !validate_tx_bytes(&self.wl_storage, tx_bytes.len())
+            .expect("Failed to get max tx bytes param from storage")
+        {
+            response.code = ErrorCodes::TooLarge.into();
+            response.log = format!("{INVALID_MSG}: Tx too large");
+            return response;
+        }
+
         // Tx format check
         let tx = match Tx::try_from(tx_bytes).map_err(Error::TxDecoding) {
             Ok(t) => t,
@@ -2091,6 +2106,7 @@ mod test_utils {
             .new_epoch(BlockHeight(1));
         // initialize parameter storage
         let params = Parameters {
+            max_tx_bytes: 1024 * 1024,
             epoch_duration: EpochDuration {
                 min_num_of_blocks: 1,
                 min_duration: DurationSecs(3600),
@@ -2945,4 +2961,57 @@ mod shell_tests {
         );
         assert_eq!(result.code, ErrorCodes::FeeError.into());
     }
+
+    /// Test max tx bytes parameter in CheckTx
+    #[test]
+    fn test_max_tx_bytes_check_tx() {
+        let (shell, _recv, _, _) = test_utils::setup();
+
+        let max_tx_bytes: u32 = {
+            let key = parameters::storage::get_max_tx_bytes_key();
+            shell
+                .wl_storage
+                .read(&key)
+                .expect("Failed to read from storage")
+                .expect("Max tx bytes should have been written to storage")
+        };
+
+        let new_tx = |size: u32| {
+            let keypair = super::test_utils::gen_keypair();
+            let mut wrapper =
+                Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new(
+                    Fee {
+                        amount_per_gas_unit: 100.into(),
+                        token: shell.wl_storage.storage.native_token.clone(),
+                    },
+                    keypair.ref_to(),
+                    Epoch(0),
+                    GAS_LIMIT_MULTIPLIER.into(),
+                    None,
+                ))));
+            wrapper.header.chain_id = shell.chain_id.clone();
+            wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
+            wrapper.set_data(Data::new(vec![0; size as usize]));
+            wrapper.add_section(Section::Signature(Signature::new(
+                wrapper.sechashes(),
+                [(0, keypair)].into_iter().collect(),
+                None,
+            )));
+            wrapper
+        };
+
+        // test a small tx
+        let result = shell.mempool_validate(
+            new_tx(50).to_bytes().as_ref(),
+            MempoolTxType::NewTransaction,
+        );
+        assert!(result.code != ErrorCodes::TooLarge.into());
+
+        // max tx bytes + 1, on the other hand, is not
+        let result = shell.mempool_validate(
+            new_tx(max_tx_bytes + 1).to_bytes().as_ref(),
+            MempoolTxType::NewTransaction,
+        );
+        assert_eq!(result.code, ErrorCodes::TooLarge.into());
+    }
 }

apps/src/lib/node/ledger/shell/process_proposal.rs

@@ -7,6 +7,7 @@ use namada::core::ledger::storage::WlStorage;
 use namada::ledger::pos::PosQueries;
 use namada::ledger::protocol::get_fee_unshielding_transaction;
 use namada::ledger::storage::TempWlStorage;
+use namada::ledger::storage_api::tx::validate_tx_bytes;
 use namada::proof_of_stake::find_validator_by_raw_hash;
 use namada::types::internal::TxInQueue;
 use namada::types::transaction::protocol::{
@@ -266,6 +267,19 @@ where
     where
         CA: 'static + WasmCacheAccess + Sync,
     {
+        // check tx bytes
+        //
+        // NB: always keep this as the first tx check,
+        // as it is a pretty cheap one
+        if !validate_tx_bytes(&self.wl_storage, tx_bytes.len())
+            .expect("Failed to get max tx bytes param from storage")
+        {
+            return TxResult {
+                code: ErrorCodes::TooLarge.into(),
+                info: "Tx too large".into(),
+            };
+        }
+
         // try to allocate space for this tx
         if let Err(e) = metadata.txs_bin.try_dump(tx_bytes) {
             return TxResult {
@@ -2039,4 +2053,69 @@ mod test_process_proposal {
             );
         }
     }
+
+    /// Test max tx bytes parameter in ProcessProposal
+    #[test]
+    fn test_max_tx_bytes_process_proposal() {
+        use namada::ledger::parameters::storage::get_max_tx_bytes_key;
+        let (shell, _recv, _, _) = test_utils::setup_at_height(3u64);
+
+        let max_tx_bytes: u32 = {
+            let key = get_max_tx_bytes_key();
+            shell
+                .wl_storage
+                .read(&key)
+                .expect("Failed to read from storage")
+                .expect("Max tx bytes should have been written to storage")
+        };
+
+        let new_tx = |size: u32| {
+            let keypair = super::test_utils::gen_keypair();
+            let mut wrapper =
+                Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new(
+                    Fee {
+                        amount_per_gas_unit: 100.into(),
+                        token: shell.wl_storage.storage.native_token.clone(),
+                    },
+                    keypair.ref_to(),
+                    Epoch(0),
+                    GAS_LIMIT_MULTIPLIER.into(),
+                    None,
+                ))));
+            wrapper.header.chain_id = shell.chain_id.clone();
+            wrapper.set_code(Code::new("wasm_code".as_bytes().to_owned()));
+            wrapper.set_data(Data::new(vec![0; size as usize]));
+            wrapper.add_section(Section::Signature(Signature::new(
+                wrapper.sechashes(),
+                [(0, keypair)].into_iter().collect(),
+                None,
+            )));
+            wrapper
+        };
+
+        let request = ProcessProposal {
+            txs: vec![new_tx(max_tx_bytes + 1).to_bytes()],
+        };
+        match shell.process_proposal(request) {
+            Ok(_) => panic!("Test failed"),
+            Err(TestError::RejectProposal(response)) => {
+                assert_eq!(
+                    response[0].result.code,
+                    u32::from(ErrorCodes::TooLarge)
+                );
+            }
+        }
+
+        let request = ProcessProposal {
+            txs: vec![new_tx(0).to_bytes()],
+        };
+        match shell.process_proposal(request) {
+            Ok(_) => panic!("Test failed"),
+            Err(TestError::RejectProposal(response)) => {
+                assert!(
+                    response[0].result.code != u32::from(ErrorCodes::TooLarge)
+                );
+            }
+        }
+    }
 }

apps/src/lib/node/ledger/storage/mod.rs

@@ -146,6 +146,7 @@ mod tests {
         let mut wl_storage = WlStorage::new(WriteLog::default(), storage);
         // initialize parameter storage
         let params = Parameters {
+            max_tx_bytes: 1024 * 1024,
             epoch_duration: EpochDuration {
                 min_num_of_blocks: 1,
                 min_duration: DurationSecs(3600),

apps/src/lib/node/ledger/tendermint_node.rs

@@ -347,22 +347,45 @@ pub fn id_from_pk(pk: &common::PublicKey) -> TendermintNodeId {
 
 async fn update_tendermint_config(
     home_dir: impl AsRef<Path>,
-    config: TendermintConfig,
+    mut config: TendermintConfig,
 ) -> Result<()> {
     let home_dir = home_dir.as_ref();
     let path = home_dir.join("config").join("config.toml");
-    let mut config = config.clone();
 
     config.moniker =
         Moniker::from_str(&format!("{}-{}", config.moniker, namada_version()))
             .expect("Invalid moniker");
 
     config.consensus.create_empty_blocks = true;
 
-    // We set this to true as we don't want any invalid tx be re-applied. This
-    // also implies that it's not possible for an invalid tx to become valid
-    // again in the future.
-    config.mempool.keep_invalid_txs_in_cache = false;
+    // mempool config
+    // https://forum.cosmos.network/t/our-understanding-of-the-cosmos-hub-mempool-issues/12040
+    {
+        // We set this to true as we don't want any invalid tx be re-applied.
+        // This also implies that it's not possible for an invalid tx to
+        // become valid again in the future.
+        config.mempool.keep_invalid_txs_in_cache = false;
+
+        // Drop txs from the mempool that are larger than 1 MiB
+        //
+        // The application (Namada) can assign arbitrary max tx sizes,
+        // which are subject to consensus. Either way, nodes are able to
+        // configure their local mempool however they please.
+        //
+        // 1 MiB is a reasonable value that allows governance proposal txs
+        // containing wasm code to be proposed by a leading validator
+        // during some round's start
+        config.mempool.max_tx_bytes = 1024 * 1024;
+
+        // Hold 50x the max amount of txs in a block
+        //
+        // 6 MiB is the default Namada max proposal size governance
+        // parameter -> 50 * 6 MiB
+        config.mempool.max_txs_bytes = 50 * 6 * 1024 * 1024;
+
+        // Hold up to 4k txs in the mempool
+        config.mempool.size = 4000;
+    }
 
     // Bumped from the default `1_000_000`, because some WASMs can be
     // quite large
@@ -408,11 +431,11 @@ async fn write_tm_genesis(
         .try_into()
         .expect("Couldn't convert DateTimeUtc to Tendermint Time");
     let size = block::Size {
-        // maximum size of a serialized Tendermint block
-        // cannot go over 100 MiB
-        max_bytes: (100 << 20) - 1, /* unsure if we are dealing with an open
-                                     * range, so it's better to subtract one,
-                                     * here */
+        // maximum size of a serialized Tendermint block.
+        // on Namada, we have a hard-cap of 16 MiB (6 MiB max
+        // txs in a block + 10 MiB reserved for evidence data,
+        // block headers and protobuf serialization overhead)
+        max_bytes: 16 * 1024 * 1024,
         // gas is metered app-side, so we disable it
         // at the Tendermint level
         max_gas: -1,

core/src/ledger/parameters/mod.rs

@@ -35,6 +35,8 @@ pub const ADDRESS: Address = Address::Internal(InternalAddress::Parameters);
     BorshSchema,
 )]
 pub struct Parameters {
+    /// Max payload size, in bytes, for a mempool tx.
+    pub max_tx_bytes: u32,
     /// Epoch duration (read only)
     pub epoch_duration: EpochDuration,
     /// Maximum expected time per block (read only)
@@ -117,6 +119,7 @@ impl Parameters {
         S: StorageRead + StorageWrite,
     {
         let Self {
+            max_tx_bytes,
             epoch_duration,
             max_expected_time_per_block,
             max_proposal_bytes,
@@ -135,6 +138,10 @@ impl Parameters {
             fee_unshielding_descriptions_limit,
         } = self;
 
+        // write max tx bytes parameter
+        let max_tx_bytes_key = storage::get_max_tx_bytes_key();
+        storage.write(&max_tx_bytes_key, max_tx_bytes)?;
+
         // write max proposal bytes parameter
         let max_proposal_bytes_key = storage::get_max_proposal_bytes_key();
         storage.write(&max_proposal_bytes_key, max_proposal_bytes)?;
@@ -542,7 +549,15 @@ where
         .ok_or(ReadError::ParametersMissing)
         .into_storage_result()?;
 
+    // read max tx bytes
+    let max_tx_bytes_key = storage::get_max_tx_bytes_key();
+    let value = storage.read(&max_tx_bytes_key)?;
+    let max_tx_bytes = value
+        .ok_or(ReadError::ParametersMissing)
+        .into_storage_result()?;
+
     Ok(Parameters {
+        max_tx_bytes,
         epoch_duration,
         max_expected_time_per_block,
         max_proposal_bytes,