From 41ebf1eb8ef3eb1b18ddbb0b47d39af44b04d9ee Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 4 Oct 2024 19:09:21 +0200 Subject: [PATCH 1/5] Avoids multiple wrapper sigs when batching `reveal_pk` --- crates/apps_lib/src/client/tx.rs | 53 +++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 2384f21b2b..b8dd687176 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -16,7 +16,9 @@ use namada_sdk::key::*; use namada_sdk::rpc::{InnerTxResult, TxBroadcastData, TxResponse}; use namada_sdk::state::EPOCH_SWITCH_BLOCKS_DELAY; use namada_sdk::tx::data::compute_inner_tx_hash; -use namada_sdk::tx::{CompressedAuthorization, Section, Signer, Tx}; +use namada_sdk::tx::{ + Authorization, CompressedAuthorization, Section, Signer, Tx, +}; use namada_sdk::wallet::alias::{validator_address, validator_consensus_key}; use namada_sdk::wallet::{Wallet, WalletIo}; use namada_sdk::{error, signing, tx, Namada}; @@ -247,9 +249,56 @@ where let (mut batched_tx, batched_signing_data) = namada_sdk::tx::build_batch(batched_tx_data)?; for sig_data in batched_signing_data { + // FIXME: also leave a not on build_batch that caller should always try + // to do this thing sign(namada, &mut batched_tx, args, sig_data).await?; } + // FIXME: maybe export this whole thing to a function that we epoxse in the + // sdk? + // Find the wrapper signature with the least targets and keep it, delete all + // the others (redundant) + if let Some(expected_wrapper_signer) = + batched_tx.header.wrapper().map(|wrapper| wrapper.pk) + { + let mut keep_wrapper_sig: Option<&Authorization> = None; + let mut remove_sigs = vec![]; + + for section in batched_tx.sections.iter() { + if let Section::Authorization(auth) = section { + let targets = auth.targets.len(); + // Examine only wrapper signatures + if targets > 1 { + // If signer is not the expected one drop this signature + match auth.signer { + Signer::PubKeys(ref sigs) + if sigs.as_slice() + == [expected_wrapper_signer.clone()] => {} + _ => { + remove_sigs.push(auth.to_owned()); + continue; + } + } + + if let Some(prev) = keep_wrapper_sig { + if targets < prev.targets.len() { + remove_sigs.push(prev.to_owned()); + keep_wrapper_sig = Some(auth); + } + } else { + keep_wrapper_sig = Some(auth); + } + } + } + } + + // Remove redundant wrapper signatures + batched_tx.sections.retain(|section| match section { + Section::Authorization(auth) => !remove_sigs.contains(auth), + _ => true, + }); + } + namada.submit(batched_tx, args).await } @@ -799,6 +848,8 @@ pub async fn submit_transparent_transfer( let transfer_data = args.clone().build(namada).await?; + // FIXME: issue here, if they dump we don't dump the reveal pk tx, so they + // sign an incomplete tx if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, transfer_data.0); } else { From aaed70de6be32b7424632bdc29b0b6339bd388d0 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 7 Oct 2024 10:28:24 +0200 Subject: [PATCH 2/5] Better handling of signer when pruning redundant sigs --- crates/apps_lib/src/client/tx.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index b8dd687176..2269f79a4e 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -255,7 +255,8 @@ where } // FIXME: maybe export this whole thing to a function that we epoxse in the - // sdk? + // sdk and call that function every time after build_batch? + // FIXME: write a test for this thing? // Find the wrapper signature with the least targets and keep it, delete all // the others (redundant) if let Some(expected_wrapper_signer) = @@ -274,6 +275,10 @@ where Signer::PubKeys(ref sigs) if sigs.as_slice() == [expected_wrapper_signer.clone()] => {} + Signer::Address(ref addr) + if addr + == &Address::from(&expected_wrapper_signer) => { + } _ => { remove_sigs.push(auth.to_owned()); continue; @@ -299,6 +304,10 @@ where }); } + // FIXME: remove after having checked that the new implementation doesn't + // carry multiple sigs for wrapper (the old one does, I checked it) + display_line!(namada.io(), "Submitting tx: {:#?}", batched_tx); + namada.submit(batched_tx, args).await } From 04598b438d62e41f519b7fa931b4b83d1a189179 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 7 Oct 2024 13:23:44 +0200 Subject: [PATCH 3/5] New re-sign strategy to de-bloat wrapper signatures --- crates/apps_lib/src/client/tx.rs | 103 ++++++++++++-------------- crates/sdk/src/lib.rs | 29 ++++++++ crates/sdk/src/signing.rs | 120 +++++++++++++++++++++++-------- crates/sdk/src/tx.rs | 6 +- 4 files changed, 171 insertions(+), 87 deletions(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 2269f79a4e..ee3896c213 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -16,9 +16,7 @@ use namada_sdk::key::*; use namada_sdk::rpc::{InnerTxResult, TxBroadcastData, TxResponse}; use namada_sdk::state::EPOCH_SWITCH_BLOCKS_DELAY; use namada_sdk::tx::data::compute_inner_tx_hash; -use namada_sdk::tx::{ - Authorization, CompressedAuthorization, Section, Signer, Tx, -}; +use namada_sdk::tx::{CompressedAuthorization, Section, Signer, Tx}; use namada_sdk::wallet::alias::{validator_address, validator_consensus_key}; use namada_sdk::wallet::{Wallet, WalletIo}; use namada_sdk::{error, signing, tx, Namada}; @@ -194,6 +192,42 @@ pub async fn sign( Ok(()) } +pub async fn rework_batch_wrapper_signatures( + context: &N, + tx: &mut Tx, + args: &args::Tx, + signing_data: SigningTxData, +) -> Result<(), error::Error> { + // Setup a reusable context for signing transactions using the Ledger + if args.use_device { + let transport = WalletTransport::from_arg(args.device_transport); + let app = NamadaApp::new(transport); + let with_hw_data = (context.wallet_lock(), &app); + // Finally, begin the re-signing with the Ledger as backup + context + .rework_batch_wrapper_signatures( + tx, + args, + signing_data, + with_hardware_wallet::, + with_hw_data, + ) + .await?; + } else { + // Otherwise re-sign without a backup procedure + context + .rework_batch_wrapper_signatures( + tx, + args, + signing_data, + default_sign, + (), + ) + .await?; + } + Ok(()) +} + // Build a transaction to reveal the signer of the given transaction. pub async fn submit_reveal_aux( context: &impl Namada, @@ -236,6 +270,7 @@ where ::Error: std::fmt::Display, { let mut batched_tx_data = vec![]; + let wrapper_signing_data = tx_data.1.clone(); for owner in owners { if let Some(reveal_pk_tx_data) = @@ -249,64 +284,16 @@ where let (mut batched_tx, batched_signing_data) = namada_sdk::tx::build_batch(batched_tx_data)?; for sig_data in batched_signing_data { - // FIXME: also leave a not on build_batch that caller should always try - // to do this thing sign(namada, &mut batched_tx, args, sig_data).await?; } - // FIXME: maybe export this whole thing to a function that we epoxse in the - // sdk and call that function every time after build_batch? - // FIXME: write a test for this thing? - // Find the wrapper signature with the least targets and keep it, delete all - // the others (redundant) - if let Some(expected_wrapper_signer) = - batched_tx.header.wrapper().map(|wrapper| wrapper.pk) - { - let mut keep_wrapper_sig: Option<&Authorization> = None; - let mut remove_sigs = vec![]; - - for section in batched_tx.sections.iter() { - if let Section::Authorization(auth) = section { - let targets = auth.targets.len(); - // Examine only wrapper signatures - if targets > 1 { - // If signer is not the expected one drop this signature - match auth.signer { - Signer::PubKeys(ref sigs) - if sigs.as_slice() - == [expected_wrapper_signer.clone()] => {} - Signer::Address(ref addr) - if addr - == &Address::from(&expected_wrapper_signer) => { - } - _ => { - remove_sigs.push(auth.to_owned()); - continue; - } - } - - if let Some(prev) = keep_wrapper_sig { - if targets < prev.targets.len() { - remove_sigs.push(prev.to_owned()); - keep_wrapper_sig = Some(auth); - } - } else { - keep_wrapper_sig = Some(auth); - } - } - } - } - - // Remove redundant wrapper signatures - batched_tx.sections.retain(|section| match section { - Section::Authorization(auth) => !remove_sigs.contains(auth), - _ => true, - }); - } - - // FIXME: remove after having checked that the new implementation doesn't - // carry multiple sigs for wrapper (the old one does, I checked it) - display_line!(namada.io(), "Submitting tx: {:#?}", batched_tx); + rework_batch_wrapper_signatures( + namada, + &mut batched_tx, + args, + wrapper_signing_data, + ) + .await?; namada.submit(batched_tx, args).await } diff --git a/crates/sdk/src/lib.rs b/crates/sdk/src/lib.rs index f3f0e2284b..21fec1941c 100644 --- a/crates/sdk/src/lib.rs +++ b/crates/sdk/src/lib.rs @@ -629,6 +629,35 @@ pub trait Namada: NamadaIo { .await } + /// Reworks the wrapper signatures of the given transaction using the given + /// signing data + async fn rework_batch_wrapper_signatures( + &self, + tx: &mut Tx, + args: &args::Tx, + signing_data: SigningTxData, + with: impl Fn(Tx, common::PublicKey, HashSet, D) -> F + + MaybeSend + + MaybeSync, + user_data: D, + ) -> crate::error::Result<()> + where + D: Clone + MaybeSend + MaybeSync, + F: MaybeSend + + MaybeSync + + std::future::Future>, + { + signing::rework_batch_wrapper_signatures( + self.wallet_lock(), + args, + tx, + signing_data, + with, + user_data, + ) + .await + } + /// Process the given transaction using the given flags async fn submit( &self, diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 98f42d0dd7..a1c1440e21 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -13,6 +13,7 @@ use masp_primitives::asset_type::AssetType; use masp_primitives::transaction::components::sapling::fees::{ InputView, OutputView, }; +use namada_account::common::CommonPublicKey; use namada_account::{AccountPublicKeysMap, InitAccount, UpdateAccount}; use namada_core::address::{Address, ImplicitAddress, InternalAddress, MASP}; use namada_core::arith::checked; @@ -215,14 +216,6 @@ pub async fn default_sign( /// Sign a transaction with a given signing key or public key of a given signer. /// If no explicit signer given, use the `default`. If no `default` is given, /// Error. -/// -/// It also takes a second, optional keypair to sign the wrapper header -/// separately. -/// -/// If this is not a dry run, the tx is put in a wrapper and returned along with -/// hashes needed for monitoring the tx on chain. -/// -/// If it is a dry run, it is not put in a wrapper, but returned as is. pub async fn sign_tx<'a, D, F, U>( wallet: &RwLock>, args: &args::Tx, @@ -301,53 +294,124 @@ where // Then try signing the wrapper header (fee payer) with the software wallet // otherwise use the fallback + sign_wrapper( + wallet, + args, + tx, + signing_data.fee_payer, + &signing_data.public_keys, + sign, + user_data, + Some(&mut used_pubkeys), + ) + .await?; + // Then make sure that the number of public keys used exceeds the threshold + let used_pubkeys_len = used_pubkeys + .len() + .try_into() + .expect("Public keys associated with account exceed 127"); + if used_pubkeys_len < signing_data.threshold { + Err(Error::from(TxSubmitError::MissingSigningKeys( + signing_data.threshold, + used_pubkeys_len, + ))) + } else { + Ok(()) + } +} + +#[allow(clippy::too_many_arguments)] +async fn sign_wrapper<'a, D, F, U>( + wallet: &RwLock>, + args: &args::Tx, + tx: &mut Tx, + fee_payer: common::PublicKey, + public_keys: &[common::PublicKey], + sign: impl Fn(Tx, common::PublicKey, HashSet, D) -> F, + user_data: D, + used_pubkeys: Option<&mut HashSet>, +) -> Result<(), Error> +where + D: Clone + MaybeSend, + U: WalletIo, + F: std::future::Future>, +{ + // Try signing the wrapper header (fee payer) with the software wallet + // otherwise use the fallback let key = { // Lock the wallet just long enough to extract a key from it without // interfering with the sign closure call let mut wallet = wallet.write().await; - find_key_by_pk(&mut *wallet, args, &signing_data.fee_payer) + find_key_by_pk(&mut *wallet, args, &fee_payer) }; match key { Ok(fee_payer_keypair) => { tx.sign_wrapper(fee_payer_keypair); } // The case where the fee payer also signs the inner transaction - Err(_) - if signing_data.public_keys.contains(&signing_data.fee_payer) => - { + Err(_) if public_keys.contains(&fee_payer) => { *tx = sign( tx.clone(), - signing_data.fee_payer.clone(), + fee_payer.clone(), HashSet::from([Signable::FeeHeader, Signable::RawHeader]), user_data, ) .await?; - used_pubkeys.insert(signing_data.fee_payer.clone()); + if let Some(used_pubkeys) = used_pubkeys { + used_pubkeys.insert(fee_payer.clone()); + } } // The case where the fee payer does not sign the inner transaction Err(_) => { *tx = sign( tx.clone(), - signing_data.fee_payer.clone(), + fee_payer.clone(), HashSet::from([Signable::FeeHeader]), user_data, ) .await?; } } - // Then make sure that the number of public keys used exceeds the threshold - let used_pubkeys_len = used_pubkeys - .len() - .try_into() - .expect("Public keys associated with account exceed 127"); - if used_pubkeys_len < signing_data.threshold { - Err(Error::from(TxSubmitError::MissingSigningKeys( - signing_data.threshold, - used_pubkeys_len, - ))) - } else { - Ok(()) - } + + Ok(()) +} + +// TODO(namada#3883): this function should become useless after the rework of +// the batching API +/// Reworks the wrapper transaction by removing all the wrapper's signatures and +/// generataing a new one. This is to avoid submitting a batch with redundant +/// signatures. +pub async fn rework_batch_wrapper_signatures( + wallet: &RwLock>, + args: &args::Tx, + tx: &mut Tx, + signing_data: SigningTxData, + sign: impl Fn(Tx, common::PublicKey, HashSet, D) -> F, + user_data: D, +) -> Result<(), Error> +where + D: Clone + MaybeSend, + F: std::future::Future>, + U: WalletIo, +{ + // Prune all the current wrapper's signatures + tx.sections.retain(|section| match section { + Section::Authorization(auth) => auth.targets.len() == 1, + _ => true, + }); + + // Sign the wrapper + crate::signing::sign_wrapper( + wallet, + args, + tx, + signing_data.fee_payer, + &signing_data.public_keys, + sign, + user_data, + None, + ) + .await } /// Return the necessary data regarding an account to be able to generate a diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index bd44791c32..31928977e7 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -2883,7 +2883,11 @@ async fn used_asset_types( } /// Constructs the batched tx from the provided list. Returns also the data for -/// signing +/// signing. +/// +/// Before submitting the batch consider calling +/// `crate::signing::rework_batch_wrapper_signatures` to remove any possible +/// redundant signature that might bloat your transaction. pub fn build_batch( mut txs: Vec<(Tx, SigningTxData)>, ) -> Result<(Tx, Vec)> { From 21589f24586316e97e8ec89ef4920c9064ee83ad Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 7 Oct 2024 13:29:47 +0200 Subject: [PATCH 4/5] Removes FIXME comment --- crates/apps_lib/src/client/tx.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index ee3896c213..33fcceca6a 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -844,8 +844,6 @@ pub async fn submit_transparent_transfer( let transfer_data = args.clone().build(namada).await?; - // FIXME: issue here, if they dump we don't dump the reveal pk tx, so they - // sign an incomplete tx if args.tx.dump_tx { tx::dump_tx(namada.io(), &args.tx, transfer_data.0); } else { From 5c085107ee9e875f64fd0e2fceef42e87f939e54 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 7 Oct 2024 13:30:16 +0200 Subject: [PATCH 5/5] Changelog #3896 --- .../unreleased/improvements/3896-avoid-redundant-sigs.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/improvements/3896-avoid-redundant-sigs.md diff --git a/.changelog/unreleased/improvements/3896-avoid-redundant-sigs.md b/.changelog/unreleased/improvements/3896-avoid-redundant-sigs.md new file mode 100644 index 0000000000..e797e95dcc --- /dev/null +++ b/.changelog/unreleased/improvements/3896-avoid-redundant-sigs.md @@ -0,0 +1,3 @@ +- The reveal pk batching process now attaches only one wrapper + signature. Added an SDK function to de-bloat wrapper signatures. + ([\#3896](https://github.com/anoma/namada/pull/3896)) \ No newline at end of file