diff --git a/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md b/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md new file mode 100644 index 00000000000..bcbba477171 --- /dev/null +++ b/.changelog/unreleased/improvements/3312-outsource-masp-sig-verification.md @@ -0,0 +1,2 @@ +- Moved the signature verifications out of the masp vp and into the affected + addresses' vps. ([\#3312](https://github.com/anoma/namada/issues/3312)) \ No newline at end of file diff --git a/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md b/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md new file mode 100644 index 00000000000..55e02df5ae0 --- /dev/null +++ b/.changelog/unreleased/improvements/3516-reduce-masp-vp-signatures.md @@ -0,0 +1,2 @@ +- Eliminates the MASP VPs requirement for all debited accounts to sign a Tx. + ([\#3516](https://github.com/anoma/namada/pull/3516)) \ No newline at end of file diff --git a/crates/namada/src/ledger/native_vp/masp.rs b/crates/namada/src/ledger/native_vp/masp.rs index c550c0a4db6..4b6aa32226a 100644 --- a/crates/namada/src/ledger/native_vp/masp.rs +++ b/crates/namada/src/ledger/native_vp/masp.rs @@ -21,7 +21,6 @@ use namada_core::ibc::apps::transfer::types::msgs::transfer::MsgTransfer as IbcM use namada_core::ibc::apps::transfer::types::packet::PacketData; use namada_core::masp::{addr_taddr, encode_asset_type, ibc_taddr, MaspEpoch}; use namada_core::storage::Key; -use namada_gas::GasMetering; use namada_governance::storage::is_proposal_accepted; use namada_ibc::core::channel::types::commitment::{ compute_packet_commitment, PacketCommitment, @@ -43,6 +42,7 @@ use namada_sdk::masp::TAddrData; use namada_state::{ConversionState, OptionExt, ResultExt, StateRead}; use namada_token::read_denom; use namada_token::validation::verify_shielded_tx; +use namada_tx::action::Read; use namada_tx::BatchedTxRef; use namada_vp_env::VpEnv; use thiserror::Error; @@ -687,33 +687,26 @@ where .read_post(&counterpart_balance_key)? .unwrap_or_default(); // Public keys must be the hash of the sources/targets - let address_hash = addr_taddr(counterpart.clone()); + let addr_hash = addr_taddr(counterpart.clone()); // Enable the decoding of these counterpart addresses result .decoder - .insert(address_hash, TAddrData::Addr(counterpart.clone())); + .insert(addr_hash, TAddrData::Addr(counterpart.clone())); + let zero = ValueSum::zero(); // Finally record the actual balance change starting with the initial // state - let pre_entry = result - .pre - .get(&address_hash) - .cloned() - .unwrap_or(ValueSum::zero()); + let pre_entry = result.pre.get(&addr_hash).unwrap_or(&zero).clone(); result.pre.insert( - address_hash, + addr_hash, checked!( pre_entry + &ValueSum::from_pair((*token).clone(), pre_balance) ) .map_err(native_vp::Error::new)?, ); - // And then record thee final state - let post_entry = result - .post - .get(&address_hash) - .cloned() - .unwrap_or(ValueSum::zero()); + // And then record the final state + let post_entry = result.post.get(&addr_hash).cloned().unwrap_or(zero); result.post.insert( - address_hash, + addr_hash, checked!( post_entry + &ValueSum::from_pair((*token).clone(), post_balance) @@ -757,6 +750,7 @@ where &self, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, + verifiers: &BTreeSet
, ) -> Result<()> { let masp_epoch_multiplier = namada_parameters::read_masp_epoch_multiplier_parameter( @@ -771,6 +765,7 @@ where })?; let conversion_state = self.ctx.state.in_mem().get_conversion_state(); let ibc_msg = self.ctx.get_ibc_message(tx_data).ok(); + let actions = self.ctx.read_actions()?; let shielded_tx = if let Some(IbcMessage::Envelope(ref envelope)) = ibc_msg { extract_masp_tx_from_envelope(envelope).ok_or_else(|| { @@ -781,7 +776,7 @@ where } else { // Get the Transaction object from the actions let masp_section_ref = - namada_tx::action::get_masp_section_ref(&self.ctx)? + namada_tx::action::get_masp_section_ref(&actions) .ok_or_else(|| { native_vp::Error::new_const( "Missing MASP section reference in action", @@ -809,19 +804,21 @@ where } // Check the validity of the keys and get the transfer data - let mut changed_balances = + let changed_balances = self.validate_state_and_get_transfer_data(keys_changed, ibc_msg)?; + // Some constants that will be used repeatedly + let zero = ValueSum::zero(); let masp_address_hash = addr_taddr(MASP); verify_sapling_balancing_value( changed_balances .pre .get(&masp_address_hash) - .unwrap_or(&ValueSum::zero()), + .unwrap_or(&zero), changed_balances .post .get(&masp_address_hash) - .unwrap_or(&ValueSum::zero()), + .unwrap_or(&zero), &shielded_tx.sapling_value_balance(), masp_epoch, &changed_balances.tokens, @@ -829,7 +826,7 @@ where )?; // The set of addresses that are required to authorize this transaction - let mut signers = BTreeSet::new(); + let mut authorizers = BTreeSet::new(); // Checks on the sapling bundle // 1. The spend descriptions' anchors are valid @@ -845,32 +842,56 @@ where self.valid_note_commitment_update(&shielded_tx)?; // Checks on the transparent bundle, if present + let mut changed_bals_minus_txn = changed_balances.clone(); validate_transparent_bundle( &shielded_tx, - &mut changed_balances, + &mut changed_bals_minus_txn, masp_epoch, conversion_state, - &mut signers, + &mut authorizers, )?; - // Ensure that every account for which balance has gone down has - // authorized this transaction - for (addr, pre) in changed_balances.pre { - if changed_balances - .post - .get(&addr) - .unwrap_or(&ValueSum::zero()) - < &pre - && addr != masp_address_hash + // Ensure that every account for which balance has gone down as a result + // of the Transaction has authorized this transaction + for (addr, minus_txn_pre) in changed_bals_minus_txn.pre { + // The pre-balance seen by all VPs including this one + let pre = changed_balances.pre.get(&addr).unwrap_or(&zero); + // The post-balance seen by all VPs including this one + let post = changed_balances.post.get(&addr).unwrap_or(&zero); + // The post-balance if the effects of the Transaction are removed + let minus_txn_post = + changed_bals_minus_txn.post.get(&addr).unwrap_or(&zero); + // Never require a signature from the MASP VP + if addr != masp_address_hash && + // Only require further authorization if without the Transaction, + // this Tx would decrease the balance of this address + minus_txn_post < &minus_txn_pre && + // Only require further authorization from this address if the + // Transaction alters its balance + (minus_txn_pre, minus_txn_post) != (pre.clone(), post) { - signers.insert(addr); + // This address will need to provide further authorization + authorizers.insert(addr); } } + let mut actions_authorizers: HashSet<&Address> = actions + .iter() + .filter_map(|action| { + if let namada_tx::action::Action::Masp( + namada_tx::action::MaspAction::MaspAuthorizer(addr), + ) = action + { + Some(addr) + } else { + None + } + }) + .collect(); // Ensure that this transaction is authorized by all involved parties - for signer in signers { + for authorizer in authorizers { if let Some(TAddrData::Addr(IBC)) = - changed_balances.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&authorizer) { // If the IBC address is a signatory, then it means that either // Tx - Transaction(s) causes a decrease in the IBC balance or @@ -887,7 +908,7 @@ where if let Some(transp_bundle) = shielded_tx.transparent_bundle() { for vout in transp_bundle.vout.iter() { if let Some(TAddrData::Ibc(_)) = - changed_balances.decoder.get(&vout.address) + changed_bals_minus_txn.decoder.get(&vout.address) { let error = native_vp::Error::new_const( "Simultaneous credit and debit of IBC account \ @@ -900,39 +921,53 @@ where } } } else if let Some(TAddrData::Addr(signer)) = - changed_balances.decoder.get(&signer) + changed_bals_minus_txn.decoder.get(&authorizer) { - // Otherwise the signer must be decodable so that we can - // manually check the signatures - let public_keys_index_map = - crate::account::public_keys_index_map( - &self.ctx.pre(), - signer, - )?; - let threshold = - crate::account::threshold(&self.ctx.pre(), signer)? - .unwrap_or(1); - let mut gas_meter = self.ctx.gas_meter.borrow_mut(); - tx_data - .tx - .verify_signatures( - &[tx_data.tx.raw_header_hash()], - public_keys_index_map, - &Some(signer.clone()), - threshold, - || gas_meter.consume(crate::gas::VERIFY_TX_SIG_GAS), - ) - .map_err(native_vp::Error::new)?; + // Otherwise the owner's vp must have been triggered and the + // relative action must have been written + if !verifiers.contains(signer) { + let error = native_vp::Error::new_alloc(format!( + "The required vp of address {signer} was not triggered" + )) + .into(); + tracing::debug!("{error}"); + return Err(error); + } + + // The action is required becuse the target vp might have been + // triggered for other reasons but we need to signal it that it + // is required to validate a discrepancy in its balance change + // because of a masp transaction, which might require a + // different validation than a normal balance change + if !actions_authorizers.swap_remove(signer) { + let error = native_vp::Error::new_alloc(format!( + "The required masp authorizer action for address \ + {signer} is missing" + )) + .into(); + tracing::debug!("{error}"); + return Err(error); + } } else { - // We are not able to decode the signer, so just fail + // We are not able to decode the authorizer, so just fail let error = native_vp::Error::new_const( - "Unable to decode a transaction signer", + "Unable to decode a transaction authorizer", ) .into(); tracing::debug!("{error}"); return Err(error); } } + // The transaction shall not push masp authorizer actions that are not + // needed cause this might lead vps to run a wrong validation logic + if !actions_authorizers.is_empty() { + let error = native_vp::Error::new_const( + "Found masp authorizer actions that are not required", + ) + .into(); + tracing::debug!("{error}"); + return Err(error); + } // Verify the proofs verify_shielded_tx(&shielded_tx, |gas| self.ctx.charge_gas(gas)) @@ -957,18 +992,17 @@ fn unepoched_tokens( Ok(()) } -// Handle transparent input fn validate_transparent_input( vin: &TxIn, changed_balances: &mut ChangedBalances, transparent_tx_pool: &mut I128Sum, epoch: MaspEpoch, conversion_state: &ConversionState, - signers: &mut BTreeSet, + authorizers: &mut BTreeSet, ) -> Result<()> { // A decrease in the balance of an account needs to be // authorized by the account of this transparent input - signers.insert(vin.address); + authorizers.insert(vin.address); // Non-masp sources add to the transparent tx pool *transparent_tx_pool = transparent_tx_pool .checked_add( @@ -1044,7 +1078,6 @@ fn validate_transparent_input( Ok(()) } -// Handle transparent output fn validate_transparent_output( out: &TxOut, changed_balances: &mut ChangedBalances, @@ -1116,7 +1149,7 @@ fn validate_transparent_bundle( changed_balances: &mut ChangedBalances, epoch: MaspEpoch, conversion_state: &ConversionState, - signers: &mut BTreeSet, + authorizers: &mut BTreeSet, ) -> Result<()> { // The Sapling value balance adds to the transparent tx pool let mut transparent_tx_pool = shielded_tx.sapling_value_balance(); @@ -1129,7 +1162,7 @@ fn validate_transparent_bundle( &mut transparent_tx_pool, epoch, conversion_state, - signers, + authorizers, )?; } @@ -1253,7 +1286,7 @@ where &self, tx_data: &BatchedTxRef<'_>, keys_changed: &BTreeSet, - _verifiers: &BTreeSet
, + verifiers: &BTreeSet
, ) -> Result<()> { let masp_keys_changed: Vec<&Key> = keys_changed.iter().filter(|key| is_masp_key(key)).collect(); @@ -1283,7 +1316,7 @@ where self.is_valid_parameter_change(tx_data) } else if masp_transfer_changes { // The MASP transfer keys can only be changed by a valid Transaction - self.is_valid_masp_transfer(tx_data, keys_changed) + self.is_valid_masp_transfer(tx_data, keys_changed, verifiers) } else { // Changing no MASP keys at all is also fine Ok(()) diff --git a/crates/namada/src/ledger/protocol/mod.rs b/crates/namada/src/ledger/protocol/mod.rs index 97616f3b34e..a3bbb88e5eb 100644 --- a/crates/namada/src/ledger/protocol/mod.rs +++ b/crates/namada/src/ledger/protocol/mod.rs @@ -415,9 +415,10 @@ where if is_accepted { // If the transaction was a masp one append the // transaction refs for the events + let actions = + state.read_actions().map_err(Error::StateError)?; if let Some(masp_section_ref) = - namada_tx::action::get_masp_section_ref(state) - .map_err(Error::StateError)? + namada_tx::action::get_masp_section_ref(&actions) { extended_tx_result .masp_tx_refs @@ -740,8 +741,10 @@ where ); } - let masp_ref = namada_tx::action::get_masp_section_ref(*state) - .map_err(Error::StateError)?; + let actions = + state.read_actions().map_err(Error::StateError)?; + let masp_ref = + namada_tx::action::get_masp_section_ref(&actions); // Ensure that the transaction is actually a masp one, otherwise // reject (is_masp_transfer(&result.changed_keys) && result.is_accepted()) diff --git a/crates/trans_token/src/storage.rs b/crates/trans_token/src/storage.rs index 1ba6f238f82..4a21b2d222e 100644 --- a/crates/trans_token/src/storage.rs +++ b/crates/trans_token/src/storage.rs @@ -1,6 +1,7 @@ use std::collections::{BTreeMap, BTreeSet}; use namada_core::address::{Address, InternalAddress}; +use namada_core::collections::HashSet; use namada_core::hints; use namada_core::token::{self, Amount, AmountError, DenominatedAmount}; use namada_storage as storage; @@ -263,15 +264,16 @@ where /// Transfer tokens from `sources` to `dests`. Returns an `Err` if any source /// has insufficient balance or if the transfer to any destination would /// overflow (This can only happen if the total supply doesn't fit in -/// `token::Amount`). +/// `token::Amount`). Returns the set of debited accounts. pub fn multi_transfer( storage: &mut S, sources: &BTreeMap<(Address, Address), Amount>, dests: &BTreeMap<(Address, Address), Amount>, -) -> storage::Result<()> +) -> storage::Result> where S: StorageRead + StorageWrite, { + let mut debited_accounts = HashSet::new(); // Collect all the accounts whose balance has changed let mut accounts = BTreeSet::new(); accounts.extend(sources.keys().cloned()); @@ -306,6 +308,7 @@ where ) .ok_or_else(overflow_err)? } else { + debited_accounts.insert(owner.to_owned()); owner_balance .checked_sub( src_amt.checked_sub(dest_amt).ok_or_else(unexpected_err)?, @@ -315,7 +318,7 @@ where // Wite the new balance storage.write(&owner_key, new_owner_balance)?; } - Ok(()) + Ok(debited_accounts) } /// Mint `amount` of `token` as `minter` to `dest`. diff --git a/crates/tx/src/action.rs b/crates/tx/src/action.rs index dbd2e4c20c0..323de1754cb 100644 --- a/crates/tx/src/action.rs +++ b/crates/tx/src/action.rs @@ -21,7 +21,7 @@ pub type Actions = Vec; /// An action applied from a tx. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum Action { Pos(PosAction), Gov(GovAction), @@ -32,7 +32,7 @@ pub enum Action { /// PoS tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum PosAction { BecomeValidator(Address), DeactivateValidator(Address), @@ -50,7 +50,7 @@ pub enum PosAction { /// Gov tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum GovAction { InitProposal { author: Address }, VoteProposal { id: u64, voter: Address }, @@ -58,17 +58,19 @@ pub enum GovAction { /// PGF tx actions. #[allow(missing_docs)] -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] pub enum PgfAction { ResignSteward(Address), UpdateStewardCommission(Address), } -/// MASP tx action. -#[derive(Clone, Debug, BorshDeserialize, BorshSerialize)] -pub struct MaspAction { +/// MASP tx actions. +#[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)] +pub enum MaspAction { /// The hash of the masp [`crate::types::Section`] - pub masp_section_ref: TxId, + MaspSectionRef(TxId), + /// A required authorizer for the transaction + MaspAuthorizer(Address), } /// Read actions from temporary storage @@ -122,17 +124,16 @@ fn storage_key() -> storage::Key { /// Helper function to get the optional masp section reference from the /// [`Actions`]. If more than one [`MaspAction`] has been found we return the /// first one -pub fn get_masp_section_ref( - reader: &T, -) -> Result, ::Err> { - Ok(reader.read_actions()?.into_iter().find_map(|action| { - // In case of multiple masp actions we get the first one - if let Action::Masp(MaspAction { masp_section_ref }) = action { - Some(masp_section_ref) +pub fn get_masp_section_ref(actions: &Actions) -> Option { + actions.iter().find_map(|action| { + if let Action::Masp(MaspAction::MaspSectionRef(masp_section_ref)) = + action + { + Some(masp_section_ref.to_owned()) } else { None } - })) + }) } /// Helper function to check if the action is IBC shielding transfer diff --git a/crates/tx_prelude/src/token.rs b/crates/tx_prelude/src/token.rs index f0cb0f09d65..5e2f630255a 100644 --- a/crates/tx_prelude/src/token.rs +++ b/crates/tx_prelude/src/token.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use namada_core::address::Address; +use namada_core::collections::HashSet; use namada_events::extend::UserAccount; use namada_events::{EmitEvents, EventLevel}; use namada_token::event::{TokenEvent, TokenOperation}; @@ -50,13 +51,14 @@ pub fn transfer( Ok(()) } -/// A transparent token transfer that can be used in a transaction. +/// A transparent token transfer that can be used in a transaction. Returns the +/// set of debited accounts. pub fn multi_transfer( ctx: &mut Ctx, sources: &BTreeMap<(Address, Address), Amount>, dests: &BTreeMap<(Address, Address), Amount>, -) -> TxResult { - namada_token::multi_transfer(ctx, sources, dests)?; +) -> crate::EnvResult> { + let debited_accounts = namada_token::multi_transfer(ctx, sources, dests)?; let mut evt_sources = BTreeMap::new(); let mut evt_targets = BTreeMap::new(); @@ -108,5 +110,5 @@ pub fn multi_transfer( }, }); - Ok(()) + Ok(debited_accounts) } diff --git a/wasm/tx_ibc/src/lib.rs b/wasm/tx_ibc/src/lib.rs index 78963875f48..a3e11558748 100644 --- a/wasm/tx_ibc/src/lib.rs +++ b/wasm/tx_ibc/src/lib.rs @@ -65,7 +65,9 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { update_masp_note_commitment_tree(&shielded) .wrap_err("Failed to update the MASP commitment tree")?; if let Some(masp_section_ref) = masp_section_ref { - ctx.push_action(Action::Masp(MaspAction { masp_section_ref }))?; + ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( + masp_section_ref, + )))?; } else { ctx.push_action(Action::IbcShielding)?; } diff --git a/wasm/tx_transfer/src/lib.rs b/wasm/tx_transfer/src/lib.rs index 0cc58998dfe..b4ee0075f71 100644 --- a/wasm/tx_transfer/src/lib.rs +++ b/wasm/tx_transfer/src/lib.rs @@ -2,9 +2,10 @@ //! This tx uses `token::TransparentTransfer` wrapped inside `SignedTxData` //! as its input as declared in `namada` crate. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use namada_tx_prelude::action::{Action, MaspAction, Write}; +use namada_tx_prelude::masp::addr_taddr; use namada_tx_prelude::*; #[transaction] @@ -33,7 +34,7 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .collect::>(); // Effect the multi transfer - token::multi_transfer(ctx, &sources, &targets) + let debited_accounts = token::multi_transfer(ctx, &sources, &targets) .wrap_err("Token transfer failed")?; // Apply the shielded transfer if there is a link to one @@ -53,7 +54,38 @@ fn apply_tx(ctx: &mut Ctx, tx_data: BatchedTx) -> TxResult { .wrap_err("Encountered error while handling MASP transaction")?; update_masp_note_commitment_tree(&shielded) .wrap_err("Failed to update the MASP commitment tree")?; - ctx.push_action(Action::Masp(MaspAction { masp_section_ref }))?; + + ctx.push_action(Action::Masp(MaspAction::MaspSectionRef( + masp_section_ref, + )))?; + // Extract the debited accounts for the masp part of the transfer and + // push the relative actions + let vin_addresses = shielded.transparent_bundle().map_or_else( + Default::default, + |bndl| { + bndl.vin + .iter() + .map(|vin| vin.address) + .collect::>() + }, + ); + let masp_authorizers: Vec<_> = debited_accounts + .into_iter() + .filter(|account| { + vin_addresses.contains(&addr_taddr(account.clone())) + }) + .collect(); + if masp_authorizers.len() != vin_addresses.len() { + return Err(Error::SimpleMessage( + "Transfer transaction does not debit all the expected accounts", + )); + } + + for authorizer in masp_authorizers { + ctx.push_action(Action::Masp(MaspAction::MaspAuthorizer( + authorizer, + )))?; + } } Ok(()) diff --git a/wasm/vp_implicit/src/lib.rs b/wasm/vp_implicit/src/lib.rs index 7a0b237d4a7..884c03a2282 100644 --- a/wasm/vp_implicit/src/lib.rs +++ b/wasm/vp_implicit/src/lib.rs @@ -101,7 +101,9 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(_) => (), + Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget + .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } } diff --git a/wasm/vp_user/src/lib.rs b/wasm/vp_user/src/lib.rs index 0cfe763c7d9..a046c19a951 100644 --- a/wasm/vp_user/src/lib.rs +++ b/wasm/vp_user/src/lib.rs @@ -101,7 +101,9 @@ fn validate_tx( &tx, &addr, )?, - Action::Masp(_) => (), + Action::Masp(MaspAction::MaspAuthorizer(source)) => gadget + .verify_signatures_when(|| source == addr, ctx, &tx, &addr)?, + Action::Masp(MaspAction::MaspSectionRef(_)) => (), Action::IbcShielding => (), } }