feat: add DIP-14 256-bit child key derivation (Derive256) - #7511
feat: add DIP-14 256-bit child key derivation (Derive256)#7511PastaPastaPasta wants to merge 1 commit into
Conversation
DIP-14 extends BIP32 child key derivation to 256-bit child indexes with an explicit hardened flag, replacing the high-bit convention. It underpins the DIP-13 platform key hierarchy and DIP-15 friendship-derived address spaces, where child indexes are 256-bit identity hashes so that derivation paths between identities cannot be ground out by a third party. Add DIP14Hash (the HMAC-SHA512 with ser256(i)), CKey::Derive256 and CPubKey::Derive256. Indexes below 2^32 are interpreted as raw BIP32 index space for compatibility, per the DIP-14 compatibility requirement and its published test vectors: the hardened flag is folded into the high bit on the private side, and the public side rejects indexes with the high bit set exactly as BIP32 does. Non-hardened 256-bit public derivation matches private derivation, which is what lets a contact derive addresses from an exported xpub without seeing private keys. Covered by the four DIP-14 spec test vectors (dashpay/dips dip-0014.md) plus BIP32-compatibility and public/private consistency checks in the new dip14_tests suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 8a6299d) |
WalkthroughThe change adds DIP-14 hashing and 256-bit child-key derivation. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CKey_or_CPubKey
participant DIP14Hash
participant ChildKey_or_ChildPubKey
Caller->>CKey_or_CPubKey: request Derive256 with child index and chain code
CKey_or_CPubKey->>DIP14Hash: hash derivation inputs for non-32-bit index
DIP14Hash-->>CKey_or_CPubKey: return tweak and child chain code
CKey_or_CPubKey->>ChildKey_or_ChildPubKey: apply tweak and store derived key
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pubkey.cpp (1)
261-287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicate tweak-add/serialize postscript between
DeriveandDerive256in bothCKeyandCPubKey. In both classes, the newDerive256method duplicates the existingDerivemethod's postscript logic verbatim, differing only in how the 64-byte hash output is computed (BIP32HashvsDIP14Hash).
src/pubkey.cpp#L261-L287: extract the pubkey-parse/tweak-add/serialize sequence (currently duplicated fromDeriveat lines 247-257) into a shared private helper that bothDeriveandDerive256call with the precomputed 64-byte hash output.src/key.cpp#L310-L335: extract the chain-code-copy/key-set/tweak-add/clear-on-failure sequence (currently duplicated fromDeriveat lines 303-307) into a shared private helper that bothDeriveandDerive256call with the precomputed 64-byte hash output.Keeping these two implementations of the same crypto invariant in sync manually risks future divergence if one path is updated without the other.
As per coding guidelines, "Keep changes narrow; do not mix cleanup, formatting, refactoring, and behavior changes unless explicitly requested," so this consolidation is better suited to a follow-up change rather than this PR.
🤖 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 `@src/pubkey.cpp` around lines 261 - 287, Defer this consolidation to a follow-up change; make no modifications at src/pubkey.cpp lines 261-287 or src/key.cpp lines 310-335 in this PR. The requested future refactor should extract the shared postscript from CPubKey::Derive and Derive256 into a private helper, and likewise extract the shared chain-code/key/tweak handling from CKey::Derive and Derive256, while preserving behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/pubkey.cpp`:
- Around line 261-287: Defer this consolidation to a follow-up change; make no
modifications at src/pubkey.cpp lines 261-287 or src/key.cpp lines 310-335 in
this PR. The requested future refactor should extract the shared postscript from
CPubKey::Derive and Derive256 into a private helper, and likewise extract the
shared chain-code/key/tweak handling from CKey::Derive and Derive256, while
preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd5455d-e1e4-45b4-a93f-f46f1d8e51b9
📒 Files selected for processing (9)
src/Makefile.test.includesrc/hash.cppsrc/hash.hsrc/key.cppsrc/key.hsrc/pubkey.cppsrc/pubkey.hsrc/test/dip14_tests.cpptest/util/data/non-backported.txt
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR adds a clean, additive DIP-14 256-bit child key derivation primitive (CKey::Derive256 / CPubKey::Derive256 / DIP14Hash) that correctly falls back to BIP32 semantics for sub-2^32 indexes and is verified against published test vectors. The implementation, build registration, and non-backported tracking are all correct; only two minor test-robustness/style suggestions were found, neither blocking.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/fuzz/key.cpp`:
- [SUGGESTION] src/test/fuzz/key.cpp:88-97: New Derive256 primitive is not exercised by the existing key/pubkey fuzz harnesses
src/test/fuzz/key.cpp already fuzzes CKey::Derive and CPubKey::Derive with randomized ChainCode/key material (lines 91-97 and around 240-244), which has historically caught edge cases in secp256k1 tweak-add failure paths and boundary values. Derive256 is new parsing/arithmetic surface — the 28-zero-byte BIP32-compatibility check, ReadBE32 boundary handling, and hardened-bit folding — that would benefit from the same fuzzing treatment, especially since Span<const unsigned char> callers could pass adversarial 32-byte indexes (all-zero, all-0xFF, boundary values around 2^32) not covered by the four fixed spec vectors in dip14_tests.cpp. This is confirmed: grep shows Derive256 is only exercised in src/test/dip14_tests.cpp, never in the fuzz targets. Not a defect in the current code, but a natural follow-up for parity with the existing harness.
| //! Derive DIP-14 child key with a 256-bit index (32 bytes, big-endian). | ||
| //! Indexes below 2^32 fall back to BIP32 derivation for compatibility | ||
| //! (the hardened flag is then folded into the 32-bit index). | ||
| [[nodiscard]] bool Derive256(CKey& keyChild, ChainCode& ccChild, Span<const unsigned char> nChild, bool hardened, const ChainCode& cc) const; |
There was a problem hiding this comment.
💬 Nitpick: Span for a fixed-32-byte index relies on a runtime assert instead of compile-time size safety
Derive256 takes Span nChild and only validates its length with assert(nChild.size() == 32) at the top of the function body (src/key.cpp:311, src/pubkey.cpp:263). A std::array<unsigned char, 32> (or a dedicated 32-byte index type) would make the 32-byte contract enforced by the type system rather than a runtime assertion, removing any reliance on assert being compiled in for safety against a mis-sized Span reaching nChild.begin() + 28 / nChild.data() + 28. This is consistent with existing Span usage elsewhere in the file, so it's a minor style preference rather than a defect.
source: ['claude']
2 participants
Issue being fixed or feature implemented
DIP-14 extends BIP32 child key derivation to 256-bit child indexes with an explicit hardened flag. It underpins the DIP-13 platform key hierarchy and DIP-15 friendship-derived address spaces, where child indexes are 256-bit identity hashes so that the derivation path between two identities cannot be ground out by a third party (a 31-bit index space would be brute-forceable by cycling identities).
Dash Core currently has no DIP-14 primitive. This PR adds it at the
CKey/CPubKeylevel, extracted from the Platform GUI work in PastaPastaPasta#49 as the first wallet-side prerequisite.What was done?
DIP14Hashinhash.{h,cpp}: the HMAC-SHA512 with a 256-bit big-endian (ser256) child index, alongsideBIP32Hash.CKey::Derive256: private child derivation with a 256-bit index and explicit hardened flag.CPubKey::Derive256: the public (non-hardened only) counterpart. Non-hardened 256-bit public derivation matches private derivation, which is what lets a counterparty derive addresses from an exported xpub without seeing private keys.Compatibility-mode semantics (worth calling out, since the DIP-14 pseudocode is loosely worded here): indexes below 2^32 are interpreted as raw BIP32 index space, per the DIP's Compatibility section ("if a child key's index is less than 2^32 … will match the derivation outlined in BIP32") and its published test vectors. Concretely, the hardened flag is folded into the high bit on the private side, and the public side rejects indexes with the high bit set exactly as BIP32 does. Vector 2 (
m/9'/5'/15'/0'/…) only reproduces under this interpretation.How Has This Been Tested?
New unit suite
dip14_tests:Derive);dip14_tests,key_tests,bip32_tests, andhash_testspass locally (macOS, clang).lint-whitespace,lint-include-guards,lint-includesclean; the new Dash-specific test file is listed intest/util/data/non-backported.txt.Breaking Changes
None. Additive API only; no existing derivation path changes behavior.
Checklist:
🤖 Generated with Claude Code