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 3 commits into
Conversation
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.
|
⛔ Blockers found — Opus deferred (commit 349d057) |
![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".
| if (candidates.empty()) return false; | ||
| seed_out = std::move(candidates.begin()->second); |
There was a problem hiding this comment.
When platform/seed-id exists but none of the active descriptor managers has the matching seed, the loop falls through and silently selects the lowest-ID candidate. This can happen after an active descriptor is replaced or a multi-seed wallet is restored incompletely, causing identity signatures, ECDH secrets, and friendship addresses to be produced from a different seed despite the pin's stated purpose. Return failure whenever preferred_id is set but unmatched.
AGENTS.md reference: AGENTS.md:L172-L172
Useful? React with 👍 / 👎.
| WalletDescriptor wallet_descriptor(std::move(parsed), /*creation_time=*/creation_time, | ||
| /*range_start=*/0, /*range_end=*/1000, | ||
| /*next_index=*/0); |
There was a problem hiding this comment.
Once a payment is observed, MarkUnusedAddresses advances next_index and TopUp expands the descriptor beyond this initial range. A subsequent import recreates the matching descriptor with range_end = 1000 and next_index = 0; AddWalletDescriptor then calls UpdateWalletDescriptor, which rejects the smaller range by throwing, so the advertised idempotent re-import fails after normal use. Reuse the existing descriptor's range and progress instead of rebuilding fresh metadata.
AGENTS.md reference: AGENTS.md:L172-L172
Useful? React with 👍 / 👎.
| if (element.hardened) return false; | ||
| if (const auto* index32 = std::get_if<uint32_t>(&element.index)) { | ||
| if (*index32 >> 31) return false; | ||
| return parent.pubkey.Derive(out.pubkey, out.chaincode, *index32, parent.chaincode); |
There was a problem hiding this comment.
When stored or externally supplied friendship data contains an empty or malformed public key, this calls CPubKey::Derive without validating the parent. That function begins with assert(IsValid()) and then reads the encoded key bytes, so debug builds abort and release builds can access invalid input instead of returning the interface's documented failure result. Check parent.pubkey.IsValid() before either derivation path.
AGENTS.md reference: AGENTS.md:L172-L172
Useful? React with 👍 / 👎.
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 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🧪 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
| std::optional<std::array<uint8_t, 8>> preferred_id; | ||
| { | ||
| const auto records{wallet.GetPlatformData("platform/seed-id")}; | ||
| const auto it{records.find("platform/seed-id")}; | ||
| if (it != records.end() && it->second.size() == 8) { | ||
| std::array<uint8_t, 8> id; | ||
| std::copy(it->second.begin(), it->second.end(), id.begin()); | ||
| preferred_id = id; | ||
| } | ||
| } | ||
| std::map<uint256, SecureVector> candidates; // spk_man id -> seed | ||
| for (const auto* spk_man : wallet.GetActiveScriptPubKeyMans()) { | ||
| const auto* desc_spk_man = dynamic_cast<const DescriptorScriptPubKeyMan*>(spk_man); | ||
| if (!desc_spk_man) continue; | ||
| SecureString mnemonic, mnemonic_passphrase; | ||
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) 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)); | ||
| } | ||
| if (candidates.empty()) return false; | ||
| seed_out = std::move(candidates.begin()->second); | ||
| return true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return false when a pinned seed id exists but no candidate matches it.
src/wallet/platformkeys.h lines 103-108 state the purpose of the stored fingerprint: platform data created from one seed must never be silently signed over with another seed. This code does not hold that invariant. If preferred_id is set and no active DescriptorScriptPubKeyMan produces a matching seed, the loop finishes and line 52 returns the lowest-ID candidate instead. A partially restored or re-keyed multi-seed wallet then signs with the wrong seed and produces a different identity, with no error.
Fail instead when the pinned seed is not available.
🐛 Proposed fix
if (candidates.empty()) return false;
+ // A recorded seed id pins the wallet's platform identity. If that seed is
+ // not present, using another one would silently fork the identity.
+ if (preferred_id) return false;
seed_out = std::move(candidates.begin()->second);
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::optional<std::array<uint8_t, 8>> preferred_id; | |
| { | |
| const auto records{wallet.GetPlatformData("platform/seed-id")}; | |
| const auto it{records.find("platform/seed-id")}; | |
| if (it != records.end() && it->second.size() == 8) { | |
| std::array<uint8_t, 8> id; | |
| std::copy(it->second.begin(), it->second.end(), id.begin()); | |
| preferred_id = id; | |
| } | |
| } | |
| std::map<uint256, SecureVector> candidates; // spk_man id -> seed | |
| for (const auto* spk_man : wallet.GetActiveScriptPubKeyMans()) { | |
| const auto* desc_spk_man = dynamic_cast<const DescriptorScriptPubKeyMan*>(spk_man); | |
| if (!desc_spk_man) continue; | |
| SecureString mnemonic, mnemonic_passphrase; | |
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) 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)); | |
| } | |
| if (candidates.empty()) return false; | |
| seed_out = std::move(candidates.begin()->second); | |
| return true; | |
| std::optional<std::array<uint8_t, 8>> preferred_id; | |
| { | |
| const auto records{wallet.GetPlatformData("platform/seed-id")}; | |
| const auto it{records.find("platform/seed-id")}; | |
| if (it != records.end() && it->second.size() == 8) { | |
| std::array<uint8_t, 8> id; | |
| std::copy(it->second.begin(), it->second.end(), id.begin()); | |
| preferred_id = id; | |
| } | |
| } | |
| std::map<uint256, SecureVector> candidates; // spk_man id -> seed | |
| for (const auto* spk_man : wallet.GetActiveScriptPubKeyMans()) { | |
| const auto* desc_spk_man = dynamic_cast<const DescriptorScriptPubKeyMan*>(spk_man); | |
| if (!desc_spk_man) continue; | |
| SecureString mnemonic, mnemonic_passphrase; | |
| if (!desc_spk_man->GetMnemonicString(mnemonic, mnemonic_passphrase)) 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)); | |
| } | |
| if (candidates.empty()) return false; | |
| // A recorded seed id pins the wallet's platform identity. If that seed is | |
| // not present, using another one would silently fork the identity. | |
| if (preferred_id) return false; | |
| seed_out = std::move(candidates.begin()->second); | |
| return true; |
🤖 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/platformseed.cpp` around lines 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.
| bool CWallet::WritePlatformData(const std::string& key, const std::vector<unsigned char>& value) | ||
| { | ||
| AssertLockHeld(cs_wallet); | ||
| WalletBatch batch(GetDatabase()); | ||
| if (value.empty()) { | ||
| m_platform_data.erase(key); | ||
| return batch.ErasePlatformData(key); | ||
| } | ||
| if (!batch.WritePlatformData(key, value)) return false; | ||
| m_platform_data[key] = value; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Erase from memory only after the database erase succeeds.
The write path writes the database first and updates m_platform_data only on success. The erase path does the opposite: it removes the in-memory entry, then erases from the database. If ErasePlatformData fails, the function returns false, but the record is already gone from memory while it still exists on disk. The next wallet load resurrects it, so memory and disk disagree until restart.
🐛 Proposed fix
if (value.empty()) {
- m_platform_data.erase(key);
- return batch.ErasePlatformData(key);
+ if (!batch.ErasePlatformData(key)) return false;
+ m_platform_data.erase(key);
+ return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool CWallet::WritePlatformData(const std::string& key, const std::vector<unsigned char>& value) | |
| { | |
| AssertLockHeld(cs_wallet); | |
| WalletBatch batch(GetDatabase()); | |
| if (value.empty()) { | |
| m_platform_data.erase(key); | |
| return batch.ErasePlatformData(key); | |
| } | |
| if (!batch.WritePlatformData(key, value)) return false; | |
| m_platform_data[key] = value; | |
| return true; | |
| } | |
| bool CWallet::WritePlatformData(const std::string& key, const std::vector<unsigned char>& value) | |
| { | |
| AssertLockHeld(cs_wallet); | |
| WalletBatch batch(GetDatabase()); | |
| if (value.empty()) { | |
| if (!batch.ErasePlatformData(key)) return false; | |
| m_platform_data.erase(key); | |
| return true; | |
| } | |
| if (!batch.WritePlatformData(key, value)) return false; | |
| m_platform_data[key] = value; | |
| return true; | |
| } |
🤖 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.cpp` around lines 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.
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:
|
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.
| if (candidates.empty()) return false; | ||
| seed_out = std::move(candidates.begin()->second); |
There was a problem hiding this comment.
🔴 Blocking: 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.
| if (candidates.empty()) return false; | |
| seed_out = std::move(candidates.begin()->second); | |
| if (preferred_id || candidates.empty()) return false; | |
| seed_out = std::move(candidates.begin()->second); |
source: ['codex', 'coderabbit']
| 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), so re-imports during | ||
| // recovery are safe. | ||
| if (!m_wallet->AddWalletDescriptor(wallet_descriptor, provider, label, /*internal=*/false)) { |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
| bool DerivePubKey(const ExtPubKey256& parent, const PathElement& element, ExtPubKey256& out) | ||
| { | ||
| if (element.hardened) return false; | ||
| if (const auto* index32 = std::get_if<uint32_t>(&element.index)) { | ||
| if (*index32 >> 31) return false; | ||
| return parent.pubkey.Derive(out.pubkey, out.chaincode, *index32, parent.chaincode); |
There was a problem hiding this comment.
🔴 Blocking: 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().
| bool DerivePubKey(const ExtPubKey256& parent, const PathElement& element, ExtPubKey256& out) | |
| { | |
| if (element.hardened) return false; | |
| if (const auto* index32 = std::get_if<uint32_t>(&element.index)) { | |
| if (*index32 >> 31) return false; | |
| return parent.pubkey.Derive(out.pubkey, out.chaincode, *index32, parent.chaincode); | |
| if (element.hardened || !parent.pubkey.IsCompressed()) return false; |
source: ['codex']
| if (value.empty()) { | ||
| m_platform_data.erase(key); | ||
| return batch.ErasePlatformData(key); |
There was a problem hiding this comment.
🔴 Blocking: 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.
| if (value.empty()) { | |
| m_platform_data.erase(key); | |
| return batch.ErasePlatformData(key); | |
| if (value.empty()) { | |
| if (!batch.ErasePlatformData(key)) return false; | |
| m_platform_data.erase(key); | |
| return true; | |
| } |
source: ['codex', 'coderabbit']
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: