New p2pk and htlc rules, sig all support #24
Conversation
📝 WalkthroughWalkthroughRenames P2PkBuilder → P2PKBuilder, adds refund-signature-threshold support, changes HTLC refund/pubkey exposure and witness nullability, extends P2PK proof secret with refund/blind signing and verification paths (nullable witnesses), and adds SigAllHandler for SIG_ALL multi-proof signing/verification. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant SigAllHandler
participant P2PKProofSecret
participant HTLCProofSecret
rect rgb(235,245,255)
Note over Client,SigAllHandler: SIG_ALL signing orchestration
Client->>SigAllHandler: TrySign(proofs, privKeys, blindedMessages, HTLCPreimage?, meltQuoteId?)
SigAllHandler->>SigAllHandler: ValidateFirstProof() / GetMessageToSign()
alt first proof is P2PK
SigAllHandler->>P2PKProofSecret: GenerateWitness(message, privKeys)
P2PKProofSecret->>P2PKProofSecret: TrySignPath(normal allowed keys)
alt normal path succeeds
P2PKProofSecret-->>SigAllHandler: P2PKWitness
else normal fails
P2PKProofSecret->>P2PKProofSecret: GetAllowedRefundPubkeys()
P2PKProofSecret->>P2PKProofSecret: TrySignPath(refund keys)
alt refund succeeds
P2PKProofSecret-->>SigAllHandler: P2PKWitness
else
P2PKProofSecret-->>SigAllHandler: null
end
end
else first proof is HTLC
SigAllHandler->>HTLCProofSecret: GenerateWitness(message, privKeys, preimage?)
HTLCProofSecret->>HTLCProofSecret: TrySignPath(normal allowed keys)
alt normal path succeeds
HTLCProofSecret-->>SigAllHandler: HTLCWitness
else
HTLCProofSecret->>HTLCProofSecret: GetAllowedRefundPubkeys()
HTLCProofSecret->>HTLCProofSecret: TrySignPath(refund keys)
alt refund succeeds
HTLCProofSecret-->>SigAllHandler: HTLCWitness
else
HTLCProofSecret-->>SigAllHandler: null
end
end
end
alt witness produced
SigAllHandler->>SigAllHandler: Serialize witness JSON
SigAllHandler-->>Client: success + witness
else no witness
SigAllHandler-->>Client: failure + null
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-29T15:42:55.222ZApplied to files:
🧬 Code graph analysis (1)DotNut/P2PKProofSecret.cs (3)
🔇 Additional comments (7)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
DotNut/HTLCBuilder.cs (2)
34-43: MissingRefundSignatureThresholdin Load method.The
Loadmethod copies properties frominnerbuilderbut doesn't include the newly addedRefundSignatureThreshold. This could cause HTLC secrets with refund thresholds to lose that information when loaded.🔎 Proposed fix
return new HTLCBuilder() { HashLock = hashLock, Lock = innerbuilder.Lock, Pubkeys = innerbuilder.Pubkeys, RefundPubkeys = innerbuilder.RefundPubkeys, SignatureThreshold = innerbuilder.SignatureThreshold, + RefundSignatureThreshold = innerbuilder.RefundSignatureThreshold, SigFlag = innerbuilder.SigFlag, Nonce = innerbuilder.Nonce };
53-61: MissingRefundSignatureThresholdin Build method.Similarly, the
Buildmethod creates a newP2PKBuilderbut doesn't propagateRefundSignatureThreshold. This means HTLC secrets built through this builder won't include then_sigs_refundtag even if set.🔎 Proposed fix
var innerBuilder = new P2PKBuilder() { Lock = Lock, Pubkeys = Pubkeys.ToArray(), RefundPubkeys = RefundPubkeys, SignatureThreshold = SignatureThreshold, + RefundSignatureThreshold = RefundSignatureThreshold, SigFlag = SigFlag, Nonce = Nonce };DotNut/HTLCProofSecret.cs (1)
198-210: Potential null reference whenPreimageis null.Since
HTLCWitness.Preimageis now nullable, callingVerifyPreimage(htlcWitness.Preimage)at line 204 could passnulltoVerifyPreimage(string preimage). TheVerifyPreimagemethod at line 102 callsConvert.FromHexString(preimage)which will throw ifpreimageis null.🔎 Proposed fix
public override bool VerifyWitnessHash(byte[] hash, P2PKWitness witness) { if (witness is not HTLCWitness htlcWitness) { return false; } - if (!VerifyPreimage(htlcWitness.Preimage)) + if (htlcWitness.Preimage is null || !VerifyPreimage(htlcWitness.Preimage)) { return false; } return base.VerifyWitnessHash(hash, witness); }
🧹 Nitpick comments (4)
DotNut/SigAllHandler.cs (2)
10-14: Consider initializing collection properties to avoid null reference issues.The
Proofs,PrivKeys, andBlindedMessagesproperties are not initialized and could be null. WhileTrySigndoes null-check them, initializing to empty collections would be more defensive.🔎 Proposed fix
- public List<Proof> Proofs { get; set; } - public List<PrivKey> PrivKeys { get; set; } - public List<BlindedMessage> BlindedMessages { get; set; } + public List<Proof> Proofs { get; set; } = new(); + public List<PrivKey> PrivKeys { get; set; } = new(); + public List<BlindedMessage> BlindedMessages { get; set; } = new();
151-154: Avoid catching genericException.Catching all exceptions masks potential bugs and makes debugging harder. Consider catching specific exceptions that
GetMessageToSignmight throw.🔎 Proposed fix
- catch(Exception ex) + catch (ArgumentException) { return false; }DotNut/P2PKProofSecret.cs (2)
89-109: Consider using aList<string>to avoid O(n²) array allocations.Using
Append().ToArray()inside a loop creates a new array on each iteration. For multi-sig scenarios with many keys, this could be inefficient.🔎 Proposed optimization
private (bool IsValid, P2PKWitness Witness) TrySignPath(ECPubKey[] allowedKeys, int requiredSignatures, ECPrivKey[] availableKeys, byte[] msg) { var allowedKeysSet = new HashSet<ECPubKey>(allowedKeys); - var result = new P2PKWitness(); + var signatures = new List<string>(); foreach (var privKey in availableKeys) { - if (result.Signatures.Length >= requiredSignatures) + if (signatures.Count >= requiredSignatures) break; var pubkey = privKey.CreatePubKey(); if (allowedKeysSet.Contains(pubkey)) { var sig = privKey.SignBIP340(msg); - result.Signatures = result.Signatures.Append(sig.ToHex()).ToArray(); + signatures.Add(sig.ToHex()); } } - return (result.Signatures.Length >= requiredSignatures, result); + return (signatures.Count >= requiredSignatures, new P2PKWitness { Signatures = signatures.ToArray() }); }The same pattern appears in
TrySignBlindPathat lines 204 and 214.
246-249: Add explicit non-null cast after filtering.The
Whereclause filters out nulls but doesn't change the inferred array type. While the values are guaranteed non-null at runtime, adding an explicit cast improves type safety and clarity.🔎 Proposed fix
var sigs = witness.Signatures .Select(s => SecpSchnorrSignature.TryCreate(Convert.FromHexString(s), out var sig) ? sig : null) .Where(signature => signature is not null) + .Cast<SecpSchnorrSignature>() .ToArray();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
DotNut.Demo/Program.csDotNut.Tests/UnitTest1.csDotNut.slnDotNut/HTLCBuilder.csDotNut/HTLCProofSecret.csDotNut/HTLCWitness.csDotNut/JsonConverters/Nut10SecretJsonConverter.csDotNut/P2PKBuilder.csDotNut/P2PKProofSecret.csDotNut/SigAllHandler.cs
💤 Files with no reviewable changes (1)
- DotNut/JsonConverters/Nut10SecretJsonConverter.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 23
File: DotNut/NUT11/SigAllHandler.cs:19-77
Timestamp: 2025-11-29T15:42:55.222Z
Learning: In DotNut, HTLCProofSecret inherits from P2PKProofSecret, forming a base-derived relationship where HTLC secrets are a specialized form of P2PK secrets. Type checks using `is P2PKProofSecret` will match both P2PK and HTLC secrets due to this inheritance.
📚 Learning: 2025-11-29T15:42:55.222Z
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 23
File: DotNut/NUT11/SigAllHandler.cs:19-77
Timestamp: 2025-11-29T15:42:55.222Z
Learning: In DotNut, HTLCProofSecret inherits from P2PKProofSecret, forming a base-derived relationship where HTLC secrets are a specialized form of P2PK secrets. Type checks using `is P2PKProofSecret` will match both P2PK and HTLC secrets due to this inheritance.
Applied to files:
DotNut/HTLCBuilder.csDotNut/P2PKBuilder.csDotNut/SigAllHandler.csDotNut.Tests/UnitTest1.csDotNut/HTLCProofSecret.csDotNut/P2PKProofSecret.cs
🧬 Code graph analysis (4)
DotNut.Demo/Program.cs (1)
DotNut/P2PKBuilder.cs (2)
P2PKBuilder(6-171)P2PKBuilder(61-111)
DotNut/P2PKBuilder.cs (1)
DotNut/P2PKProofSecret.cs (1)
P2PKProofSecret(8-276)
DotNut/SigAllHandler.cs (3)
DotNut/Proof.cs (1)
Proof(7-31)DotNut/BlindedMessage.cs (1)
BlindedMessage(5-11)DotNut/Nut10ProofSecret.cs (1)
Nut10ProofSecret(5-18)
DotNut/P2PKProofSecret.cs (3)
DotNut/P2PKBuilder.cs (2)
P2PKBuilder(6-171)P2PKBuilder(61-111)DotNut/Cashu.cs (19)
ECPubKey(16-20)ECPubKey(22-26)ECPubKey(27-42)ECPubKey(51-54)ECPubKey(61-64)ECPubKey(66-69)ECPubKey(71-75)ECPubKey(77-81)ECPubKey(128-132)ECPrivKey(56-59)ECPrivKey(84-95)ECPrivKey(145-156)ToBytes(180-185)ToHex(175-178)ToHex(193-196)ToHex(197-200)ToHex(201-204)Cashu(8-205)ComputeZx(134-143)DotNut/P2PKWitness.cs (1)
P2PKWitness(5-8)
🪛 Gitleaks (8.30.0)
DotNut/SigAllHandler.cs
[high] 219-219: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (17)
DotNut.sln (1)
11-12: Consider removing or renaming the ConsoleApp1 project.The generic name "ConsoleApp1" suggests this may be a temporary development/debugging project. If it's intended for permanent inclusion, consider giving it a more descriptive name. If it was added for local testing, it should likely be excluded from the PR.
DotNut.Demo/Program.cs (1)
478-483: LGTM!The type rename from
P2PkBuildertoP2PKBuilderis correctly applied. The demo code provides good examples of multisig and time-locked P2PK usage.DotNut.Tests/UnitTest1.cs (2)
358-386: Good test coverage for new P2PK rules.The test properly validates that after locktime expiry, proofs are spendable on both standard and refund paths per the new rules from cashubtc/nuts#315. The test verifies both paths with appropriate signatures.
388-477: Comprehensive SIG_ALL test coverage.Excellent test coverage for the new
SigAllHandlerfunctionality including:
- Basic swap request verification
- Multisig scenarios
- Refund locktime scenarios
- HTLC with preimage
- Melt requests with quote ID
The tests validate both valid and invalid cases, ensuring robust verification logic.
DotNut/HTLCProofSecret.cs (2)
14-19: LGTM!The
GetAllowedPubkeysoverride properly delegates to the builder and returns the correct signature threshold.
21-37: LGTM!The refund path logic correctly implements the new behavior:
- Returns
requiredSignatures = 0with empty array when no refund keys exist (proof spendable without signature after locktime)- Uses
RefundSignatureThreshold ?? 1as the default threshold- Returns
nullforrequiredSignatureswhen lock hasn't expired (no refund condition)DotNut/HTLCWitness.cs (1)
7-10: LGTM!Making
Preimagenullable aligns with the new P2PK rules where HTLC proofs can be spent on the refund path after locktime without requiring the preimage. TheJsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)ensures clean serialization.DotNut/P2PKBuilder.cs (3)
6-17: LGTM!The class rename to
P2PKBuilderfollows conventional casing for acronyms. The newRefundSignatureThresholdproperty cleanly extends the builder for multi-sig refund scenarios.
40-45: LGTM!The logic correctly gates the
n_sigs_refundtag:
- Only adds when
RefundSignatureThresholdis set- Validates that enough refund keys exist (
refundKeys.Length >= refundSignatureThreshold)- Only applies within the
Lock.HasValueblock, since refund conditions are only relevant with timelocks
86-92: LGTM!The loading of
n_sigs_refundcorrectly parses the tag value and setsRefundSignatureThreshold. The pattern is consistent with how other tags liken_sigsare loaded.DotNut/SigAllHandler.cs (2)
197-221: LGTM - Static analysis false positive.The Gitleaks warning about "Generic API Key" at line 219 is a false positive. The variable
secretis a local variable representing a proof secret, not an API key. The naming is appropriate for this domain.
223-229: LGTM!The
CheckIfEqualToFirsthelper correctly validates that all proofs share identicalDataandTags, which is required for SIG_ALL signing where all inputs must have matching conditions.DotNut/P2PKProofSecret.cs (5)
12-19: LGTM!The naming convention fix from
P2PkBuildertoP2PKBuilderis consistent with the updated class name. TheGetAllowedPubkeysmethod correctly delegates to the builder.
21-37: LGTM!The refund path logic correctly handles three scenarios:
- Timelock expired with no refund keys → spendable without signature
- Timelock expired with refund keys → requires threshold signatures
- Timelock not expired or no lock → refund path unavailable
111-121: Verify that signature verification logic is intentional for multi-sig.The current implementation counts valid signature-key pairs but doesn't enforce that:
- Each signature is only verified against one key
- Each key is only used to verify one signature
If duplicate signatures are provided, or if a signature matches multiple keys, the count could be inflated.
Consider whether the protocol requires 1:1 matching between signatures and keys:
private bool VerifyPath(ECPubKey[] allowedKeys, int requiredSignatures, SecpSchnorrSignature[] sigs, byte[] hash) { if (sigs.Length < requiredSignatures) return false; var xonlyKeys = allowedKeys.Select(k => k.ToXOnlyPubKey()).ToArray(); - var validCount = sigs.Count(s => xonlyKeys.Any(xonly => xonly.SigVerifyBIP340(s, hash))); - - return validCount >= requiredSignatures; + var usedKeys = new HashSet<int>(); + var validCount = 0; + + foreach (var sig in sigs) + { + for (int i = 0; i < xonlyKeys.Length; i++) + { + if (usedKeys.Contains(i)) continue; + if (xonlyKeys[i].SigVerifyBIP340(sig, hash)) + { + usedKeys.Add(i); + validCount++; + break; + } + } + } + + return validCount >= requiredSignatures; }
178-222: LGTM!The blinded path signing logic correctly handles both the positive and negated key paths, which is necessary for BIP340 x-only pubkey compatibility. The slot tracking prevents key reuse.
Note: The same
Append().ToArray()allocation pattern at lines 204 and 214 could be optimized as mentioned earlier.
253-268: LGTM!The verification logic correctly mirrors the signing logic by:
- Checking if no signature is required (returns true)
- Trying normal path verification first
- Falling back to refund path if available and normal path fails
Fix/new p2pk htlc rules
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
DotNut/P2PKProofSecret.cs (2)
73-73: Typo: "notmal" → "normal".This typo has already been flagged in a previous review.
164-165: Inconsistency resolved.A previous review flagged this line as inconsistent with line 70, noting that
GenerateBlindWitnessreturnednew P2PKWitness()whileGenerateWitnessreturnednull. The current code shows both methods now returnnullwhenrequiredRefundSignatures == 0, making them consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
DotNut/P2PKProofSecret.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-29T15:42:55.222Z
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 23
File: DotNut/NUT11/SigAllHandler.cs:19-77
Timestamp: 2025-11-29T15:42:55.222Z
Learning: In DotNut, HTLCProofSecret inherits from P2PKProofSecret, forming a base-derived relationship where HTLC secrets are a specialized form of P2PK secrets. Type checks using `is P2PKProofSecret` will match both P2PK and HTLC secrets due to this inheritance.
Applied to files:
DotNut/P2PKProofSecret.cs
🔇 Additional comments (4)
DotNut/P2PKProofSecret.cs (4)
45-91: Approve: Witness generation with refund path fallback.The nullable return type and dual-path logic (normal → refund fallback) correctly implement the intended semantics. Early return when
requiredRefundSignatures == 0is appropriate (no witness required when refund has no signature threshold). The exception on line 90 ensures callers are notified when neither path can produce sufficient signatures.
93-125: Approve: Well-structured signing and verification helpers.
TrySignPathefficiently matches available keys against allowed keys using a HashSet and accumulates signatures up to the required threshold.VerifyPathcorrectly counts valid signatures and enforces the threshold. Both methods are clear and correct.
135-183: Approve: Blind witness generation mirrors standard path.The nullable return types and dual-path fallback logic (normal → refund) correctly mirror the non-blind
GenerateWitnessbehavior. The implementation is consistent and sound.
250-282: Approve: Dual-path witness verification.The verification logic correctly checks both normal and refund paths, mirroring the generation logic. The early return when
requiredRefundSignatures == 0appropriately treats the "no witness required" case as always valid. Defensive null filtering on line 256-257 prevents issues with malformed signatures.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
DotNut/P2PKProofSecret.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 23
File: DotNut/NUT11/SigAllHandler.cs:19-77
Timestamp: 2025-11-29T15:42:55.222Z
Learning: In DotNut, HTLCProofSecret inherits from P2PKProofSecret, forming a base-derived relationship where HTLC secrets are a specialized form of P2PK secrets. Type checks using `is P2PKProofSecret` will match both P2PK and HTLC secrets due to this inheritance.
📚 Learning: 2025-11-29T15:42:55.222Z
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 23
File: DotNut/NUT11/SigAllHandler.cs:19-77
Timestamp: 2025-11-29T15:42:55.222Z
Learning: In DotNut, HTLCProofSecret inherits from P2PKProofSecret, forming a base-derived relationship where HTLC secrets are a specialized form of P2PK secrets. Type checks using `is P2PKProofSecret` will match both P2PK and HTLC secrets due to this inheritance.
Applied to files:
DotNut/P2PKProofSecret.cs
🧬 Code graph analysis (1)
DotNut/P2PKProofSecret.cs (3)
DotNut/P2PKBuilder.cs (2)
P2PKBuilder(6-171)P2PKBuilder(61-111)DotNut/Cashu.cs (19)
ECPubKey(16-20)ECPubKey(22-26)ECPubKey(27-42)ECPubKey(51-54)ECPubKey(61-64)ECPubKey(66-69)ECPubKey(71-75)ECPubKey(77-81)ECPubKey(128-132)ECPrivKey(56-59)ECPrivKey(84-95)ECPrivKey(145-156)ToBytes(180-185)ToHex(175-178)ToHex(193-196)ToHex(197-200)ToHex(201-204)Cashu(8-205)ComputeZx(134-143)DotNut/P2PKWitness.cs (1)
P2PKWitness(5-8)
🔇 Additional comments (4)
DotNut/P2PKProofSecret.cs (4)
21-37: LGTM - Timelock logic correctly implemented.The refund pubkey retrieval logic correctly handles the timelock condition: when
Lock >= Now, the timelock hasn't expired yet, so refund keys are not accessible. The special case at lines 30-34 whererequiredSignatures = 0(freely spendable after lock expiry with no refund keys) is also handled appropriately.
45-91: Well-structured witness generation with proper path precedence.The witness generation flow correctly:
- Returns
nullwhen no witness is required (line 70)- Attempts the normal signing path first (lines 73-78)
- Falls back to the refund path when the timelock has expired and refund keys are available (lines 81-88)
- Throws a clear exception when neither path can produce sufficient signatures (line 90)
The nullable return type
P2PKWitness?appropriately signals to callers when witness generation is not necessary.
186-230: Good defensive programming with slot tracking.The
TrySignBlindPathmethod correctly uses aHashSet<int>(line 192) to track which key slots have been used, preventing the same slot from being signed multiple times (line 201, 210, 221). This ensures proper multi-signature semantics where each signature must correspond to a distinct key.This defensive approach should also be applied to the verification logic in
VerifyPathto prevent duplicate signature attacks.
250-282: Verification flow structure is sound, but relies on flawedVerifyPath.The overall verification logic correctly:
- Filters invalid signatures (line 256)
- Checks the normal path first (line 266)
- Falls back to the refund path when applicable (lines 270-274)
- Returns
truewhen no witness is required (lines 261-264)However, both verification branches call
VerifyPath, which has a critical vulnerability allowing duplicate signatures to bypass the multi-sig threshold. OnceVerifyPathis fixed, this method will work correctly.The security of this verification depends on fixing the duplicate signature issue in
VerifyPath(lines 115-125).
|
cashubtc/nuts#315 has now been merged, so this can be completed and merged when ready. |
2 participants
cashubtc/nuts#302
cashubtc/nuts#315
Summary by CodeRabbit
New Features
API Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.