fix: prevent duplicate signatures in _signature_map#2219
Conversation
Up to standards ✅🟢 Issues
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTransaction.sign now checks existing signature pairs' ChangesSignature Deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30a7e170-2598-402d-8fc2-75740caa0be5
📒 Files selected for processing (2)
src/hiero_sdk_python/transaction/transaction.pytests/unit/test_signature_deduplication.py
de0f4ce to
287e6a5
Compare
|
cc @exploreriii |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction.py (1)
182-200: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHoist the dedup check before
private_key.sign()to avoid unnecessary crypto work
public_key_bytesis loop-invariant andprivate_key.sign()+sig_pairobject construction both happen unconditionally before the dedup guard fires. Signing the same key twice per node causes a full crypto operation + proto allocation that is immediately discarded.♻️ Proposed refactor
- # We sign the bodies for each node in case we need to switch nodes during execution. - for body_bytes in self._transaction_body_bytes.values(): - signature = private_key.sign(body_bytes) - - public_key_bytes = private_key.public_key().to_bytes_raw() - - if private_key.is_ed25519(): - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) - else: - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) - - # We initialize the signature map for this body_bytes if it doesn't exist yet - self._signature_map.setdefault(body_bytes, basic_types_pb2.SignatureMap()) - - # deduplication check - already_signed = any(sp.pubKeyPrefix == public_key_bytes for sp in self._signature_map[body_bytes].sigPair) - - # append only if not already signed - if not already_signed: - self._signature_map[body_bytes].sigPair.append(sig_pair) + public_key_bytes = private_key.public_key().to_bytes_raw() + + # We sign the bodies for each node in case we need to switch nodes during execution. + for body_bytes in self._transaction_body_bytes.values(): + self._signature_map.setdefault(body_bytes, basic_types_pb2.SignatureMap()) + + # Skip if this key has already signed this body + if any(sp.pubKeyPrefix == public_key_bytes for sp in self._signature_map[body_bytes].sigPair): + continue + + signature = private_key.sign(body_bytes) + + if private_key.is_ed25519(): + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) + else: + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) + + self._signature_map[body_bytes].sigPair.append(sig_pair)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3856665f-4072-4ef9-9a3d-00a5fb1b4bdf
📒 Files selected for processing (2)
src/hiero_sdk_python/transaction/transaction.pytests/unit/test_signature_deduplication.py
473de8a to
f8b8363
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction.py (1)
182-200: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSkip cryptographic signing when the key is already present.
private_key.sign(body_bytes)is computed before deduplication; moving it after thealready_signedcheck avoids unnecessary signing work on repeated calls.♻️ Proposed refactor
- for body_bytes in self._transaction_body_bytes.values(): - signature = private_key.sign(body_bytes) - - public_key_bytes = private_key.public_key().to_bytes_raw() - - if private_key.is_ed25519(): - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) - else: - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) + public_key_bytes = private_key.public_key().to_bytes_raw() + is_ed25519 = private_key.is_ed25519() + for body_bytes in self._transaction_body_bytes.values(): # We initialize the signature map for this body_bytes if it doesn't exist yet self._signature_map.setdefault(body_bytes, basic_types_pb2.SignatureMap()) # deduplication check already_signed = any(sp.pubKeyPrefix == public_key_bytes for sp in self._signature_map[body_bytes].sigPair) # append only if not already signed if not already_signed: + signature = private_key.sign(body_bytes) + if is_ed25519: + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) + else: + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) self._signature_map[body_bytes].sigPair.append(sig_pair)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4a50307-2bbc-4c25-a259-c6dac726b38a
📒 Files selected for processing (2)
src/hiero_sdk_python/transaction/transaction.pytests/unit/test_signature_deduplication.py
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
afe9691 to
dbd9743
Compare
|
Hi @Akshat8510 @nadineloepfe , I’ve addressed all the previous issues. Fixed DCO check (commit is now signed) The implementation now ensures no duplicate SignaturePair entries are added while preserving correct multi-key signing behavior. Requesting review. |
|
Hi, this is WorkflowBot.
|
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction.py (1)
183-200: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMove the deduplication check before the cryptographic sign operation.
private_key.sign(body_bytes)(an expensive crypto operation) is executed unconditionally before the dedup guard. When the same key signs twice — the exact bug this PR fixes — a full signature is computed and immediately discarded. Moving the check beforesign()and hoistingpublic_key_bytesout of the loop eliminates the wasted work.⚡ Proposed fix
- for body_bytes in self._transaction_body_bytes.values(): - signature = private_key.sign(body_bytes) - - public_key_bytes = private_key.public_key().to_bytes_raw() - - if private_key.is_ed25519(): - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) - else: - sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) - - # We initialize the signature map for this body_bytes if it doesn't exist yet - self._signature_map.setdefault(body_bytes, basic_types_pb2.SignatureMap()) - - # deduplication check - already_signed = any(sp.pubKeyPrefix == public_key_bytes for sp in self._signature_map[body_bytes].sigPair) - - # append only if not already signed - if not already_signed: - self._signature_map[body_bytes].sigPair.append(sig_pair) + public_key_bytes = private_key.public_key().to_bytes_raw() + + for body_bytes in self._transaction_body_bytes.values(): + # Initialize signature map and skip signing if key is already present + self._signature_map.setdefault(body_bytes, basic_types_pb2.SignatureMap()) + + if any(sp.pubKeyPrefix == public_key_bytes for sp in self._signature_map[body_bytes].sigPair): + continue + + signature = private_key.sign(body_bytes) + + if private_key.is_ed25519(): + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ed25519=signature) + else: + sig_pair = basic_types_pb2.SignaturePair(pubKeyPrefix=public_key_bytes, ECDSA_secp256k1=signature) + + self._signature_map[body_bytes].sigPair.append(sig_pair)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b2ae8e8-56a2-4f5f-8779-728523139ae2
📒 Files selected for processing (3)
src/hiero_sdk_python/transaction/transaction.pytests/unit/signature_deduplication_test.pytests/unit/test_signature_deduplication.py
78919e2 to
496afbc
Compare
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
496afbc to
03023fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 007af472-df85-4649-9d4e-3a764b62607f
📒 Files selected for processing (1)
tests/unit/signature_deduplication_test.py
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
a502268 to
e368d7c
Compare
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
|
@MonaaEid ready for reveiw |
exploreriii
added
step: 1st
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #2219 +/- ##
=======================================
Coverage 93.73% 93.73%
=======================================
Files 145 145
Lines 9480 9482 +2
=======================================
+ Hits 8886 8888 +2
Misses 594 594 🚀 New features to boost your workflow:
|
manishdait
added
step: 2nd
|
@manishdait i think the failing ClusterFuzzLite job seems to be due to an external package fetch/network issue, not related to this chang...... Re-running CI should resolve it |
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
4 participants
Description:
Prevent duplicate
SignaturePairentries from being added to_signature_mapwhen signing a transaction multiple times with the same key.pubKeyPrefixbefore appending signatures(body_bytes, pubKeyPrefix)combinationRelated issue(s):
Fixes #2208
Notes for reviewer:
Checklist