Skip to content

Commit

Permalink
Merge pull request #3983 from anoma/grarco/improve-masp-fee-messages
Browse files Browse the repository at this point in the history 
Reworks masp fee payment error handling
  • Loading branch information
@mergify
mergify[bot] authored Nov 5, 2024
2 parents aa1667c + 836095a commit ccbd651
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 115 deletions.
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/3983-improve-masp-fee-messages.md
View file
Original file line number Diff line number Diff line change
@@ -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))
196 changes: 95 additions & 101 deletions crates/node/src/protocol.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
};

Expand Down Expand Up @@ -675,7 +678,7 @@ fn try_masp_fee_payment<S, D, H, CA>(
}: &mut ShellParams<'_, S, D, H, CA>,
tx: &Tx,
tx_index: &TxIndex,
) -> Result<Option<MaspTxResult>>
) -> Result<MaspTxResult>
where
S: 'static
+ State<D = D, H = H>
Expand Down Expand Up @@ -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();

Expand All @@ -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));
}
}
};
Expand Down Expand Up @@ -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),
)
Expand Down
26 changes: 12 additions & 14 deletions crates/node/src/shell/process_proposal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit ccbd651

Please sign in to comment.