feat: p2pk keys deterministic derivation - #33
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR adds NUT-13 deterministic P2PK derivation, purpose-specific counters, wallet support, and deterministic P2PK locking for mint quotes. Unit and integration tests cover derivation vectors, counter behavior, key recovery, and swaps. ChangesDeterministic P2PK locking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant MintQuoteBuilder
participant DerivationCounter
participant Nut13
participant Mint
Wallet->>MintQuoteBuilder: configure deterministic P2PK locking
MintQuoteBuilder->>DerivationCounter: fetch and increment P2Pk counter
MintQuoteBuilder->>Nut13: derive P2PK key
Nut13-->>MintQuoteBuilder: return derived key
MintQuoteBuilder->>Mint: create locked outputs
Mint-->>Wallet: return mint quote and proofs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
NUT-13 specifies m/129373'/10'/0'/0'/{counter}, but this derived from
129372' — the NUT-13 secret purpose. The test vectors were generated with
our own code, so they confirmed the wrong path; they are replaced with the
official ones from tests/13-tests.md.
Upstream fixed the purpose in cashubtc/nuts@04cce64 ("use correct purpose
and tests"), eight days after this derivation was written here.
The counter is also narrowed from ulong to uint, matching the rest of
NUT-13. The spec calls it a non-hardened child index, so anything from 2^31
up is invalid — KeyPath parsed those as a hardened index and silently
returned a colliding key instead of failing.
The P2PK derivation path has no keyset in it, so the existing per-keyset counters do not fit. Adds a DerivationPurpose-keyed counter, scoped so the NUT-20 quote locking counter can reuse it — the spec calls that one independent from the NUT-13 counters, and it needs the same shape. It lives in its own IDerivationCounter rather than on ICounter, so implementations outside this repo keep compiling; InMemoryCounter implements both. Reading a purpose counter is a single GetCounter call, so there is no export counterpart to ICounter.Export.
Wires the NUT-13 P2PK derivation into the wallet. WithDeterministicP2PkLock takes the next P2PK counter value, derives the key from the seed and makes it the primary key on the builder, so the lock can be recovered on restore instead of depending on a key the caller kept somewhere. Wallet keeps the counter as IDerivationCounter when the injected ICounter implements it, and GetDerivationCounter returns null otherwise — so an older counter still works for everything else and only this feature is unavailable. Any pubkeys already on the builder are kept after the derived one, which leaves multisig locks with one recoverable key available.
ff380a6 to
b3aaf70
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DotNut/Abstractions/Wallet.cs`:
- Around line 128-132: Update WalletBuilder.WithCounter and the counter
restoration flow to persist and restore both keyset counters and
derivation-purpose counters, rather than initializing _derivationCounter with
empty state. Add a dedicated persisted counter-state type or an overload
accepting both dictionaries, then add a restart test verifying the next P2PK
derivation counter continues from its previous value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd125dd2-74a3-420f-a6e9-2b856920c679
📒 Files selected for processing (11)
DotNut.Tests/Integration.csDotNut.Tests/Unit/Nut13Tests.csDotNut.Tests/Unit/UnitTests2.csDotNut/Abstractions/DerivationPurpose.csDotNut/Abstractions/InMemoryCounter.csDotNut/Abstractions/Interfaces/IDerivationCounter.csDotNut/Abstractions/Interfaces/IMintQuoteBuilder.csDotNut/Abstractions/Interfaces/IWalletBuilder.csDotNut/Abstractions/MintQuoteBuilder.csDotNut/Abstractions/Wallet.csDotNut/NUT13/Nut13.cs
| public IWalletBuilder WithCounter(IDictionary<KeysetId, uint> counter) | ||
| { | ||
| this._counter = new InMemoryCounter(counter); | ||
| var inMemory = new InMemoryCounter(counter); | ||
| this._counter = inMemory; | ||
| this._derivationCounter = inMemory; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Persist derivation-purpose counters with dictionary-backed wallets.
This overload restores only keyset counters. It initializes _derivationCounter with empty purpose state. After a wallet restart, the P2PK counter starts again at zero and can reuse deterministic mint-lock keys for the same seed.
Persist and restore purpose counters with the keyset counters. Add a builder overload or a dedicated persisted counter-state type. Add a restart test that verifies the next P2PK counter does not reset.
Based on PR objectives: wallet quote locks use wallet-level derivation counters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DotNut/Abstractions/Wallet.cs` around lines 128 - 132, Update
WalletBuilder.WithCounter and the counter restoration flow to persist and
restore both keyset counters and derivation-purpose counters, rather than
initializing _derivationCounter with empty state. Add a dedicated persisted
counter-state type or an overload accepting both dictionaries, then add a
restart test verifying the next P2PK derivation counter continues from its
previous value.
Adds the m/129373'/20'/0'/0'/{counter} derivation from cashubtc/nuts#373,
covered by the vectors in tests/20-test.md. The counter is its own
DerivationPurpose, which the spec requires to be independent of the NUT-13
keyset counters.
WithDeterministicPubkey locks a quote to the derived key instead of one the
caller has to keep, and the handler then signs the mint request itself. The
point is recovery: a quote that was locked but not yet minted is otherwise
lost with the key.
NUT-20: deterministic quote locking key derivation
1 participant
cashubtc/nuts#331
Summary by CodeRabbit
New Features
Tests