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 2 commits 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 f16cbb9) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
| assert(size() == COMPRESSED_SIZE); | ||
| // DIP-14 compatibility mode: indexes below 2^32 derive exactly as BIP32. | ||
| // A hardened (high bit set) 32-bit index cannot be derived from a pubkey. | ||
| if (std::all_of(nChild.begin(), nChild.begin() + 28, [](unsigned char c) { return c == 0; })) { |
There was a problem hiding this comment.
Am not confident about keeping a compatibility mode, since Derive256 now has an implicit case purely based on input and if we have behaviour assuming that Derive256 will always emit a DIP-14 derivation, silently switching to BIP-32 derivation might not be ideal, would be preferable to fail entirely.
There was a problem hiding this comment.
Keeping compatibility mode is intentional here. DIP-14 defines the below-2^32 compatibility behavior, and this PR's vectors explicitly lock that contract to BIP-32. In the DashPay PR train, higher-level callers do not expose this as an untyped choice: wallet::platformkeys::PathElement distinguishes uint32_t from the 32-byte identity path element and routes them to Derive and Derive256 respectively. Removing compatibility in this low-level primitive would diverge from the stated DIP-14 contract without improving the later caller boundary.
🤖 Posted autonomously by Codex on behalf of pasta.
| CHMAC_SHA512(chainCode.begin(), chainCode.size()).Write(&header, 1).Write(data, 32).Write(num, 4).Finalize(output); | ||
| } | ||
|
|
||
| void DIP14Hash(const ChainCode& chainCode, const unsigned char nChild[32], unsigned char header, const unsigned char data[32], unsigned char output[64]) |
There was a problem hiding this comment.
We could probably create a type for children that can accept 32-bit or 256-bit integers and encodes them appropriately embedding the BE semantics as part of the wire definition. Would also apply to the WriteBE32() in BIP32Hash().
There was a problem hiding this comment.
That typed distinction belongs at the wallet path-policy layer, and the later DashPay train already supplies it: wallet::platformkeys::PathElement is a variant of uint32_t and std::array<uint8_t, 32>, routing each case to Derive or Derive256. Pulling that policy type into BIP32Hash/CKey in this foundational PR would couple the low-level crypto API to a later wallet abstraction and widen the dependency PR. I am therefore leaving this primitive byte-oriented and keeping the type-safe selection in the downstream layer.
🤖 Posted autonomously by Codex on behalf of pasta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head correctly adds DIP-14 derivation and now rejects malformed child-index spans before performing offset arithmetic; no correctness or security defects were confirmed. Two non-blocking improvements remain: extend the existing key fuzz harness to cover Derive256 and squash the immediate input-validation correction into the feature commit.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
2 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:93: New Derive256 primitive is not exercised by the existing key/pubkey fuzz harnesses
The key fuzz target exercises CKey::Derive here and CPubKey::Derive around line 242, but it never invokes the newly added Derive256 methods. Add private and public derivations with randomized 32-byte indexes so fuzzing reaches the BIP32 compatibility check, the 32-bit boundary, hardened-bit handling, general 256-bit indexes, and secp256k1 tweak-add failures. The fixed unit tests validate published vectors and malformed lengths, but they do not provide the broad valid-index coverage already given to the existing Derive paths.
In `<commit:f16cbb9>`:
- [SUGGESTION] <commit:f16cbb9>:1: Squash the child-index size fix into the feature commit
Commit f16cbb97d53 changes assertions in the Derive256 implementations introduced by the immediately preceding commit 8a6299d1ca8 into unconditional input validation and adds the corresponding regression tests. Because the primitive has not shipped between these commits, squash the corrective commit into the feature commit so the API and its complete malformed-input behavior form one atomic change for history, blame, and bisect.
3 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