KeysetId V2 once again #25
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @DotNut/NUT13/Nut13.cs:
- Around line 51-57: The counter parameter must be unsigned to prevent negative
indices from wrapping when cast to ulong; change the parameter type from int to
uint in DeriveHmac and propagate that change through its callers
DeriveBlindingFactor and DeriveSecret (and any other call sites) so the
derivation index is enforced as non-negative at compile time, update all
signatures and call-site arguments accordingly, and ensure
BitConverter.GetBytes((ulong)counter) still receives the uint value.
🧹 Nitpick comments (1)
DotNut/Keyset.cs (1)
42-70: Consider using a more efficient byte array builder.The repeated
.Concat(...).ToArray()calls (lines 55, 62, 68-69) allocate intermediate arrays. For better efficiency, consider using aList<byte>orMemoryStream.♻️ Optional refactor using List
case 0x01: { - var sortedBytes = Encoding.UTF8.GetBytes( + var sortedBytes = new List<byte>(Encoding.UTF8.GetBytes( string.Join( ",", sortedKeys .Select(pair => $"{pair.Key}:{pair.Value.ToString().ToLowerInvariant()}") ) - ); + )); if (String.IsNullOrWhiteSpace(unit)) { throw new ArgumentNullException( nameof(unit), $"Unit parameter is required with version: {version}"); } - sortedBytes = sortedBytes.Concat(Encoding.UTF8.GetBytes($"|unit:{unit.Trim().ToLowerInvariant()}")).ToArray(); + sortedBytes.AddRange(Encoding.UTF8.GetBytes($"|unit:{unit.Trim().ToLowerInvariant()}")); if (inputFeePpk.HasValue && inputFeePpk.Value != 0) { - sortedBytes = sortedBytes.Concat(Encoding.UTF8.GetBytes($"|input_fee_ppk:{inputFeePpk.Value}")).ToArray(); + sortedBytes.AddRange(Encoding.UTF8.GetBytes($"|input_fee_ppk:{inputFeePpk.Value}")); } if (!string.IsNullOrWhiteSpace(finalExpiration)) { - sortedBytes = sortedBytes.Concat(Encoding.UTF8.GetBytes($"|final_expiry:{finalExpiration.Trim()}")) - .ToArray(); + sortedBytes.AddRange(Encoding.UTF8.GetBytes($"|final_expiry:{finalExpiration.Trim()}")); } - var hash = sha256.ComputeHash(sortedBytes); + var hash = sha256.ComputeHash(sortedBytes.ToArray());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
DotNut.Tests/UnitTest1.csDotNut/Encoding/CashuTokenHelper.csDotNut/Keyset.csDotNut/KeysetId.csDotNut/NUT13/Nut13.cs
🧰 Additional context used
🧬 Code graph analysis (4)
DotNut/NUT13/Nut13.cs (1)
DotNut/KeysetId.cs (1)
GetBytes(81-84)
DotNut/Keyset.cs (1)
DotNut/KeysetId.cs (4)
KeysetId(52-62)GetBytes(81-84)ToString(70-73)GetVersion(75-79)
DotNut/Encoding/CashuTokenHelper.cs (4)
DotNut/Keyset.cs (1)
KeysetId(13-83)DotNut/KeysetId.cs (3)
KeysetId(52-62)GetVersion(75-79)ToString(70-73)DotNut/JsonConverters/KeysetIdJsonConverter.cs (1)
KeysetId(8-23)DotNut/Proof.cs (1)
Proof(7-31)
DotNut.Tests/UnitTest1.cs (5)
DotNut/Encoding/CashuTokenHelper.cs (1)
KeysetId(86-91)DotNut/Keyset.cs (1)
KeysetId(13-83)DotNut/KeysetId.cs (1)
KeysetId(52-62)DotNut/JsonConverters/KeysetJsonConverter.cs (1)
Keyset(8-54)DotNut/NUT13/Nut13.cs (2)
DeriveBlindingFactor(10-11)DeriveBlindingFactor(18-32)
🔇 Additional comments (9)
DotNut/KeysetId.cs (2)
64-68: LGTM - Appropriate deprecation for v2 compatibility.The obsolete attribute correctly warns users that this constructor only supports v1 keyset ID derivation. The internal call to
GetKeysetId()uses default parameters (version=0x00), which aligns with the warning message.
54-57: Improved readability with inline comments.The reformatted validation with explanatory comments for each valid length (66, 16, 12) enhances code clarity without changing behavior.
DotNut/Keyset.cs (1)
85-93: LGTM - Short ID verification logic is correct.The
VerifyKeysetIdmethod correctly handles both full-length and shortened keyset IDs:
- If the presented ID is longer than derived, it fails (line 90)
- Otherwise, it accepts exact matches or prefix matches (lines 91-92)
This allows v2 short IDs (16 chars) to verify against full-length derived IDs (66 chars).
DotNut.Tests/UnitTest1.cs (3)
203-208: LGTM - Test method signature updated correctly.The
Nut02Tests_KeysetIdMatchmethod now accepts the optionalinputFeeparameter and correctly passes it toGetKeysetId.
508-526: HMAC-KDF implementation for v0x01 keyset IDs is structurally correct, but test vector values cannot be independently verified.The DeriveHmac method correctly implements HMAC-SHA256-based KDF with the message format
"Cashu_KDF_HMAC_SHA256" || keysetId_bytes || counter (big-endian) || flag, and the test properly exercises the derivation for both secrets and blinding factors across counters 0-4. However, the specific expected test vector values lack verification against public Cashu NUT-13 documentation, which currently specifies only BIP32-based derivation (v0x00). These values should be validated against the official Cashu reference implementation or a formal specification update for v0x01 HMAC derivation.
184-202: Test vectors correctly cover v2 keyset ID derivation scenarios.The test data properly validates the preimage construction rules:
- Line 184-190: inputFee=100 (included when non-zero)
- Line 191-197: inputFee=0 (omitted per spec requirement at line 59-60 of Keyset.cs)
- Line 198-202: no inputFee parameter (omitted)
The implementation in
Keyset.cs(lines 60-63) confirms the behavior:|input_fee_ppkis only added to the preimage when the value is non-zero. The test vectors align with this specification.DotNut/Encoding/CashuTokenHelper.cs (3)
50-50: Good API change: Decode now acceptsList<KeysetId>instead ofList<Keyset>.This reduces coupling - the caller only needs to provide keyset IDs for short ID mapping, not full keyset objects. This is a cleaner interface.
93-103: LGTM - Short keyset ID mapping logic is correct.The condition on line 95 correctly identifies when mapping is needed:
- Returns early if all proofs are either non-v0x01 OR have full-length IDs
- Only proceeds to mapping when short v0x01 IDs (16 chars) exist
The null check with descriptive exception message (lines 99-103) provides clear feedback when keysets are required but not provided.
105-125: Clean proof mapping implementation.The updated logic correctly:
- Skips non-v0x01 proofs (line 107-108)
- Matches short IDs using case-insensitive prefix matching (line 111)
- Creates new Proof objects with the resolved full KeysetId (line 123)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @DotNut/NUT13/Nut13.cs:
- Around line 9-17: You changed counter parameters from int to uint on
DeriveBlindingFactor and DeriveSecret (e.g., the extension overloads that call
DeriveBlindingFactor(mnemonic.DeriveSeed(), keysetId, counter)), which is a
breaking API for callers using int loop variables; update all call sites
(notably the Demo loop that passes i) to use a uint counter or an explicit cast
to (uint)i when calling these methods, or change the loop variable to uint, so
the types match and intent is clear.
- Around line 68-71: GetNut13DerivationPath currently doesn't constrain the uint
counter to the BIP32 hardened index range; update it to mirror GetKeysetIdInt's
modulo constraint so counter is reduced to the 0..2^31-1 range before building
the path. Compute a mod value = (long)Math.Pow(2,31)-1 (or 1<<31 - 1), derive
constrainedCounter = (long)counter % mod, and use constrainedCounter in the
KeyPath.Parse string interpolation instead of the raw counter so hardened
derivation indices remain valid.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
DotNut/NUT13/Nut13.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-08T01:49:27.299Z
Learnt from: d4rp4t
Repo: Kukks/DotNut PR: 21
File: DotNut/P2PkBuilder.cs:100-107
Timestamp: 2025-11-08T01:49:27.299Z
Learning: In the DotNut codebase, the PrivKey class (DotNut/PrivKey.cs) defines an implicit operator that converts PrivKey to ECPrivKey by returning privKey.Key. This allows PrivKey instances to be passed directly to methods expecting ECPrivKey parameters without explicit conversion.
Applied to files:
DotNut/NUT13/Nut13.cs
🔇 Additional comments (1)
DotNut/NUT13/Nut13.cs (1)
50-65: LGTM!The
ulongcast at line 52 correctly produces an 8-byte counter buffer for the HMAC message, and the endianness handling is appropriate.
| public static KeyPath GetNut13DerivationPath(KeysetId keysetId, uint counter, bool secretOrr) | ||
| { | ||
| return (KeyPath) KeyPath.Parse($"m/{Purpose}/0'/{GetKeysetIdInt(keysetId)}'/{counter}'/{(secretOrr?0:1)}")!; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and examine the context around the GetNut13DerivationPath method
find . -name "Nut13.cs" -type fRepository: Kukks/DotNut
Length of output: 79
🏁 Script executed:
# Search for NBip32Fast imports and usage
rg -i "nbip32fast|keypath" --type cs -A 2 -B 2Repository: Kukks/DotNut
Length of output: 1741
🏁 Script executed:
# Look for test files related to Nut13
find . -name "*Nut13*Test*" -o -name "*test*Nut13*" | head -20Repository: Kukks/DotNut
Length of output: 38
🌐 Web query:
BIP32 specification hardened derivation index range limits
💡 Result:
Hardened child indexes are the 32-bit values from 2^31 through 2^32−1 (0x80000000 … 0xFFFFFFFF). They’re commonly written as i' (meaning i + 0x80000000). [1][2]
Sources:
- BIP32 specification (ranges and hardened-index notation). [1]
- HD-wallet documentation (explanatory summary of 0…2^31−1 for normal, 2^31…2^32−1 for hardened). [2]
🌐 Web query:
NBip32Fast C# library hardened derivation index validation
💡 Result:
Short answer: NBip32Fast follows standard BIP32 rules — a hardened child index is any index with the MSB set (i ≥ 2^31, i.e. 0x80000000). Produce/validate hardened indexes by adding/setting 2^31 (index | 0x80000000) and ensure non-hardened indexes are in 0..2^31-1 before use. NBip32Fast examples use KeyPathElement.Hard(i) / Harden-style helpers when deriving hardened children. (nuget.org)
Checklist you can use in code:
- Require raw (unhardened) index in range [0, 2^31-1] for user inputs.
- Hardened index = index | 0x80000000 (or index + 2^31).
- When deriving with NBip32Fast, use the library’s Hard helper / KeyPathElement.Hard(i) or its HardenIndex helper to build the element. (nuget.org)
If you want, I can show a tiny C# snippet that validates an input index and produces the NBip32Fast hardened KeyPathElement.
Citations:
- 1: https://www.nuget.org/packages/NBip32Fast/2.0.0?utm_source=openai
- 2: https://www.nuget.org/packages/NBip32Fast/2.0.0?utm_source=openai
🏁 Script executed:
# Read the Nut13.cs file to see full context and check for validation
cat -n ./DotNut/NUT13/Nut13.cs | head -100Repository: Kukks/DotNut
Length of output: 3524
🏁 Script executed:
# Check if KeyPath is defined in NBip32Fast or if it's custom
rg "class KeyPath|struct KeyPath" --type cs -A 5Repository: Kukks/DotNut
Length of output: 38
🏁 Script executed:
# Search for test cases of GetNut13DerivationPath
rg "GetNut13DerivationPath" --type cs -B 2 -A 5Repository: Kukks/DotNut
Length of output: 2485
🏁 Script executed:
# Check if counter values are validated anywhere before calling GetNut13DerivationPath
rg "counter" --type cs -B 2 -A 2 | head -80Repository: Kukks/DotNut
Length of output: 4616
Add validation to ensure counter stays within BIP32 hardened index range.
GetKeysetIdInt already constrains the keysetId to [0, 2^31-1] using modulo, but counter lacks this validation. Since hardened BIP32 derivation requires indices < 2^31, a uint counter exceeding 2,147,483,647 would violate the spec. Apply the same modulo constraint to counter:
public static KeyPath GetNut13DerivationPath(KeysetId keysetId, uint counter, bool secretOrr)
{
var mod = (long)Math.Pow(2, 31) - 1;
var constrainedCounter = (long)counter % mod;
return (KeyPath)KeyPath.Parse($"m/{Purpose}/0'/{GetKeysetIdInt(keysetId)}'/{constrainedCounter}'/{(secretOrr?0:1)}")!;
}🤖 Prompt for AI Agents
In @DotNut/NUT13/Nut13.cs around lines 68 - 71, GetNut13DerivationPath currently
doesn't constrain the uint counter to the BIP32 hardened index range; update it
to mirror GetKeysetIdInt's modulo constraint so counter is reduced to the
0..2^31-1 range before building the path. Compute a mod value =
(long)Math.Pow(2,31)-1 (or 1<<31 - 1), derive constrainedCounter = (long)counter
% mod, and use constrainedCounter in the KeyPath.Parse string interpolation
instead of the raw counter so hardened derivation indices remain valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
DotNut/Keyset.cs (1)
36-83: Add using statement for MemoryStream disposal.The MemoryStream should be properly disposed to follow best practices, even though it doesn't hold unmanaged resources.
♻️ Recommended refactor
case 0x01: { - MemoryStream stream = new MemoryStream(); + using MemoryStream stream = new MemoryStream(); // 2 - concatenate each amount and its corresponding public key hex string (as "amount:publickey_hex")Note: The preimage construction logic correctly implements the v0x01 specification, including proper handling of the optional
inputFeePpkparameter (omitted when null or zero).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DotNut.Demo/Program.csDotNut.Tests/UnitTest1.csDotNut/Keyset.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.Tests/UnitTest1.cs
🧬 Code graph analysis (1)
DotNut/Keyset.cs (2)
DotNut/KeysetId.cs (4)
KeysetId(52-62)GetBytes(81-84)ToString(70-73)GetVersion(75-79)DotNut/PubKey.cs (1)
ToString(26-29)
🔇 Additional comments (8)
DotNut.Demo/Program.cs (1)
439-439: LGTM! Counter type updated to align with NUT13 API.The loop variable type change from
inttouintcorrectly matches the updated NUT13 derivation methods that now expect unsigned counter parameters.DotNut/Keyset.cs (3)
1-1: LGTM! API extended to support input fee in keyset derivation.The addition of the optional
inputFeePpkparameter maintains backward compatibility while enabling v0x01 keysets to include input fee information in the preimage.Also applies to: 11-11
15-33: LGTM! Version 0x00 path refactored without semantic changes.The refactoring maintains the original keyset ID derivation logic: sorting keys by amount, concatenating public key bytes, and hashing. The
inputFeePpkparameter is correctly ignored for version 0x00.
90-98: LGTM! Verification logic correctly updated.The
VerifyKeysetIdmethod properly threads theinputFeePpkparameter through toGetKeysetIdand correctly handles both full and short keyset ID comparisons.DotNut.Tests/UnitTest1.cs (4)
178-202: LGTM! Test coverage expanded for v2 keyset ID derivation.The test data now includes v2 keyset scenarios with various combinations of
inputFeeandfinalExpirationparameters, properly exercising the new API surface.
203-208: LGTM! Test method updated to match new API.The test signature and implementation correctly thread the
inputFeeparameter through to theGetKeysetIdcall, validating the extended keyset derivation logic.
406-406: HTLC test vector updated.The HTLC secret test data has been updated. Ensure this aligns with the latest test vectors from the Cashu specification.
510-526: LGTM! NUT13 test vectors updated for v2 keyset.The test now uses a v2 keyset ID (matching the test case at lines 184-190 with
inputFee: 100ULandfinalExpiration: "2059210353"), and all derived secrets and blinding factors have been updated accordingly. This validates that deterministic derivation works correctly with the new keyset ID format.
1 participant
cashubtc/nuts#182
Summary by CodeRabbit
New Features
Tests
Refactor
Deprecated
✏️ Tip: You can customize this high-level summary in your review settings.