From 18c0117c39893ab60f4bc1eeae07cff35c2eb7d1 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 5 Nov 2024 13:41:36 +0100 Subject: [PATCH 1/3] Reworks masp fee payment error handling --- crates/node/src/protocol.rs | 196 +++++++++++++++++------------------- 1 file changed, 95 insertions(+), 101 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index ad8bdd5723..43893518dc 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -561,60 +561,63 @@ where } else { // See if the first inner transaction of the batch pays the fees // with a masp unshield - if let Ok(Some(valid_batched_tx_result)) = - try_masp_fee_payment(shell_params, tx, tx_index) - { - #[cfg(not(fuzzing))] - let balance = token::read_balance( - shell_params.state, - &wrapper.fee.token, - &wrapper.fee_payer(), - ) - .expect("Could not read balance key from storage"); - #[cfg(fuzzing)] - let balance = Amount::max().checked_div_u64(2).unwrap(); - - let post_bal = match balance.checked_sub(fees) { - Some(post_bal) => { - // This cannot fail given the checked_sub check here - // above - fee_token_transfer( - shell_params.state, - &wrapper.fee.token, - &wrapper.fee_payer(), - block_proposer, - fees, - )?; - - post_bal - } - None => { - // This shouldn't happen as it should be prevented - // from process_proposal. - tracing::error!( - "Transfer of tx fee cannot be applied to due \ - to insufficient funds. This shouldn't happen." - ); - return Err(Error::FeeError( - "Insufficient funds for fee payment" - .to_string(), - )); - } - }; + match try_masp_fee_payment(shell_params, tx, tx_index) { + Ok(valid_batched_tx_result) => { + #[cfg(not(fuzzing))] + let balance = token::read_balance( + shell_params.state, + &wrapper.fee.token, + &wrapper.fee_payer(), + ) + .expect("Could not read balance key from storage"); + #[cfg(fuzzing)] + let balance = Amount::max().checked_div_u64(2).unwrap(); + + let post_bal = match balance.checked_sub(fees) { + Some(post_bal) => { + // This cannot fail given the checked_sub check + // here above + fee_token_transfer( + shell_params.state, + &wrapper.fee.token, + &wrapper.fee_payer(), + block_proposer, + fees, + )?; + + post_bal + } + None => { + // This shouldn't happen as it should be + // prevented + // from process_proposal. + tracing::error!( + "Transfer of tx fee cannot be applied to \ + due to insufficient funds. This \ + shouldn't happen." + ); + return Err(Error::FeeError( + "Insufficient funds for fee payment" + .to_string(), + )); + } + }; - // Batched tx result must be returned (and considered) only - // if fee payment was successful - (post_bal, Some(valid_batched_tx_result)) - } else { - // This shouldn't happen as it should be prevented by - // process_proposal. - tracing::error!( - "Transfer of tx fee cannot be applied to due to \ - insufficient funds. This shouldn't happen." - ); - return Err(Error::FeeError( - "Insufficient funds for fee payment".to_string(), - )); + // Batched tx result must be returned (and considered) + // only if fee payment was + // successful + (post_bal, Some(valid_batched_tx_result)) + } + Err(e) => { + // This shouldn't happen as it should be prevented by + // process_proposal. + tracing::error!( + "Transfer of tx fee cannot be applied because of \ + {}. This shouldn't happen.", + e + ); + return Err(e); + } } }; @@ -675,7 +678,7 @@ fn try_masp_fee_payment( }: &mut ShellParams<'_, S, D, H, CA>, tx: &Tx, tx_index: &TxIndex, -) -> Result> +) -> Result where S: 'static + State @@ -730,24 +733,28 @@ where // free masp operations. We can commit only after the entire fee // payment has been deemed valid. Also, do not commit to batch // cause we might need to discard the effects of this valid - // unshield (e.g. if it unshield an amount which is not enough + // unshield (e.g. if it unshields an amount which is not enough // to pay the fees) let is_masp_transfer = is_masp_transfer(&result.changed_keys); // Ensure that the transaction is actually a masp one, otherwise // reject if is_masp_transfer && result.is_accepted() { - get_optional_masp_ref( + let masp_section_ref = get_optional_masp_ref( *state, first_tx.cmt, Either::Left(true), )? - .map(|masp_section_ref| { - MaspTxResult { - tx_result: result, - masp_section_ref, - } - }) + .ok_or_else(|| { + Error::FeeError( + "Missing expected masp section reference" + .to_string(), + ) + })?; + MaspTxResult { + tx_result: result, + masp_section_ref, + } } else { state.write_log_mut().drop_tx(); @@ -763,21 +770,15 @@ where } tracing::error!(error_msg); - None + return Err(Error::FeeError(error_msg)); } } Err(e) => { state.write_log_mut().drop_tx(); - tracing::error!( - "{MASP_FEE_PAYMENT_ERROR} Wasm run failed: {}", - e - ); - if let Error::GasError(_) = e { - // Propagate only if it is a gas error - return Err(e); - } - - None + let error_msg = + format!("{MASP_FEE_PAYMENT_ERROR} Wasm run failed: {}", e); + tracing::error!(error_msg); + return Err(Error::FeeError(error_msg)); } } }; @@ -886,35 +887,28 @@ where |_| { // See if the first inner transaction of the batch pays // the fees with a masp unshield - if let Ok(valid_batched_tx_result @ Some(_)) = - try_masp_fee_payment( - shell_params, - tx, - &TxIndex::default(), - ) - { - let balance = token::read_balance( - shell_params.state, - &wrapper.fee.token, - &wrapper.fee_payer(), - ) - .map_err(Error::Error)?; + let valid_batched_tx_result = try_masp_fee_payment( + shell_params, + tx, + &TxIndex::default(), + )?; + let balance = token::read_balance( + shell_params.state, + &wrapper.fee.token, + &wrapper.fee_payer(), + ) + .map_err(Error::Error)?; - checked!(balance - fees).map_or_else( - |_| { - Err(Error::FeeError( - "Masp fee payment unshielded an \ - insufficient amount" - .to_string(), - )) - }, - |_| Ok(valid_batched_tx_result), - ) - } else { - Err(Error::FeeError( - "Failed masp fee payment".to_string(), - )) - } + checked!(balance - fees).map_or_else( + |_| { + Err(Error::FeeError( + "Masp fee payment unshielded an insufficient \ + amount" + .to_string(), + )) + }, + |_| Ok(Some(valid_batched_tx_result)), + ) }, |_| Ok(None), ) From b639a623cf7477e75752b3a019d2894974626421 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 5 Nov 2024 15:17:46 +0100 Subject: [PATCH 2/3] Updates unit tests assertions --- crates/node/src/shell/process_proposal.rs | 26 +++++++++++------------ 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index aeccbc79ac..3b9069b1c6 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -1045,13 +1045,12 @@ mod test_process_proposal { } }; assert_eq!(response.result.code, u32::from(ResultCode::FeeError)); - assert_eq!( - response.result.info, - String::from( - "Error trying to apply a transaction: Error while processing \ - transaction's fees: Insufficient funds for fee payment" - ) - ); + assert!(response.result.info.contains( + "Error trying to apply a transaction: Error while processing \ + transaction's fees: The first transaction in the batch failed to \ + pay fees via the MASP. Wasm run failed: Transaction runner \ + error: Wasm validation error" + )); } /// Test that if the account submitting the tx does @@ -1105,13 +1104,12 @@ mod test_process_proposal { } }; assert_eq!(response.result.code, u32::from(ResultCode::FeeError)); - assert_eq!( - response.result.info, - String::from( - "Error trying to apply a transaction: Error while processing \ - transaction's fees: Insufficient funds for fee payment" - ) - ); + assert!(response.result.info.contains( + "Error trying to apply a transaction: Error while processing \ + transaction's fees: The first transaction in the batch failed to \ + pay fees via the MASP. Wasm run failed: Transaction runner \ + error: Wasm validation error" + )); } /// Process Proposal should reject a block containing a RawTx, but not panic From 836095a4d9b3189089a8f3c63f6d69a7b9d8f20a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 5 Nov 2024 15:19:12 +0100 Subject: [PATCH 3/3] Changelog #3983 --- .../unreleased/improvements/3983-improve-masp-fee-messages.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/3983-improve-masp-fee-messages.md diff --git a/.changelog/unreleased/improvements/3983-improve-masp-fee-messages.md b/.changelog/unreleased/improvements/3983-improve-masp-fee-messages.md new file mode 100644 index 0000000000..c4efbe6285 --- /dev/null +++ b/.changelog/unreleased/improvements/3983-improve-masp-fee-messages.md @@ -0,0 +1,2 @@ +- Streamlined the error propagation of masp fee payment. Improved the logs + provided to the user. ([\#3983](https://github.com/anoma/namada/pull/3983)) \ No newline at end of file