feat(wallet): add Platform key provider, data records and DIP-15 friendship keychain seams - #7581
Conversation
|
✅ Final review complete — no blockers (commit b2a3c40) |
![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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. WalkthroughThe wallet 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, recovery, restoration, ownership, ECDH, idempotency, and database behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This PR adds wallet-layer Platform key derivation, persistent data records, and friendship keychain support with reported unit-test and lint coverage; no actionable merge-blocking risk remains beyond normal checks and review. 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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
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".
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".
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.
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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35e69369dc
ℹ️ 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 four findings from the previous review are fixed at the exact head, and the earlier CodeRabbit issues are either fixed or inapplicable. Two blocking issues remain: corruption of any non-seed Platform cache record now prevents the wallet from opening, and a nonempty but invalid stored descriptor mnemonic can still derive an unrelated Platform seed.
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
🤖 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:916-919: Limit fatal Platform record corruption to the seed pin
This classifies every malformed `PLATFORM_DATA` record as fatal wallet corruption. The new store is explicitly generic and opaque, and records other than `platform/seed-id` are non-load-bearing caches or metadata. A truncated cached flow or identity record therefore makes `CWallet::Create()` refuse to open an otherwise usable wallet, contrary to the surrounding loader policy of tolerating damaged non-key records. Only a failure involving the exact `platform/seed-id` key needs to be fatal to preserve seed identity. `ReadKeyValue()` successfully extracts the Platform key before deserializing the value at lines 659-662, so the load state can retain whether the failed record was the reserved seed pin and classify other Platform record failures as noncritical.
In `src/wallet/platformseed.cpp`:
- [BLOCKING] src/wallet/platformseed.cpp:46-52: Validate descriptor mnemonics before deriving the Platform seed
Skipping an empty mnemonic fixes raw-xprv descriptors, but it does not validate a nonempty mnemonic loaded from the wallet database. `ReadKeyValue()` stores any nonempty serialized mnemonic without calling `CMnemonic::Check()`, and the descriptor-key integrity hash covers only the public/private key pair, not the appended mnemonic bytes. A damaged mnemonic that remains deserializable and nonempty is therefore accepted by `CMnemonic::ToSeed()`, which derives a different seed from arbitrary text. In an unpinned wallet, Platform public keys, signatures, ECDH secrets, and friendship paths can then be produced under that unrelated seed. Legitimate descriptor mnemonics enter through creation paths that already require a valid BIP39 phrase, so invalid phrases should be skipped before conversion.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has conflicts, please rebase. |
The PlatformKeyType switch had no failure path for values outside the enum: the path stayed empty and DeriveExtKey() returned the BIP32 master key, so getPlatformPubKey() could expose the master public key and signPlatformDigest() could sign with the master private key. An empty path now fails instead; the switch stays default-free so -Wswitch keeps flagging new enum values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A PLATFORM_DATA record that fails to deserialize was classified as a noncritical load error, so the wallet opened with only a warning and without the record. For platform/seed-id that silently unpins the platform seed: a multi-seed wallet would fall back to the lowest-ID seed and derive signatures, ECDH secrets and friendship keys under a different identity. The in-memory malformed-pin check cannot cover this because the corrupt record never reaches the map; fail the load as corrupt instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
importFriendshipKeychains() forwarded its label to AddWalletDescriptor(), but labels only apply to non-ranged descriptors and the friendship descriptor is always ranged, so every supplied label was silently discarded while the import reported success. Remove the parameter; a friendship-to-label association can arrive later via a mechanism that works for ranged descriptors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact head fixes both previously verified blockers: Platform record corruption is fatal only when the reserved seed pin is or could be affected, and descriptor mnemonics must pass BIP39 validation before seed derivation. No in-scope code defect remains; the only retained suggestion is to fold the long in-PR correction sequence into the three feature commits so the merged history does not preserve known-incomplete intermediate states.
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 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)
🟡 1 suggestion(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 `<commit:1474489>`:
- [SUGGESTION] <commit:1474489>:1: Fold the in-PR correction train into the feature commits
Commits 1474489, 47aefb4, f64d46c, 293a144, 4863f44, ef5fad6, b7c04f8, 7ac4cdd, 9ceadf0, f7788bf, 35e6936, fef7914, and d4a5a22 all correct, harden, or clean up implementations introduced by the first three commits in this unshipped stack. Keeping them separate preserves known-incomplete intermediate behavior; for example, f7788bf makes every malformed Platform record fatal before fef7914 narrows that policy to the seed pin. Fold the parent-public-key fix and relevant include cleanup into 150d0a9, the Platform-data erase and locking changes into f908fa0, and the seed-selection, provider, friendship-import, API, and corruption-policy corrections into 2b11d77. Commit 9d1015e adds independent cross-implementation coverage and can remain separate.
d4a5a22 to
3d684be
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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>
Only a malformed reserved platform/seed-id record is fatal wallet corruption: silently dropping it could unpin the Platform seed and let a multi-seed wallet sign under another identity. All other Platform data records are opaque cache/metadata, so a damaged one now follows the wallet's noncritical-record policy instead of preventing the wallet from opening. The reserved record key moves to a shared constant, and walletdb_tests pins both sides of the policy.
GetPlatformSeed accepted any nonempty stored mnemonic, but CMnemonic::ToSeed hashes arbitrary strings, so a corrupt mnemonic record would silently derive an unrelated Platform key universe. Require the phrase to pass BIP39 validation before it can become a seed candidate; an invalid phrase is skipped like an unreadable one, so a pinned wallet fails closed. platformkeys_tests covers both the rejected invalid phrase and the unchanged valid-mnemonic path.
3d684be to
6a92a1c
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 6a92a1c, the previously reported seed-pin, mnemonic-validation, descriptor re-import, public-key validation, and database consistency defects are fixed. One blocking interoperability defect remains: registration and unbound top-up funding keys harden their final DIP-13 indices, producing different keys from the specified non-hardened paths; the prior commit-history cleanup suggestion also remains valid.
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)
🔴 1 blocking | 🟡 1 suggestion(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/wallet/platformkeys.cpp`:
- [BLOCKING] src/wallet/platformkeys.cpp:32-46: Keep registration and top-up funding indices non-hardened
`IdentityFundingPath()` hardens the final index for every funding subtype. DIP-13 defines registration funding as `m/9'/coin'/5'/1'/identity_index` and unbound top-up funding as `m/9'/coin'/5'/2'/funding_index`, with non-hardened final components; only invitation funding uses a hardened final index. The `TopupFunding` interface exposes only one index and ignores `account`, so it represents the unbound form. The current implementation therefore derives incompatible registration and top-up keys, preventing conforming wallet implementations from recovering or spending outputs created through this provider. Use normal final indices for registration and unbound top-up funding, retain the hardened invitation index, and add cross-implementation vectors for all three paths.
In `<commit:a67dbc2>`:
- [SUGGESTION] <commit:a67dbc2>:1: Fold the in-PR correction train into the feature commits
Commits a67dbc2, d583a42, 2cd5bcd, 005459f, 31637d7, 3a430b5, b78f3cf, 80a46ec, 3f954b4, b1da3d4, d5e41f4, d58facb, and 6a92a1c all correct, harden, or clean up implementations introduced by the first three commits in this unshipped stack. Keeping them separate preserves known-incomplete intermediate behavior, including the temporary overcorrection where b1da3d4 makes every malformed Platform record fatal before d58facb narrows that policy to the seed pin. Fold the parent-public-key validation and relevant include cleanup into 3b8fc64, the Platform-data erase ordering and locking annotation into c6fe429, and the seed-selection, provider, friendship-import, API, mnemonic-validation, and corruption-policy corrections into 01c8a8a. The independent cross-implementation test commit 47c0751 can remain separate.
| Path IdentityFundingPath(uint32_t coin_type, uint32_t subfeature, uint32_t index) | ||
| { | ||
| // dashj DerivationPathFactory.blockchainIdentity{Registration,Topup}Funding- | ||
| // DerivationPath() / identityInvitationFundingDerivationPath(), plus the | ||
| // hardened address index appended by AuthenticationKeyChain: | ||
| // m/9'/coin'/5'/{1,2,3}'/index' | ||
| assert(subfeature == IDENTITY_REGISTRATION_FUNDING || subfeature == IDENTITY_TOPUP_FUNDING || | ||
| subfeature == IDENTITY_INVITATION_FUNDING); | ||
| return { | ||
| PathElement::Hardened(FEATURE_PURPOSE), | ||
| PathElement::Hardened(coin_type), | ||
| PathElement::Hardened(FEATURE_IDENTITIES), | ||
| PathElement::Hardened(subfeature), | ||
| PathElement::Hardened(index), | ||
| }; |
There was a problem hiding this comment.
🔴 Blocking: Keep registration and top-up funding indices non-hardened
IdentityFundingPath() hardens the final index for every funding subtype. DIP-13 defines registration funding as m/9'/coin'/5'/1'/identity_index and unbound top-up funding as m/9'/coin'/5'/2'/funding_index, with non-hardened final components; only invitation funding uses a hardened final index. The TopupFunding interface exposes only one index and ignores account, so it represents the unbound form. The current implementation therefore derives incompatible registration and top-up keys, preventing conforming wallet implementations from recovering or spending outputs created through this provider. Use normal final indices for registration and unbound top-up funding, retain the hardened invitation index, and add cross-implementation vectors for all three paths.
| Path IdentityFundingPath(uint32_t coin_type, uint32_t subfeature, uint32_t index) | |
| { | |
| // dashj DerivationPathFactory.blockchainIdentity{Registration,Topup}Funding- | |
| // DerivationPath() / identityInvitationFundingDerivationPath(), plus the | |
| // hardened address index appended by AuthenticationKeyChain: | |
| // m/9'/coin'/5'/{1,2,3}'/index' | |
| assert(subfeature == IDENTITY_REGISTRATION_FUNDING || subfeature == IDENTITY_TOPUP_FUNDING || | |
| subfeature == IDENTITY_INVITATION_FUNDING); | |
| return { | |
| PathElement::Hardened(FEATURE_PURPOSE), | |
| PathElement::Hardened(coin_type), | |
| PathElement::Hardened(FEATURE_IDENTITIES), | |
| PathElement::Hardened(subfeature), | |
| PathElement::Hardened(index), | |
| }; | |
| Path IdentityFundingPath(uint32_t coin_type, uint32_t subfeature, uint32_t index) | |
| { | |
| assert(subfeature == IDENTITY_REGISTRATION_FUNDING || subfeature == IDENTITY_TOPUP_FUNDING || | |
| subfeature == IDENTITY_INVITATION_FUNDING); | |
| const PathElement funding_index{subfeature == IDENTITY_INVITATION_FUNDING | |
| ? PathElement::Hardened(index) | |
| : PathElement::Normal(index)}; | |
| return { | |
| PathElement::Hardened(FEATURE_PURPOSE), | |
| PathElement::Hardened(coin_type), | |
| PathElement::Hardened(FEATURE_IDENTITIES), | |
| PathElement::Hardened(subfeature), | |
| funding_index, | |
| }; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Fixed in 81ba952. The positional enum API is replaced by typed requests, and PlatformKeyPath() now derives registration and unbound top-up final indices normally while keeping invitation indices hardened. dip13_funding_vectors adds independent BIP32 key/chaincode vectors for all three path forms.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved in 81ba952 — Keep registration and top-up funding indices non-hardened 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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 449da14, three blocking issues remain: registration and unbound top-up paths use incompatible hardened indices, encrypted mnemonic candidates are not authenticated against their descriptor master keys, and a syntactically valid mutation of the serialized seed-pin key can silently remove the pin. The long correction sequence should also be folded into the original feature commits 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)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
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:49-65: Authenticate mnemonic candidates against their descriptor master keys
`CMnemonic::Check()` validates the words and checksum but does not establish that the mnemonic plus passphrase derives the master key stored by this descriptor. This remains exploitable through encrypted descriptor records: `AddCryptedKey()` loads the encrypted key and encrypted mnemonic fields independently, and `GetMnemonicString()` decrypts the mnemonic and passphrase without comparing their derived master public key with the stored public key. If those fields decrypt cleanly to a valid mnemonic but an altered passphrase or otherwise mismatched pair, this loop accepts an unrelated Platform seed. An unpinned wallet can then sign, perform ECDH, and derive friendship addresses under the wrong identity; a pinned wallet becomes unnecessarily unavailable. Derive the BIP32 master key from each candidate and require its public key to match the descriptor manager's stored master public key before accepting the candidate.
In `src/wallet/walletdb.cpp`:
- [BLOCKING] src/wallet/walletdb.cpp:663-673: Preserve the pin when its serialized record key is damaged
The fail-closed load policy works only while the serialized record still identifies itself as `PLATFORM_DATA` with the exact inner key `platform/seed-id`. A syntactically valid mutation of that inner key, such as the same-length `platform/seed-ie`, sets `platform_seed_pin_corrupt` to false and loads the value as ordinary opaque Platform data. Mutation of the outer type similarly bypasses this branch. `GetPlatformSeed()` then sees no pin and selects the lowest-ID candidate, silently changing the Platform identity. Persist an independently checked indication that a seed pin is expected, or integrity-bind the critical pin state so a missing or renamed record can be distinguished from an explicit atomic pin erase. Add coverage that mutates the serialized key, not only the value.
In `<commit:a67dbc2>`:
- [SUGGESTION] <commit:a67dbc2>:1: Fold the in-PR correction train into the feature commits
Commits a67dbc2, d583a42, 2cd5bcd, 005459f, 31637d7, 3a430b5, b78f3cf, 80a46ec, 3f954b4, b1da3d4, d5e41f4, d58facb, 6a92a1c, and 449da14 all correct, harden, clean up, or remove implementations introduced by the first three commits in this unshipped stack. Keeping them separate preserves known-incomplete intermediate behavior: b1da3d4 temporarily makes every malformed Platform record fatal before d58facb narrows that policy, while 3a430b5 hardens a legacy-wallet seed branch that 449da14 later deletes. Fold the parent-public-key validation and relevant include cleanup into 3b8fc64, the Platform-data erase ordering and locking annotation into c6fe429, and the seed-selection, descriptor-only provider, friendship-import, API, mnemonic-validation, and corruption-policy corrections into 01c8a8a. The independent cross-implementation coverage in 47c0751 can remain separate.
In `src/wallet/platformkeys.cpp`:
- [BLOCKING] src/wallet/platformkeys.cpp:32-46: Keep registration and top-up funding indices non-hardened
(existing thread: https://github.com/dashpay/dash/pull/7581#discussion_r3800248053)
`IdentityFundingPath()` hardens the final index for every funding subtype. DIP-13 defines registration funding as `m/9'/coin'/5'/1'/identity_index` and unbound top-up funding as `m/9'/coin'/5'/2'/funding_index`, with non-hardened final components; only invitation funding uses a hardened final index. The `TopupFunding` interface exposes only one index and ignores `account`, so it represents the unbound form. The current implementation therefore derives incompatible registration and top-up keys, preventing conforming wallet implementations from recovering or spending outputs created through this provider. Use normal final indices for registration and unbound top-up funding, retain the hardened invitation index, update the path documentation, and add cross-implementation vectors for all three paths.
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) continue; | ||
| // Descriptors imported from a raw xprv store an empty mnemonic; | ||
| // ToSeed("") would yield the publicly reproducible empty-mnemonic | ||
| // seed rather than a wallet secret. | ||
| if (mnemonic.empty()) continue; | ||
| // ToSeed() hashes any string, so a corrupt stored mnemonic would | ||
| // silently derive an unrelated Platform key universe; only a | ||
| // phrase that passes BIP39 validation may become a candidate. | ||
| if (!CMnemonic::Check(mnemonic)) continue; | ||
| SecureVector seed; | ||
| CMnemonic::ToSeed(mnemonic, mnemonic_passphrase, seed); | ||
| if (seed.empty()) continue; | ||
| if (preferred_id && SeedFingerprint(seed) == *preferred_id) { | ||
| seed_out = std::move(seed); | ||
| return true; | ||
| } | ||
| candidates.emplace(desc_spk_man->GetID(), std::move(seed)); |
There was a problem hiding this comment.
🔴 Blocking: Authenticate mnemonic candidates against their descriptor master keys
CMnemonic::Check() validates the words and checksum but does not establish that the mnemonic plus passphrase derives the master key stored by this descriptor. This remains exploitable through encrypted descriptor records: AddCryptedKey() loads the encrypted key and encrypted mnemonic fields independently, and GetMnemonicString() decrypts the mnemonic and passphrase without comparing their derived master public key with the stored public key. If those fields decrypt cleanly to a valid mnemonic but an altered passphrase or otherwise mismatched pair, this loop accepts an unrelated Platform seed. An unpinned wallet can then sign, perform ECDH, and derive friendship addresses under the wrong identity; a pinned wallet becomes unnecessarily unavailable. Derive the BIP32 master key from each candidate and require its public key to match the descriptor manager's stored master public key before accepting the candidate.
source: ['codex']
There was a problem hiding this comment.
Fixed in 81ba952 as part of removing raw seed extraction. The mnemonic-owning DescriptorScriptPubKeyMan now regenerates the BIP39 master and compares its full public key with the manager source before deriving any requested Platform child. Invalid, mismatched, ambiguous, xprv-only, and empty-path sources fail closed.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved in 81ba952 — Authenticate mnemonic candidates against their descriptor master keys 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.
| } else if (strType == DBKeys::PLATFORM_DATA) { | ||
| std::string strKey; | ||
| std::vector<unsigned char> vchValue; | ||
| // Until the record key proves otherwise, assume a failure hits the | ||
| // seed pin: an unreadable key could belong to the pin record itself. | ||
| wss.platform_seed_pin_corrupt = true; | ||
| ssKey >> strKey; | ||
| wss.platform_seed_pin_corrupt = (strKey == PLATFORM_SEED_ID_RECORD); | ||
| ssValue >> vchValue; | ||
| wss.platform_seed_pin_corrupt = false; | ||
| pwallet->LoadPlatformData(strKey, vchValue); |
There was a problem hiding this comment.
🔴 Blocking: Preserve the pin when its serialized record key is damaged
The fail-closed load policy works only while the serialized record still identifies itself as PLATFORM_DATA with the exact inner key platform/seed-id. A syntactically valid mutation of that inner key, such as the same-length platform/seed-ie, sets platform_seed_pin_corrupt to false and loads the value as ordinary opaque Platform data. Mutation of the outer type similarly bypasses this branch. GetPlatformSeed() then sees no pin and selects the lowest-ID candidate, silently changing the Platform identity. Persist an independently checked indication that a seed pin is expected, or integrity-bind the critical pin state so a missing or renamed record can be distinguished from an explicit atomic pin erase. Add coverage that mutates the serialized key, not only the value.
source: ['codex']
There was a problem hiding this comment.
Resolved structurally in 81ba952. The persisted platform/seed-id selection mechanism and its special database corruption policy are removed. CWallet now accepts Platform operations only when all active mnemonic-bearing descriptor managers identify the same source; multiple sources return AMBIGUOUS_SOURCE, so there is no pin record whose loss or renamed key can silently redirect derivation.
🤖 Posted autonomously by Codex on behalf of pasta.
There was a problem hiding this comment.
Resolved in 81ba952 — Preserve the pin when its serialized record key is damaged 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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
No blocking code defect remains at exact head 939ee20. The mnemonic-to-descriptor authentication fix still needs direct regression coverage, and the long sequence of in-PR corrections should be folded into the original feature commits before merge.
Source: reviewers gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex dash-core-commit-history); final verifier 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),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)
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/wallet/scriptpubkeyman.cpp`:
- [SUGGESTION] src/wallet/scriptpubkeyman.cpp:2777-2779: Exercise the mnemonic/source-key mismatch check
Add a regression test that reaches this identity check with a BIP39-valid mnemonic and passphrase whose derived master public key differs from the descriptor manager's stored source public key. The existing invalid-mnemonic test exits at `CMnemonic::Check()`, the xprv-only test has no mnemonic source, and the ambiguity test compares separate managers, so none would fail if this comparison were removed. Construct the mismatched encrypted descriptor state and assert that Platform derivation returns `INVALID_SOURCE`; this pins the check that prevents signing, ECDH, and friendship derivation under an unrelated root.
In `<commit:a67dbc2>`:
- [SUGGESTION] <commit:a67dbc2>:1: Fold the in-PR correction train into the feature commits
Commits a67dbc2, d583a42, 2cd5bcd, 005459f, 31637d7, 3a430b5, b78f3cf, 80a46ec, 3f954b4, b1da3d4, d5e41f4, d58facb, 6a92a1c, 449da14, 81ba952, and 939ee20 all amend, harden, replace, or remove implementations introduced by the first three feature commits. The sequence preserves superseded states, including the temporary type-wide fatal Platform-record policy, the later-removed seed-pin/provider architecture, and the friendship descriptor construction corrected immediately after the key-manager refactor. Fold the final derivation helpers into 3b8fc64, the Platform-data ordering and locking changes into c6fe429, and the final key-manager provider, friendship import, validation, API, and corruption behavior into 01c8a8a. The independent cross-implementation coverage in 47c0751 can remain separate.
| if (!master_valid || identifier.size() != 1 + master_pubkey.size() || | ||
| !std::equal(identifier.begin() + 1, identifier.end(), master_pubkey.begin())) { | ||
| return PlatformKeyStatus::INVALID_SOURCE; |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the mnemonic/source-key mismatch check
Add a regression test that reaches this identity check with a BIP39-valid mnemonic and passphrase whose derived master public key differs from the descriptor manager's stored source public key. The existing invalid-mnemonic test exits at CMnemonic::Check(), the xprv-only test has no mnemonic source, and the ambiguity test compares separate managers, so none would fail if this comparison were removed. Construct the mismatched encrypted descriptor state and assert that Platform derivation returns INVALID_SOURCE; this pins the check that prevents signing, ECDH, and friendship derivation under an unrelated root.
source: ['codex']
There was a problem hiding this comment.
Resolved in b2a3c40 — Exercise the mnemonic/source-key mismatch check 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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 b2a3c40, no blocking code defect remains: descriptor-root Platform derivation supersedes the prior mnemonic/source comparison, and the Platform-data erase ordering is fixed. The remaining in-scope suggestion is to fold the 17 corrective commits into the original feature commits so the permanent history does not retain superseded wallet architectures.
Source: reviewers gpt-5.6-sol (Codex general) and gpt-5.6-sol (Codex dash-core-commit-history); final verifier 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),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)
🟡 1 suggestion(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 `<commit:a67dbc2>`:
- [SUGGESTION] <commit:a67dbc2>:1: Fold the in-PR correction train into the feature commits
Commits a67dbc2, d583a42, 2cd5bcd, 005459f, 31637d7, 3a430b5, b78f3cf, 80a46ec, 3f954b4, b1da3d4, d5e41f4, d58facb, 6a92a1c, 449da14, 81ba952, 939ee20, and b2a3c40 amend, harden, replace, or remove implementations introduced by the first three feature commits. Merging this sequence as-is preserves superseded states, including the temporary type-wide fatal Platform-record policy, the removed seed-pin and mnemonic-provider architecture, friendship xprv serialization corrected after the key-manager refactor, and the non-root source model replaced by descriptor-root derivation. Fold the final derivation and descriptor-root helpers into 3b8fc64, the Platform-data ordering and locking changes into c6fe429, and the final provider, friendship import, validation, API, and corruption behavior into 01c8a8a. The independent cross-implementation coverage in 47c0751 can remain separate.
5 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:
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).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. They are not consulted for Platform key-source selection or key derivation.
3. DIP-15 friendship keychain import + Platform key provider (
interfaces::Wallet)getPlatformPubKey/signPlatformDigest/platformECDHSecret: DIP-13 identity authentication and funding keys derived on demand inside the active descriptor key manager; root private key material never crosses the interface.ensureFriendshipReceivingKeychain: derives the wallet's own DIP-15 receiving chain, imports it idempotently as a ranged private descriptor, and returns its public chain in one wallet-locked operation. The stored descriptor uses the xpub plus its private key rather than serializing the friendship xprv.DeriveFriendshipPaymentDestination: derives contact payment destinations statelessly from a contact's stored friendship xpub, without touching any wallet keypool.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.
New/extended unit tests, all passing:
platformkeys_tests(22 cases): DIP-14 vectors 1-4 from dashpay/dips dip-0014.md through the path walker; public/private derivation consistency including hardened-step rejection; ECDH symmetry; descriptor root-source agreement; root-xprv-only derivation without a mnemonic; child-xprv rejection; chain-code-sensitive source identity; invalid mnemonic metadata not redirecting derivation; own friendship chainISMINE_SPENDABLEwith coins visible toAvailableCoins; contact chainISMINE_NO; recovery rederivation of authentication keys, friendship xpubs/destinations, ECDH secrets and compact signatures; import-after-restore making pre-loss payments spendable; import idempotency without script-pub-key-manager duplication.descriptor_tests: extraction of a unique root extended public key and recovery of its matching root extended private key from the signing provider, including ambiguity and mismatch rejection.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: