From f05a4de4f6a676fa00580699807eb802a54ae7db Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 18 Oct 2024 15:45:54 +0200 Subject: [PATCH 1/4] Improves gas error messages --- crates/node/src/shell/mod.rs | 18 +++++++++------- crates/node/src/shell/prepare_proposal.rs | 1 - crates/node/src/shell/process_proposal.rs | 26 +++++++++++++++++++---- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index 755de45ee6..7e97e88199 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -1250,16 +1250,18 @@ where } // Max block gas - let block_gas_limit: Gas = Gas::from_whole_units( - parameters::get_max_block_gas(&self.state).unwrap().into(), - gas_scale, - ) - .expect("Gas limit from parameter must not overflow"); + let max_block_gas = + parameters::get_max_block_gas(&self.state).unwrap(); + let block_gas_limit: Gas = + Gas::from_whole_units(max_block_gas.into(), gas_scale) + .expect("Gas limit from parameter must not overflow"); if gas_meter.tx_gas_limit > block_gas_limit { response.code = ResultCode::AllocationError.into(); - response.log = "{INVALID_MSG}: Wrapper transaction \ - exceeds the maximum block gas limit" - .to_string(); + response.log = format!( + "{INVALID_MSG}: Wrapper transaction exceeds the \ + maximum block gas limit: {}", + max_block_gas + ); return response; } diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index b6a077b511..ad282599d9 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/crates/node/src/shell/prepare_proposal.rs @@ -313,7 +313,6 @@ where super::replay_protection_checks(&tx, temp_state).map_err(|_| ())?; // Check fees and extract the gas limit of this transaction - // TODO(namada#2597): check if masp fee payment is required match prepare_proposal_fee_check( &wrapper, &tx, diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index 1ccac598a0..9371b97db8 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -440,15 +440,33 @@ where } }; let mut tx_gas_meter = TxGasMeter::new(gas_limit); - if tx_gas_meter.add_wrapper_gas(tx_bytes).is_err() - || allocated_gas.is_err() - { + if tx_gas_meter.add_wrapper_gas(tx_bytes).is_err() { return TxResult { code: ResultCode::TxGasLimit.into(), - info: "Wrapper transactions exceeds its gas limit" + info: "Wrapper transaction exceeds its gas limit" .to_string(), }; } + if let Err(e) = allocated_gas { + let info = match e { + AllocFailure::Rejected { bin_resource_left } => { + format!( + "Wrapper transaction exceeds the remaining \ + available gas in the block: {}", + bin_resource_left + ) + } + AllocFailure::OverflowsBin { bin_resource } => format!( + "Wrapper transaction exceeds the maximum block \ + gas limit: {}", + bin_resource + ), + }; + return TxResult { + code: ResultCode::AllocationError.into(), + info, + }; + }; // ChainId check if tx_chain_id != self.chain_id { From ed55c49a1e3d5a26fc31781c7831ec2bc2a466b2 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 18 Oct 2024 15:46:25 +0200 Subject: [PATCH 2/4] Improves docs for masp fee payment gas limit --- crates/node/src/protocol.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 6466c75b83..ad8bdd5723 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -693,10 +693,11 @@ where // The fee payment is subject to a gas limit imposed by a protocol // parameter. Here we instantiate a custom gas meter for this step where the // gas limit is actually the lowest between the protocol parameter and the - // actual remaining gas of the transaction. The latter is because we only - // care about the masp execution, not any gas used before this step, which - // could prevent some transactions (e.g. batches consuming a lot of gas for - // their size) from being accepted + // actual remaining gas of the transaction. The latter is because we want to + // enforce that no tx exceeds its own gas limit, which could happen for + // some transactions (e.g. batches consuming a lot of gas for + // their size) if we were to take their gas limit instead of the remaining + // gas let max_gas_limit = state .read::(¶meters::storage::get_masp_fee_payment_gas_limit_key()) .expect("Error reading the storage") From 8e640a01504b57195db65cd72889ebc8e6e81f86 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 18 Oct 2024 18:40:04 +0200 Subject: [PATCH 3/4] Fixes and extends unit tests for gas --- crates/node/src/shell/prepare_proposal.rs | 42 ++++++++++++++++++++ crates/node/src/shell/process_proposal.rs | 48 ++++++++++++++++++++++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index ad282599d9..8ab4bb90e0 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/crates/node/src/shell/prepare_proposal.rs @@ -986,6 +986,48 @@ mod test_prepare_proposal { assert!(result.txs.is_empty()); } + /// Check that a tx requiring more gas than available in the block is not + /// included + #[test] + fn test_exceeding_available_block_gas_tx() { + let (shell, _recv, _, _) = test_utils::setup(); + + let block_gas_limit = + namada_sdk::parameters::get_max_block_gas(&shell.state).unwrap(); + let keypair = namada_apps_lib::wallet::defaults::albert_keypair(); + + let mut txs = vec![]; + for _ in 0..2 { + let mut wrapper = + Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount_per_gas_unit: DenominatedAmount::native( + 100.into(), + ), + token: shell.state.in_mem().native_token.clone(), + }, + keypair.ref_to(), + (block_gas_limit + 1).div_ceil(2).into(), + )))); + wrapper.header.chain_id = shell.chain_id.clone(); + wrapper + .set_code(Code::new("wasm_code".as_bytes().to_owned(), None)); + wrapper + .set_data(Data::new("transaction data".as_bytes().to_owned())); + wrapper.sign_wrapper(keypair.clone()); + txs.push(wrapper.to_bytes().into()); + } + + let req = RequestPrepareProposal { + txs, + max_tx_bytes: 0, + time: None, + ..Default::default() + }; + let result = shell.prepare_proposal(req); + assert_eq!(result.txs.len(), 1); + } + // Check that a wrapper requiring more gas than its limit is not included in // the block #[test] diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index 9371b97db8..8cae8d3de2 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -1493,7 +1493,53 @@ mod test_process_proposal { Err(TestError::RejectProposal(response)) => { assert_eq!( response[0].result.code, - u32::from(ResultCode::TxGasLimit) + u32::from(ResultCode::AllocationError) + ); + } + } + } + + /// Check that a tx requiring more gas than the available gas in the block + /// causes a block rejection + #[test] + fn test_exceeding_available_block_gas_tx() { + let (shell, _recv, _, _) = test_utils::setup(); + + let block_gas_limit = + parameters::get_max_block_gas(&shell.state).unwrap(); + let keypair = namada_apps_lib::wallet::defaults::albert_keypair(); + + let mut txs = vec![]; + for _ in 0..2 { + let mut wrapper = + Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount_per_gas_unit: DenominatedAmount::native( + 100.into(), + ), + token: shell.state.in_mem().native_token.clone(), + }, + keypair.ref_to(), + (block_gas_limit + 1).div_ceil(2).into(), + )))); + wrapper.header.chain_id = shell.chain_id.clone(); + wrapper + .set_code(Code::new("wasm_code".as_bytes().to_owned(), None)); + wrapper + .set_data(Data::new("transaction data".as_bytes().to_owned())); + wrapper.sign_wrapper(keypair.clone()); + txs.push(wrapper.to_bytes()); + } + + // Run validation + let request = ProcessProposal { txs }; + match shell.process_proposal(request) { + Ok(_) => panic!("Test failed"), + Err(TestError::RejectProposal(response)) => { + assert_eq!(response[0].result.code, u32::from(ResultCode::Ok)); + assert_eq!( + response[1].result.code, + u32::from(ResultCode::AllocationError) ); } } From 9e807be5c08a9800519367f56880fb17d9a6b58f Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 18 Oct 2024 18:42:52 +0200 Subject: [PATCH 4/4] Changelog #3928 --- .changelog/unreleased/improvements/3928-improve-gas-msgs.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/improvements/3928-improve-gas-msgs.md diff --git a/.changelog/unreleased/improvements/3928-improve-gas-msgs.md b/.changelog/unreleased/improvements/3928-improve-gas-msgs.md new file mode 100644 index 0000000000..874aef7243 --- /dev/null +++ b/.changelog/unreleased/improvements/3928-improve-gas-msgs.md @@ -0,0 +1,3 @@ +- Improved the mesagges related to gas errors in + `process_proposal` and mempool validation. Added more tests. + ([\#3928](https://github.com/anoma/namada/pull/3928)) \ No newline at end of file