Merge pull request #3928 from anoma/grarco/improve-gas-msgs

Improve gas messages

.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))

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::<u64>(&parameters::storage::get_masp_fee_payment_gas_limit_key())
         .expect("Error reading the storage")

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;
                 }
 

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,
@@ -987,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]

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 {
@@ -1475,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)
                 );
             }
         }