Avoid duplicated signatures when building transactions #4230
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4230 +/- ##
==========================================
+ Coverage 74.59% 74.61% +0.02%
==========================================
Files 342 342
Lines 108724 108787 +63
==========================================
+ Hits 81101 81175 +74
+ Misses 27623 27612 -11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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...
| && 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 |
There was a problem hiding this comment.
Nice, this is probably a more robust approach to equality checking here. (I had made the mistake of forgetting to update this function when introducing the shielded_hash field to SigningTxData, oops.)
| .iter() | ||
| .all(|pubkey| other.public_keys.contains(pubkey)) | ||
| .all(|pubkey| other_public_keys.contains(pubkey)) |
There was a problem hiding this comment.
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)?
| // target(s) and the signatures match ,regardless of how the signer is | ||
| // expressed | ||
|
|
||
| // Check equivalence of the targets ignoring the specific ordering |
There was a problem hiding this comment.
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)?
| signatures: other_signatures, | ||
| } = other; | ||
|
|
||
| // Two authorizations are equal when they are computed over the same |
There was a problem hiding this comment.
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?
| @@ -442,6 +441,39 @@ pub struct Authorization { | |||
| pub signatures: BTreeMap<u8, common::Signature>, | |||
| } | |||
|
|
|||
| impl PartialEq for Authorization { | |||
| fn eq(&self, other: &Self) -> bool { | |||
There was a problem hiding this comment.
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...)
Describe your changes
Closes #4203.
This PR modifies adds a new method on the
Txtype 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 thedump_txfunction (in case the dumped tx needed to be signed offline).To support this logic the
PartialEqimplementation for theAuthorizationtype 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
PartialEqimplementation forSigningTxDatahas been updated to account for the shielded hash.Checklist before merging
breaking::labelsnamada-docsreponamada-indexerornamada-masp-indexer, a corresponding PR is opened in that repo