feat(wallet): add Platform key provider, data records and DIP-15 friendship keychain seams - #7581
feat(wallet): add Platform key provider, data records and DIP-15 friendship keychain seams#7581PastaPastaPasta wants to merge 11 commits into
Conversation
|
⛔ Blockers found — Opus deferred (commit b7c04f8) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349d0573b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe wallet now enables secp256k1 ECDH and adds Platform key derivation, signing, ECDH, seed identification, friendship keychain, and payment destination APIs. It supports Platform seed recovery for descriptor and legacy wallets. It stores opaque Platform key/value records in memory and the wallet database, with prefix retrieval and deletion. Tests cover derivation vectors, recovery, ownership, restoration, ECDH, idempotency, and database behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The wallet-layer additions are merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Wallet
participant WalletImpl
participant GetPlatformSeed
participant platformkeys
Wallet->>WalletImpl: request Platform key
WalletImpl->>GetPlatformSeed: retrieve wallet seed
GetPlatformSeed-->>WalletImpl: return selected seed
WalletImpl->>platformkeys: derive key from path
platformkeys-->>WalletImpl: return derived key
WalletImpl-->>Wallet: return public key
Possibly related PRs
🚥 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.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/wallet/platformkeys.cpp (1)
5-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing
<algorithm>include in both new Platform sources. Both files callstd::copybut neither includes<algorithm>; they compile only through transitive includes.
src/wallet/platformkeys.cpp#L5-L12: add#include <algorithm>for thestd::copycalls at lines 56-57 and line 137.src/wallet/platformseed.cpp#L5-L16: add#include <algorithm>for thestd::copycall at line 32.🤖 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/wallet/platformkeys.cpp` around lines 5 - 12, Add the standard <algorithm> header to both src/wallet/platformkeys.cpp lines 5-12 and src/wallet/platformseed.cpp lines 5-16 so their std::copy calls have a direct declaration; no other changes are required.src/wallet/wallet.h (1)
489-492: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAnnotate
m_platform_datawithGUARDED_BY(cs_wallet).All three accessors declare
EXCLUSIVE_LOCKS_REQUIRED(cs_wallet), but the member itself carries no annotation. Clang thread-safety analysis then cannot catch a future unlocked access.🔒 Proposed fix
- std::map<std::string, std::vector<unsigned char>> m_platform_data; + std::map<std::string, std::vector<unsigned char>> m_platform_data GUARDED_BY(cs_wallet);🤖 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/wallet/wallet.h` around lines 489 - 492, Annotate the Wallet member m_platform_data with GUARDED_BY(cs_wallet), preserving its existing type and placement so thread-safety analysis enforces the lock required by its accessors.src/wallet/interfaces.cpp (1)
339-341: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffName the descriptor range constant and confirm the top-up cost.
range_endis the literal1000.AddWalletDescriptorcallsTopUp(), so each imported friendship derives and stores 1000 scripts. A wallet with many contacts pays that cost per contact in derivation time, keypool size, and rescan filter size.Define a named constant for the range, and confirm 1000 is the intended gap limit for DIP-15 friendship chains.
🤖 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/wallet/interfaces.cpp` around lines 339 - 341, In the wallet_descriptor construction within AddWalletDescriptor, replace the literal range_end value 1000 with a clearly named constant for the DIP-15 friendship-chain gap limit. Define the constant at the appropriate shared scope and verify that its value remains the intended 1000 before using it for TopUp-derived descriptors.
🤖 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 `@src/wallet/interfaces.cpp`:
- Around line 334-337: Update the Parse call in importFriendshipKeychains to
store its failure text in a local temporary variable rather than the
caller-visible error string. If parsing fails, replace error with a fixed
non-sensitive message and return false, ensuring the detailed Parse text cannot
expose the embedded xprv.
In `@src/wallet/platformseed.cpp`:
- Around line 26-53: Update the seed selection flow after iterating active
DescriptorScriptPubKeyMan instances: when preferred_id is set and no candidate
matched it, return false instead of selecting candidates.begin()->second.
Preserve the existing lowest-ID fallback only when no pinned seed ID exists.
In `@src/wallet/wallet.cpp`:
- Around line 3809-3820: Update CWallet::WritePlatformData so the value is
erased from m_platform_data only after batch.ErasePlatformData(key) succeeds;
preserve the existing failure return and leave the in-memory entry unchanged
when the database erase fails.
---
Nitpick comments:
In `@src/wallet/interfaces.cpp`:
- Around line 339-341: In the wallet_descriptor construction within
AddWalletDescriptor, replace the literal range_end value 1000 with a clearly
named constant for the DIP-15 friendship-chain gap limit. Define the constant at
the appropriate shared scope and verify that its value remains the intended 1000
before using it for TopUp-derived descriptors.
In `@src/wallet/platformkeys.cpp`:
- Around line 5-12: Add the standard <algorithm> header to both
src/wallet/platformkeys.cpp lines 5-12 and src/wallet/platformseed.cpp lines
5-16 so their std::copy calls have a direct declaration; no other changes are
required.
In `@src/wallet/wallet.h`:
- Around line 489-492: Annotate the Wallet member m_platform_data with
GUARDED_BY(cs_wallet), preserving its existing type and placement so
thread-safety analysis enforces the lock required by its accessors.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6c643ea-e52b-4ebe-8c3a-2552b2aa6c9e
📒 Files selected for processing (16)
configure.acsrc/Makefile.amsrc/Makefile.test.includesrc/interfaces/wallet.hsrc/wallet/interfaces.cppsrc/wallet/platformkeys.cppsrc/wallet/platformkeys.hsrc/wallet/platformseed.cppsrc/wallet/platformseed.hsrc/wallet/test/platformkeys_tests.cppsrc/wallet/test/walletdb_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/walletdb.htest/util/data/non-backported.txt
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Platform wallet seams are well scoped and substantially tested, but four in-scope correctness defects remain: an unavailable pinned seed falls back to another identity, friendship re-import can throw after normal address use, invalid contact keys can trigger assertions, and a failed database erase leaves memory inconsistent with disk. These issues affect the identity and recovery guarantees central to this PR and should be fixed before merge.
Source: reviewer backends gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex dash-core-commit-history); final verifier backend gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
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: not run (deferred by blocker gate)
🔴 4 blocking
🤖 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/wallet/platformseed.cpp`:
- [BLOCKING] src/wallet/platformseed.cpp:51-52: Fail when the pinned Platform seed is unavailable
When a valid `platform/seed-id` record exists but none of the active descriptor managers exposes the matching mnemonic, this falls through to the lowest-ID candidate. That contradicts the pin's documented purpose and can silently derive identity signatures, ECDH secrets, and friendship addresses from a different seed after descriptor replacement or an incomplete multi-seed restore. An unmatched pin must make seed retrieval fail; the deterministic lowest-ID fallback is valid only when no pin exists.
In `src/wallet/interfaces.cpp`:
- [BLOCKING] src/wallet/interfaces.cpp:339-351: Preserve friendship descriptor state on re-import
Each re-import recreates the matching descriptor with `range_end = 1000`, `next_index = 0`, an empty cache, and the newly supplied creation time. After index 0 is observed, `MarkUnusedAddresses()` advances `next_index` and `TopUp()` expands the existing range to 1001. A later re-import then calls `UpdateWalletDescriptor()`, whose `CanUpdateToWalletDescriptor()` check rejects the smaller range and throws through this boolean interface. Even before range expansion, replacing the descriptor can discard its progress and move its creation time forward, potentially excluding older transactions from a later rescan. Retrieve the matching descriptor under the wallet and descriptor locks, preserve its range, next index, cache, and earliest creation time, and then update it.
In `src/wallet/platformkeys.cpp`:
- [BLOCKING] src/wallet/platformkeys.cpp:98-103: Validate the parent public key before public derivation
`DerivePubKey()` is reached with externally supplied contact xpub data, but it calls `CPubKey::Derive()` without validating that the parent is a compressed public key. `CPubKey::Derive()` asserts `IsValid()` and a 33-byte compressed size, so an empty or uncompressed key aborts assertion-enabled builds instead of returning the API's documented failure result. Requiring `IsCompressed()` rejects both cases; a syntactically compressed but invalid curve point is subsequently rejected by `secp256k1_ec_pubkey_parse()`.
In `src/wallet/wallet.cpp`:
- [BLOCKING] src/wallet/wallet.cpp:3813-3815: Only erase the in-memory record after the database erase succeeds
The erase path removes the record from `m_platform_data` before checking whether `ErasePlatformData()` succeeded. If the database operation fails, the method returns false while the running process treats the record as absent and the persisted wallet still contains it; the record then reappears after restart. For `platform/seed-id`, this divergence can also change which seed subsequent Platform operations select. Commit the database erase before mutating the in-memory map, matching the ordering already used by the write path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 838d45b0a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
One blocking issue remains: a malformed platform/seed-id record is treated as no pin, allowing Platform operations to fall back to another seed instead of failing closed. The four prior blockers are fixed, and the reported identity byte-order issue does not apply to the intended caller, which constructs uint256 directly from raw Platform identifier bytes.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/wallet/platformseed.cpp`:
- [BLOCKING] src/wallet/platformseed.cpp:28-34: Fail closed when the stored seed pin is malformed
When the exact `platform/seed-id` record exists but its value is not eight bytes, the code treats it as though no pin exists and selects the lowest-ID seed. The generic Platform-data interface accepts arbitrary byte values at this reserved key, and malformed persisted state is also possible. Silently ignoring the malformed record defeats the same identity-safety invariant enforced for a valid but unavailable pin: signing, ECDH, and friendship derivation can resume under a different seed. If the reserved key is present, seed selection must fail unless it contains exactly one valid eight-byte fingerprint.
838d45b to
c88e23f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c88e23fd9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The earlier fixes for unavailable valid pins, descriptor re-import state, invalid contact public keys, and database erase ordering are present, but two identity-safety blockers remain. A malformed seed pin still falls back to another seed, and an encrypted legacy wallet exposes Platform seed-backed operations during a mixing-only unlock.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet/platformseed.cpp`:
- [BLOCKING] src/wallet/platformseed.cpp:59-63: Reject Platform operations while legacy wallets are mixing-only
An encrypted legacy wallet unlocked with `mixingonly=true` retains `vMasterKey`, while `wallet.IsLocked()` remains true to prohibit non-CoinJoin private-key operations. This branch calls `GetDecryptedHDChain()` without checking that full-unlock state, and that helper decrypts through `WithEncryptionKey()`, so `signPlatformDigest()`, `platformECDHSecret()`, and other seed-backed Platform methods remain usable during a mixing-only unlock. Descriptor wallets already reject this state because `GetMnemonicString()` checks `IsLocked(false)`, and the interface states that Platform methods fail while locked. Require a full wallet unlock before exposing the legacy HD seed.
- [BLOCKING] src/wallet/platformseed.cpp:28-34: Fail closed when the stored seed pin is malformed
(existing thread: https://github.com/dashpay/dash/pull/7581#discussion_r3769489753)
When the exact `platform/seed-id` record exists but its value is not eight bytes, this code leaves `preferred_id` unset and later selects the lowest-ID seed. The generic Platform-data interface accepts arbitrary byte values at this reserved key, so malformed client-written or persisted state is possible. Silently treating the malformed record as no pin defeats the identity-safety invariant already enforced for a valid but unavailable pin: signatures, ECDH secrets, and friendship keys can be produced from a different seed. Presence of the reserved record must make seed selection fail unless the value is exactly eight bytes.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 6ea2498, both previously verified identity-safety blockers are fixed: malformed descriptor seed pins fail closed, and the legacy seed path requires a full wallet unlock. The remaining CodeRabbit issues are either fixed at the current head or inapplicable because the descriptor parser receives only fixed syntax and a successfully derived internal extended key; no actionable in-scope findings remain.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol (Codex verifier). 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) - 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)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59a7947bab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::string strKey; | ||
| std::vector<unsigned char> vchValue; | ||
| ssKey >> strKey; | ||
| ssValue >> vchValue; |
There was a problem hiding this comment.
When a serialized platform/seed-id value is truncated or corrupt, this extraction throws and LoadWallet treats the non-key record failure as noncritical, continuing with the record absent. GetPlatformSeed then interprets that absence as an unpinned wallet and selects the lowest-ID active seed, allowing Platform signatures, ECDH, and friendship derivation under a different seed despite the pin's fail-closed invariant. Preserve an invalid pin marker or make a seed-pin deserialization failure fatal.
AGENTS.md reference: AGENTS.md:L172-L180
Useful? React with 👍 / 👎.
Split from the original platform-gui-rust commit: the generic per-wallet platform data records that accompanied this change are superseded by the wallet-seams branch (PR dashpay#7581) and are not re-applied here; only the state transition builder contract (platform/statetransitions.h) and its build wiring are kept. (cherry picked from commit 9d2590d5f914963899b73825ea86662e07af9464) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wallet-seams importFriendshipKeychains (PR dashpay#7581) takes an explicit creation_time that bounds later rescans of the imported ranged descriptor. Derive it from the contact request document's created_at timestamp: the friendship chain cannot have received funds before the request existed. Falls back to 0 (genesis) when the document carries no timestamp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
59a7947 to
c7e24bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7e24bb8ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| wallet_descriptor.next_index = current.next_index; | ||
| wallet_descriptor.creation_time = std::min(wallet_descriptor.creation_time, current.creation_time); | ||
| } | ||
| if (!m_wallet->AddWalletDescriptor(wallet_descriptor, provider, label, /*internal=*/false)) { |
There was a problem hiding this comment.
When a caller supplies a nonempty contact label, the descriptor constructed above is always ranged because it uses /*, while CWallet::AddWalletDescriptor explicitly applies labels only to non-ranged descriptors. Passing label here is therefore always a no-op even though the import reports success, leaving received friendship payments without the requested association; persist the label in a ranged-chain-compatible form or remove/reject the parameter instead of silently discarding it.
AGENTS.md reference: AGENTS.md:L172-L180
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Platform seed selection, descriptor re-import state, public derivation validation, and database erase ordering fixes are present at the exact head. However, corrupt serialized Platform records can still bypass the seed pin during wallet loading, and the new friendship import API silently ignores every supplied label because its descriptor is ranged.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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/wallet/walletdb.cpp`:
- [BLOCKING] src/wallet/walletdb.cpp:658-663: Fail closed when a serialized seed-pin record is corrupt
If the serialized value of a `platform/seed-id` record is truncated or otherwise malformed, `ssValue >> vchValue` throws and `ReadKeyValue()` returns false without adding anything to `m_platform_data`. `WalletBatch::LoadWallet()` classifies this `PLATFORM_DATA` failure as noncritical, and `CWallet::Create()` consequently opens the wallet with only a warning. `GetPlatformSeed()` then sees no pin and can select the lowest-ID seed in a multi-seed descriptor wallet, allowing signatures, ECDH secrets, and friendship keys to be derived from a different identity seed. The existing malformed-size check does not cover this path because the corrupt record never reaches the map. Treat Platform-data deserialization failure as fatal, or preserve an explicit invalid-pin state that causes seed retrieval to fail.
In `src/wallet/interfaces.cpp`:
- [SUGGESTION] src/wallet/interfaces.cpp:339-362: Remove or explicitly handle the unsupported friendship label
`importFriendshipKeychains()` accepts a `label` and passes it to `AddWalletDescriptor()`, but the imported `pkh(xprv/*)` descriptor is always ranged. `CWallet::AddWalletDescriptor()` explicitly disables labels for ranged descriptors, so every nonempty label supplied through this new API is silently discarded while the import reports success. Remove the parameter, reject nonempty labels, or persist the friendship association through a ranged-descriptor-compatible mechanism so callers are not told that an import carrying a label succeeded when no association was stored.
| } else if (strType == DBKeys::PLATFORM_DATA) { | ||
| std::string strKey; | ||
| std::vector<unsigned char> vchValue; | ||
| ssKey >> strKey; | ||
| ssValue >> vchValue; | ||
| pwallet->LoadPlatformData(strKey, vchValue); |
There was a problem hiding this comment.
🔴 Blocking: Fail closed when a serialized seed-pin record is corrupt
If the serialized value of a platform/seed-id record is truncated or otherwise malformed, ssValue >> vchValue throws and ReadKeyValue() returns false without adding anything to m_platform_data. WalletBatch::LoadWallet() classifies this PLATFORM_DATA failure as noncritical, and CWallet::Create() consequently opens the wallet with only a warning. GetPlatformSeed() then sees no pin and can select the lowest-ID seed in a multi-seed descriptor wallet, allowing signatures, ECDH secrets, and friendship keys to be derived from a different identity seed. The existing malformed-size check does not cover this path because the corrupt record never reaches the map. Treat Platform-data deserialization failure as fatal, or preserve an explicit invalid-pin state that causes seed retrieval to fail.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Fail closed when a serialized seed-pin record is corrupt no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| WalletDescriptor wallet_descriptor(std::move(parsed), /*creation_time=*/creation_time, | ||
| /*range_start=*/0, /*range_end=*/1000, | ||
| /*next_index=*/0); | ||
|
|
||
| LOCK(m_wallet->cs_wallet); | ||
| if (!m_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { | ||
| error = "DashPay contact payments require a descriptor wallet"; | ||
| return false; | ||
| } | ||
| // A friendship that is already imported matches its existing spk_man | ||
| // and is updated in place (AddWalletDescriptor). The update must keep | ||
| // the existing range, derivation progress and earliest birth time: | ||
| // TopUp() may have grown the range past the initial one (a shrinking | ||
| // update throws in CanUpdateToWalletDescriptor), and a later creation | ||
| // time could exclude old history from rescans. | ||
| if (auto* existing = m_wallet->GetDescriptorScriptPubKeyMan(wallet_descriptor)) { | ||
| LOCK(existing->cs_desc_man); | ||
| const WalletDescriptor current{existing->GetWalletDescriptor()}; | ||
| wallet_descriptor.range_start = current.range_start; | ||
| wallet_descriptor.range_end = std::max(wallet_descriptor.range_end, current.range_end); | ||
| wallet_descriptor.next_index = current.next_index; | ||
| wallet_descriptor.creation_time = std::min(wallet_descriptor.creation_time, current.creation_time); | ||
| } | ||
| if (!m_wallet->AddWalletDescriptor(wallet_descriptor, provider, label, /*internal=*/false)) { |
There was a problem hiding this comment.
🟡 Suggestion: Remove or explicitly handle the unsupported friendship label
importFriendshipKeychains() accepts a label and passes it to AddWalletDescriptor(), but the imported pkh(xprv/*) descriptor is always ranged. CWallet::AddWalletDescriptor() explicitly disables labels for ranged descriptors, so every nonempty label supplied through this new API is silently discarded while the import reports success. Remove the parameter, reject nonempty labels, or persist the friendship association through a ranged-descriptor-compatible mechanism so callers are not told that an import carrying a label succeeded when no association was stored.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Remove or explicitly handle the unsupported friendship label no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Pure BIP32/DIP-14 key-path math for the wallet's Platform key provider: DIP-9 feature-purpose paths, DIP-13 identity authentication/funding paths, DIP-15 friendship keychain paths with 256-bit non-hardened identity components, private and public (watch-only) derivation, the libsecp256k1 ECDH KDF used for DashPay contact request encryption, and a keyed seed fingerprint for pinning multi-seed wallets to one platform seed. The secp256k1 subtree is now built with the ECDH module enabled, which ComputeECDHSecret requires. Tests pin the DIP-14 test vectors (dashpay/dips dip-0014.md) through the path walker, public/private derivation consistency, ECDH symmetry and the seed fingerprint.
Adds an opaque string-keyed key/value store to the wallet database (DBKeys::PLATFORM_DATA) with write/erase, prefix queries, and a load path into CWallet::m_platform_data, exposed through interfaces::Wallet. Records persist in the wallet database and travel with backups; the wallet itself never interprets them. Tests cover write/prefix-query/erase and the ReadKeyValue load path.
…provider seams Exposes a platform key provider through interfaces::Wallet: DIP-13 identity authentication/funding pubkeys and compact signatures, ECDH secrets for DashPay contact requests, DIP-15 friendship xpubs, and a stateless contact payment-destination derivation from a stored xpub. importFriendshipKeychains imports only the wallet's OWN receiving chain as a ranged private descriptor. The contact's receiving chain is deliberately never imported: its scriptPubKeys must not be IsMine, or payments to the contact would decompose as payments-to-self. Contact payment destinations are derived statelessly from the contact's xpub instead. GetPlatformSeed picks the backing BIP39 seed deterministically for multi-seed descriptor wallets: a pinned platform/seed-id record wins, otherwise the candidate from the lowest spk_man ID; legacy wallets use their HD chain seed. Tests cover own-chain spendability (ISMINE_SPENDABLE and AvailableCoins), the contact chain staying ISMINE_NO, deterministic seed selection with the seed-id override, and seed-only-restore rederivation of auth keys, friendship xpubs, ECDH secrets and imported funds, including import idempotency.
Contact xpubs reaching DerivePubKey() are externally supplied, but CPubKey::Derive()/Derive256() assert a valid compressed parent, so an empty or uncompressed key aborted assertion-enabled builds instead of returning the documented failure. Reject non-compressed parents up front; invalid curve points are still rejected by pubkey parsing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vailable When a platform/seed-id record exists but no active descriptor manager holds the matching seed (descriptor replacement, incomplete multi-seed restore), GetPlatformSeed() silently fell back to the lowest-ID candidate, deriving identity signatures, ECDH secrets and friendship addresses from a different seed than the one the pin protects. An unmatched pin now fails seed selection; the deterministic fallback applies only to unpinned wallets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
importFriendshipKeychains() recreated the descriptor with range_end=1000, next_index=0 and the newly supplied creation time. After a payment is observed, MarkUnusedAddresses()/TopUp() grow the live descriptor past that range, so a later re-import threw from CanUpdateToWalletDescriptor() through the boolean interface, and a re-import could also reset derivation progress and move the birth time forward, excluding old history from rescans. Merge the existing descriptor's range, next index and earliest creation time before updating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rase succeeds The erase path removed the record from m_platform_data before checking ErasePlatformData(), so a failed database erase left memory and disk disagreeing until the record resurfaced on the next load. Commit the database erase first, matching the write path's ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A platform/seed-id record whose value is not exactly eight bytes was treated as if no pin existed, re-enabling the lowest-ID fallback the pin is meant to prevent. A present-but-malformed pin now fails seed selection, matching the fail-closed behaviour for a valid but unmatched pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… seed A mixing-only unlock keeps vMasterKey populated while CWallet::IsLocked() reports locked for non-CoinJoin operations, and GetDecryptedHDChain() decrypts through WithEncryptionKey() regardless, so seed-backed Platform methods (signPlatformDigest, platformECDHSecret, friendship derivation) stayed usable on encrypted legacy wallets unlocked for mixing only. Descriptor wallets already reject this state inside GetMnemonicString(); the legacy branch now requires a full unlock too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a cross-implementation vector for the DIP-15 friendship path: the same seed, path and asymmetric identity ids derived through rust-dashcore's key-wallet crate (the backend behind dashwallet-ios/android via rs-platform-wallet, ChildNumber::Normal256 at rev 36b49cb7f9c0) must yield byte-identical keys. The asymmetric ids make any byte-order reversal between raw Platform identifier bytes and the DIP-14 256-bit index change the result, which the existing symmetric fixtures could not detect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GUARDED_BY(cs_wallet) on m_platform_data to match its accessors' EXCLUSIVE_LOCKS_REQUIRED declarations, and explicit <algorithm> includes where std::copy was relying on transitive headers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c7e24bb to
b7c04f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7c04f809d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) continue; | ||
| SecureVector seed; | ||
| CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed); |
There was a problem hiding this comment.
For an unencrypted descriptor wallet whose active descriptor was imported from an xprv, AddDescriptorKeyWithDB inserts an entry with the default empty mnemonic, so GetMnemonicString reports success here. CMnemonic::ToSeed then derives the same publicly reproducible seed from "" for every such wallet, causing Platform pubkeys, signatures, and ECDH results to use unrelated keys instead of failing for a wallet without a recoverable mnemonic. Reject an empty or invalid mnemonic before calling ToSeed.
AGENTS.md reference: AGENTS.md:L172-L180
Useful? React with 👍 / 👎.
| if (!getPlatformSeed(seed)) return false; | ||
| const auto coin_type{static_cast<uint32_t>(Params().ExtCoinType())}; | ||
| platformkeys::Path path; | ||
| switch (type) { |
There was a problem hiding this comment.
If an API boundary or caller casts an unknown integer to PlatformKeyType, none of these cases populates path; DeriveExtKey accepts the resulting empty path and returns the BIP32 master key. Consequently, getPlatformPubKey and signPlatformDigest expose or use the wallet master key rather than rejecting the unsupported key class. Add a default failure path to the switch.
AGENTS.md reference: AGENTS.md:L172-L180
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Platform wallet seams are well scoped and extensively tested, but three identity-safety blockers remain: corrupt serialized Platform records can bypass a seed pin, active xprv descriptors can derive from a publicly reproducible empty mnemonic, and unknown key-type values select the wallet master key. The friendship import API also silently discards every supplied label because its descriptor is ranged.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet/platformseed.cpp`:
- [BLOCKING] src/wallet/platformseed.cpp:46-49: Reject active descriptors that have no mnemonic
An active descriptor imported from an xprv can contain a private key but no recovery mnemonic. `AddWalletDescriptor()` calls `AddDescriptorKey()` with its default empty mnemonic, and `AddDescriptorKeyWithDB()` inserts that empty value into `m_mnemonics`. `GetMnemonicString()` then reports success because the map contains an entry, even though the returned mnemonic is empty. `CMnemonic::ToSeed("", "", ...)` still produces a 64-byte seed, so the subsequent `seed.empty()` check does not reject it. Platform operations can consequently derive the same publicly reproducible empty-mnemonic seed instead of failing for a wallet without a BIP39 seed. Validate the mnemonic before converting it.
In `src/wallet/interfaces.cpp`:
- [BLOCKING] src/wallet/interfaces.cpp:265-279: Reject unknown Platform key types before derivation
The switch has no default failure case. If an API adapter or caller passes a value outside `PlatformKeyType`, none of the cases populates `path`; `DeriveExtKey()` accepts the empty path and returns the BIP32 master key. `getPlatformPubKey()` would then expose the wallet master public key, while `signPlatformDigest()` would sign with the master private key rather than rejecting the unsupported key class. Return false for every unknown enum value.
- [SUGGESTION] src/wallet/interfaces.cpp:362: Remove or explicitly handle the unsupported friendship label
(existing thread: https://github.com/dashpay/dash/pull/7581#discussion_r3772404838)
`importFriendshipKeychains()` accepts a `label` and passes it to `AddWalletDescriptor()`, but the imported `pkh(xprv/*)` descriptor is always ranged. `CWallet::AddWalletDescriptor()` explicitly applies labels only to non-ranged descriptors, and the existing `importdescriptors` RPC similarly rejects labels for ranged descriptors. Every nonempty label supplied through this new API is therefore silently discarded while the import reports success. Remove the parameter, reject nonempty labels, or persist the friendship association through a ranged-descriptor-compatible mechanism.
In `src/wallet/walletdb.cpp`:
- [BLOCKING] src/wallet/walletdb.cpp:911: Fail closed when a serialized seed-pin record is corrupt
(existing thread: https://github.com/dashpay/dash/pull/7581#discussion_r3772404834)
If the serialized value of a `platform/seed-id` record is truncated or otherwise malformed, `ssValue >> vchValue` throws and `ReadKeyValue()` returns false without adding anything to `m_platform_data`. `WalletBatch::LoadWallet()` classifies this `PLATFORM_DATA` failure as noncritical, so `CWallet::Create()` opens the wallet with only a warning. `GetPlatformSeed()` then sees no pin and can select the lowest-ID seed in a multi-seed descriptor wallet, allowing signatures, ECDH secrets, and friendship keys to be derived from a different identity seed. The malformed-size check in `GetPlatformSeed()` cannot cover this path because the corrupt record never reaches the map. Treat unreadable Platform records as fatal, or preserve an explicit invalid-pin state that makes seed retrieval fail.
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) continue; | ||
| SecureVector seed; | ||
| CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed); | ||
| if (seed.empty()) continue; |
There was a problem hiding this comment.
🔴 Blocking: Reject active descriptors that have no mnemonic
An active descriptor imported from an xprv can contain a private key but no recovery mnemonic. AddWalletDescriptor() calls AddDescriptorKey() with its default empty mnemonic, and AddDescriptorKeyWithDB() inserts that empty value into m_mnemonics. GetMnemonicString() then reports success because the map contains an entry, even though the returned mnemonic is empty. CMnemonic::ToSeed("", "", ...) still produces a 64-byte seed, so the subsequent seed.empty() check does not reject it. Platform operations can consequently derive the same publicly reproducible empty-mnemonic seed instead of failing for a wallet without a BIP39 seed. Validate the mnemonic before converting it.
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) continue; | |
| SecureVector seed; | |
| CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed); | |
| if (seed.empty()) continue; | |
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase) || !CMnemonic::Check(mnemonic)) continue; | |
| SecureVector seed; | |
| CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed); |
source: ['codex']
| switch (type) { | ||
| case PlatformKeyType::IdentityAuth: | ||
| path = platformkeys::IdentityAuthKeyPath(coin_type, account, index); | ||
| break; | ||
| case PlatformKeyType::RegistrationFunding: | ||
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_REGISTRATION_FUNDING, index); | ||
| break; | ||
| case PlatformKeyType::TopupFunding: | ||
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_TOPUP_FUNDING, index); | ||
| break; | ||
| case PlatformKeyType::InvitationFunding: | ||
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_INVITATION_FUNDING, index); | ||
| break; | ||
| } | ||
| return platformkeys::DeriveExtKey(seed, path, out); |
There was a problem hiding this comment.
🔴 Blocking: Reject unknown Platform key types before derivation
The switch has no default failure case. If an API adapter or caller passes a value outside PlatformKeyType, none of the cases populates path; DeriveExtKey() accepts the empty path and returns the BIP32 master key. getPlatformPubKey() would then expose the wallet master public key, while signPlatformDigest() would sign with the master private key rather than rejecting the unsupported key class. Return false for every unknown enum value.
| switch (type) { | |
| case PlatformKeyType::IdentityAuth: | |
| path = platformkeys::IdentityAuthKeyPath(coin_type, account, index); | |
| break; | |
| case PlatformKeyType::RegistrationFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_REGISTRATION_FUNDING, index); | |
| break; | |
| case PlatformKeyType::TopupFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_TOPUP_FUNDING, index); | |
| break; | |
| case PlatformKeyType::InvitationFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_INVITATION_FUNDING, index); | |
| break; | |
| } | |
| return platformkeys::DeriveExtKey(seed, path, out); | |
| switch (type) { | |
| case PlatformKeyType::IdentityAuth: | |
| path = platformkeys::IdentityAuthKeyPath(coin_type, account, index); | |
| break; | |
| case PlatformKeyType::RegistrationFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_REGISTRATION_FUNDING, index); | |
| break; | |
| case PlatformKeyType::TopupFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_TOPUP_FUNDING, index); | |
| break; | |
| case PlatformKeyType::InvitationFunding: | |
| path = platformkeys::IdentityFundingPath(coin_type, platformkeys::IDENTITY_INVITATION_FUNDING, index); | |
| break; | |
| default: | |
| return false; | |
| } | |
| return platformkeys::DeriveExtKey(seed, path, out); |
source: ['codex']
2 participants
Issue being fixed or feature implemented
Part of the Dash Platform GUI PR train tracked in #7512 (the tracking issue's body still describes an older architecture; the current reference implementation is PastaPastaPasta#67). This PR extracts the wallet-layer Platform seams: pure C++ wallet code with no Rust/FFI dependency, so it can be reviewed and merged in parallel with the build-system PR #7580.
Builds on the DIP-14
Derive256primitives merged in #7511.What was done?
Three seams, one commit each:
1. Platform key derivation helpers (
src/wallet/platformkeys.{h,cpp})Pure BIP32/DIP-14 path math, independent of Platform documents/contracts/network:
Secp256k1ECDHAgreementused for DashPay contact request encryption. The secp256k1 subtree is now configured with--enable-module-ecdh(previously disabled).SeedFingerprint) used to pin multi-seed wallets to one platform seed.2. Generic per-wallet Platform data records (walletdb)
A string-keyed, opaque key/value store in the wallet database (
DBKeys::PLATFORM_DATA):WalletBatch::{Write,Erase}PlatformData,CWallet::{Load,Write,Get}PlatformData(prefix queries), theReadKeyValueload path, andinterfaces::Wallet::{write,get}PlatformData. Records persist in the wallet database and travel with backups.These records are opaque to the wallet by design. The wallet stores and returns bytes; interpretation lives entirely with the Platform client layers. This is deliberate pending the seed-only-recovery design work: everything that must survive a seed-only restore is derived from the seed (see the recovery tests below), and the records only cache/pin state (e.g.
platform/seed-id) rather than being load-bearing for fund recovery.3. DIP-15 friendship keychain import + platform key provider (
interfaces::Wallet)getPlatformPubKey/signPlatformDigest/platformECDHSecret: DIP-13 identity auth and funding keys served on demand from the HD seed; raw private keys never cross the interface.getFriendshipXpub: the DIP-15 friendship extended pubkey for an (account, userA, userB) chain.importFriendshipKeychains: imports the wallet's own receiving chain for a friendship as a ranged private descriptor (pkh(xprv/*)), idempotently (re-imports update in place).getFriendshipPaymentDestination: derives contact payment destinations statelessly from the contact's stored xpub, without touching any wallet keypool.wallet/platformseed.{h,cpp}: deterministic choice of the backing BIP39 seed for multi-seed descriptor wallets — a pinnedplatform/seed-idrecord wins, otherwise the candidate from the lowest spk_man ID; legacy wallets use their HD chain seed.Design invariant (please review against it): the contact's own receiving chain is deliberately never imported. If its scriptPubKeys became
IsMine, payments to the contact would decompose as payments-to-self and the contact's outputs would be counted as our own coins. Payment destinations for a contact are instead derived statelessly from their xpub.friendship_contact_chain_is_not_ourspins this (ISMINE_NOfor both the reversed-id chain and a genuinely foreign contact xpub).Adaptations relative to the reference branch
--enable-platform-gui. That flag does not exist ondevelop, so the extracted code compiles and is tested unconditionally (like feat: add DIP-14 256-bit child key derivation (Derive256) #7511). TheENABLE_PLATFORM_GUIifdefs and their#elsestubs were removed, and the secp256k1 ECDH module is enabled unconditionally inconfigure.ac.createAssetLockTransaction),startRescanFromHeight,wallet/rpc/platform.cpp, and everything Qt/GUI or Rust/FFI.dip14_tests: that suite pins the rawCKey::Derive256primitives, whileplatformkeys_testspins the same vectors through the newPath/DeriveExtKeywalker (mixed 31-bit/256-bit paths). The duplication is deliberate.test/util/data/non-backported.txtso Dash-specific lint (cppcheck, clang-format-diff) covers them.How Has This Been Tested?
Built with autotools on macOS (aarch64, depends prefix) from a clean tree; every commit builds on its own.
New/extended unit tests, all passing:
platformkeys_tests(12 cases): DIP-14 vectors 1-4 from dashpay/dips dip-0014.md through the path walker; public/private derivation consistency incl. hardened-step rejection; ECDH symmetry; seed fingerprint stability; own friendship chainISMINE_SPENDABLEwith coins visible toAvailableCoins; contact chainISMINE_NO; deterministic multi-seed selection withplatform/seed-idoverride; seed-only-restore rederivation of auth keys, friendship xpubs/destinations, ECDH secrets and compact signatures; import-after-restore making pre-loss payments spendable; import idempotency (no spk_man duplication).walletdb_tests: platform data record write/prefix-query/erase and theReadKeyValueload path.Also run locally:
dip14_tests(sanity anchor for #7511 interplay) pluswallet_tests,scriptpubkeyman_tests,ismine_tests,spend_tests,availablecoins_tests,coinselector_tests,descriptor_tests— all green.test/lint/all-lint.pypasses.Breaking Changes
None. New wallet records are additive and ignored-by-absence; no existing serialization changes. Enabling the secp256k1 ECDH module only adds symbols to the static subtree library.
Checklist: