Avoid duplicated signatures when building transactions

.changelog/unreleased/improvements/4230-remove-duplicated-sigs.md

@@ -0,0 +1,2 @@
+- Improved the transaction building process to avoid duplicated sections.
+  ([\#4230](https://github.com/anoma/namada/pull/4230))

crates/sdk/src/signing.rs

@@ -83,22 +83,45 @@ pub struct SigningTxData {
 
 impl PartialEq for SigningTxData {
     fn eq(&self, other: &Self) -> bool {
-        if !(self.owner == other.owner
-            && self.threshold == other.threshold
-            && self.account_public_keys_map == other.account_public_keys_map
-            && self.fee_payer == other.fee_payer)
+        // Deconstruct the two instances to ensure we don't forget any new field
+        let SigningTxData {
+            owner,
+            public_keys,
+            threshold,
+            account_public_keys_map,
+            fee_payer,
+            shielded_hash,
+        } = self;
+        let SigningTxData {
+            owner: other_owner,
+            public_keys: other_public_keys,
+            threshold: other_threshold,
+            account_public_keys_map: other_account_public_keys_map,
+            fee_payer: other_fee_payer,
+            shielded_hash: other_shielded_hash,
+        } = other;
+
+        if !(owner == other_owner
+            && threshold == other_threshold
+            && account_public_keys_map == other_account_public_keys_map
+            && fee_payer == other_fee_payer
+            && shielded_hash == other_shielded_hash)
         {
             return false;
         }
 
         // Check equivalence of the public keys ignoring the specific ordering
-        if self.public_keys.len() != other.public_keys.len() {
-            return false;
-        }
+        // and duplicates (the PartialEq implementation of IndexSet ignores the
+        // order)
+        let unique_public_keys = HashSet::<
+            &namada_account::common::CommonPublicKey,
+        >::from_iter(public_keys.iter());
+        let unique_other_public_keys =
+            HashSet::<&namada_account::common::CommonPublicKey>::from_iter(
+                other_public_keys.iter(),
+            );
 
-        self.public_keys
-            .iter()
-            .all(|pubkey| other.public_keys.contains(pubkey))
+        unique_public_keys == unique_other_public_keys
     }
 }
 
@@ -313,6 +336,10 @@ where
         }
     }
 
+    // Before signing the wrapper tx prune all the possible duplicated sections
+    // (including duplicated raw signatures)
+    tx.prune_duplicated_sections();
+
     // Then try signing the wrapper header (fee payer). Check if there's a
     // provided wrapper signature, otherwise sign with the software wallet or
     // use the fallback

crates/sdk/src/tx.rs

@@ -201,6 +201,10 @@ pub fn dump_tx<IO: Io>(io: &IO, args: &args::Tx, mut tx: Tx) -> Result<()> {
         ));
     }
 
+    // Remove duplicated sections before dumping. This is useful in case the
+    // dumped tx needed to be signed offline
+    tx.prune_duplicated_sections();
+
     match args.output_folder.clone() {
         Some(path) => {
             let tx_path = path.join(format!(

crates/tx/src/section.rs

@@ -431,7 +431,6 @@ pub enum Signer {
     BorshSchema,
     Serialize,
     Deserialize,
-    PartialEq,
 )]
 pub struct Authorization {
     /// The hash of the section being signed
@@ -442,6 +441,41 @@ pub struct Authorization {
     pub signatures: BTreeMap<u8, common::Signature>,
 }
 
+impl PartialEq for Authorization {
+    fn eq(&self, other: &Self) -> bool {
+        // Deconstruct the two instances to ensure we don't forget any new field
+        let Authorization {
+            targets,
+            signer: _,
+            signatures,
+        } = self;
+        let Authorization {
+            targets: other_targets,
+            signer: _,
+            signatures: other_signatures,
+        } = other;
+
+        // Two authorizations are equal when they are computed over the same
+        // target(s) and the signatures match, regardless of how the signer is
+        // expressed
+
+        // Check equivalence of the targets ignoring the specific ordering and
+        // duplicates (the PartialEq implementation of IndexSet ignores the
+        // order)
+        let unique_targets =
+            HashSet::<&namada_account::Hash>::from_iter(targets.iter());
+        let unique_other_targets =
+            HashSet::<&namada_account::Hash>::from_iter(other_targets.iter());
+
+        if unique_targets != unique_other_targets {
+            return false;
+        }
+
+        // Check equivalence of the signatures
+        signatures == other_signatures
+    }
+}
+
 impl Authorization {
     /// Sign the given section hash with the given key and return a section
     pub fn new(

crates/tx/src/types.rs

@@ -188,6 +188,17 @@ impl Tx {
         self.header.batch.insert(cmt)
     }
 
+    /// Remove duplicated sections from the transaction
+    pub fn prune_duplicated_sections(&mut self) {
+        let sections = std::mem::take(&mut self.sections);
+        let mut unique_sections = HashMap::with_capacity(sections.len());
+        for section in sections {
+            unique_sections.insert(section.get_hash(), section);
+        }
+
+        self.sections = unique_sections.into_values().collect();
+    }
+
     /// Get the transaction header
     pub fn header(&self) -> Header {
         self.header.clone()