From 8e3809437c9bf2d1aca760416bcae837cdb55dee Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 20 May 2025 14:11:29 +0200 Subject: [PATCH 1/8] Sends part of the fees to the PGF account or burns them --- crates/node/src/protocol.rs | 366 ++++++++++++++-------- crates/node/src/shell/mod.rs | 83 +++-- crates/node/src/shell/prepare_proposal.rs | 17 +- crates/node/src/shell/process_proposal.rs | 17 +- 4 files changed, 306 insertions(+), 177 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 63e0b748ea9..b8c9db49420 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -501,13 +501,31 @@ where .write_log_mut() .write_tx_hash(tx.header_hash()) .expect("Error while writing tx hash to storage"); + let minimum_gas_price = + parameters::read_gas_cost(shell_params.state, &wrapper.fee.token) + .expect("Must be able to read gas cost parameter") + .ok_or(Error::FeeError(format!( + "The provided {} token is not allowed for fee payment", + wrapper.fee.token + )))?; + let fee_components = crate::shell::get_fee_components( + wrapper, + minimum_gas_price, + shell_params.state, + ) + .map_err(|e| Error::FeeError(e.to_string()))?; // Charge or check fees, propagate any errors to prevent committing invalid // data let payment_result = match block_proposer { - Some(block_proposer) => { - transfer_fee(shell_params, block_proposer, tx, wrapper, tx_index)? - } + Some(block_proposer) => transfer_fee( + shell_params, + block_proposer, + tx, + wrapper, + tx_index, + &fee_components, + )?, None => check_fees(shell_params, tx, wrapper)?, }; @@ -562,15 +580,32 @@ where Ok(batch_results) } -/// Perform the actual transfer of fees from the fee payer to the block -/// proposer. No modifications to the write log are committed or dropped in this -/// function: this logic is up to the caller. +/// The fee amounts split between the tip and the base fee +pub struct FeeComponents { + pub base: token::Amount, + pub tip: token::Amount, +} + +impl FeeComponents { + pub fn get_total_fee(&self) -> Result { + checked!(self.base + self.tip).map_err(|_| { + Error::FeeError( + "Overflow in total fee reconstruction, this should not happen" + .to_string(), + ) + }) + } +} + +/// Perform the actual transfer of fees. No modifications to the write log are +/// committed or dropped in this function: this logic is up to the caller. pub fn transfer_fee( shell_params: &mut ShellParams<'_, S, D, H, CA>, block_proposer: &Address, tx: &Tx, wrapper: &WrapperTx, tx_index: &TxIndex, + fee_components: &FeeComponents, ) -> Result> where S: 'static @@ -584,149 +619,169 @@ where H: 'static + StorageHasher + Sync, CA: 'static + WasmCacheAccess + Sync, { - match wrapper.get_tx_fee() { - Ok(fees) => { - let fees = token::denom_to_amount( - fees, - &wrapper.fee.token, - shell_params.state, - ) - .map_err(Error::Error)?; + #[cfg(not(fuzzing))] + let balance = token::read_balance( + shell_params.state, + &wrapper.fee.token, + &wrapper.fee_payer(), + ) + .map_err(Error::Error)?; - #[cfg(not(fuzzing))] - let balance = token::read_balance( - shell_params.state, - &wrapper.fee.token, - &wrapper.fee_payer(), - ) - .map_err(Error::Error)?; + // Use half of the max value to make the balance check pass + // sometimes with arbitrary fees + #[cfg(fuzzing)] + let balance = Amount::max().checked_div_u64(2).unwrap(); + + let fees = fee_components.get_total_fee()?; - // Use half of the max value to make the balance check pass - // sometimes with arbitrary fees - #[cfg(fuzzing)] - let balance = Amount::max().checked_div_u64(2).unwrap(); + let (post_bal, valid_batched_tx_result) = if let Some(post_bal) = + balance.checked_sub(fees) + { + fee_token_transfer( + shell_params.state, + &wrapper.fee.token, + &wrapper.fee_payer(), + block_proposer, + fee_components, + )?; - let (post_bal, valid_batched_tx_result) = if let Some(post_bal) = - balance.checked_sub(fees) - { - fee_token_transfer( + (post_bal, None) + } else { + // See if the first inner transaction of the batch pays the fees + // with a masp unshield + 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(), - block_proposer, - fees, - )?; - - (post_bal, None) - } else { - // See if the first inner transaction of the batch pays the fees - // with a masp unshield - match try_masp_fee_payment(shell_params, tx, tx_index) { - Ok(valid_batched_tx_result) => { - #[cfg(not(fuzzing))] - let balance = token::read_balance( + ) + .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(), - ) - .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(), - )); - } - }; + block_proposer, + fee_components, + )?; - // Batched tx result must be returned (and considered) - // only if fee payment was - // successful - (post_bal, Some(valid_batched_tx_result)) + post_bal } - Err(e) => { + None => { // This shouldn't happen as it should be prevented by // process_proposal. tracing::error!( - "Transfer of tx fee cannot be applied because of \ - an error: {}. This shouldn't happen.", - e + "Transfer of tx fee cannot be applied to due to \ + insufficient funds. This shouldn't happen." ); - return Err(e.into()); + return Err(Error::FeeError( + "Insufficient funds for fee payment".to_string(), + )); } - } - }; - - let target_post_balance = Some( - token::read_balance( - shell_params.state, - &wrapper.fee.token, - block_proposer, - ) - .map_err(Error::Error)? - .into(), - ); + }; - const FEE_PAYMENT_DESCRIPTOR: std::borrow::Cow<'static, str> = - std::borrow::Cow::Borrowed("wrapper-fee-payment"); - let current_block_height = shell_params - .state - .in_mem() - .get_last_block_height() - .next_height(); - shell_params.state.write_log_mut().emit_event( - TokenEvent { - descriptor: FEE_PAYMENT_DESCRIPTOR, - level: EventLevel::Tx, - operation: TokenOperation::transfer( - UserAccount::Internal(wrapper.fee_payer()), - UserAccount::Internal(block_proposer.clone()), - wrapper.fee.token.clone(), - fees.into(), - post_bal.into(), - target_post_balance, - ), - } - .with(HeightAttr(current_block_height)) - .with(TxHashAttr(tx.header_hash())), - ); + // 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 an \ + error: {}. This shouldn't happen.", + e + ); + return Err(e.into()); + } + } + }; - Ok(valid_batched_tx_result) + const FEE_PAYMENT_DESCRIPTOR: std::borrow::Cow<'static, str> = + std::borrow::Cow::Borrowed("wrapper-fee-payment"); + let current_block_height = shell_params + .state + .in_mem() + .get_last_block_height() + .next_height(); + + // Event for the tip + let proposer_post_balance = Some( + token::read_balance( + shell_params.state, + &wrapper.fee.token, + block_proposer, + ) + .map_err(Error::Error)? + .into(), + ); + shell_params.state.write_log_mut().emit_event( + TokenEvent { + descriptor: FEE_PAYMENT_DESCRIPTOR, + level: EventLevel::Tx, + operation: TokenOperation::transfer( + UserAccount::Internal(wrapper.fee_payer()), + UserAccount::Internal(block_proposer.clone()), + wrapper.fee.token.clone(), + fee_components.tip.into(), + post_bal.into(), + proposer_post_balance, + ), } - Err(e) => { - // Fee overflow. This shouldn't happen as it should be prevented - // by process_proposal. - tracing::error!( - "Transfer of tx fee cannot be applied to due to fee overflow. \ - This shouldn't happen." - ); - Err(Error::FeeError(format!("{}", e))) + .with(HeightAttr(current_block_height)) + .with(TxHashAttr(tx.header_hash())), + ); + // Event for the base fee + let operation = if wrapper.fee.token + == shell_params + .state + .get_native_token() + .map_err(Error::Error)? + { + TokenOperation::Burn { + target_account: UserAccount::Internal(wrapper.fee_payer()), + token: wrapper.fee.token.clone(), + amount: fee_components.base.into(), + post_balance: post_bal.into(), } - } + } else { + let pgf_post_balance = Some( + token::read_balance( + shell_params.state, + &wrapper.fee.token, + &namada_sdk::address::PGF, + ) + .map_err(Error::Error)? + .into(), + ); + TokenOperation::transfer( + UserAccount::Internal(wrapper.fee_payer()), + UserAccount::Internal(namada_sdk::address::PGF), + wrapper.fee.token.clone(), + fee_components.base.into(), + post_bal.into(), + pgf_post_balance, + ) + }; + shell_params.state.write_log_mut().emit_event( + TokenEvent { + descriptor: FEE_PAYMENT_DESCRIPTOR, + level: EventLevel::Tx, + operation, + } + .with(HeightAttr(current_block_height)) + .with(TxHashAttr(tx.header_hash())), + ); + + Ok(valid_batched_tx_result) } /// Custom wrapper type for masp fee payment errors. The purpose of this type is @@ -926,24 +981,61 @@ fn get_optional_masp_ref>( } // Manage the token transfer for the fee payment. If an error is detected the -// write log is dropped to prevent committing an inconsistent state. Propagates -// the result to the caller +// write log is dropped to prevent committing an inconsistent state. +// WARNING: the `token::burn_tokens` function does not return an error if the +// amount to burn exceeds the balance of the target address: it just burns +// whatever balance is available. Because of this, the caller of this function +// must ensure that the balance of the fee payer is enough to cover the entire +// fee cost. fn fee_token_transfer( state: &mut WLS, token: &Address, src: &Address, dest: &Address, - amount: Amount, + fee: &FeeComponents, ) -> Result<()> where WLS: State + StorageRead + TxWrites, { - token::transfer(&mut state.with_tx_writes(), token, src, dest, amount) - .map_err(|err| { - state.write_log_mut().drop_tx(); + fn fee_transfer_inner( + state: &mut WLS, + token: &Address, + src: &Address, + dest: &Address, + fee: &FeeComponents, + ) -> std::result::Result<(), state::Error> + where + WLS: State + StorageRead + TxWrites, + { + if token == &state.get_native_token()? { + // Burn the base fee + token::burn_tokens( + &mut state.with_tx_writes(), + token, + src, + fee.base, + )?; + } else { + // Transfer base fee to the PGF account + token::transfer( + &mut state.with_tx_writes(), + token, + src, + &namada_sdk::address::PGF, + fee.base, + )?; + } - Error::Error(err) - }) + // Transfer tip to the block proposer + token::transfer(&mut state.with_tx_writes(), token, src, dest, fee.tip) + } + + // Make sure to drop the content of the write log in case of any error + fee_transfer_inner(state, token, src, dest, fee).map_err(|err| { + state.write_log_mut().drop_tx(); + + Error::Error(err) + }) } /// Check if the fee payer has enough transparent balance to pay fees diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index a6e47671f82..74fcf138321 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -74,7 +74,7 @@ use tokio::sync::mpsc::{Receiver, UnboundedSender}; use super::ethereum_oracle::{self as oracle, last_processed_block}; use crate::config::{self, TendermintMode, ValidatorLocalConfig, genesis}; -use crate::protocol::ShellParams; +use crate::protocol::{FeeComponents, ShellParams}; use crate::shims::abcipp_shim_types::shim; use crate::shims::abcipp_shim_types::shim::TakeSnapshot; use crate::shims::abcipp_shim_types::shim::response::TxResult; @@ -1518,49 +1518,72 @@ where wrapper.fee.token ))))?; - fee_data_check(wrapper, minimum_gas_price, shell_params)?; + fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; protocol::check_fees(shell_params, tx, wrapper) .map_err(Error::TxApply) .map(|_| ()) } -/// Check the validity of the fee data -pub fn fee_data_check( +/// Check the validity of the fee data and return the fee information split +/// between base and tip +pub fn fee_data_check( wrapper: &WrapperTx, minimum_gas_price: token::Amount, - shell_params: &mut ShellParams<'_, TempWlState<'_, D, H>, D, H, CA>, -) -> ShellResult<()> -where - D: DB + for<'iter> DBIter<'iter> + Sync + 'static, - H: StorageHasher + Sync + 'static, - CA: 'static + WasmCacheAccess + Sync, -{ + storage: &impl StorageRead, +) -> ShellResult { match token::denom_to_amount( wrapper.fee.amount_per_gas_unit, &wrapper.fee.token, - shell_params.state, + storage, ) { - Ok(amount_per_gas_unit) if amount_per_gas_unit < minimum_gas_price => { - // The fees do not match the minimum required - return Err(Error::TxApply(protocol::Error::FeeError(format!( - "Fee amount {:?} does not match the minimum required amount \ - {:?} for token {}", - wrapper.fee.amount_per_gas_unit, - minimum_gas_price, - wrapper.fee.token - )))); - } - Ok(_) => {} - Err(err) => { - return Err(Error::TxApply(protocol::Error::FeeError(format!( - "The precision of the fee amount {:?} is higher than the \ - denomination for token {}: {}", - wrapper.fee.amount_per_gas_unit, wrapper.fee.token, err, - )))); + Ok(amount_per_gas_unit) => { + if amount_per_gas_unit < minimum_gas_price { + // The fees do not match the minimum required + Err(Error::TxApply(protocol::Error::FeeError(format!( + "Fee amount {:?} does not match the minimum required \ + amount {:?} for token {}", + wrapper.fee.amount_per_gas_unit, + minimum_gas_price, + wrapper.fee.token + )))) + } else { + get_fee_components(wrapper, minimum_gas_price, storage) + } } + Err(err) => Err(Error::TxApply(protocol::Error::FeeError(format!( + "The precision of the fee amount {:?} is higher than the \ + denomination for token {}: {}", + wrapper.fee.amount_per_gas_unit, wrapper.fee.token, err, + )))), } +} - Ok(()) +pub fn get_fee_components( + wrapper: &WrapperTx, + minimum_gas_price: token::Amount, + storage: &impl StorageRead, +) -> ShellResult { + let denom_amt = wrapper.get_tx_fee().map_err(|e| { + Error::TxApply(protocol::Error::FeeError(e.to_string())) + })?; + let fees = token::denom_to_amount(denom_amt, &wrapper.fee.token, storage)?; + let base_fee = minimum_gas_price + .checked_mul(token::Amount::from(wrapper.gas_limit)) + .ok_or_else(|| { + Error::TxApply(protocol::Error::FeeError( + "Overflow in base fee computation".to_string(), + )) + })?; + let tip = fees.checked_sub(base_fee).ok_or_else(|| { + Error::TxApply(protocol::Error::FeeError( + "Underflow in tip computation".to_string(), + )) + })?; + + Ok(FeeComponents { + base: base_fee, + tip, + }) } /// for the shell diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index 5461021deb6..5f46c3f0ee7 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/crates/node/src/shell/prepare_proposal.rs @@ -349,11 +349,18 @@ where proposer_local_config, shell_params.state, )?; - - super::fee_data_check(wrapper, minimum_gas_price, shell_params)?; - - protocol::transfer_fee(shell_params, proposer, tx, wrapper, tx_index) - .map_or_else(|e| Err(Error::TxApply(e)), |_| Ok(())) + let fee_components = + super::fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; + + protocol::transfer_fee( + shell_params, + proposer, + tx, + wrapper, + tx_index, + &fee_components, + ) + .map_or_else(|e| Err(Error::TxApply(e)), |_| Ok(())) } fn compute_min_gas_price( diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index 915b8daf9e0..928ac1c264d 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -583,11 +583,18 @@ where "The provided {} token is not allowed for fee payment", wrapper.fee.token ))))?; - - fee_data_check(wrapper, minimum_gas_price, shell_params)?; - - protocol::transfer_fee(shell_params, proposer, tx, wrapper, tx_index) - .map_or_else(|e| Err(Error::TxApply(e)), |_| Ok(())) + let fee_components = + fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; + + protocol::transfer_fee( + shell_params, + proposer, + tx, + wrapper, + tx_index, + &fee_components, + ) + .map_or_else(|e| Err(Error::TxApply(e)), |_| Ok(())) } /// We test the failure cases of [`process_proposal`]. The happy flows From 6a15d793a4514e428c33057ab7e845b87e295d53 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 20 May 2025 16:00:20 +0200 Subject: [PATCH 2/8] Fixes broken unit and integration tests --- crates/node/src/shell/finalize_block.rs | 71 +++++++++++++------- crates/tests/src/integration/ledger_tests.rs | 8 +-- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index db8eed3d1dc..82367067b92 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -1372,7 +1372,7 @@ mod test_finalize_block { let mut wrapper_tx = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair.ref_to(), @@ -1412,7 +1412,7 @@ mod test_finalize_block { let mut batch = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, sk.ref_to(), @@ -3276,7 +3276,7 @@ mod test_finalize_block { let mut wrapper = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair.ref_to(), @@ -3289,7 +3289,7 @@ mod test_finalize_block { let mut new_wrapper = wrapper.clone(); new_wrapper.update_header(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair_2.ref_to(), @@ -3383,7 +3383,7 @@ mod test_finalize_block { let mut wrapper = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair.ref_to(), @@ -3405,7 +3405,7 @@ mod test_finalize_block { let mut new_wrapper = wrapper.clone(); new_wrapper.update_header(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair_2.ref_to(), @@ -3491,7 +3491,7 @@ mod test_finalize_block { Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { amount_per_gas_unit: DenominatedAmount::native( - 1.into(), + 10.into(), ), token: shell.state.in_mem().native_token.clone(), }, @@ -3509,9 +3509,7 @@ mod test_finalize_block { let mut unsigned_wrapper = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native( - Amount::zero(), - ), + amount_per_gas_unit: DenominatedAmount::native(10.into()), token: shell.state.in_mem().native_token.clone(), }, keypair.ref_to(), @@ -3703,7 +3701,9 @@ mod test_finalize_block { let mut wrapper = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(0.into()), + amount_per_gas_unit: DenominatedAmount::native( + Amount::zero(), + ), token: shell.state.in_mem().native_token.clone(), }, keypair.ref_to(), @@ -3991,10 +3991,13 @@ mod test_finalize_block { assert_eq!(balance, 0.into()); } - // Test that the fees collected from a block are withdrew from the wrapper - // signer and credited to the block proposer + // Test that the tip of the fees collected from a block are withdrew from + // the wrapper signer and credited to the block proposer. Also checks + // that the base fee is burned. FIXME: need another test just like this + // one but with fees paid in a foreing token (or I could just extend this + // test) #[test] - fn test_fee_payment_to_block_proposer() { + fn test_tip_fee_payment_to_block_proposer() { let (mut shell, _, _, _) = setup(); let validator = shell.mode.get_validator_address().unwrap().to_owned(); @@ -4014,11 +4017,14 @@ mod test_finalize_block { &validator, ) .unwrap(); + let total_supply = + namada_sdk::token::get_effective_total_native_supply(&shell.state) + .unwrap(); let mut wrapper = Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { - amount_per_gas_unit: DenominatedAmount::native(1.into()), + amount_per_gas_unit: DenominatedAmount::native(11.into()), token: shell.state.in_mem().native_token.clone(), }, namada_apps_lib::wallet::defaults::albert_keypair().ref_to(), @@ -4029,11 +4035,15 @@ mod test_finalize_block { .add_code(TestWasms::TxNoOp.read_bytes(), None) .add_data("Transaction data"); wrapper.sign_wrapper(albert_keypair()); - let fee_amount = - wrapper.header().wrapper().unwrap().get_tx_fee().unwrap(); - let fee_amount = namada_sdk::token::denom_to_amount( - fee_amount, + let minimum_gas_price = parameters::read_gas_cost( + &shell.state, &wrapper.header().wrapper().unwrap().fee.token, + ) + .unwrap() + .unwrap(); + let fee_components = get_fee_components( + &wrapper.header().wrapper().unwrap(), + minimum_gas_price, &shell.state, ) .unwrap(); @@ -4074,7 +4084,7 @@ mod test_finalize_block { .unwrap(); assert_eq!( new_proposer_balance, - proposer_balance.checked_add(fee_amount).unwrap() + proposer_balance.checked_add(fee_components.tip).unwrap() ); let new_signer_balance = namada_sdk::token::read_balance( @@ -4085,7 +4095,18 @@ mod test_finalize_block { .unwrap(); assert_eq!( new_signer_balance, - signer_balance.checked_sub(fee_amount).unwrap() + signer_balance + .checked_sub(fee_components.get_total_fee().unwrap()) + .unwrap() + ); + + // Check burning of the base fee + let new_total_supply = + namada_sdk::token::get_effective_total_native_supply(&shell.state) + .unwrap(); + assert_eq!( + new_total_supply, + total_supply.checked_sub(fee_components.base).unwrap() ) } @@ -6095,7 +6116,7 @@ mod test_finalize_block { Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { amount_per_gas_unit: DenominatedAmount::native( - 1.into(), + 10.into(), ), token: shell.state.in_mem().native_token.clone(), }, @@ -6177,7 +6198,7 @@ mod test_finalize_block { Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { amount_per_gas_unit: DenominatedAmount::native( - 1.into(), + 10.into(), ), token: shell.state.in_mem().native_token.clone(), }, @@ -6255,7 +6276,7 @@ mod test_finalize_block { Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { amount_per_gas_unit: DenominatedAmount::native( - 1.into(), + 10.into(), ), token: shell.state.in_mem().native_token.clone(), }, @@ -6326,7 +6347,7 @@ mod test_finalize_block { Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( Fee { amount_per_gas_unit: DenominatedAmount::native( - 1.into(), + 10.into(), ), token: shell.state.in_mem().native_token.clone(), }, diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index d2727bb628a..2cdee00a578 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -554,10 +554,10 @@ fn pos_rewards() -> Result<()> { }); assert_matches!(captured.result, Ok(_)); let _res = captured - .matches(r"Current annual staking rewards rate: 65.705255877354") + .matches(r"Current annual staking rewards rate: 65.705256154607") .expect("Test failed"); let _res = captured - .matches(r"PoS inflation rate: 0.066593164444") + .matches(r"PoS inflation rate: 0.066593164725") .expect("Test failed"); Ok(()) @@ -1299,7 +1299,7 @@ fn pgf_governance_proposal() -> Result<()> { CapturedOutput::of(|| run(&node, Bin::Client, query_total_supply_args)); assert_matches!(captured.result, Ok(_)); assert!(captured.contains( - "token tnam1q9kn74xfzytqkqyycfrhycr8ajam8ny935cge0z5: 118400024.740301" + "token tnam1q9kn74xfzytqkqyycfrhycr8ajam8ny935cge0z5: 118400022.740301" )); let query_native_supply_args = @@ -1308,7 +1308,7 @@ fn pgf_governance_proposal() -> Result<()> { run(&node, Bin::Client, query_native_supply_args) }); assert_matches!(captured.result, Ok(_)); - assert!(captured.contains("nam: 118400010.473048")); + assert!(captured.contains("nam: 118400008.473048")); // 8. Submit proposal funding let albert = defaults::albert_address(); From 458d0bfdd2a3957bc168726ea22fae00676266ae Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 20 May 2025 16:47:35 +0200 Subject: [PATCH 3/8] Fixes usage of the wrong minimum gas cost in `prepare_proposal` --- crates/node/src/protocol.rs | 52 ++++++++++++------------- crates/node/src/shell/finalize_block.rs | 7 ---- crates/node/src/shell/mod.rs | 11 +++++- 3 files changed, 34 insertions(+), 36 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index b8c9db49420..c4a5c0d5f33 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -501,31 +501,23 @@ where .write_log_mut() .write_tx_hash(tx.header_hash()) .expect("Error while writing tx hash to storage"); - let minimum_gas_price = - parameters::read_gas_cost(shell_params.state, &wrapper.fee.token) - .expect("Must be able to read gas cost parameter") - .ok_or(Error::FeeError(format!( - "The provided {} token is not allowed for fee payment", - wrapper.fee.token - )))?; - let fee_components = crate::shell::get_fee_components( - wrapper, - minimum_gas_price, - shell_params.state, - ) - .map_err(|e| Error::FeeError(e.to_string()))?; // Charge or check fees, propagate any errors to prevent committing invalid // data let payment_result = match block_proposer { - Some(block_proposer) => transfer_fee( - shell_params, - block_proposer, - tx, - wrapper, - tx_index, - &fee_components, - )?, + Some(block_proposer) => { + let fee_components = + crate::shell::get_fee_components(wrapper, shell_params.state) + .map_err(|e| Error::FeeError(e.to_string()))?; + transfer_fee( + shell_params, + block_proposer, + tx, + wrapper, + tx_index, + &fee_components, + )? + } None => check_fees(shell_params, tx, wrapper)?, }; @@ -992,7 +984,7 @@ fn fee_token_transfer( token: &Address, src: &Address, dest: &Address, - fee: &FeeComponents, + fee_components: &FeeComponents, ) -> Result<()> where WLS: State + StorageRead + TxWrites, @@ -1002,7 +994,7 @@ where token: &Address, src: &Address, dest: &Address, - fee: &FeeComponents, + fee_components: &FeeComponents, ) -> std::result::Result<(), state::Error> where WLS: State + StorageRead + TxWrites, @@ -1013,7 +1005,7 @@ where &mut state.with_tx_writes(), token, src, - fee.base, + fee_components.base, )?; } else { // Transfer base fee to the PGF account @@ -1022,16 +1014,22 @@ where token, src, &namada_sdk::address::PGF, - fee.base, + fee_components.base, )?; } // Transfer tip to the block proposer - token::transfer(&mut state.with_tx_writes(), token, src, dest, fee.tip) + token::transfer( + &mut state.with_tx_writes(), + token, + src, + dest, + fee_components.tip, + ) } // Make sure to drop the content of the write log in case of any error - fee_transfer_inner(state, token, src, dest, fee).map_err(|err| { + fee_transfer_inner(state, token, src, dest, fee_components).map_err(|err| { state.write_log_mut().drop_tx(); Error::Error(err) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 82367067b92..69acdd45710 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -4035,15 +4035,8 @@ mod test_finalize_block { .add_code(TestWasms::TxNoOp.read_bytes(), None) .add_data("Transaction data"); wrapper.sign_wrapper(albert_keypair()); - let minimum_gas_price = parameters::read_gas_cost( - &shell.state, - &wrapper.header().wrapper().unwrap().fee.token, - ) - .unwrap() - .unwrap(); let fee_components = get_fee_components( &wrapper.header().wrapper().unwrap(), - minimum_gas_price, &shell.state, ) .unwrap(); diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index 74fcf138321..b0987a12bf6 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -1547,7 +1547,7 @@ pub fn fee_data_check( wrapper.fee.token )))) } else { - get_fee_components(wrapper, minimum_gas_price, storage) + get_fee_components(wrapper, storage) } } Err(err) => Err(Error::TxApply(protocol::Error::FeeError(format!( @@ -1558,15 +1558,22 @@ pub fn fee_data_check( } } +/// Extract the components of the fee for the provided wrapper transaction pub fn get_fee_components( wrapper: &WrapperTx, - minimum_gas_price: token::Amount, storage: &impl StorageRead, ) -> ShellResult { let denom_amt = wrapper.get_tx_fee().map_err(|e| { Error::TxApply(protocol::Error::FeeError(e.to_string())) })?; let fees = token::denom_to_amount(denom_amt, &wrapper.fee.token, storage)?; + let minimum_gas_price = + parameters::read_gas_cost(storage, &wrapper.fee.token) + .expect("Must be able to read gas cost parameter") + .ok_or(Error::TxApply(protocol::Error::FeeError(format!( + "The provided {} token is not allowed for fee payment", + wrapper.fee.token + ))))?; let base_fee = minimum_gas_price .checked_mul(token::Amount::from(wrapper.gas_limit)) .ok_or_else(|| { From 60f11797699153d7a723d68476f2d5dc595b2816 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 21 May 2025 11:20:43 +0200 Subject: [PATCH 4/8] Adds unit test for fee payment with a foreign token --- crates/node/src/shell/finalize_block.rs | 155 +++++++++++++++++++++++- 1 file changed, 149 insertions(+), 6 deletions(-) diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index 69acdd45710..b2908fcb5d5 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -3991,13 +3991,11 @@ mod test_finalize_block { assert_eq!(balance, 0.into()); } - // Test that the tip of the fees collected from a block are withdrew from - // the wrapper signer and credited to the block proposer. Also checks - // that the base fee is burned. FIXME: need another test just like this - // one but with fees paid in a foreing token (or I could just extend this - // test) + // Test that when fees are paid in the native token, the tip is withdrew + // from the wrapper signer and credited to the block proposer and the + // base fee is burned #[test] - fn test_tip_fee_payment_to_block_proposer() { + fn test_fee_distribution_native_token() { let (mut shell, _, _, _) = setup(); let validator = shell.mode.get_validator_address().unwrap().to_owned(); @@ -4103,6 +4101,151 @@ mod test_finalize_block { ) } + // Test that when fees are paid in a foreign token, the tip is withdrew from + // the wrapper signer and credited to the block proposer and the base fee is + // sent to the PGF account + #[test] + fn test_fee_distribution_foreign_token() { + let (mut shell, _, _, _) = setup(); + let btc = namada_sdk::address::testing::btc(); + let btc_denom = read_denom(&shell.state, &btc).unwrap().unwrap(); + let fee_amount: Amount = (WRAPPER_GAS_LIMIT * 2).into(); + + // Credit some tokens for fee payment + namada_sdk::token::credit_tokens( + &mut shell.state, + &btc, + &Address::from(&albert_keypair().to_public()), + fee_amount, + ) + .unwrap(); + let signer_balance = read_balance( + &shell.state, + &btc, + &Address::from(&albert_keypair().to_public()), + ) + .unwrap(); + assert_eq!(signer_balance, fee_amount.clone()); + + // Whitelist BTC for fee payment + let gas_cost_key = namada_sdk::parameters::storage::get_gas_cost_key(); + let mut gas_prices: BTreeMap = + shell.read_storage_key(&gas_cost_key).unwrap(); + gas_prices.insert(btc.clone(), 1.into()); + shell.shell.state.write(&gas_cost_key, gas_prices).unwrap(); + shell.commit(); + + let validator = shell.mode.get_validator_address().unwrap().to_owned(); + let pos_params = read_pos_params(&shell.state).unwrap(); + let consensus_key = + proof_of_stake::storage::validator_consensus_key_handle(&validator) + .get(&shell.state, Epoch::default(), &pos_params) + .unwrap() + .unwrap(); + let proposer_address = HEXUPPER + .decode(consensus_key.tm_raw_hash().as_bytes()) + .unwrap(); + + let proposer_balance = + namada_sdk::token::read_balance(&shell.state, &btc, &validator) + .unwrap(); + let total_btc_supply = + namada_sdk::token::read_total_supply(&shell.state, &btc).unwrap(); + + let mut wrapper = + Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount_per_gas_unit: DenominatedAmount::new( + 2.into(), + btc_denom, + ), + token: btc.clone(), + }, + namada_apps_lib::wallet::defaults::albert_keypair().ref_to(), + WRAPPER_GAS_LIMIT.into(), + )))); + wrapper.header.chain_id = shell.chain_id.clone(); + wrapper + .add_code(TestWasms::TxNoOp.read_bytes(), None) + .add_data("Transaction data"); + wrapper.sign_wrapper(albert_keypair()); + let fee_components = get_fee_components( + &wrapper.header().wrapper().unwrap(), + &shell.state, + ) + .unwrap(); + + let signer_balance = namada_sdk::token::read_balance( + &shell.state, + &btc, + &wrapper.header().wrapper().unwrap().fee_payer(), + ) + .unwrap(); + let pgf_balance = namada_sdk::token::read_balance( + &shell.state, + &btc, + &namada_sdk::address::PGF, + ) + .unwrap(); + + let processed_tx = ProcessedTx { + tx: wrapper.to_bytes().into(), + result: TxResult { + code: ResultCode::Ok.into(), + info: "".into(), + }, + }; + + let event = &shell + .finalize_block(FinalizeBlock { + txs: vec![processed_tx], + proposer_address, + ..Default::default() + }) + .expect("Test failed")[0]; + + // Check fee payment + assert_eq!(*event.kind(), APPLIED_TX); + let code = event.read_attribute::().expect("Test failed"); + assert_eq!(code, ResultCode::Ok); + + let new_proposer_balance = + namada_sdk::token::read_balance(&shell.state, &btc, &validator) + .unwrap(); + assert_eq!( + new_proposer_balance, + proposer_balance.checked_add(fee_components.tip).unwrap() + ); + + let new_signer_balance = namada_sdk::token::read_balance( + &shell.state, + &btc, + &wrapper.header().wrapper().unwrap().fee_payer(), + ) + .unwrap(); + assert_eq!( + new_signer_balance, + signer_balance + .checked_sub(fee_components.get_total_fee().unwrap()) + .unwrap() + ); + + // Check that the base fee has been sent to pgf and no burning happened + let new_pgf_balance = namada_sdk::token::read_balance( + &shell.state, + &btc, + &namada_sdk::address::PGF, + ) + .unwrap(); + assert_eq!( + new_pgf_balance, + pgf_balance.checked_add(fee_components.base).unwrap() + ); + let new_total_supply = + namada_sdk::token::read_total_supply(&shell.state, &btc).unwrap(); + assert_eq!(new_total_supply, total_btc_supply) + } + #[test] fn test_ledger_slashing() -> namada_sdk::state::Result<()> { let num_validators = 7_u64; From 230b4ba1dbfd6c8b600d4af178ee0b4a18a5dab0 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 21 May 2025 11:23:56 +0200 Subject: [PATCH 5/8] Changelog #4644 --- .../unreleased/improvements/4644-fee-foreign-reserve.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .changelog/unreleased/improvements/4644-fee-foreign-reserve.md diff --git a/.changelog/unreleased/improvements/4644-fee-foreign-reserve.md b/.changelog/unreleased/improvements/4644-fee-foreign-reserve.md new file mode 100644 index 00000000000..25844165392 --- /dev/null +++ b/.changelog/unreleased/improvements/4644-fee-foreign-reserve.md @@ -0,0 +1,4 @@ +- Modified the protocol logic of fees so that only the tip is sent to the + block proposer. The base fee is instead burnt if fees are paid with the + native token or sent to an internal account in case of a foreign token. + ([\#4644](https://github.com/anoma/namada/pull/4644)) \ No newline at end of file From 46791ba052a7bb3bb90818b5721260157927c400 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 21 May 2025 15:48:57 +0200 Subject: [PATCH 6/8] Fixes broken ibc e2e test --- crates/tests/src/e2e/ibc_tests.rs | 38 +++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 77eebb1b5b3..12e829281a9 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -854,14 +854,14 @@ fn fee_payment_with_ibc_token() -> Result<()> { epoch = epoch_sleep(&test, &rpc, 120)?; } - // Transfer 250 samoleans from Gaia to Namada + // Transfer 500 samoleans from Gaia to Namada let namada_receiver = find_address(&test, ALBERT_KEY)?.to_string(); transfer_from_cosmos( &test_gaia, COSMOS_USER, &namada_receiver, COSMOS_COIN, - 250, + 500, &port_id_gaia, &channel_id_gaia, None, @@ -870,8 +870,10 @@ fn fee_payment_with_ibc_token() -> Result<()> { wait_for_packet_relay(&hermes_dir, &port_id_gaia, &channel_id_gaia, &test)?; // Check the token on Namada - check_balance(&test, ALBERT_KEY, &ibc_token_on_namada, 250)?; - check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 750)?; + check_balance(&test, ALBERT_KEY, &ibc_token_on_namada, 500)?; + check_cosmos_balance(&test_gaia, COSMOS_USER, COSMOS_COIN, 500)?; + check_balance(&test, PGF_ADDRESS.to_string(), &ibc_token_on_namada, 0)?; + check_ibc_token_supply(&test, &ibc_token_on_namada, 500)?; // Transparent transfer in Namada paying gas with samoleans transfer_on_chain( @@ -882,12 +884,21 @@ fn fee_payment_with_ibc_token() -> Result<()> { NAM, 50, ALBERT_KEY, - &["--gas-token", &ibc_token_on_namada, "--gas-limit", "250"], + &[ + "--gas-token", + &ibc_token_on_namada, + "--gas-limit", + "250", + "--gas-price", + "2", + ], )?; check_balance(&test, ALBERT, NAM, 1_999_950)?; check_balance(&test, CHRISTEL, NAM, 2_000_050)?; check_balance(&test, ALBERT_KEY, &ibc_token_on_namada, 0)?; check_balance(&test, "validator-0", &ibc_token_on_namada, 250)?; + check_balance(&test, PGF_ADDRESS.to_string(), &ibc_token_on_namada, 250)?; + check_ibc_token_supply(&test, &ibc_token_on_namada, 500)?; Ok(()) } @@ -3089,6 +3100,23 @@ fn check_balance( Ok(()) } +fn check_ibc_token_supply( + test: &Test, + token: impl AsRef, + expected_amount: u64, +) -> Result<()> { + let rpc = get_actor_rpc(test, Who::Validator(0)); + + let query_args = + vec!["total-supply", "--token", token.as_ref(), "--node", &rpc]; + let mut client = run!(test, Bin::Client, query_args, Some(40))?; + let expected = format!("Total supply of token tnam.*: {expected_amount}"); + + client.exp_regex(&expected)?; + client.assert_success(); + Ok(()) +} + fn check_shielded_balance( test: &Test, owner: impl AsRef, From 93de5ad128f3cca0dec420aaff4d496a566862fa Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 26 May 2025 11:51:33 +0200 Subject: [PATCH 7/8] Splits the event descriptor for base fee and tip. No tip event if unnecessary --- crates/node/src/protocol.rs | 62 ++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index c4a5c0d5f33..5cdb1d20851 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -697,40 +697,44 @@ where } }; - const FEE_PAYMENT_DESCRIPTOR: std::borrow::Cow<'static, str> = - std::borrow::Cow::Borrowed("wrapper-fee-payment"); + const BASE_FEE_PAYMENT_DESCRIPTOR: std::borrow::Cow<'static, str> = + std::borrow::Cow::Borrowed("wrapper-base-fee"); + const FEE_TIP_PAYMENT_DESCRIPTOR: std::borrow::Cow<'static, str> = + std::borrow::Cow::Borrowed("wrapper-fee-tip"); let current_block_height = shell_params .state .in_mem() .get_last_block_height() .next_height(); - // Event for the tip - let proposer_post_balance = Some( - token::read_balance( - shell_params.state, - &wrapper.fee.token, - block_proposer, - ) - .map_err(Error::Error)? - .into(), - ); - shell_params.state.write_log_mut().emit_event( - TokenEvent { - descriptor: FEE_PAYMENT_DESCRIPTOR, - level: EventLevel::Tx, - operation: TokenOperation::transfer( - UserAccount::Internal(wrapper.fee_payer()), - UserAccount::Internal(block_proposer.clone()), - wrapper.fee.token.clone(), - fee_components.tip.into(), - post_bal.into(), - proposer_post_balance, - ), - } - .with(HeightAttr(current_block_height)) - .with(TxHashAttr(tx.header_hash())), - ); + // Event for the tip (if present) + if fee_components.tip.is_positive() { + let proposer_post_balance = Some( + token::read_balance( + shell_params.state, + &wrapper.fee.token, + block_proposer, + ) + .map_err(Error::Error)? + .into(), + ); + shell_params.state.write_log_mut().emit_event( + TokenEvent { + descriptor: FEE_TIP_PAYMENT_DESCRIPTOR, + level: EventLevel::Tx, + operation: TokenOperation::transfer( + UserAccount::Internal(wrapper.fee_payer()), + UserAccount::Internal(block_proposer.clone()), + wrapper.fee.token.clone(), + fee_components.tip.into(), + post_bal.into(), + proposer_post_balance, + ), + } + .with(HeightAttr(current_block_height)) + .with(TxHashAttr(tx.header_hash())), + ); + } // Event for the base fee let operation = if wrapper.fee.token == shell_params @@ -765,7 +769,7 @@ where }; shell_params.state.write_log_mut().emit_event( TokenEvent { - descriptor: FEE_PAYMENT_DESCRIPTOR, + descriptor: BASE_FEE_PAYMENT_DESCRIPTOR, level: EventLevel::Tx, operation, } From 1e7cdcf65c10e09024d2a811fcb08d7140c39d7c Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 26 May 2025 15:33:18 +0200 Subject: [PATCH 8/8] Avoids re-reading the gas price when checking transactions --- crates/node/src/protocol.rs | 39 ++++++++++++- crates/node/src/shell/finalize_block.rs | 15 +++++ crates/node/src/shell/mod.rs | 29 +++++----- crates/node/src/shell/prepare_proposal.rs | 68 ++++++++++++++++------- crates/node/src/shell/process_proposal.rs | 6 +- 5 files changed, 116 insertions(+), 41 deletions(-) diff --git a/crates/node/src/protocol.rs b/crates/node/src/protocol.rs index 5cdb1d20851..5ca9f7c29bc 100644 --- a/crates/node/src/protocol.rs +++ b/crates/node/src/protocol.rs @@ -469,6 +469,29 @@ pub struct MaspTxResult { masp_section_ref: MaspTxRef, } +pub(crate) struct ProtocolGasPrice(pub(crate) token::Amount); + +/// A trait to handle the available gas prices for a given token, the protocol +/// one and the optional override coming from a block proposer local +/// configuration +pub(crate) trait MinimumGasPrice { + /// The gas price, potentially coming from the local configuration of the + /// block proposer (only for prepare_proposal) + fn price(&self) -> token::Amount; + /// The protocol imposed gas price + fn protocol_price(&self) -> token::Amount; +} + +impl MinimumGasPrice for ProtocolGasPrice { + fn price(&self) -> token::Amount { + self.0 + } + + fn protocol_price(&self) -> token::Amount { + self.0 + } +} + /// Performs the required operation on a wrapper transaction: /// - replay protection /// - fee payment @@ -501,14 +524,24 @@ where .write_log_mut() .write_tx_hash(tx.header_hash()) .expect("Error while writing tx hash to storage"); + let protocol_minimum_gas_price = + parameters::read_gas_cost(shell_params.state, &wrapper.fee.token) + .expect("Must be able to read gas cost parameter") + .ok_or(Error::FeeError(format!( + "The provided {} token is not allowed for fee payment", + wrapper.fee.token + )))?; // Charge or check fees, propagate any errors to prevent committing invalid // data let payment_result = match block_proposer { Some(block_proposer) => { - let fee_components = - crate::shell::get_fee_components(wrapper, shell_params.state) - .map_err(|e| Error::FeeError(e.to_string()))?; + let fee_components = crate::shell::get_fee_components( + wrapper, + shell_params.state, + &ProtocolGasPrice(protocol_minimum_gas_price), + ) + .map_err(|e| Error::FeeError(e.to_string()))?; transfer_fee( shell_params, block_proposer, diff --git a/crates/node/src/shell/finalize_block.rs b/crates/node/src/shell/finalize_block.rs index b2908fcb5d5..8e8957eb04c 100644 --- a/crates/node/src/shell/finalize_block.rs +++ b/crates/node/src/shell/finalize_block.rs @@ -1353,6 +1353,7 @@ mod test_finalize_block { use super::*; use crate::oracle::control::Command; + use crate::protocol::ProtocolGasPrice; use crate::shell::test_utils::*; use crate::shims::abcipp_shim_types::shim::request::{ FinalizeBlock, ProcessedTx, @@ -4033,9 +4034,16 @@ mod test_finalize_block { .add_code(TestWasms::TxNoOp.read_bytes(), None) .add_data("Transaction data"); wrapper.sign_wrapper(albert_keypair()); + let protocol_minimum_gas_price = parameters::read_gas_cost( + &shell.state, + &wrapper.header.wrapper().unwrap().fee.token, + ) + .unwrap() + .unwrap(); let fee_components = get_fee_components( &wrapper.header().wrapper().unwrap(), &shell.state, + &ProtocolGasPrice(protocol_minimum_gas_price), ) .unwrap(); @@ -4169,9 +4177,16 @@ mod test_finalize_block { .add_code(TestWasms::TxNoOp.read_bytes(), None) .add_data("Transaction data"); wrapper.sign_wrapper(albert_keypair()); + let protocol_minimum_gas_price = parameters::read_gas_cost( + &shell.state, + &wrapper.header.wrapper().unwrap().fee.token, + ) + .unwrap() + .unwrap(); let fee_components = get_fee_components( &wrapper.header().wrapper().unwrap(), &shell.state, + &ProtocolGasPrice(protocol_minimum_gas_price), ) .unwrap(); diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index b0987a12bf6..c94ae87a3a3 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -1510,7 +1510,7 @@ where H: StorageHasher + Sync + 'static, CA: 'static + WasmCacheAccess + Sync, { - let minimum_gas_price = + let protocol_gas_price = parameters::read_gas_cost(shell_params.state, &wrapper.fee.token) .expect("Must be able to read gas cost parameter") .ok_or(Error::TxApply(protocol::Error::FeeError(format!( @@ -1518,7 +1518,11 @@ where wrapper.fee.token ))))?; - fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; + fee_data_check( + wrapper, + &crate::protocol::ProtocolGasPrice(protocol_gas_price), + shell_params.state, + )?; protocol::check_fees(shell_params, tx, wrapper) .map_err(Error::TxApply) .map(|_| ()) @@ -1526,9 +1530,9 @@ where /// Check the validity of the fee data and return the fee information split /// between base and tip -pub fn fee_data_check( +pub(crate) fn fee_data_check( wrapper: &WrapperTx, - minimum_gas_price: token::Amount, + minimum_gas_price: &impl crate::protocol::MinimumGasPrice, storage: &impl StorageRead, ) -> ShellResult { match token::denom_to_amount( @@ -1537,17 +1541,17 @@ pub fn fee_data_check( storage, ) { Ok(amount_per_gas_unit) => { - if amount_per_gas_unit < minimum_gas_price { + if amount_per_gas_unit < minimum_gas_price.price() { // The fees do not match the minimum required Err(Error::TxApply(protocol::Error::FeeError(format!( "Fee amount {:?} does not match the minimum required \ amount {:?} for token {}", wrapper.fee.amount_per_gas_unit, - minimum_gas_price, + minimum_gas_price.price(), wrapper.fee.token )))) } else { - get_fee_components(wrapper, storage) + get_fee_components(wrapper, storage, minimum_gas_price) } } Err(err) => Err(Error::TxApply(protocol::Error::FeeError(format!( @@ -1559,22 +1563,17 @@ pub fn fee_data_check( } /// Extract the components of the fee for the provided wrapper transaction -pub fn get_fee_components( +pub(crate) fn get_fee_components( wrapper: &WrapperTx, storage: &impl StorageRead, + minimum_gas_price: &impl crate::protocol::MinimumGasPrice, ) -> ShellResult { let denom_amt = wrapper.get_tx_fee().map_err(|e| { Error::TxApply(protocol::Error::FeeError(e.to_string())) })?; let fees = token::denom_to_amount(denom_amt, &wrapper.fee.token, storage)?; - let minimum_gas_price = - parameters::read_gas_cost(storage, &wrapper.fee.token) - .expect("Must be able to read gas cost parameter") - .ok_or(Error::TxApply(protocol::Error::FeeError(format!( - "The provided {} token is not allowed for fee payment", - wrapper.fee.token - ))))?; let base_fee = minimum_gas_price + .protocol_price() .checked_mul(token::Amount::from(wrapper.gas_limit)) .ok_or_else(|| { Error::TxApply(protocol::Error::FeeError( diff --git a/crates/node/src/shell/prepare_proposal.rs b/crates/node/src/shell/prepare_proposal.rs index 5f46c3f0ee7..9c0d5394c9d 100644 --- a/crates/node/src/shell/prepare_proposal.rs +++ b/crates/node/src/shell/prepare_proposal.rs @@ -344,13 +344,16 @@ where H: StorageHasher + Sync + 'static, CA: 'static + WasmCacheAccess + Sync, { - let minimum_gas_price = compute_min_gas_price( + let proposer_gas_price = get_proposer_gas_price( &wrapper.fee.token, proposer_local_config, shell_params.state, )?; - let fee_components = - super::fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; + let fee_components = super::fee_data_check( + wrapper, + &proposer_gas_price, + shell_params.state, + )?; protocol::transfer_fee( shell_params, @@ -363,17 +366,35 @@ where .map_or_else(|e| Err(Error::TxApply(e)), |_| Ok(())) } -fn compute_min_gas_price( +// The gas prices of the block proposer. +// WARNING: this type should not be exposed to other modules as the local config +// price override shall not be considered outside the prepare proposal step +struct ProposerGasPrice { + protocol: Amount, + proposer: Option, +} + +impl protocol::MinimumGasPrice for ProposerGasPrice { + fn price(&self) -> namada_sdk::token::Amount { + self.proposer.unwrap_or(self.protocol) + } + + fn protocol_price(&self) -> namada_sdk::token::Amount { + self.protocol + } +} + +fn get_proposer_gas_price( fee_token: &Address, proposer_local_config: Option<&ValidatorLocalConfig>, temp_state: &TempWlState<'_, D, H>, -) -> Result +) -> Result where D: DB + for<'iter> DBIter<'iter> + Sync + 'static, H: StorageHasher + Sync + 'static, { #[cfg(not(fuzzing))] - let consensus_min_gas_price = + let protocol_min_gas_price = namada_sdk::parameters::read_gas_cost(temp_state, fee_token) .expect("Must be able to read gas cost parameter") .ok_or_else(|| { @@ -383,16 +404,20 @@ where ))) })?; #[cfg(fuzzing)] - let consensus_min_gas_price = { + let protocol_min_gas_price = { let _ = temp_state; Amount::from_u64(10) }; + let mut minimum_gas_price = ProposerGasPrice { + protocol: protocol_min_gas_price, + proposer: None, + }; let Some(config) = proposer_local_config else { - return Ok(consensus_min_gas_price); + return Ok(minimum_gas_price); }; - let validator_min_gas_price = config + let proposer_min_gas_price = config .accepted_gas_tokens .get(fee_token) .ok_or_else(|| { @@ -403,23 +428,23 @@ where })? .to_owned(); - // The validator's local config overrides the consensus param - // when creating a block, as long as its min gas price for - // `token` is not lower than the consensus value - Ok(if validator_min_gas_price < consensus_min_gas_price { + // The validator's local config overrides the consensus param when creating + // a block, as long as its min gas price for `token` is not lower than the + // consensus value + if proposer_min_gas_price < protocol_min_gas_price { tracing::warn!( fee_token = %fee_token, - validator_min_gas_price = %DenominatedAmount::from(validator_min_gas_price), - consensus_min_gas_price = %DenominatedAmount::from(consensus_min_gas_price), + validator_min_gas_price = %DenominatedAmount::from(proposer_min_gas_price), + consensus_min_gas_price = %DenominatedAmount::from(protocol_min_gas_price), "The gas price for the given token set by the block proposer \ is lower than the value agreed upon by consensus. \ Falling back to consensus value." ); - - consensus_min_gas_price } else { - validator_min_gas_price - }) + minimum_gas_price.proposer = Some(proposer_min_gas_price); + }; + + Ok(minimum_gas_price) } #[allow(clippy::cast_possible_wrap, clippy::cast_possible_truncation)] @@ -450,6 +475,7 @@ mod test_prepare_proposal { use namada_vote_ext::{ethereum_events, ethereum_tx_data_variants}; use super::*; + use crate::protocol::MinimumGasPrice; use crate::shell::EthereumTxData; use crate::shell::test_utils::{ self, TestShell, gen_keypair, get_pkh_from_address, @@ -1508,13 +1534,13 @@ mod test_prepare_proposal { m }, }; - let computed_min_gas_price = compute_min_gas_price( + let computed_min_gas_price = get_proposer_gas_price( &shell.state.in_mem().native_token, Some(&config), &temp_state, ) .unwrap(); - assert_eq!(computed_min_gas_price, consensus_min_gas_price); + assert_eq!(computed_min_gas_price.price(), consensus_min_gas_price); } } diff --git a/crates/node/src/shell/process_proposal.rs b/crates/node/src/shell/process_proposal.rs index 928ac1c264d..18e525b5c31 100644 --- a/crates/node/src/shell/process_proposal.rs +++ b/crates/node/src/shell/process_proposal.rs @@ -576,15 +576,17 @@ where H: StorageHasher + Sync + 'static, CA: 'static + WasmCacheAccess + Sync, { - let minimum_gas_price = + let protocol_minimum_gas_price = parameters::read_gas_cost(shell_params.state, &wrapper.fee.token) .expect("Must be able to read gas cost parameter") .ok_or(Error::TxApply(protocol::Error::FeeError(format!( "The provided {} token is not allowed for fee payment", wrapper.fee.token ))))?; + let minimum_gas_price = + protocol::ProtocolGasPrice(protocol_minimum_gas_price); let fee_components = - fee_data_check(wrapper, minimum_gas_price, shell_params.state)?; + fee_data_check(wrapper, &minimum_gas_price, shell_params.state)?; protocol::transfer_fee( shell_params,