From 458c28b0c6c39a4d39564faeb54b5ea602c3f19f Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Wed, 22 Mar 2023 12:16:47 +0100 Subject: [PATCH] Removes MASP shielded fees --- apps/src/lib/client/tx.rs | 62 +++++-------------- .../lib/node/ledger/shell/finalize_block.rs | 9 +-- apps/src/lib/node/ledger/shell/mod.rs | 27 ++++---- .../lib/node/ledger/shell/process_proposal.rs | 13 +--- benches/mod.rs | 6 +- 5 files changed, 32 insertions(+), 85 deletions(-) diff --git a/apps/src/lib/client/tx.rs b/apps/src/lib/client/tx.rs index 4d98ad7bde5..aef1ed5f34f 100644 --- a/apps/src/lib/client/tx.rs +++ b/apps/src/lib/client/tx.rs @@ -40,7 +40,7 @@ use namada::ledger::governance::storage as gov_storage; use namada::ledger::masp; use namada::ledger::pos::{CommissionPair, PosParams}; use namada::proto::Tx; -use namada::types::address::{masp, masp_tx_key, Address}; +use namada::types::address::{masp, Address}; use namada::types::governance::{ OfflineProposal, OfflineVote, Proposal, ProposalVote, }; @@ -69,7 +69,7 @@ use super::types::ShieldedTransferContext; use crate::cli::context::WalletAddress; use crate::cli::{args, safe_exit, Context}; use crate::client::rpc::{query_conversion, query_storage_value}; -use crate::client::signing::{find_keypair, sign_tx, tx_signer, TxSigningKey}; +use crate::client::signing::{find_keypair, sign_tx, TxSigningKey}; use crate::client::tendermint_rpc_types::{TxBroadcastData, TxResponse}; use crate::client::types::ParsedTxTransferArgs; use crate::facade::tendermint_config::net::Address as TendermintAddress; @@ -1353,7 +1353,6 @@ fn convert_amount( pub async fn gen_shielded_transfer( ctx: &mut C, args: &ParsedTxTransferArgs, - shielded_gas: bool, ) -> Result, builder::Error> where C: ShieldedTransferContext, @@ -1372,26 +1371,20 @@ where // Convert transaction amount into MASP types let (asset_type, amount) = convert_amount(epoch, &args.token, args.amount); + // Fees are always paid outside of MASP + builder.set_fee(Amount::zero())?; + // Transactions with transparent input and shielded output // may be affected if constructed close to epoch boundary let mut epoch_sensitive: bool = false; // If there are shielded inputs if let Some(sk) = spending_key { - // Transaction fees need to match the amount in the wrapper Transfer - // when MASP source is used - let (_, fee) = - convert_amount(epoch, &args.tx.fee_token, args.tx.fee_amount); - builder.set_fee(fee.clone())?; - // FIXME: fix gas here? - // If the gas is coming from the shielded pool, then our shielded inputs - // must also cover the gas fee - let required_amt = if shielded_gas { amount + fee } else { amount }; // Locate unspent notes that can help us meet the transaction amount let (_, unspent_notes, used_convs) = ctx .collect_unspent_notes( args.tx.ledger_address.clone(), &to_viewing_key(&sk).vk, - required_amt, + amount, epoch, ) .await; @@ -1410,10 +1403,6 @@ where } } } else { - // No transfer fees come from the shielded transaction for non-MASP - // sources - //FIXME: fix gas here? - builder.set_fee(Amount::zero())?; // We add a dummy UTXO to our transaction, but only the source of the // parent Transfer object is used to validate fund availability let secp_sk = @@ -1612,37 +1601,15 @@ pub async fn submit_transfer(mut ctx: Context, args: args::TxTransfer) { }; let masp_addr = masp(); - // For MASP sources, use a special sentinel key recognized by VPs as default - // signer. Also, if the transaction is shielded, redact the amount and token + // For MASP sources, redact the amount and token // types by setting the transparent value to 0 and token type to a constant. // This has no side-effect because transaction is to self. - let (default_signer, amount, token) = - if source == masp_addr && target == masp_addr { - // TODO Refactor me, we shouldn't rely on any specific token here. - ( - TxSigningKey::SecretKey(masp_tx_key()), - 0.into(), - ctx.native_token.clone(), - ) - } else if source == masp_addr { - ( - TxSigningKey::SecretKey(masp_tx_key()), - args.amount, - parsed_args.token.clone(), - ) - } else { - ( - TxSigningKey::WalletAddress(args.source.to_address()), - args.amount, - parsed_args.token.clone(), - ) - }; - // If our chosen signer is the MASP sentinel key, then our shielded inputs - // will need to cover the gas fees. - let chosen_signer = tx_signer(&mut ctx, &args.tx, default_signer.clone()) - .await - .ref_to(); - let shielded_gas = masp_tx_key().ref_to() == chosen_signer; + let (amount, token) = if source == masp_addr && target == masp_addr { + // TODO Refactor me, we shouldn't rely on any specific token here. + (0.into(), ctx.native_token.clone()) + } else { + (args.amount, parsed_args.token.clone()) + }; // Determine whether to pin this transaction to a storage key let key = match ctx.get(&args.target) { TransferTarget::PaymentAddress(pa) if pa.is_pinned() => Some(pa.hash()), @@ -1682,8 +1649,7 @@ pub async fn submit_transfer(mut ctx: Context, args: args::TxTransfer) { // short-circuited let _ = ctx.shielded.save(); let stx_result = - gen_shielded_transfer(&mut ctx, &parsed_args, shielded_gas) - .await; + gen_shielded_transfer(&mut ctx, &parsed_args).await; match stx_result { Ok(stx) => stx.map(|x| x.0), Err(builder::Error::ChangeIsNegative(_)) => { diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index e7e7597ebcb..82375a8873e 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -531,13 +531,8 @@ where #[cfg(not(feature = "mainnet"))] has_valid_pow: bool, ) -> Result<()> { // Charge fee - let fee_payer = if wrapper.pk != address::masp_tx_key().ref_to() { - wrapper.fee_payer() - } else { - address::masp() //FIXME: here? - }; - - let balance_key = token::balance_key(&wrapper.fee.token, &fee_payer); + let balance_key = + token::balance_key(&wrapper.fee.token, &wrapper.fee_payer()); let balance: token::Amount = self .wl_storage .read(&balance_key) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index bb089877111..52c9702e64e 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -35,7 +35,7 @@ use namada::ledger::storage_api::{self, StorageRead}; use namada::ledger::{ibc, parameters, pos, protocol, replay_protection}; use namada::proof_of_stake::{self, read_pos_params, slash}; use namada::proto::{self, Tx}; -use namada::types::address::{masp, masp_tx_key, Address}; +use namada::types::address::Address; use namada::types::chain::ChainId; use namada::types::internal::WrapperTxInQueue; use namada::types::key::*; @@ -734,14 +734,9 @@ where return response; } - // Check balance for fee - let fee_payer = if wrapper.pk != masp_tx_key().ref_to() { - wrapper.fee_payer() - } else { - masp() - }; // check that the fee payer has sufficient balance - let balance = self.get_balance(&wrapper.fee.token, &fee_payer); + let balance = + self.get_balance(&wrapper.fee.token, &wrapper.fee_payer()); // In testnets with a faucet, tx is allowed to skip fees if // it includes a valid PoW @@ -1166,7 +1161,7 @@ mod test_utils { }, &keypair, Epoch(0), - 0.into(), + 1.into(), tx, Default::default(), #[cfg(not(feature = "mainnet"))] @@ -1237,9 +1232,7 @@ mod test_mempool_validate { use crate::facade::tendermint_proto::abci::RequestInitChain; use namada::proof_of_stake::Epoch; use namada::proto::SignedTxData; - use namada::types::transaction::{ - Fee, GasLimit, WrapperTx, GAS_LIMIT_RESOLUTION, - }; + use namada::types::transaction::{Fee, GasLimit, WrapperTx}; use super::test_utils::TestShell; use super::{MempoolTxType, *}; @@ -1406,6 +1399,14 @@ mod test_mempool_validate { #[test] fn test_replay_attack() { let (mut shell, _) = TestShell::new(); + shell.init_chain(RequestInitChain { + time: Some(Timestamp { + seconds: 0, + nanos: 0, + }), + chain_id: ChainId::default().to_string(), + ..Default::default() + }); let keypair = super::test_utils::gen_keypair(); @@ -1423,7 +1424,7 @@ mod test_mempool_validate { }, &keypair, Epoch(0), - 0.into(), + 1.into(), tx, Default::default(), #[cfg(not(feature = "mainnet"))] diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index a971b473ee8..72eb271ad84 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -401,20 +401,9 @@ where "Couldn't write wrapper tx hash to write log", ); - // If the public key corresponds to the MASP sentinel - // transaction key, then the fee payer is effectively - // the MASP, otherwise derive - // they payer from public key. - //FIXME: review this part - let fee_payer = if wrapper.pk != masp_tx_key().ref_to() - { - wrapper.fee_payer() - } else { - masp() - }; // check that the fee payer has sufficient balance let balance = - self.get_balance(&wrapper.fee.token, &fee_payer); + self.get_balance(&wrapper.fee.token, &wrapper.fee_payer()); // In testnets, tx is allowed to skip fees if it // includes a valid PoW diff --git a/benches/mod.rs b/benches/mod.rs index 5cf5f8731d3..00c2a8bfd79 100644 --- a/benches/mod.rs +++ b/benches/mod.rs @@ -614,11 +614,7 @@ impl BenchShieldedCtx { &[], )); let shielded = async_runtime - .block_on(gen_shielded_transfer( - self, - &mock_parsed_transfer_args, - false, - )) + .block_on(gen_shielded_transfer(self, &mock_parsed_transfer_args)) .unwrap() .map(|x| x.0);