Avoid duplicated signatures when building transactions #4230

grarco · 2025-01-10T19:13:48Z

Describe your changes

Closes #4203.

This PR modifies adds a new method on the Tx type to remove duplicated sections. This function is then called in the signing function before producing the wrapper signature (which is computed on all the sections) and the dump_tx function (in case the dumped tx needed to be signed offline).

To support this logic the PartialEq implementation for the Authorization type is now manually provided and ignores the way the signer is specified, focusing instead on the targets and the actual signature.

As a side note, also the PartialEq implementation for SigningTxData has been updated to account for the shielded hash.

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes
If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
- Relevant PR if applies:
If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
- Relevant PR if applies:

codecov · 2025-01-10T20:24:08Z

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.60%. Comparing base (9527ea9) to head (fa5bdbf).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/tx/src/section.rs	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4230      +/-   ##
==========================================
+ Coverage   74.59%   74.60%   +0.01%     
==========================================
  Files         342      342              
  Lines      108724   108792      +68     
==========================================
+ Hits        81101    81169      +68     
  Misses      27623    27623

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

murisi

While the overall approach taken here to avoid duplicate signatures makes sense, checking whether Authorization sections are duplicates after they have been produced (by checking structural equality) seems less robust than not producing duplicates in the first place. But perhaps this (situation where a signing process is redundantly being done twice) is unavoidable...

crates/sdk/src/signing.rs

murisi · 2025-01-13T17:55:33Z

crates/sdk/src/signing.rs

            .iter()
-            .all(|pubkey| other.public_keys.contains(pubkey))
+            .all(|pubkey| other_public_keys.contains(pubkey))


Doesn't this code only prove that public_keys is a subset of other_public_keys? Imagine that public_keys = vec![x, x, y, y] and other_public_keys = vec![w,x,y,z], wouldn't this logic conclude that public_keys and other_public_keys are equivalent (because both vectors have the same length and all the elements of the former are contained in the latter)?

You're right! I tried a different approach in fa5bdbf

murisi · 2025-01-13T17:59:54Z

crates/tx/src/section.rs

+        // target(s) and the signatures match ,regardless of how the signer is
+        // expressed
+
+        // Check equivalence of the targets ignoring the specific ordering


Doesn't this code only prove that targets is a subset of other_targets? Imagine that targets = vec![x, x, y, y] and other_targets = vec![w,x,y,z], wouldn't this logic conclude that targets and other_targets are equivalent (because both vectors have the same length and all the elements of the former are contained in the latter)?

Same as above, thanks!

murisi · 2025-01-13T18:17:43Z

crates/tx/src/section.rs

+            signatures: other_signatures,
+        } = other;
+
+        // Two authorizations are equal when they are computed over the same


This definition could work. But it has the implication that if Authorization::signer contains the variant Signer::PubKeys and someone tampers with one of the public keys in that list so that Authorization::verify_signature can no longer pass, this PartialEq implementation would still say that the tampered instance is still equal to the original. No? If so, is this desirable?

Also, given that this function defines a partial equivalence relation, I guess false should be returned in those cases where there is insufficient information to check/conclude equality of the signers? No?

That's correct. The representation of the signer being different in the duplicated authorizations I could see on mainnet was the drive to modify this trait implementation. My reasoning was that once we establish the equality of the signature's targets and of the signatures themselves we can safely consider the two Authorizations to be the same even if the signers are expressed differently. In case of the signers being straight different (which would cause one of the two instances to fail the signature verification check) I believe we are looking at something else than just equality: I guess one should check the integrity of the sections at the byte level using a digest or similar techniques (see also my answer to your next question since it's related to this one)

murisi · 2025-01-13T18:27:23Z

crates/tx/src/section.rs

@@ -442,6 +441,39 @@ pub struct Authorization {
    pub signatures: BTreeMap<u8, common::Signature>,
 }

+impl PartialEq for Authorization {
+    fn eq(&self, other: &Self) -> bool {


Does it matter that partially equivalent Authorizations might not have the same cryptographic hash? Or is this impossible? Could this sort of definition of partial equality confuse people in the future? (This definition seems to be calculated specifically to deal with our duplicate signatures issue (no?), but could be unintuitive in other cases/situations...)

I don't believe PartialEq should guarantee a byte-level match between the two instances but rather a logical equivalence (as I said in the previous question, when safety is critical one should explicitly rely on the digest of the expected instance).

I do see your point on this implementation being potentially confusing in other situations: I could try to reintroduce the check on the signer with a more refined logic. My only concern is the case when a multisig signer is expressed as a set of keys in one instance and as an address in the other one: in this case there's no way I can resolve the equivalence without a query to storage (but there's a chance we always express multisig signers as a set of keys which would solve this issue).

An alternative could be to come up with a different function for this comparison and restore the previous implementation of PartialEq (net of ignoring the order of the set of keys which I think is a required update regardless).

…thorization`

grarco · 2025-01-14T18:36:03Z

While the overall approach taken here to avoid duplicate signatures makes sense, checking whether Authorization sections are duplicates after they have been produced (by checking structural equality) seems less robust than not producing duplicates in the first place. But perhaps this (situation where a signing process is redundantly being done twice) is unavoidable...

We already try to filter duplicated instanced of SigningTxData but there are cases where this logic fails (such as in case of different wrapper signers, even though we should only produce a single wrapper signature, see #3901, or the case of an offline procedure where we could get duplicated signatures from the members of a multisig account): hence the need to integrate this logic with a pruning mechanism for duplicated sections. I do agree with you though and we should try to improve the signing procedure over time to avoid producing duplicated signatures as much as possible

grarco added 3 commits January 10, 2025 20:12

Update PartialEq for SigningTxData

d4c6e33

Custom implementaion of PartialEq for Authorization

aecd5af

Removes duplicated sections from transactions before signing or dumping

650f0b8

grarco added SDK breaking:SDK labels Jan 10, 2025

Changelog #4230

7430933

grarco marked this pull request as ready for review January 10, 2025 19:46

grarco requested review from tzemanovic and murisi January 10, 2025 19:47

tzemanovic approved these changes Jan 13, 2025

View reviewed changes

murisi requested changes Jan 13, 2025

View reviewed changes

Fixes bugs in PartialEq implementations for SigningTxData and `Au…

fa5bdbf

…thorization`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid duplicated signatures when building transactions #4230

Avoid duplicated signatures when building transactions #4230

grarco commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

murisi left a comment

murisi Jan 13, 2025

grarco Jan 14, 2025

murisi Jan 13, 2025

grarco Jan 14, 2025

murisi Jan 13, 2025 •

edited

Loading

grarco Jan 14, 2025 •

edited

Loading

murisi Jan 13, 2025

grarco Jan 14, 2025

grarco commented Jan 14, 2025

Avoid duplicated signatures when building transactions #4230

Are you sure you want to change the base?

Avoid duplicated signatures when building transactions #4230

Conversation

grarco commented Jan 10, 2025 • edited Loading

Describe your changes

Checklist before merging

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

murisi left a comment

Choose a reason for hiding this comment

murisi Jan 13, 2025

Choose a reason for hiding this comment

grarco Jan 14, 2025

Choose a reason for hiding this comment

murisi Jan 13, 2025

Choose a reason for hiding this comment

grarco Jan 14, 2025

Choose a reason for hiding this comment

murisi Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

grarco Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

murisi Jan 13, 2025

Choose a reason for hiding this comment

grarco Jan 14, 2025

Choose a reason for hiding this comment

grarco commented Jan 14, 2025

grarco commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

murisi Jan 13, 2025 •

edited

Loading

grarco Jan 14, 2025 •

edited

Loading