From d4c6e33d2618ea8f4444b820c709040709a21ad7 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 30 Dec 2024 16:50:40 +0100 Subject: [PATCH 1/5] Update `PartialEq` for `SigningTxData` --- crates/sdk/src/signing.rs | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index c3cb2427dc..a7d99c3330 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -83,22 +83,42 @@ 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() { + if public_keys.len() != other_public_keys.len() { return false; } - self.public_keys + public_keys .iter() - .all(|pubkey| other.public_keys.contains(pubkey)) + .all(|pubkey| other_public_keys.contains(pubkey)) } } From aecd5affa1e9c229c94fd4e933a75392a02cb7fc Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Thu, 9 Jan 2025 13:12:31 +0100 Subject: [PATCH 2/5] Custom implementaion of `PartialEq` for `Authorization` --- crates/tx/src/section.rs | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/crates/tx/src/section.rs b/crates/tx/src/section.rs index fb416d24c7..b88208cabe 100644 --- a/crates/tx/src/section.rs +++ b/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,39 @@ pub struct Authorization { pub signatures: BTreeMap, } +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 + if targets.len() != other_targets.len() { + return false; + } + + if !targets.iter().all(|pubkey| other_targets.contains(pubkey)) { + 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( From 650f0b81e81927a8224d07ad7c4f0a19e5e23bb8 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 10 Jan 2025 20:10:43 +0100 Subject: [PATCH 3/5] Removes duplicated sections from transactions before signing or dumping --- crates/sdk/src/signing.rs | 4 ++++ crates/sdk/src/tx.rs | 4 ++++ crates/tx/src/types.rs | 11 +++++++++++ 3 files changed, 19 insertions(+) diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index a7d99c3330..6a19dd40eb 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -333,6 +333,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 diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 19a6a5ba17..d73dcc24f9 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -201,6 +201,10 @@ pub fn dump_tx(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!( diff --git a/crates/tx/src/types.rs b/crates/tx/src/types.rs index 0d83576fec..00d01d4e80 100644 --- a/crates/tx/src/types.rs +++ b/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() From 7430933335fdf39b985c9d2a5c476fe6ac94913a Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 10 Jan 2025 20:36:52 +0100 Subject: [PATCH 4/5] Changelog #4230 --- .../unreleased/improvements/4230-remove-duplicated-sigs.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/4230-remove-duplicated-sigs.md diff --git a/.changelog/unreleased/improvements/4230-remove-duplicated-sigs.md b/.changelog/unreleased/improvements/4230-remove-duplicated-sigs.md new file mode 100644 index 0000000000..5cee60f64d --- /dev/null +++ b/.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)) \ No newline at end of file From fa5bdbf4a183ea4783e31c434ab5a0634a676d47 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 14 Jan 2025 17:04:00 +0100 Subject: [PATCH 5/5] Fixes bugs in `PartialEq` implementations for `SigningTxData` and `Authorization` --- crates/sdk/src/signing.rs | 19 +++++++++++-------- crates/tx/src/section.rs | 18 ++++++++++-------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 6a19dd40eb..5fb359865f 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -83,8 +83,7 @@ pub struct SigningTxData { impl PartialEq for SigningTxData { fn eq(&self, other: &Self) -> bool { - // Deconstruct the two instances to ensure we don't forget any new - // field + // Deconstruct the two instances to ensure we don't forget any new field let SigningTxData { owner, public_keys, @@ -112,13 +111,17 @@ impl PartialEq for SigningTxData { } // Check equivalence of the public keys ignoring the specific ordering - if 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(), + ); - public_keys - .iter() - .all(|pubkey| other_public_keys.contains(pubkey)) + unique_public_keys == unique_other_public_keys } } diff --git a/crates/tx/src/section.rs b/crates/tx/src/section.rs index b88208cabe..d57a29a462 100644 --- a/crates/tx/src/section.rs +++ b/crates/tx/src/section.rs @@ -443,8 +443,7 @@ pub struct Authorization { impl PartialEq for Authorization { fn eq(&self, other: &Self) -> bool { - // Deconstruct the two instances to ensure we don't forget any new - // field + // Deconstruct the two instances to ensure we don't forget any new field let Authorization { targets, signer: _, @@ -457,15 +456,18 @@ impl PartialEq for Authorization { } = other; // Two authorizations are equal when they are computed over the same - // target(s) and the signatures match ,regardless of how the signer is + // target(s) and the signatures match, regardless of how the signer is // expressed - // Check equivalence of the targets ignoring the specific ordering - if targets.len() != other_targets.len() { - return false; - } + // 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 !targets.iter().all(|pubkey| other_targets.contains(pubkey)) { + if unique_targets != unique_other_targets { return false; }