Avoid redundant wrapper signatures

.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))

crates/apps_lib/src/client/tx.rs

@@ -192,6 +192,42 @@ pub async fn sign<N: Namada>(
     Ok(())
 }
 
+pub async fn rework_batch_wrapper_signatures<N: Namada>(
+    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::<N::WalletUtils, _>,
+                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,
@@ -234,6 +270,7 @@ where
     <N::Client as namada_sdk::io::Client>::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) =
@@ -250,6 +287,14 @@ where
         sign(namada, &mut batched_tx, args, sig_data).await?;
     }
 
+    rework_batch_wrapper_signatures(
+        namada,
+        &mut batched_tx,
+        args,
+        wrapper_signing_data,
+    )
+    .await?;
+
     namada.submit(batched_tx, args).await
 }
 

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<D, F>(
+        &self,
+        tx: &mut Tx,
+        args: &args::Tx,
+        signing_data: SigningTxData,
+        with: impl Fn(Tx, common::PublicKey, HashSet<signing::Signable>, D) -> F
+        + MaybeSend
+        + MaybeSync,
+        user_data: D,
+    ) -> crate::error::Result<()>
+    where
+        D: Clone + MaybeSend + MaybeSync,
+        F: MaybeSend
+            + MaybeSync
+            + std::future::Future<Output = crate::error::Result<Tx>>,
+    {
+        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,

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<Wallet<U>>,
     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<Wallet<U>>,
+    args: &args::Tx,
+    tx: &mut Tx,
+    fee_payer: common::PublicKey,
+    public_keys: &[common::PublicKey],
+    sign: impl Fn(Tx, common::PublicKey, HashSet<Signable>, D) -> F,
+    user_data: D,
+    used_pubkeys: Option<&mut HashSet<CommonPublicKey>>,
+) -> Result<(), Error>
+where
+    D: Clone + MaybeSend,
+    U: WalletIo,
+    F: std::future::Future<Output = Result<Tx, Error>>,
+{
+    // 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<D, F, U>(
+    wallet: &RwLock<Wallet<U>>,
+    args: &args::Tx,
+    tx: &mut Tx,
+    signing_data: SigningTxData,
+    sign: impl Fn(Tx, common::PublicKey, HashSet<Signable>, D) -> F,
+    user_data: D,
+) -> Result<(), Error>
+where
+    D: Clone + MaybeSend,
+    F: std::future::Future<Output = std::result::Result<Tx, Error>>,
+    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

crates/sdk/src/tx.rs

@@ -2883,7 +2883,11 @@ async fn used_asset_types<P, K, N>(
 }
 
 /// 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<SigningTxData>)> {