chore: bump dashpay/platform pin to PR #3968 tip (seedless rehydration + shielded viewing-key persistence) - #919
Conversation
Bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from d18020f to f376d32, the tip of platform PR #3968 — "feat(platform- wallet-storage): embeddable SQLite persistence backend with seedless rehydration". Cargo.lock is regenerated scoped to the platform package set: only dashpay-ecosystem git deps moved (platform, rust-dashcore be6e776→0091c4a, grovedb v5.0.0→v5.0.1, orchard) — all transitively pinned by the new platform rev; no crates.io registry drift. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - AssetLockFunding::FromExistingAssetLock gained a consume_invitation_voucher field. Set false at the three generic funding sites (identity register, identity top-up, platform-address funding) — none is the DashPay invitation-voucher reclaim flow, so a bearer-voucher lock is never consumed. - shielded_transfer_to / shielded_unshield_to / shielded_withdraw_to each gained a seed: &[u8] parameter. This is the seedless-rehydration privilege- separation change: the Orchard spend keyset (ASK included) is re-derived from the seed for the single call and dropped on return, never left resident. DET's shielded_transfer/unshield/withdraw now resolve the HD seed just-in-time through the secret-seam chokepoint (with_secret_session), mirroring the existing shield_from_balance path; stale "no seed scope needed" doc comments corrected. - PlatformWalletError gained PersisterStore and PersisterRestore variants. Added to both exhaustive (no wildcard) match sites — map_shielded_op_error and identity_op_error_kind — in the generic persistence bucket alongside PersisterLoad / Persistence. Verification (via cargo-cached.sh ledger): - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2009 passed, 0 failed, 1 ignored - cargo fmt --all -- --check — exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-ups from the security due-diligence pass on the dashpay/platform PR #3968 diff (companion to the pin-bump commit on this branch). No behavior change — comment-only edits plus a supply-chain audit. - Verified the new reserve-on-hand-out receive-address behavior does NOT reach DET. Upstream PlatformAddressWallet::next_unused_receive_address now reserves (via key-wallet next_unused_and_reserve), but DET calls neither it (DET's platform receive path is its own index-based generate_platform_receive_address_with_seed) nor a reserving Core variant. DET's Core receive path (next_receive_address → next_receive_address_for_- account → AddressPool::next_unused) stays non-reserving; its only change (!used → is_available()) is behavior-neutral because DET never reserves. Refreshed the TC-012 TODO's stale merge-status note: rust-dashcore#818 is now present in the pinned rev, while the Core reserving surface (CoreWallet::next_receive_address_and_reserve_for_account) still is not. - Fixed two stale doc comments spotted in the diff: new_watch_only → new_external_signable (context/wallet_lifecycle/tests.rs) and AddressInfo.used → AddressInfo.is_used() (backend_task/dashpay/payments.rs). - cargo audit over the regenerated Cargo.lock: advisory-ID set is identical to base v1.0-dev (14 advisories; zero introduced, zero removed) and the yanked- crate set is identical — no new advisory beyond the pre-existing, already- acknowledged bincode RUSTSEC-2025-0141. dashcore/key-wallet unify to a single rev (0091c4a) in the lock; no conflicting revisions. Verification (via cargo-cached.sh ledger, final tree): - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2009 passed, 0 failed, 1 ignored - cargo fmt --all -- --check — exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…02/003)
Adversarial-review follow-ups on the shielded seed-resolution change from the
platform#3968 pin bump. No functional change to the happy path.
- QA-003: the doc comments on shielded_transfer/unshield/withdraw claimed a
locked protected wallet yields WalletLocked "if its seed isn't cached". That
is wrong: with_secret_session PROMPTS for a protected wallet; a dismissed or
headless prompt surfaces as SecretPromptCancelled / SecretPromptUnavailable.
WalletLocked comes ONLY from a non-HD-seed secret sitting at an HD scope (a
misconfigured wallet). Rewrote all three docs to describe the real prompt
path and reframe WalletLocked as that defensive type guard.
- QA-002: the new expose_hd_seed()->WalletLocked branch (a genuinely new
failure mode — these fns took no seed before the bump) had zero unit coverage,
reachable today only via network-gated #[ignore] e2e. Extracted the mapping
into a small documented helper, hd_seed_for_shielded_spend, and added two unit
tests pinning HdSeed->Ok and SingleKey->WalletLocked.
- QA-005: hoisted resolve_wallet + CachedOrchardProver::new() (neither needs the
seed) out of the secret session in all three fns, matching the already-hoisted
coordinator — trims the secret-session window's front edge and fails fast
(ShieldedNotConfigured / WalletNotLoaded) without prompting.
- QA-004: CHANGELOG now names the transitive dashpay git-dep bumps carried by the
platform pin — rust-dashcore be6e776->0091c4a, grovedb v5.0.0->v5.0.1, and the
orchard fork dashified-0.14.0->0.14.1.
Verification (via cargo-cached.sh ledger, final tree):
- cargo clippy --all-features --all-targets -- -D warnings — exit 0
- cargo test --lib --all-features — 2011 passed, 0 failed, 1 ignored
(+2 new: hd_seed_for_shielded_spend_{accepts_hd_seed,
rejects_non_hd_secret_as_wallet_locked})
- cargo fmt --all -- --check — exit 0
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThe update repins the upstream wallet backend, shares its SQLite persister across wallet operations, derives shielded spend seeds just in time, prevents invitation-voucher consumption during generic asset-lock resumes, and adds storage, password, migration, broadcast, and lifecycle changes. ChangesWallet backend update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SecretAccess
participant WalletBackend
participant ShieldedOperations
SecretAccess->>WalletBackend: provide HD-scoped SecretPlaintext
WalletBackend->>WalletBackend: derive HD seed for one spend
WalletBackend->>ShieldedOperations: execute transfer, unshield, or withdraw with seed
ShieldedOperations-->>WalletBackend: return operation result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from f376d32 to ebbd15c4, the current tip of platform PR #3968 — "feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration". Verified via git ls-remote against feat/platform-wallet-storage-rehydration that the PR has not moved past this rev. Cargo.lock is regenerated scoped to the platform package set: 27 platform git packages moved to ebbd15c (document-history-contract added transitively); rust-dashcore, grovedb, orchard, and grovestark pins are unchanged; no crates.io registry drift. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - PlatformWalletError gained PlatformNodePool, CoreInsufficientFunds, AssetLockNotTracked, AssetLockAlreadyConsumed, and AssetLockFundingMismatch variants. Classified in both exhaustive (no wildcard) match sites -- map_shielded_op_error and identity_op_error_kind -- alongside the existing precondition/wallet-state bucket. - The upstream changeset schema gained a shielded.viewing_keys field with a new PersistenceCapabilities::SHIELDED_VIEWING_KEYS contract, needed for seedless shielded restart (Orchard FVKs must survive a restart without the HD seed). Added src/wallet_backend/persister.rs: a thin DetPersister adapter wrapping the upstream SqlitePersister -- delegates everything except the new viewing-key changeset, which it persists itself via the existing DetKv abstraction (already used for app data alongside wallet state), scoped per wallet_id and guarded by a mutex. Rejects any mixed changeset (viewing keys plus anything else) to preserve upstream's ATOMIC_CHANGESETS invariant rather than splitting one commit across the typed SQLite tables and the metadata table. Note this supersedes the prior "DET does not write its own persister" doc comment: DET now owns exactly the one host-specific extension upstream doesn't yet provide, not a reimplementation of the persister itself. - The later review tip also retries transient startup rehydration, selects platform-address transfer/withdrawal inputs from hydrated candidates with authoritative on-chain balances, freezes the SPV sync watermark when persistence fails, and persists address-reservation timestamps plus DashPay address used-state updates -- no DET call-site changes required for these; CHANGELOG updated to describe the user-visible effect. Verification ledger logs under /data/artifacts/dash-evo-tool/2026-07-22/platform-pin-bump/final-ledger/logs/, all exit 0: fmt --check, build --all-features, clippy --all-features --all-targets -D warnings, plus targeted lib tests -- wallet_backend (327 passed), context::wallet_lifecycle (62 passed), shielded (71 passed). Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
…007, RUST-001) Independent security (Smythe) and structural (Adams) due-diligence review of f7ca95f (the platform#3968 pin re-bump) surfaced two MEDIUM findings that escalated to a confirmed HIGH once Marvin produced a red repro, plus two cheap LOW nits. All fixed here, on top of f7ca95f, before push. - SEC-007 (HIGH, confirmed via red repro): DetPersister::load() previously `?`-propagated ANY single wallet's corrupt/undecodable shielded-FVK row, failing cold-boot rehydration for every wallet in the store, not just the corrupted one. load_viewing_keys now distinguishes decode/schema-version/ truncated/oversized-value failures (degrade to an empty FVK set for the affected wallet, with a warning log carrying the wallet id and error shape, no FVK bytes) from genuine backend/IO failures (still fatal, e.g. LockPoisoned), matching this module's established kv_get_logged degrade-to-absent convention (kv.rs) and the sibling cold_boot_keeps_wallet_visible_when_persisted_transaction_txid_is_corrupt precedent. - RUST-001 (MEDIUM): is_viewing_key_only now exhaustively destructures the nested ShieldedChangeSet (all six upstream fields), not just field-access checks — a future 7th upstream field is now a compile error instead of being silently ignored by the mixed-changeset reject guard. - PROJ-001 (LOW): SHIELDED_FVK_KEY renamed "shielded_fvks.v1" -> "shielded:fvks:v1" to match this module's colon-namespaced key convention, before the old spelling became on-disk schema. - PROJ-002 (LOW): corrected the Inner.persister doc comment, which read as if `pwm` held this exact Arc directly; it now describes that `pwm` consumes the DetPersister wrapper around this same SqlitePersister. - Added 6 tests: a table-driven is_viewing_key_only classification test; a store/load FVK round-trip; a test proving oversized/decode failures degrade to absent while genuine backend failures stay fatal; mixed- changeset and cross-wallet-id rejection tests (both asserting no partial write); and a real two-wallet, two-boot cold-start regression test (cold_boot_skips_corrupt_fvks_for_one_wallet_and_restores_healthy_wallet) that persists both wallets for real, corrupts one's on-disk FVK row under its actual upstream wallet id, and confirms cold boot succeeds with the healthy wallet's viewing key still installed. A prior throwaway proof-of-concept test from the review pass (green-from-start, non-probative) was reverted rather than kept. An open follow-up from the same review — whether persister.rs scoping FVK rows by the upstream WalletId instead of this module's usual WalletSeedHash orphans the row when a wallet is later removed (both are bare `[u8; 32]` aliases, so it type-checks either way) — is still being verified empirically and will land separately if it proves to be a real gap. Verification (via cargo-cached.sh ledger): - cargo fmt --all -- --check — exit 0 - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features wallet_backend — 332 passed, 0 failed - cargo test --lib --all-features context::wallet_lifecycle — 63 passed, 0 failed - cargo test --lib --all-features shielded — 71 passed, 0 failed Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
…8/SEC-010) Fast-follow from the same platform#3968 pin-bump due-diligence review that produced eb7e0fa. Confirmed empirically (Smythe, twice): removing a wallet via forget_wallet_local_state + remove_upstream_wallet never deletes the persisted shielded:fvks:v1 row persister.rs (eb7e0fa) added. Root cause: upstream's PlatformWalletPersistence trait has no delete_wallet method at this rev (a separately-tracked, pre-existing upstream gap - out of scope here), and DET's own removal path never touched this new KV row either. Net effect: an Orchard viewing key outlives the wallet it belonged to, orphaned on disk indefinitely. Privacy-only (no spend secrets involved - those are already correctly wiped by the seed-vault deletion this function already does) and fund-safety-neutral, so this is a fast-follow rather than a blocker alongside SEC-007/RUST-001, per reviewer sign-off. - persister.rs: added forget_wallet_viewing_keys(kv, wallet_id), keyed by the upstream WalletId (matching what store_viewing_keys/load_viewing_keys actually key by - not this module's usual WalletSeedHash), reusing the existing SHIELDED_FVK_KEY constant rather than re-typing the key string. - mod.rs: forget_wallet_local_state now calls this best-effort in the `if let Some(wallet_id)` block (wallet_id is only Some when the wallet was actually upstream-registered, so there's never a row to reap otherwise), logging a warning on failure rather than aborting the rest of the wipe - consistent with this function's existing resilience contract for lesser (non-secret) cleanup steps. - tests.rs: added remove_wallet_reaps_persisted_shielded_viewing_keys, which registers a wallet, binds shielded (persists an FVK row), verifies the row exists, runs the real removal path, then re-opens the SQLite file directly and asserts the row is gone. Verification (via cargo-cached.sh ledger): - cargo fmt --all -- --check - exit 0 - cargo clippy --all-features --all-targets -- -D warnings - exit 0 - cargo test --lib --all-features wallet_backend - 332 passed, 0 failed (includes remove_wallet_reaps_persisted_shielded_viewing_keys) Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
|
🕓 Ready for review — 12 ahead in queue (commit 2b2da63) |
Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from ebbd15c4 to e75f259, the current tip of platform PR #3968. Verified via gh pr view headRefOid immediately before commit. Cargo.lock is regenerated scoped to the platform package set: 28 platform git packages moved to e75f259. rust-dashcore, grovedb, orchard, and grovestark pins are unchanged; no crates.io registry versions, checksums, or dependency edges moved. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - The hardened file and SQLite backends now validate permissions on every parent directory. Normalize the app-data root, SPV directory, and per-network directory to owner-only on Unix before opening storage; copied cold-boot fixtures preserve that invariant. - PlatformWalletPersistence gained delete_wallet with a default UnsupportedOperation result. DetPersister now delegates the method to its inner SqlitePersister so wallet deletion keeps the typed backend behavior and cascade cleanup. - The secret store now enforces an eight-byte passphrase floor. Reject incompatible passwords for new HD wallets before creating an envelope, while keeping unmigrated short-password legacy wallets usable by deferring rejected Tier-2 rewraps and retaining the legacy envelope. - Upstream now persists shielded viewing keys natively. The legacy DET metadata adapter remains in place for databases written by prior pins; platform-wallet and platform-wallet-storage retain matching shielded feature flags. Known upstream compatibility limitation: July 2026 weekly builds could already write a short-password Tier-2 seed under the previous one-byte floor and delete its legacy envelope. e75f259 rejects that password before decryption, and no public downstream compatibility reader exists. CHANGELOG documents that affected profiles must not upgrade until upstream provides read-old/write-new support; this commit must not be pushed or merged as-is for those profiles. PlatformWalletError did not gain variants in this jump, so the existing exhaustive match classifications remain complete. Verification: - cargo fmt --all -- --check — exit 0 - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2054 passed, 0 failed, 1 ignored - cargo test --all-features wallet_backend — 335 passed, 0 failed - cargo test --all-features context::wallet_lifecycle — 64 passed, 0 failed - cargo test --all-features shielded — 72 passed, 0 failed, plus 3 ignored backend-e2e cases Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
Use upstream SqlitePersister for native viewing-key persistence. Drive delete_wallet after runtime detachment to trigger its verified cascade. Retain the removal integration test and drop redundant workaround coverage. Document why unreleased interim metadata rows are not migrated. Co-Authored-By: OpenAI Codex <noreply@openai.com>
lklimek
added
the
claudius-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/backend_task/error.rs (1)
1923-1927: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame actionable-text gap as the model-layer message; doc comment also overstates the migration trigger.
This message inherits the same "no call-to-action" gap as
WalletCreationError::PasswordTooShort(see consolidated comment). Separately, the doc comment says the password "could not be migrated to Tier-2 protection," but the only current caller of this conversion isWallet::new_from_seedat wallet-creation time — the lazy-migration path (decrypt_jit) instead logs and defers on a short legacy password rather than producing this error. Consider tightening the doc comment to name the actual trigger so future readers don't assume this fires during migration.🤖 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/backend_task/error.rs` around lines 1923 - 1927, Update WalletPasswordTooShort’s error text to include a clear corrective action, matching the actionable guidance used by WalletCreationError::PasswordTooShort. Revise its doc comment to describe wallet creation via Wallet::new_from_seed as the current trigger, rather than stating that migration to Tier-2 protection failed.Source: Path instructions
src/app_dir.rs (1)
80-92: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winClose the create-then-chmod window; also secure any newly-created parent directories.
create_dir_allfollowed by a separateset_permissionsleaves a brief window where a freshly-created directory sits at the OS/umask default mode before being tightened, and only the final leaf directory is chmod'd — any parent directoriescreate_dir_allhad to create along the way stay at the default mode.DirBuilder::recursive(true).mode(0o700)creates the whole missing chain with the target mode applied to every newly-created directory in one call, per the standard library docs: "Parents that do not exist are created with the same security and permissions settings."🔒 Proposed fix using DirBuilder
pub fn ensure_data_dir_exists(data_dir: &Path) -> Result<(), std::io::Error> { - fs::create_dir_all(data_dir)?; - let metadata = fs::metadata(data_dir)?; - if !metadata.is_dir() { - return Err(std::io::Error::other("Created path is not a directory")); - } #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - fs::set_permissions(data_dir, fs::Permissions::from_mode(0o700))?; + use std::os::unix::fs::DirBuilderExt; + fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(data_dir) + .or_else(|e| if data_dir.is_dir() { Ok(()) } else { Err(e) })?; + // Re-tighten in case the directory already existed with looser permissions + // (DirBuilder's mode only applies to directories it actually creates). + fs::set_permissions(data_dir, fs::Permissions::from_mode(0o700))?; + } + #[cfg(not(unix))] + { + fs::create_dir_all(data_dir)?; + } + let metadata = fs::metadata(data_dir)?; + if !metadata.is_dir() { + return Err(std::io::Error::other("Created path is not a directory")); } Ok(()) }🤖 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/app_dir.rs` around lines 80 - 92, Update ensure_data_dir_exists to use a recursive fs::DirBuilder configured with mode 0o700 on Unix, so every newly-created directory in the missing parent chain is created with the target permissions without a create-then-chmod window. Preserve the existing metadata validation and non-Unix behavior, and remove the separate final-directory set_permissions step.
🤖 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/backend_task/error.rs`:
- Around line 1923-1927: Update the #[error] message for
TaskError::WalletPasswordTooShort in src/backend_task/error.rs:1923-1927 and
WalletCreationError::PasswordTooShort in src/model/wallet/mod.rs:54-56 to append
an actionable instruction such as asking the user to choose a longer password
and try again, keeping both messages identical.
In `@src/wallet_backend/secret_access.rs`:
- Line 2653: Remove the hardcoded SHORT_LEGACY_PASSWORD literal in the
secret-access test and generate a runtime passphrase that is shorter than the
storage minimum. In src/boot.rs at lines 297 and 332, replace the hardcoded test
passphrases with runtime-generated values and reuse each generated passphrase
for its corresponding passphrase-vault creation; update all affected test setup
while preserving the existing short-passphrase behavior.
---
Nitpick comments:
In `@src/app_dir.rs`:
- Around line 80-92: Update ensure_data_dir_exists to use a recursive
fs::DirBuilder configured with mode 0o700 on Unix, so every newly-created
directory in the missing parent chain is created with the target permissions
without a create-then-chmod window. Preserve the existing metadata validation
and non-Unix behavior, and remove the separate final-directory set_permissions
step.
In `@src/backend_task/error.rs`:
- Around line 1923-1927: Update WalletPasswordTooShort’s error text to include a
clear corrective action, matching the actionable guidance used by
WalletCreationError::PasswordTooShort. Revise its doc comment to describe wallet
creation via Wallet::new_from_seed as the current trigger, rather than stating
that migration to Tier-2 protection failed.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2da42431-3594-4220-9702-10960a4c3986
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdCargo.tomlsrc/app_dir.rssrc/backend_task/error.rssrc/boot.rssrc/context/mod.rssrc/context/wallet_lifecycle/tests.rssrc/model/wallet/mod.rssrc/wallet_backend/mod.rssrc/wallet_backend/secret_access.rssrc/wallet_backend/single_key.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
…word Tier-2 seeds The new pin's password-length floor rejects a sub-8-character passphrase on read, not just write, mirroring the write-side check as a deliberate defense against a backend-write attacker planting a weakly sealed envelope. Mark the affected call site so it gets revisited once upstream adds a scoped migration-read capability. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from e75f259 to 4a1ba64, the current tip of platform PR #3968. Cargo.lock is regenerated scoped to the platform package set: 28 platform git packages moved to 4a1ba64 and 11 rust-dashcore packages moved from 0091c4a to 18c68d4. No crates.io registry versions, checksums, dependency edges, or unrelated sources moved. API-drift fixes required by the upstream jump: - DataContractUpdate waits for affected state instead of generic broadcast completion, while retaining the typed TaskError conversion and existing DriveProofError recovery path. - Contact receiving accounts use ManagedAccountOperations so newly added accounts invalidate stale compact-filter coverage. Existing contact keys remain a true no-op, and only real insertions bump monitor_revision. - The changelog records authoritative pure-SPV broadcast outcomes and the contact-account filter-coverage fix without claiming shielded identity creation support in DET. Verification ledger evidence (all exit 0): - cargo fmt --all — key 9ff5de5fa46203d0ebbf5580836bddf9 — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T111934-9ff5de5fa46203d0ebbf5580836bddf9-2.log - cargo test contact_account_registration_invalidates_filter_coverage_once --all-features — key 3cdb20d7ef39ab598af859ff3fcf6020 — 1 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T111944-3cdb20d7ef39ab598af859ff3fcf6020-2.log - cargo test --lib --all-features wallet_backend — key b062125daa4c22c77c2199c8f74d74a2 — 330 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112028-b062125daa4c22c77c2199c8f74d74a2-2.log - cargo test --lib --all-features backend_task — key 2fa67207f0f111422cde6544d6a29bac — 411 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112049-2fa67207f0f111422cde6544d6a29bac-2.log - cargo clippy --lib --all-features -- -D warnings — key 4a6af394e0e2293a6da5a0e5f824af5c — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112100-4a6af394e0e2293a6da5a0e5f824af5c-2.log - git diff --check — exit 0 Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
Extract the post-derivation contact-account registration loop into the smallest production seam that can be exercised without wallet persistence, SPV, or network infrastructure. The regression now proves that a first contact registration increments the aggregate monitor revision, a duplicate registration leaves it unchanged, and a mixed existing/new batch increments it exactly once. It retains the account-generation and filter-coverage invalidation assertions from the original lower-level test. Document the DashpayReceivingFunds-only helper contract and its boolean result. Keep the defensive variant check for future callers, and retain the contact-key pre-check because upstream reports duplicate managed account types as InvalidParameter instead of providing idempotent no-op semantics. Verification ledger evidence (all exit 0): - cargo fmt --all — key 7e9402392e7b60075b2c66376240c2b1 — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115050-7e9402392e7b60075b2c66376240c2b1-2.log - cargo test contact_registration_bumps_monitor_revision_only_for_new_keys --all-features — key 979201f10ee2e2db3aba25ed91ba11d6 — 1 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115104-979201f10ee2e2db3aba25ed91ba11d6-2.log - cargo test --lib --all-features wallet_backend — key c35703b8723b6de9ce7b907df35e85e9 — 330 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115206-c35703b8723b6de9ce7b907df35e85e9-2.log - cargo clippy --lib --all-features -- -D warnings — key 7cf2092cee6223ee239e8c37f9e4a35b — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115214-7cf2092cee6223ee239e8c37f9e4a35b-2.log - git diff --check — exit 0 Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/wallet_backend/mod.rs (2)
2135-2144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring claims narrower behavior than the code delivers.
The comment states the bump is "only for genuinely newly inserted accounts," but see the companion comment on
register_contact_accounts_in_managed_wallet(Lines 2641-2675) — the implementation currently bumps everyDashpayReceivingFundsaccount whenever any single one is new. Update the doc once the helper is fixed, or vice versa.🤖 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_backend/mod.rs` around lines 2135 - 2144, The documentation around the monitored-account rebuild incorrectly claims only newly inserted accounts trigger bump_monitor_revision, while register_contact_accounts_in_managed_wallet currently bumps for every DashpayReceivingFunds account when any one is new. Align the implementation and documentation by changing that helper to bump only genuinely newly inserted accounts, preserving the existing behavior for already registered accounts.
2151-2209: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
bump_monitor_revisionfires for everyDashpayReceivingFundsaccount, not just the newly-inserted one.
register_contact_accounts_in_managed_walletbumps the revision of allDashpayReceivingFundsaccounts whenevernewly_inserted > 0, contradicting the doc comment onregister_contact_receiving_accounts("Only newly-added accounts trigger abump_monitor_revision") and the test itself: after the second call,first_contact's revision goes from 1→2 even though onlysecond_contactis new (test assertsrevisions == [1, 2], not[1, 1]). This causes needless revision churn on already-covered accounts every time a single new contact is added.♻️ Bump only the newly-inserted accounts
let mut newly_inserted = 0; + let mut newly_inserted_keys = Vec::new(); for (account_type, account_xpub) in accounts { match add_contact_receiving_account(info, account_type, account_xpub) { - Ok(true) => newly_inserted += 1, + Ok(true) => { + newly_inserted += 1; + newly_inserted_keys.push(account_type); + } Ok(false) => {} Err(error) => { tracing::debug!(%error, "Skipping contact account: managed insert failed"); } } } - if newly_inserted > 0 { - for account in info.accounts.all_funding_accounts_mut() { - if matches!( - account.managed_account_type(), - ManagedAccountType::DashpayReceivingFunds { .. } - ) { - account.bump_monitor_revision(); - } - } - } + for account in info.accounts.all_funding_accounts_mut() { + if newly_inserted_keys + .iter() + .any(|k| *k == account.managed_account_type().into()) + { + account.bump_monitor_revision(); + } + } newly_inserted(Sketch — adapt to whatever equality/lookup the
AccountType/ManagedAccountTypetypes actually support.)Also applies to: 2641-2675, 3058-3133
🤖 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_backend/mod.rs` around lines 2151 - 2209, Update register_contact_accounts_in_managed_wallet and its callers so bump_monitor_revision is applied only to accounts newly inserted during the current registration, not every DashpayReceivingFunds account. Track each successful insertion or identify newly inserted account types before performing the bumps, while preserving the existing return count and no-op behavior when nothing is added. Apply the same correction to the related registration paths noted in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/wallet_backend/mod.rs`:
- Around line 2135-2144: The documentation around the monitored-account rebuild
incorrectly claims only newly inserted accounts trigger bump_monitor_revision,
while register_contact_accounts_in_managed_wallet currently bumps for every
DashpayReceivingFunds account when any one is new. Align the implementation and
documentation by changing that helper to bump only genuinely newly inserted
accounts, preserving the existing behavior for already registered accounts.
- Around line 2151-2209: Update register_contact_accounts_in_managed_wallet and
its callers so bump_monitor_revision is applied only to accounts newly inserted
during the current registration, not every DashpayReceivingFunds account. Track
each successful insertion or identify newly inserted account types before
performing the bumps, while preserving the existing return count and no-op
behavior when nothing is added. Apply the same correction to the related
registration paths noted in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99fdec5a-0de7-4216-a016-a51a4a1f252e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdCargo.tomlsrc/backend_task/update_data_contract.rssrc/wallet_backend/mod.rssrc/wallet_backend/secret_access.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- src/wallet_backend/secret_access.rs
- CHANGELOG.md
- cover corrupt FVK cold boot and surface asynchronous removal failures (PROJ-001, CALL-001) - align password validation, documentation, and actionable errors (PROJ-003, PROJ-004, CODE-001) - narrow lazy Tier-2 re-wrap handling and fix shielded invariant mapping (CODE-003, CODE-004) - document protected headless shielded spends and harden data-directory creation (CALL-003, SEC-004/CALL-002) Co-Authored-By: Codex GPT-5 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The dependency adapters generally match the pinned upstream APIs, including the corrected contract-update wait path and contact-account revision gating. One blocking privacy defect remains because user-requested wallet deletion creates a pre-deletion backup containing the unencrypted shielded viewing key; the local password guard also rejects some passwords that satisfy the upstream byte-length policy.
Validated blockers were found in the Codex precheck. Sonnet 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— rust-quality (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_backend/mod.rs`:
- [BLOCKING] src/wallet_backend/mod.rs:1389: Wallet deletion preserves the viewing key in an automatic backup
`WalletBackend::new` opens this persister with `SqlitePersisterConfig::new`, whose default enables automatic backups, and the pinned `delete_wallet` implementation takes a full pre-deletion database backup under the network database directory's `backups/auto` subdirectory before cascading wallet-owned rows. That snapshot includes the newly native, unencrypted `shielded_viewing_keys` row, so the Remove Wallet flow deletes the key from the live database while retaining a copy on disk despite telling the user that this version's local wallet data will be cleared. Use the pinned no-backup deletion entry point for this explicit user-requested removal, and extend the regression test beyond querying the live database so it detects creation of a pre-delete backup.
In `src/model/wallet/mod.rs`:
- [SUGGESTION] src/model/wallet/mod.rs:439: Match the storage policy's byte-length password check
`MIN_PASSPHRASE_LEN` is the persistent store's post-trim byte-length floor, and upstream measures it with `SecretString::trimmed().len()`. This guard instead counts Unicode scalar values, so a password such as four two-byte non-ASCII characters satisfies the eight-byte storage requirement but is rejected by DET before storage is reached. Since this guard was introduced specifically to mirror the upstream floor, use Rust string byte length and add a multibyte boundary case to the existing short-password test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/context/wallet_lifecycle/removal.rs`:
- Around line 65-80: Update both warning paths in the wallet removal flow to
clone and retain an egui context handle, then request a repaint immediately
after show_wallet_data_removal_warning updates the banner. Ensure the
synchronous spawn-failure path also receives a cloned context handle and
requests repaint after surfacing TaskError::TaskManagerShuttingDown.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf797716-0fc3-49d0-b79a-bd33cf01b3f8
📒 Files selected for processing (10)
docs/CLI.mddocs/MCP.mddocs/user-stories.mdsrc/app_dir.rssrc/backend_task/error.rssrc/context/wallet_lifecycle/removal.rssrc/context/wallet_lifecycle/tests.rssrc/model/wallet/mod.rssrc/wallet_backend/secret_access.rssrc/wallet_backend/shielded.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend_task/error.rs
- src/wallet_backend/secret_access.rs
- src/model/wallet/mod.rs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius' grumpy-review — PR #919 (platform pin bump to #3968 tip)
A disciplined, genuinely well-tested dependency bump. I dispatched four specialists (security, structural, adversarial QA, docs) over the 22-file diff — static verification only, since bash/cargo were unavailable in this sandbox; CI remains the compile/test backstop.
Headline: thepastaclaw's still-open blocking thread — that wallet deletion retains the unencrypted shielded viewing key (FVK) in an automatic pre-delete backup — is CONFIRMED. Verified against the pinned upstream source (dashpay/platform @ 4a1ba64c): SqlitePersisterConfig::new defaults auto-backup ON, delete_wallet takes a full physical DB copy to backups/auto/pre-delete-*.db before cascading rows, and delete_wallet_skip_backup exists as the intended remedy. The two remove-wallet regression tests only inspect the live DB, so they are structurally blind to it. Details and the fix are on that thread. This is the one genuinely blocking item — it contradicts an explicit user-facing promise and defeats the PR's own SEC-008/SEC-010 reaping.
Findings: 2 MEDIUM, 9 LOW, 2 INFO.
- 🔴 MEDIUM — Wallet deletion retains the unencrypted FVK in a pre-delete backup (SEC-001 + QA-001 test gap) → see the open
delete_wallet_skip_backupthread. - 🟡 MEDIUM — CHANGELOG omits the new per-operation passphrase prompt for protected shielded wallets (this review's CHANGELOG comment).
- LOW/INFO: password-floor "characters" vs UTF-8-bytes wording drift across three artifacts; duplicated
#[error]sentence with no shared source; untestedbroadcast_and_wait_for_affected_stateswap + missing rationale comment; whitespace-only password boundary untested; a dangling CHANGELOG cross-reference in a TODO; a mega-bullet burying the Tier-2 lockout; MCP/CLI caveat naming 3 of 5 shielded tools; a durable-delete failure surfaced in one caller but silently logged inclear_network_database.
What held up under scrutiny (credit where due): the byte-length password floor (with a genuine multibyte test), the narrowed Tier-2 re-wrap error handling, the WalletStateInconsistent mapping, the corrupt-FVK cold-boot resilience test, the monitor-revision gating test, atomic owner-only data-dir creation, and the secret-chokepoint hygiene are all real and non-vacuous. This PR did its homework — two fixable items stand between it and merge.
Full report (HTML + JSON) written to the review artifacts. Not approving while the confirmed privacy regression stands.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Claudius' grumpy-review — PR #919 (platform pin bump to #3968 tip), fresh pass
I put four specialists across the 22-file diff — Smythe (security), Adams (structural), Marvin (adversarial QA), Trillian (docs). Static verification only: bash was wedged sandbox-wide this run (a bwrap / /home/.mcp.json HOME misconfig killed every shell for me and every agent), so no cargo/consolidation scripts ran. CI remains the compile/test backstop; the report was hand-assembled.
Headline — still not mergeable, and it's the same blocker. thepastaclaw's open thread — wallet deletion retains the unencrypted shielded viewing key (FVK) in an automatic pre-delete backup — is re-confirmed against the pinned upstream source (dashpay/platform @ 4a1ba64c): SqlitePersisterConfig::new defaults auto-backup ON, delete_wallet (src/wallet_backend/mod.rs:1389) takes a full physical DB copy to backups/auto/pre-delete-*.db before cascading rows, delete_wallet_skip_backup exists as the intended remedy, and the two remove-wallet tests only query the live DB. This pass adds two fresh amplifiers: the F60 "delete all local data" sweep multiplies these snapshots and never removes them, and the removal dialog literally points users at that path as the remediation. Details on that thread.
Findings: 5 MEDIUM, 9 LOW, 1 INFO. Three of the five MEDIUMs are new and posted inline:
- 🔴 MEDIUM (blocking) — FVK retained in un-pruned pre-delete auto-backup after user-requested deletion → the open
delete_wallet_skip_backupthread. - 🟡 MEDIUM (new) —
remove_walletsilentlywarn!s a failed secret-bearing local wipe while surfacing the non-secret upstream failure; the siblingclear_network_databasedoes the inverse (removal.rs). - 🟡 MEDIUM (new) — the
broadcast_and_wait→broadcast_and_wait_for_affected_stateswap forDataContractUpdateis the only call site of that SDK method in the tree and has zero test coverage (update_data_contract.rs). - 🟡 MEDIUM (new) — the headless shielded caveat in
docs/CLI.md/docs/MCP.mdnames 3 of ≥5 affected tools, omitting shield-from-core, shield-from-platform, andshielded_init. - 🟡 MEDIUM — CHANGELOG omits the user-facing per-shielded-spend passphrase prompt (already on an open thread).
- LOW/INFO: char-vs-byte wording drift across three artifacts; unbounded backup growth; app-dir repair TOCTOU (documented residual); monitor-revision doc/code mismatch; duplicated error literal with no cross-test; missing password-floor boundary tests; untested Tier-2-lockout path with misleading copy; a 40-line CHANGELOG mega-bullet burying the "do not upgrade" blocker; a
WalletPasswordTooShortdoc comment that names a migration path it never fires from; and a missing repaint after the async removal banner (CodeRabbit's open thread).
Credit where due — this PR did its homework. The byte-length password floor (with a genuine multibyte "öäüß" test), atomic owner-only data-dir creation with symlink rejection, the corrupt-FVK cold-boot resilience test that drives the real path, clean typed TaskError variants, and secret-seam chokepoint hygiene all held up under adversarial scrutiny.
Full report (HTML + JSON) written to the review artifacts. Not approving while the confirmed FVK-backup privacy regression stands — the fix is a one-line switch to delete_wallet_skip_backup plus a backup-aware regression test.
🤖 Co-authored by Claudius the Magnificent AI Agent
|
|
||
| match state_transition.broadcast_and_wait(sdk, None).await { | ||
| match state_transition | ||
| .broadcast_and_wait_for_affected_state(sdk, None) |
There was a problem hiding this comment.
🟡 MEDIUM — a consensus/broadcast-method swap with zero test coverage.
This is the sole call site of broadcast_and_wait_for_affected_state in the entire tree. Every other state-transition broadcast — identity/add_key_to_identity.rs, tokens/pause_tokens.rs, tokens/destroy_frozen_funds.rs, tokens/update_token_config.rs, tokens/resume_tokens.rs — still uses broadcast_and_wait::<StateTransitionProofResult>. The swap arrived with this platform-pin bump, on the one path DET uses to publish an updated data contract on-chain, and its return value feeds self.replace_contract(...) directly (line 82) to replace the app's cached copy.
No unit or integration test exercises update_data_contract at all, and the CHANGELOG doesn't mention the swap or its rationale. Statically I couldn't confirm the exact semantic difference from broadcast_and_wait without the pinned SDK source — specifically whether "affected state" can return a different or partial contract object than the one actually applied. The PR description asserts the two paths are equivalent by direct source comparison; that reasoning is plausible but is exactly the kind of claim that deserves an executable pin.
Recommendation: add at least one test — a mocked-SDK unit test, or an #[ignore]d backend-e2e/live test per tests/backend-e2e/README.md — asserting the local contract cache updates correctly after the broadcast round trip, matching the pattern the token/identity call sites already exercise. And document the method-swap rationale in the CHANGELOG entry.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Agreed this needs test coverage — deliberately deferred rather than dropped. Adding a proper mocked-SDK/e2e test for the broadcast_and_wait_for_affected_state swap needs test infrastructure update_data_contract.rs doesn't have yet, and that's disproportionate scope for a dependency pin-bump PR. Tracked as follow-up work; the PR description's due-diligence section already discloses this gap explicitly.
🤖 Co-authored by Claudius the Magnificent AI Agent
…al warning, scope monitor-revision bump to new contacts Follow-up to the PR #919 comment-triage pass: - Wallet deletion now uses delete_wallet_skip_backup instead of delete_wallet, so removing a wallet no longer leaves an unencrypted copy of its shielded viewing key in an automatic pre-delete backup snapshot (BLOCKING privacy leak, thepastaclaw). Regression test extended to assert no auto-backup directory is created. - show_wallet_data_removal_warning now calls ctx.request_repaint(), so the async wallet-removal failure banner actually appears on an idle UI instead of waiting for an unrelated repaint (CodeRabbit). New unit test covers this directly. - register_contact_accounts_in_managed_wallet now bumps monitor_revision only on the specific newly-inserted DashpayReceivingFunds account, not every such account in the wallet, matching the function's own doc comment and avoiding needless bloom-filter/rescan churn on already -covered contacts (CodeRabbit nitpick). Regression test corrected and strengthened to assert per-account revisions individually. - CHANGELOG.md: password floor reworded to UTF-8 bytes (not characters, matching the actual fix), added the per-operation shielded passphrase prompt as its own user-facing bullet, and un-buried the Tier-2 lockout compatibility note into its own paragraph. Implemented by Codex Sol (gpt-5.6-sol, high effort), independently re-verified by the coordinator: fmt/clippy clean, wallet_backend (337 passed) and context::wallet_lifecycle (67 passed) test scopes re-run with the new/changed test names confirmed present and passing in the logs, not just an aggregate count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claudius-Maginificent
changed the title
|
Also fixed the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/context/wallet_lifecycle/tests.rs (1)
1699-1699: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove plaintext seed and password fixtures.
Generate these values at test runtime instead of embedding raw seeds or
"cold-boot-fvk-password"in the repository.
src/context/wallet_lifecycle/tests.rs#L1699-L1699: generate the removal-test seed at runtime.src/context/wallet_lifecycle/tests.rs#L1769-L1769: generate the failure-path seed at runtime.src/context/wallet_lifecycle/tests.rs#L2586-L2586: generate the alias-validation seed at runtime.src/context/wallet_lifecycle/tests.rs#L3784-L3788: generate both cold-boot seeds and the protected-wallet password at runtime.As per coding guidelines, “Never commit plaintext recovery phrases, private keys, passwords, seeds, or API tokens in source, tests, fixtures, or documentation.”
🤖 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/context/wallet_lifecycle/tests.rs` at line 1699, Remove embedded plaintext seed and password fixtures by generating equivalent random values at test runtime. Update the removal-test seed at src/context/wallet_lifecycle/tests.rs lines 1699-1699, the failure-path seed at lines 1769-1769, and the alias-validation seed at lines 2586-2586; update both cold-boot seeds and the protected-wallet password at lines 3784-3788. Preserve each test’s existing behavior while ensuring no raw seed or password remains in the source.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/context/wallet_lifecycle/tests.rs`:
- Line 1699: Remove embedded plaintext seed and password fixtures by generating
equivalent random values at test runtime. Update the removal-test seed at
src/context/wallet_lifecycle/tests.rs lines 1699-1699, the failure-path seed at
lines 1769-1769, and the alias-validation seed at lines 2586-2586; update both
cold-boot seeds and the protected-wallet password at lines 3784-3788. Preserve
each test’s existing behavior while ensuring no raw seed or password remains in
the source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d2e286b-aad1-4937-b347-dfe0873c4e54
📒 Files selected for processing (10)
CHANGELOG.mddocs/user-stories.mdsrc/backend_task/dashpay/payments.rssrc/backend_task/error.rssrc/context/mod.rssrc/context/wallet_lifecycle/removal.rssrc/context/wallet_lifecycle/tests.rssrc/model/wallet/mod.rssrc/wallet_backend/mod.rssrc/wallet_backend/single_key.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend_task/dashpay/payments.rs
- src/context/mod.rs
- src/wallet_backend/single_key.rs
- src/backend_task/error.rs
- src/context/wallet_lifecycle/removal.rs
- docs/user-stories.md
- src/model/wallet/mod.rs
- src/wallet_backend/mod.rs
There was a problem hiding this comment.
Grumpy-review pass — PR #919 (platform pin bump to #3968 tip)
Ran the full crew (Smythe/security · Adams/consistency · Marvin/QA) against d99073ac. Verdict: a disciplined, genuinely well-tested bump — the JIT shielded seam never leaves spend authority resident, data-dir creation is hardened to owner-only with symlink rejection, FVK reaping has a fault-injection test, and the upstream error enums are matched exhaustively. No new HIGH or CRITICAL.
Two new MEDIUM nits posted inline:
- The password-floor error message still says "characters" while the rule is enforced and documented in UTF-8 bytes — the one place this PR's own headline correction leaked (and two tests currently pin the wrong wording).
- The per-op passphrase-prompt caveat lists 3 of ≥6 shielded ops that actually prompt a protected wallet; that omission has now spread into the CHANGELOG.
The three standing MEDIUMs already tracked in earlier threads all still hold at HEAD: the silent secret-bearing wallet-wipe failure (removal.rs — note this diff widened the inversion by adding a banner to the less-sensitive path), the untested broadcast_and_wait_for_affected_state swap (update_data_contract.rs), and the incomplete shielded-ops docs. Plus a handful of LOWs (a WalletLocked→WalletStateInconsistent mapping left half-applied in two shielded siblings, an unreachable WalletPasswordTooShort variant, and two coverage gaps).
Nothing blocking that isn't already on record. Not approving while those standing MEDIUM threads remain open — disposition them and this is clean.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Round-4 grumpy-review — parallel security / structural / QA / docs pass
Credit where it's due: this is a well-scoped pin bump, and the regression coverage added across the earlier rounds is genuinely non-vacuous — a real SQL-corrupted FVK row with per-wallet isolation asserted, a real SQLite BEFORE DELETE trigger forcing an actual persister failure, per-key monitor_revision() assertions rather than an aggregate. All four independent reviewers agree there is nothing CRITICAL or HIGH here. The JIT shielded-seed lifetime is correct (never persisted, dropped on return), deletion correctly uses delete_wallet_skip_backup, and the data-dir hardening is careful work.
That said, three MEDIUM items survive verification against the current tree — each reinforcing an already-open thread with a new sub-angle. Posted inline:
removal.rs— the secret-bearingforget_wallet_local_statefailure is silent (warn-log only) while the no-secret async step got a banner; a fresh trace shows a single I/O fault can leave the seed and meta sidecar intact, resurrecting a deleted wallet (seed resolvable) on the next cold boot, with no user signal. This is the one I'd fix before merge.update_data_contract.rs— the lone-caller broadcast swap persists the returned contract keyed by the pre-broadcast id with noid()-match assertion, still with no test or in-code rationale.docs/MCP.md(and identicallyCLI.md,CHANGELOG.md,user-stories.md) — the protected-wallet caveat names 3 of the 6 shielded operations that actually prompt, so an automation dev hits an undocumentedSecretPromptUnavailableon the firstshielded-init/ shield step.
The remaining items (LOW/INFO) are documented, accepted, or cosmetic: unencrypted-at-rest FVKs (view-only, accepted trade-off), the unmerged-upstream-pin residual, the sub-8-byte Tier-2 lockout cohort, the app_dir chmod TOCTOU residual, a WalletLocked vs WalletStateInconsistent inconsistency the new helper left on two sibling paths, and the enduring "characters" vs "UTF-8 bytes" wording drift (defensible for the everyday-user register — but reconcile it with the docs or comment the choice).
Not approving while these three MEDIUM threads — plus the earlier open ones they build on — remain unaddressed. None are hard blockers on the PR's own terms; the wallet-resurrection consequence is the one worth a fix rather than a defer.
🤖 Co-authored by Claudius the Magnificent AI Agent
Branch feat/platform-wallet-storage-rehydration advanced with mainline v4.1-dev merges (release/version bumps, kotlin-sdk fix, docker test-suite image, drive proof wire-format versioning) since the last pin. No changes to dash-sdk/platform-wallet/platform-wallet-storage/rs-sdk-trusted-context-provider themselves in this range. Lockfile regenerated scoped to the platform git package set only (59 packages moved 4.0.0 -> 4.1.0-rc.1); no crates.io registry drift. Formatting and lint gates (all-features, all-targets, warnings-as-errors) both clean at this pin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Claudius review — fresh grumpy-review pass (head 0146af3)
Four specialist agents (security · structural · QA · docs) went over this bump. Credit where due first: this is a clean platform-pin bump. The JIT secret chokepoint is genuinely disciplined — borrow-only, zeroizing, poison-safe, never parked across an await, redacted Debug. Deletion moved to the no-backup entry point (a real privacy win, and test-asserted). All four platform pins sit at one consistent rev. And the API-drift adaptations ship real fault-injection regression tests, not smoke tests. No CRITICAL or HIGH findings surfaced. So, restrained applause. 🍬
A candid note on environment: this review ran in a sandbox with no working shell — cargo, git, and python were all unavailable — so every finding below is static (Read/Grep) verified against the PR-head tree and the GitHub diff. Full Cargo.lock registry drift and anything requiring compilation are left to this repo's own CI as the backstop. The consolidated report (report.json / report.html) was hand-assembled for the same reason.
Medium — worth settling before merge (all three already tracked on open threads)
The fresh pass independently re-confirmed the three open MEDIUM threads rather than raising duplicates; I've replied on each with the current-state verification:
- SEC-001 —
remove_walletstill swallows the secret-bearing wipe failure into atracing::warn!and reports success, while the less sensitive async removal keeps its banner. Visibility inverted against sensitivity. (thread) - CODE-001 —
update_data_contract's lonebroadcast_and_wait_for_affected_stateswap: no id-match guard beforereplace_contract, no rationale comment, zero test coverage at any level. (thread) - DOC-001 — docs/CHANGELOG/user-stories name 3 of the 6 shielded ops that prompt a protected wallet; the 3 omitted are the setup tools a headless integrator hits first. (thread)
One new item, not yet on any thread (LOW, but worth a glance)
- SEC-003 — password-floor unit asymmetry + trim/encrypt split (
src/model/wallet/mod.rs:448-457). The seed-protecting HD password floor is a byte count (soöäüß— 4 chars / 8 bytes — passes), while the single-key import floor is a character count. The more sensitive secret has the weaker floor. Separately, the HD path validates the trimmed length but callsencrypt_seedwith the untrimmed password — if the unlock modal doesn't trim identically, a whitespace-padded password becomes a latent lockout. Low probability, worth aligning the two floors and trimming consistently at validate/encrypt/unlock (or not at all). (I flagged the trim/encrypt half as static-only — I couldn't execute the unlock path to confirm the round-trip, so treat that sub-claim as unverified pending a quick check.)
The remaining items (FVK-at-rest, Tier-2 lockout cohort, app_dir chmod TOCTOU, the "Compatibility blocker/note" comment drift, and the "characters" vs UTF-8-bytes error wording) are LOW/INFO and captured in report.json.
Not approving while the three MEDIUM threads remain open — but none are blocking-critical, and each is a modest, well-scoped fix. Settle or explicitly defer them and this is ready to go.
🤖 Co-authored by Claudius the Magnificent AI Agent
* fix: tolerate and prevent empty-body Claude PR reviews The "Append report link to last Claude review" step PUTs a new body onto the last claude[bot] review. GitHub's review-update API rejects that PUT with 422 "Could not edit a review with a missing body" whenever the review's original body was empty — which happens whenever Claude posts a review with only inline comments and no summary text (confirmed on dashpay/dash-evo-tool#919). The step now falls back to a plain PR comment in that case, same as when no claude[bot] review exists at all. Also close the gap at the source: the review prompt (steps 3-4) now requires every review Claudius submits to carry a non-empty summary body, so this class of failure shouldn't recur. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
remove_wallet's local secret wipe (forget_wallet_local_state: seed vault, session cache, shielded rows) previously only logged a warning and still returned Ok(()) on failure, while the async upstream-removal step already surfaced its own failure via a persistent warning banner. Route the local-wipe failure through the same banner so a user is told when a secret-bearing wipe may not have fully completed, instead of being told removal succeeded. Adds a fault-injection regression test covering the new path. Addresses PR review findings CMT-001/CMT-002. Co-Authored-By: Codex Sol <noreply@openai.com>
Add validate_updated_contract_id: before caching the contract returned by a data-contract update broadcast, assert its id matches the contract that was updated, rejecting a mismatch via a new dedicated TaskError::UpdatedContractIdMismatch instead of trusting the response blindly. Covered by unit tests for both the accept and reject paths. Also reword TaskError::WalletPasswordTooShort and WalletCreationError::PasswordTooShort to say "UTF-8 bytes after trimming" instead of "characters" - the floor is enforced as a byte count, and the prior wording under-rejected multi-byte passwords by implying a stricter character-based rule than actually enforced. Updates the two display tests pinning this wording and the matching TODO comment in secret_access.rs. Addresses PR review findings CMT-004 and CMT-008. Co-Authored-By: Codex Sol <noreply@openai.com>
docs/MCP.md, docs/CLI.md, and CHANGELOG.md described the protected (Tier-2) wallet just-in-time seed resolution caveat as covering only shielded transfer/unshield/withdraw, but src/wallet_backend/shielded.rs routes six operations through the same with_secret_session mechanism: initialization, shield-from-Core, shield-from-Platform, transfer, unshield, and withdraw. Generalize all three artifacts to the full list so automation developers aren't surprised by an undocumented SecretPromptUnavailable on the other three. Addresses PR review findings CMT-005/CMT-006/CMT-007. Co-Authored-By: Codex Sol <noreply@openai.com>
Follow-up to 958c32a8: adds the forget_wallet_local_state_test_failure test-only hook on WalletBackend (cfg(test)-gated, mirrors the existing registration_test_failure/clear_shielded_test_failure pattern in this file) that the remove_wallet_warns_when_local_secret_wipe_fails regression test forces to exercise the new warning-banner path. Co-Authored-By: Codex Sol <noreply@openai.com>
…rror The dashpay/platform pin bump (0146af3) picked up a stricter upstream persister: core_state::load_state in platform-wallet-storage now validates the core_transactions.txid column's fixed width on every row and fails the entire cold-boot load hard when it's malformed, where the prior pin never read that column at load time at all (a corrupt row was silently untouched). This is an intentional upstream tightening, accepted here rather than worked around. Add TaskError::WalletLocalDataLoadFailed with an accurate, actionable message ("Saved wallet data appears damaged and cannot be loaded. Restore the wallet from its recovery phrase to keep using it.") and map only PlatformWalletError::PersisterLoad(PersistenceError::Backend { kind: Fatal, .. }) to it in load_from_persistor_seedless, leaving Transient/Constraint and every other PersisterLoad-adjacent call site on its existing behavior. The prior generic "please retry" wording was actively misleading for this case: the corruption is permanent, so retrying fails identically every time. Renamed and rewrote the now-stale regression test cold_boot_keeps_wallet_visible_when_persisted_transaction_txid_is_corrupt (assumed the old lenient, skip-corrupt-row contract) to cold_boot_surfaces_typed_error_when_persisted_transaction_txid_is_corrupt, asserting the new typed error instead. Co-Authored-By: Codex Sol <noreply@openai.com>
There was a problem hiding this comment.
Claudius grumpy-review — PR #919 (platform-pin bump: seedless rehydration + shielded FVK persistence)
Three independent specialist passes (security · project-consistency · adversarial QA), run statically against HEAD b17e71ca. Verdict up front: a clean, well-tested bump. No CRITICAL, no HIGH, and — pleasingly — every previously-open finding from the earlier rounds is confirmed fixed at this tip. What remains is 3 MEDIUM (two are one-line changelog corrections), plus LOW/INFO carried in the attached report.
Environment note: this run had no working shell (a
bwrapinfra failure blocked allcargo/git), so verification was static — reading code, the diff, and git history. The repo's own CI remains the full-suite backstop; one QA item (below) is explicitly flagged unverified because it needs a compile to confirm.
🟡 MEDIUM findings
1. PROJ-001 — the CHANGELOG names the wrong platform commit. CHANGELOG.md:64 announces the pin moved d18020f → 4a1ba64, but Cargo.toml/Cargo.lock actually pin 288a6cae4f9653d6085d2b3d6c7410210a0c95ba. 4a1ba64 appears nowhere in the manifest — the tip advanced past it in commit 0146af3 and the headline never caught up. Transitive SHAs are correct; only the platform tip is stale. For a PR whose entire purpose is the pin, its release note pointing at an unshipped revision is worth a one-line fix: use 288a6ca.
2. SEC-001 — shielded viewing keys are plaintext at rest, even for Tier-2 wallets. Native FVK persistence writes the Orchard Full Viewing Key unencrypted into shielded_viewing_keys. An FVK cannot spend — but it grants complete read access to the wallet's entire shielded transaction graph (amounts, memos). The asymmetry bites password-protected wallets: the spend seed is sealed under the Tier-2 object password, yet the viewing key for the same wallet is cleartext, so anyone who can read platform-wallet.sqlite deanonymises the whole shielded history with no password. This is a documented, upstream-classified ("viewing-grade, not secret") accepted residual and the 0o700 hardening in this PR helps — the gap is that the Tier-2-specific at-rest exposure isn't surfaced to the user. Either seal the FVK under the same Tier-2 envelope when the wallet is protected, or state plainly in the protected-wallet caveats that viewing keys are not protected at rest.
3. COR-001 — one corrupt persisted transaction row now bricks cold-boot for all wallets (independently found by both security and QA). src/wallet_backend/mod.rs:673-711: upstream reclassifies a corrupt Core-transaction row as Fatal, DET maps it to TaskError::WalletLocalDataLoadFailed, and because load_from_persistor_seedless makes a single whole-persister load and ?-propagates through WalletBackend::new, one damaged row denies every wallet in the profile and advises a destructive "restore from recovery phrase". This regresses the prior skip-the-row behaviour and is inconsistent with this same PR's corrupt-FVK handling, which correctly isolates per wallet (cold_boot_skips_corrupt_fvk_for_one_wallet_and_restores_healthy_wallet). The typed error and actionable message are good; the all-wallets blast radius and its inconsistency are the concern. Confirm whether upstream can report per-wallet partial failures (prefer per-row quarantine); at minimum soften the message/CHANGELOG so users know this is an all-wallets, not single-wallet, event.
✅ Confirmed fixed at HEAD (not re-reported)
Local secret-wipe failure now surfaced via warning banner; validate_updated_contract_id guards the broadcast-swap with a typed UpdatedContractIdMismatch and genuine accept+reject tests (closes the prior "zero coverage" gap for real); FVK-in-auto-backup closed via delete_wallet_skip_backup with a reaping test; the password floor is measured in UTF-8 bytes with a real multibyte test and byte-identical messaging across all four sites; all six shielded ops enumerated consistently across the docs; JIT seed handling is chokepoint-disciplined, Zeroizing, and dropped on every path. The disputed SHORT_LEGACY_PASSWORD="short" test fixture remains a test-only placeholder — not re-escalated.
🔵 LOW / INFO (see attached report, not blocking)
SEC-002 (TOCTOU on the dir-repair chmod), SEC-004 (fire-and-forget durable delete can resurrect a deleted wallet on non-completion), SEC-005 (deps pinned to an unmerged PR tip — re-pin to the merged commit before release), PROJ-002 ("no crates.io deps change" vs transitive itertools/windows-sys deltas), PROJ-003 (dangling "Compatibility blocker" TODO ref), CODE-001 (8-byte floor only on the HD path, not single-key), QA-002 (2 of 6 shielded ops still map the structural condition to WalletLocked), QA-003 (removal-warning test asserts banner presence, not content), QA-004 (unverified statically: whether the txid fixture actually yields a Fatal-kind error — CI confirms).
Not approving solely because the three MEDIUM items deserve a look first; none is a correctness or security blocker. Tidy work overall — a rare pleasure to review a pin bump this thoroughly self-audited.
🤖 Co-authored by Claudius the Magnificent AI Agent
github-actions
Bot
removed
the
claudius-review
The pin-bump changelog entry still cited the d18020f -> 4a1ba64 range from an earlier round; the manifest has since moved on to 288a6cae. Update the cited commit so the changelog matches what is actually pinned in Cargo.toml/Cargo.lock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n + shielded viewing-key persistence) (dashpay#919) * chore: bump dashpay/platform pin to PR #3968 tip (d18020f → f376d32) Bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from d18020f to f376d32, the tip of platform PR #3968 — "feat(platform- wallet-storage): embeddable SQLite persistence backend with seedless rehydration". Cargo.lock is regenerated scoped to the platform package set: only dashpay-ecosystem git deps moved (platform, rust-dashcore be6e776→0091c4a, grovedb v5.0.0→v5.0.1, orchard) — all transitively pinned by the new platform rev; no crates.io registry drift. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - AssetLockFunding::FromExistingAssetLock gained a consume_invitation_voucher field. Set false at the three generic funding sites (identity register, identity top-up, platform-address funding) — none is the DashPay invitation-voucher reclaim flow, so a bearer-voucher lock is never consumed. - shielded_transfer_to / shielded_unshield_to / shielded_withdraw_to each gained a seed: &[u8] parameter. This is the seedless-rehydration privilege- separation change: the Orchard spend keyset (ASK included) is re-derived from the seed for the single call and dropped on return, never left resident. DET's shielded_transfer/unshield/withdraw now resolve the HD seed just-in-time through the secret-seam chokepoint (with_secret_session), mirroring the existing shield_from_balance path; stale "no seed scope needed" doc comments corrected. - PlatformWalletError gained PersisterStore and PersisterRestore variants. Added to both exhaustive (no wildcard) match sites — map_shielded_op_error and identity_op_error_kind — in the generic persistence bucket alongside PersisterLoad / Persistence. Verification (via cargo-cached.sh ledger): - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2009 passed, 0 failed, 1 ignored - cargo fmt --all -- --check — exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: fold in security-review follow-ups for the platform#3968 pin bump Follow-ups from the security due-diligence pass on the dashpay/platform PR #3968 diff (companion to the pin-bump commit on this branch). No behavior change — comment-only edits plus a supply-chain audit. - Verified the new reserve-on-hand-out receive-address behavior does NOT reach DET. Upstream PlatformAddressWallet::next_unused_receive_address now reserves (via key-wallet next_unused_and_reserve), but DET calls neither it (DET's platform receive path is its own index-based generate_platform_receive_address_with_seed) nor a reserving Core variant. DET's Core receive path (next_receive_address → next_receive_address_for_- account → AddressPool::next_unused) stays non-reserving; its only change (!used → is_available()) is behavior-neutral because DET never reserves. Refreshed the TC-012 TODO's stale merge-status note: rust-dashcore#818 is now present in the pinned rev, while the Core reserving surface (CoreWallet::next_receive_address_and_reserve_for_account) still is not. - Fixed two stale doc comments spotted in the diff: new_watch_only → new_external_signable (context/wallet_lifecycle/tests.rs) and AddressInfo.used → AddressInfo.is_used() (backend_task/dashpay/payments.rs). - cargo audit over the regenerated Cargo.lock: advisory-ID set is identical to base v1.0-dev (14 advisories; zero introduced, zero removed) and the yanked- crate set is identical — no new advisory beyond the pre-existing, already- acknowledged bincode RUSTSEC-2025-0141. dashcore/key-wallet unify to a single rev (0091c4a) in the lock; no conflicting revisions. Verification (via cargo-cached.sh ledger, final tree): - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2009 passed, 0 failed, 1 ignored - cargo fmt --all -- --check — exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(shielded): correct WalletLocked doc, add guard test (Marvin QA-002/003) Adversarial-review follow-ups on the shielded seed-resolution change from the platform#3968 pin bump. No functional change to the happy path. - QA-003: the doc comments on shielded_transfer/unshield/withdraw claimed a locked protected wallet yields WalletLocked "if its seed isn't cached". That is wrong: with_secret_session PROMPTS for a protected wallet; a dismissed or headless prompt surfaces as SecretPromptCancelled / SecretPromptUnavailable. WalletLocked comes ONLY from a non-HD-seed secret sitting at an HD scope (a misconfigured wallet). Rewrote all three docs to describe the real prompt path and reframe WalletLocked as that defensive type guard. - QA-002: the new expose_hd_seed()->WalletLocked branch (a genuinely new failure mode — these fns took no seed before the bump) had zero unit coverage, reachable today only via network-gated #[ignore] e2e. Extracted the mapping into a small documented helper, hd_seed_for_shielded_spend, and added two unit tests pinning HdSeed->Ok and SingleKey->WalletLocked. - QA-005: hoisted resolve_wallet + CachedOrchardProver::new() (neither needs the seed) out of the secret session in all three fns, matching the already-hoisted coordinator — trims the secret-session window's front edge and fails fast (ShieldedNotConfigured / WalletNotLoaded) without prompting. - QA-004: CHANGELOG now names the transitive dashpay git-dep bumps carried by the platform pin — rust-dashcore be6e776->0091c4a, grovedb v5.0.0->v5.0.1, and the orchard fork dashified-0.14.0->0.14.1. Verification (via cargo-cached.sh ledger, final tree): - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2011 passed, 0 failed, 1 ignored (+2 new: hd_seed_for_shielded_spend_{accepts_hd_seed, rejects_non_hd_secret_as_wallet_locked}) - cargo fmt --all -- --check — exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: bump dashpay/platform pin to PR #3968 tip (f376d32 → ebbd15c4) Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from f376d32 to ebbd15c4, the current tip of platform PR #3968 — "feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration". Verified via git ls-remote against feat/platform-wallet-storage-rehydration that the PR has not moved past this rev. Cargo.lock is regenerated scoped to the platform package set: 27 platform git packages moved to ebbd15c (document-history-contract added transitively); rust-dashcore, grovedb, orchard, and grovestark pins are unchanged; no crates.io registry drift. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - PlatformWalletError gained PlatformNodePool, CoreInsufficientFunds, AssetLockNotTracked, AssetLockAlreadyConsumed, and AssetLockFundingMismatch variants. Classified in both exhaustive (no wildcard) match sites -- map_shielded_op_error and identity_op_error_kind -- alongside the existing precondition/wallet-state bucket. - The upstream changeset schema gained a shielded.viewing_keys field with a new PersistenceCapabilities::SHIELDED_VIEWING_KEYS contract, needed for seedless shielded restart (Orchard FVKs must survive a restart without the HD seed). Added src/wallet_backend/persister.rs: a thin DetPersister adapter wrapping the upstream SqlitePersister -- delegates everything except the new viewing-key changeset, which it persists itself via the existing DetKv abstraction (already used for app data alongside wallet state), scoped per wallet_id and guarded by a mutex. Rejects any mixed changeset (viewing keys plus anything else) to preserve upstream's ATOMIC_CHANGESETS invariant rather than splitting one commit across the typed SQLite tables and the metadata table. Note this supersedes the prior "DET does not write its own persister" doc comment: DET now owns exactly the one host-specific extension upstream doesn't yet provide, not a reimplementation of the persister itself. - The later review tip also retries transient startup rehydration, selects platform-address transfer/withdrawal inputs from hydrated candidates with authoritative on-chain balances, freezes the SPV sync watermark when persistence fails, and persists address-reservation timestamps plus DashPay address used-state updates -- no DET call-site changes required for these; CHANGELOG updated to describe the user-visible effect. Verification ledger logs under /data/artifacts/dash-evo-tool/2026-07-22/platform-pin-bump/final-ledger/logs/, all exit 0: fmt --check, build --all-features, clippy --all-features --all-targets -D warnings, plus targeted lib tests -- wallet_backend (327 passed), context::wallet_lifecycle (62 passed), shielded (71 passed). Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * fix(wallet-backend): close FVK-persister due-diligence findings (SEC-007, RUST-001) Independent security (Smythe) and structural (Adams) due-diligence review of f7ca95f (the platform#3968 pin re-bump) surfaced two MEDIUM findings that escalated to a confirmed HIGH once Marvin produced a red repro, plus two cheap LOW nits. All fixed here, on top of f7ca95f, before push. - SEC-007 (HIGH, confirmed via red repro): DetPersister::load() previously `?`-propagated ANY single wallet's corrupt/undecodable shielded-FVK row, failing cold-boot rehydration for every wallet in the store, not just the corrupted one. load_viewing_keys now distinguishes decode/schema-version/ truncated/oversized-value failures (degrade to an empty FVK set for the affected wallet, with a warning log carrying the wallet id and error shape, no FVK bytes) from genuine backend/IO failures (still fatal, e.g. LockPoisoned), matching this module's established kv_get_logged degrade-to-absent convention (kv.rs) and the sibling cold_boot_keeps_wallet_visible_when_persisted_transaction_txid_is_corrupt precedent. - RUST-001 (MEDIUM): is_viewing_key_only now exhaustively destructures the nested ShieldedChangeSet (all six upstream fields), not just field-access checks — a future 7th upstream field is now a compile error instead of being silently ignored by the mixed-changeset reject guard. - PROJ-001 (LOW): SHIELDED_FVK_KEY renamed "shielded_fvks.v1" -> "shielded:fvks:v1" to match this module's colon-namespaced key convention, before the old spelling became on-disk schema. - PROJ-002 (LOW): corrected the Inner.persister doc comment, which read as if `pwm` held this exact Arc directly; it now describes that `pwm` consumes the DetPersister wrapper around this same SqlitePersister. - Added 6 tests: a table-driven is_viewing_key_only classification test; a store/load FVK round-trip; a test proving oversized/decode failures degrade to absent while genuine backend failures stay fatal; mixed- changeset and cross-wallet-id rejection tests (both asserting no partial write); and a real two-wallet, two-boot cold-start regression test (cold_boot_skips_corrupt_fvks_for_one_wallet_and_restores_healthy_wallet) that persists both wallets for real, corrupts one's on-disk FVK row under its actual upstream wallet id, and confirms cold boot succeeds with the healthy wallet's viewing key still installed. A prior throwaway proof-of-concept test from the review pass (green-from-start, non-probative) was reverted rather than kept. An open follow-up from the same review — whether persister.rs scoping FVK rows by the upstream WalletId instead of this module's usual WalletSeedHash orphans the row when a wallet is later removed (both are bare `[u8; 32]` aliases, so it type-checks either way) — is still being verified empirically and will land separately if it proves to be a real gap. Verification (via cargo-cached.sh ledger): - cargo fmt --all -- --check — exit 0 - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features wallet_backend — 332 passed, 0 failed - cargo test --lib --all-features context::wallet_lifecycle — 63 passed, 0 failed - cargo test --lib --all-features shielded — 71 passed, 0 failed Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * fix(wallet-backend): reap persisted FVK row on wallet removal (SEC-008/SEC-010) Fast-follow from the same platform#3968 pin-bump due-diligence review that produced eb7e0fa. Confirmed empirically (Smythe, twice): removing a wallet via forget_wallet_local_state + remove_upstream_wallet never deletes the persisted shielded:fvks:v1 row persister.rs (eb7e0fa) added. Root cause: upstream's PlatformWalletPersistence trait has no delete_wallet method at this rev (a separately-tracked, pre-existing upstream gap - out of scope here), and DET's own removal path never touched this new KV row either. Net effect: an Orchard viewing key outlives the wallet it belonged to, orphaned on disk indefinitely. Privacy-only (no spend secrets involved - those are already correctly wiped by the seed-vault deletion this function already does) and fund-safety-neutral, so this is a fast-follow rather than a blocker alongside SEC-007/RUST-001, per reviewer sign-off. - persister.rs: added forget_wallet_viewing_keys(kv, wallet_id), keyed by the upstream WalletId (matching what store_viewing_keys/load_viewing_keys actually key by - not this module's usual WalletSeedHash), reusing the existing SHIELDED_FVK_KEY constant rather than re-typing the key string. - mod.rs: forget_wallet_local_state now calls this best-effort in the `if let Some(wallet_id)` block (wallet_id is only Some when the wallet was actually upstream-registered, so there's never a row to reap otherwise), logging a warning on failure rather than aborting the rest of the wipe - consistent with this function's existing resilience contract for lesser (non-secret) cleanup steps. - tests.rs: added remove_wallet_reaps_persisted_shielded_viewing_keys, which registers a wallet, binds shielded (persists an FVK row), verifies the row exists, runs the real removal path, then re-opens the SQLite file directly and asserts the row is gone. Verification (via cargo-cached.sh ledger): - cargo fmt --all -- --check - exit 0 - cargo clippy --all-features --all-targets -- -D warnings - exit 0 - cargo test --lib --all-features wallet_backend - 332 passed, 0 failed (includes remove_wallet_reaps_persisted_shielded_viewing_keys) Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * chore: bump dashpay/platform pin to PR #3968 tip (ebbd15c4 → e75f259) Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from ebbd15c4 to e75f259, the current tip of platform PR #3968. Verified via gh pr view headRefOid immediately before commit. Cargo.lock is regenerated scoped to the platform package set: 28 platform git packages moved to e75f259. rust-dashcore, grovedb, orchard, and grovestark pins are unchanged; no crates.io registry versions, checksums, or dependency edges moved. API-drift fixes required by the upstream jump (each verified against the new upstream source, not guessed): - The hardened file and SQLite backends now validate permissions on every parent directory. Normalize the app-data root, SPV directory, and per-network directory to owner-only on Unix before opening storage; copied cold-boot fixtures preserve that invariant. - PlatformWalletPersistence gained delete_wallet with a default UnsupportedOperation result. DetPersister now delegates the method to its inner SqlitePersister so wallet deletion keeps the typed backend behavior and cascade cleanup. - The secret store now enforces an eight-byte passphrase floor. Reject incompatible passwords for new HD wallets before creating an envelope, while keeping unmigrated short-password legacy wallets usable by deferring rejected Tier-2 rewraps and retaining the legacy envelope. - Upstream now persists shielded viewing keys natively. The legacy DET metadata adapter remains in place for databases written by prior pins; platform-wallet and platform-wallet-storage retain matching shielded feature flags. Known upstream compatibility limitation: July 2026 weekly builds could already write a short-password Tier-2 seed under the previous one-byte floor and delete its legacy envelope. e75f259 rejects that password before decryption, and no public downstream compatibility reader exists. CHANGELOG documents that affected profiles must not upgrade until upstream provides read-old/write-new support; this commit must not be pushed or merged as-is for those profiles. PlatformWalletError did not gain variants in this jump, so the existing exhaustive match classifications remain complete. Verification: - cargo fmt --all -- --check — exit 0 - cargo clippy --all-features --all-targets -- -D warnings — exit 0 - cargo test --lib --all-features — 2054 passed, 0 failed, 1 ignored - cargo test --all-features wallet_backend — 335 passed, 0 failed - cargo test --all-features context::wallet_lifecycle — 64 passed, 0 failed - cargo test --all-features shielded — 72 passed, 0 failed, plus 3 ignored backend-e2e cases Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * refactor(wallet-backend): drop obsolete DetPersister Use upstream SqlitePersister for native viewing-key persistence. Drive delete_wallet after runtime detachment to trigger its verified cascade. Retain the removal integration test and drop redundant workaround coverage. Document why unreleased interim metadata rows are not migrated. Co-Authored-By: OpenAI Codex <noreply@openai.com> * docs(wallet-backend): note upstream read-path floor blocks short-password Tier-2 seeds The new pin's password-length floor rejects a sub-8-character passphrase on read, not just write, mirroring the write-side check as a deliberate defense against a backend-write attacker planting a weakly sealed envelope. Mark the affected call site so it gets revisited once upstream adds a scoped migration-read capability. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump dashpay/platform pin to PR #3968 tip (e75f259 → 4a1ba64) Re-bumps the four dashpay/platform git dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) from e75f259 to 4a1ba64, the current tip of platform PR #3968. Cargo.lock is regenerated scoped to the platform package set: 28 platform git packages moved to 4a1ba64 and 11 rust-dashcore packages moved from 0091c4a to 18c68d4. No crates.io registry versions, checksums, dependency edges, or unrelated sources moved. API-drift fixes required by the upstream jump: - DataContractUpdate waits for affected state instead of generic broadcast completion, while retaining the typed TaskError conversion and existing DriveProofError recovery path. - Contact receiving accounts use ManagedAccountOperations so newly added accounts invalidate stale compact-filter coverage. Existing contact keys remain a true no-op, and only real insertions bump monitor_revision. - The changelog records authoritative pure-SPV broadcast outcomes and the contact-account filter-coverage fix without claiming shielded identity creation support in DET. Verification ledger evidence (all exit 0): - cargo fmt --all — key 9ff5de5fa46203d0ebbf5580836bddf9 — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T111934-9ff5de5fa46203d0ebbf5580836bddf9-2.log - cargo test contact_account_registration_invalidates_filter_coverage_once --all-features — key 3cdb20d7ef39ab598af859ff3fcf6020 — 1 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T111944-3cdb20d7ef39ab598af859ff3fcf6020-2.log - cargo test --lib --all-features wallet_backend — key b062125daa4c22c77c2199c8f74d74a2 — 330 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112028-b062125daa4c22c77c2199c8f74d74a2-2.log - cargo test --lib --all-features backend_task — key 2fa67207f0f111422cde6544d6a29bac — 411 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112049-2fa67207f0f111422cde6544d6a29bac-2.log - cargo clippy --lib --all-features -- -D warnings — key 4a6af394e0e2293a6da5a0e5f824af5c — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T112100-4a6af394e0e2293a6da5a0e5f824af5c-2.log - git diff --check — exit 0 Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * test(wallet-backend): cover contact monitor revision gating Extract the post-derivation contact-account registration loop into the smallest production seam that can be exercised without wallet persistence, SPV, or network infrastructure. The regression now proves that a first contact registration increments the aggregate monitor revision, a duplicate registration leaves it unchanged, and a mixed existing/new batch increments it exactly once. It retains the account-generation and filter-coverage invalidation assertions from the original lower-level test. Document the DashpayReceivingFunds-only helper contract and its boolean result. Keep the defensive variant check for future callers, and retain the contact-key pre-check because upstream reports duplicate managed account types as InvalidParameter instead of providing idempotent no-op semantics. Verification ledger evidence (all exit 0): - cargo fmt --all — key 7e9402392e7b60075b2c66376240c2b1 — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115050-7e9402392e7b60075b2c66376240c2b1-2.log - cargo test contact_registration_bumps_monitor_revision_only_for_new_keys --all-features — key 979201f10ee2e2db3aba25ed91ba11d6 — 1 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115104-979201f10ee2e2db3aba25ed91ba11d6-2.log - cargo test --lib --all-features wallet_backend — key c35703b8723b6de9ce7b907df35e85e9 — 330 passed — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115206-c35703b8723b6de9ce7b907df35e85e9-2.log - cargo clippy --lib --all-features -- -D warnings — key 7cf2092cee6223ee239e8c37f9e4a35b — /data/git-worktrees/home-ubuntu-git-dash-evo-tool-platform-pin-bump/.claudius-ledger/logs/20260723T115214-7cf2092cee6223ee239e8c37f9e4a35b-2.log - git diff --check — exit 0 Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com> * fix(wallet-backend): address grumpy-review findings from PR dashpay#919 - cover corrupt FVK cold boot and surface asynchronous removal failures (PROJ-001, CALL-001) - align password validation, documentation, and actionable errors (PROJ-003, PROJ-004, CODE-001) - narrow lazy Tier-2 re-wrap handling and fix shielded invariant mapping (CODE-003, CODE-004) - document protected headless shielded spends and harden data-directory creation (CALL-003, SEC-004/CALL-002) Co-Authored-By: Codex GPT-5 <noreply@openai.com> * fix(wallet-backend): skip auto-backup on wallet delete, repaint removal warning, scope monitor-revision bump to new contacts Follow-up to the PR dashpay#919 comment-triage pass: - Wallet deletion now uses delete_wallet_skip_backup instead of delete_wallet, so removing a wallet no longer leaves an unencrypted copy of its shielded viewing key in an automatic pre-delete backup snapshot (BLOCKING privacy leak, thepastaclaw). Regression test extended to assert no auto-backup directory is created. - show_wallet_data_removal_warning now calls ctx.request_repaint(), so the async wallet-removal failure banner actually appears on an idle UI instead of waiting for an unrelated repaint (CodeRabbit). New unit test covers this directly. - register_contact_accounts_in_managed_wallet now bumps monitor_revision only on the specific newly-inserted DashpayReceivingFunds account, not every such account in the wallet, matching the function's own doc comment and avoiding needless bloom-filter/rescan churn on already -covered contacts (CodeRabbit nitpick). Regression test corrected and strengthened to assert per-account revisions individually. - CHANGELOG.md: password floor reworded to UTF-8 bytes (not characters, matching the actual fix), added the per-operation shielded passphrase prompt as its own user-facing bullet, and un-buried the Tier-2 lockout compatibility note into its own paragraph. Implemented by Codex Sol (gpt-5.6-sol, high effort), independently re-verified by the coordinator: fmt/clippy clean, wallet_backend (337 passed) and context::wallet_lifecycle (67 passed) test scopes re-run with the new/changed test names confirmed present and passing in the logs, not just an aggregate count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump dashpay/platform pin to PR #3968 tip (4a1ba64c -> 288a6cae) Branch feat/platform-wallet-storage-rehydration advanced with mainline v4.1-dev merges (release/version bumps, kotlin-sdk fix, docker test-suite image, drive proof wire-format versioning) since the last pin. No changes to dash-sdk/platform-wallet/platform-wallet-storage/rs-sdk-trusted-context-provider themselves in this range. Lockfile regenerated scoped to the platform git package set only (59 packages moved 4.0.0 -> 4.1.0-rc.1); no crates.io registry drift. Formatting and lint gates (all-features, all-targets, warnings-as-errors) both clean at this pin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): surface local secret-wipe failures on removal remove_wallet's local secret wipe (forget_wallet_local_state: seed vault, session cache, shielded rows) previously only logged a warning and still returned Ok(()) on failure, while the async upstream-removal step already surfaced its own failure via a persistent warning banner. Route the local-wipe failure through the same banner so a user is told when a secret-bearing wipe may not have fully completed, instead of being told removal succeeded. Adds a fault-injection regression test covering the new path. Addresses PR review findings CMT-001/CMT-002. Co-Authored-By: Codex Sol <noreply@openai.com> * fix(contracts): reject mismatched update responses, fix password wording Add validate_updated_contract_id: before caching the contract returned by a data-contract update broadcast, assert its id matches the contract that was updated, rejecting a mismatch via a new dedicated TaskError::UpdatedContractIdMismatch instead of trusting the response blindly. Covered by unit tests for both the accept and reject paths. Also reword TaskError::WalletPasswordTooShort and WalletCreationError::PasswordTooShort to say "UTF-8 bytes after trimming" instead of "characters" - the floor is enforced as a byte count, and the prior wording under-rejected multi-byte passwords by implying a stricter character-based rule than actually enforced. Updates the two display tests pinning this wording and the matching TODO comment in secret_access.rs. Addresses PR review findings CMT-004 and CMT-008. Co-Authored-By: Codex Sol <noreply@openai.com> * docs: cover all six shielded operations in protected-wallet caveats docs/MCP.md, docs/CLI.md, and CHANGELOG.md described the protected (Tier-2) wallet just-in-time seed resolution caveat as covering only shielded transfer/unshield/withdraw, but src/wallet_backend/shielded.rs routes six operations through the same with_secret_session mechanism: initialization, shield-from-Core, shield-from-Platform, transfer, unshield, and withdraw. Generalize all three artifacts to the full list so automation developers aren't surprised by an undocumented SecretPromptUnavailable on the other three. Addresses PR review findings CMT-005/CMT-006/CMT-007. Co-Authored-By: Codex Sol <noreply@openai.com> * test(wallet): add local secret-wipe fault-injection hook Follow-up to 958c32a8: adds the forget_wallet_local_state_test_failure test-only hook on WalletBackend (cfg(test)-gated, mirrors the existing registration_test_failure/clear_shielded_test_failure pattern in this file) that the remove_wallet_warns_when_local_secret_wipe_fails regression test forces to exercise the new warning-banner path. Co-Authored-By: Codex Sol <noreply@openai.com> * fix(wallet): surface fatal persisted-load corruption as a dedicated error The dashpay/platform pin bump (0146af3) picked up a stricter upstream persister: core_state::load_state in platform-wallet-storage now validates the core_transactions.txid column's fixed width on every row and fails the entire cold-boot load hard when it's malformed, where the prior pin never read that column at load time at all (a corrupt row was silently untouched). This is an intentional upstream tightening, accepted here rather than worked around. Add TaskError::WalletLocalDataLoadFailed with an accurate, actionable message ("Saved wallet data appears damaged and cannot be loaded. Restore the wallet from its recovery phrase to keep using it.") and map only PlatformWalletError::PersisterLoad(PersistenceError::Backend { kind: Fatal, .. }) to it in load_from_persistor_seedless, leaving Transient/Constraint and every other PersisterLoad-adjacent call site on its existing behavior. The prior generic "please retry" wording was actively misleading for this case: the corruption is permanent, so retrying fails identically every time. Renamed and rewrote the now-stale regression test cold_boot_keeps_wallet_visible_when_persisted_transaction_txid_is_corrupt (assumed the old lenient, skip-corrupt-row contract) to cold_boot_surfaces_typed_error_when_persisted_transaction_txid_is_corrupt, asserting the new typed error instead. Co-Authored-By: Codex Sol <noreply@openai.com> * docs: fix stale platform pin reference in CHANGELOG The pin-bump changelog entry still cited the d18020f -> 4a1ba64 range from an earlier round; the manifest has since moved on to 288a6cae. Update the cited commit so the changelog matches what is actually pinned in Cargo.toml/Cargo.lock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex gpt-5.6-sol <noreply@openai.com>
|
This pin regressed fetch_current: descending-epoch query now fails proof verification every sync, silently disabling shielded-tx gating + fee-multiplier refresh. Fix upstream: dashpay/platform#4231. 🤖 Co-authored by Claudius the Magnificent AI Agent |
3 participants
TL;DR: Bump the pinned
dashpay/platformdependency to the current tip of upstream PR #3968, landing seedless wallet rehydration and native persistence for shielded (Orchard) viewing keys, and adapting DET's wallet backend to further API changes as the branch has advanced. Closes review findings from a security/structural/adversarial due-diligence pass on each adaptation, across four rounds so far (initial bump review, a grumpy-review follow-up, a PR-comment triage follow-up, and a further re-bump + regression-fix + comment-triage round): one HIGH and one MEDIUM security issue, three LOW privacy leaks, one MEDIUM test-coverage gap, one MEDIUM cold-boot resilience regression (an intentional upstream behavior change, accepted and given an accurate error message rather than worked around), and several LOW/MEDIUM nits — all fixed and verified before merge. Correction (2026-07-23): a follow-up grumpy-review pass found this description previously (and incorrectly) claimed "no user-observable behavior change" — see "User-observable changes" below for what actually changed.User story
As a DET user with a password-protected wallet that sends or receives shielded (Orchard) funds, I want the wallet's underlying platform dependency updated with upstream's latest persistence and security improvements, so my shielded funds stay fully recoverable from my seed phrase and my data directories are properly protected — even though it means a stricter password floor and an extra passphrase prompt per shielded transaction.
Scenario
Base flow
A user creates a wallet (optionally password-protected), uses it for transparent and shielded sends/receives/withdrawals, and expects that data to survive app restarts and be fully recoverable from the seed phrase alone.
Actual behavior (before this PR)
Expected behavior (after this PR)
Detailed discussion
Shielded module changes
This bump changes how DET handles shielded (Orchard) key material end-to-end:
platform-wallet-storagenow persists each wallet's shielded Full Viewing Key (FVK) natively in its ownshielded_viewing_keystable, replacing DET's bespokeDetPersistermetadata adapter (retired once upstream gained this support). This is what makes seedless shielded rehydration possible: DET can resync/rebind shielded state on a later launch without re-entering a passphrase.m / 32' / coin_type' / account'); only the FVK (view-only, cannot authorize a spend) is what's cached to disk, computed once at bind time. The spend authority itself is re-derived from the seed transiently on every shielded operation and dropped immediately after — never persisted. Losing the persisted FVK costs a rescan, not funds; restoring from seed phrase alone recovers everything. (Verified against the pinnedorchard/rust-dashcore/platform-walletsources — see the PR discussion for the full derivation-path citation.)shielded_transfer/shielded_unshield/shielded_withdrawnow resolve the HD seed through the secret-seam chokepoint fresh for each single spend (src/wallet_backend/shielded.rs), rather than operating on an already-bound, session-resident spend key as before. Net effect: a password-protected (Tier-2) wallet now prompts for its passphrase on every shielded spend, not once per session — see "User-observable changes" below anddocs/MCP.md/docs/CLI.mdfor the headless/MCP automation implication.User-observable changes
This is primarily an internal dependency bump, but it is not behavior-neutral. Three things a user can notice:
öäüß, 8 bytes, is accepted) — creation is rejected below that floor.docs/MCP.md/docs/CLI.mdfor the headless/MCP automation implication.What was done
dash-sdk,rs-sdk-trusted-context-provider,platform-wallet,platform-wallet-storagegit pins, cumulativelyd18020f5→f376d32b→ebbd15c4→e75f259f→4a1ba64c→288a6cae(dashpay/platformPR #3968 tip, branchfeat/platform-wallet-storage-rehydration). This is a re-bump within the same open PR: the branch has advanced substantially past each previous pin every time this update runs.4a1ba64c→288a6cae(2026-07-24): picked up mainlinev4.1-devmerges (release/version bumps, akotlin-sdkfix, the docker test-suite image, adriveproof wire-format versioning fix) — no direct changes to the four pinned crates' own code in this range. See "Re-bump + cold-boot regression fix + comment triage" below for what this pin move required on the DET side.ebbd15c4→e75f259f(previously landed, not yet reflected in this description until now): the hardened file and SQLite storage backends now validate permissions on every parent directory (app-data root, SPV dir, per-network dir normalized to owner-only on Unix);PlatformWalletPersistencegaineddelete_wallet, withDetPersisterdelegating to the innerSqlitePersisterso wallet deletion keeps the typed backend behavior; the secret store enforces an eight-byte passphrase floor for new HD wallets while keeping unmigrated short-password legacy wallets usable; upstream now persists shielded viewing keys natively, with DET's legacy metadata adapter retained for databases written by prior pins.e75f259f→4a1ba64c(this update): two further API-drift adaptations forced by the upstream jump —DataContractUpdatestate transitions now wait onbroadcast_and_wait_for_affected_stateinstead of the genericbroadcast_and_wait, matching upstream's reclassification of contract updates as non-execution-binding (a correctness fix, not a weaker proof: both paths run identical GroveDB + quorum-BLS verification and reject consensus errors first, only the tag-gate differs); and contact receiving-account registration (register_contact_receiving_accounts) now goes throughManagedAccountOperations::add_managed_account_from_xpubinstead of a lower-level insert, because the nestedrust-dashcorepin bump changed account-generation semantics so that adding a managed account now correctly invalidates stale SPV compact-filter coverage. Only extended public keys cross this seam — no seed or private key material is touched.Cargo.lockis regenerated scoped to the platform package set only at each bump — no crates.io registry version, checksum, or dependency-edge drift introduced.Due-diligence review
Independent security (Smythe) + structural (Adams) + adversarial (Marvin) review of each adaptation, before merge:
DetPersister::load()originally propagated any single wallet's corrupt/undecodable FVK row as a hard error, failing cold-boot rehydration for every wallet in the store. Fixed to degrade a decode/schema/oversized-value failure to "absent" for the affected wallet only (logged), matching this module's existingkv_get_loggedconvention. Covered by a two-wallet, two-boot regression test. Update (2026-07-23): this fix and its test lived only inDetPersister, which a later commit in this same PR deleted once upstream gained native FVK support — see "Grumpy-review follow-up fixes" below; the underlying protection is real (now provided by upstream) but DET's own regression coverage for it had lapsed until this update restored it.ShieldedChangeSetby field access instead of exhaustive destructuring. Fixed to destructure all six fields.monitor_revisiongating inregister_contact_receiving_accounts— the mechanism this change exists to protect (silent-missed-funds prevention via bloom-filter rebuild). Fixed: the gating logic was extracted into its own production seam (register_contact_accounts_in_managed_wallet), now covered by a regression proving first registration bumps the revision, a duplicate registration is a no-op, and a mixed existing/new batch bumps exactly once.add_contact_receiving_accounthelper lacked a doc comment, unlike its sibling free functions in the same file. Fixed — documents theDashpayReceivingFunds-only precondition and the boolean-return meaning.DashpayReceivingFundsbranch is kept (one-line comment: validated at this seam so future callers can't register the wrong account type), and DET's own duplicate-key pre-check is kept (upstream returnsInvalidParameteron a duplicate insert rather than an idempotent no-op, so DET's check is load-bearing, not redundant).update_data_contract.rsbroadcast-API switch has no dedicated automated test (verified correct by direct comparison against upstream's verifier source, but flagged rather than covered in this round); a live-testnet smoke test of upstream's tightened freshness-anchor enforcement remains a recommended follow-up (not covered by the committed lib-test ledger, and backend-E2E is excluded from CI).Grumpy-review follow-up fixes (2026-07-23)
A full 3-agent grumpy-review pass (security/project-consistency/QA) plus coordinator validation against live source found the description above overstated what this PR verifies and ships cleanly. 6 blocking + 3 non-blocking findings were fixed in a follow-up commit:
docs/user-stories.md(WAL-001/002) to document the new password floor.DetPersisterwas retired; the underlying protection now lives upstream, but DET had zero test pinning it down until this fix)."öäüß"— 4 chars, 8 bytes). Now measured in bytes, matching upstream exactly, with a regression test.docs/MCP.md/docs/CLI.md— standalone/headless transports cannot prompt and will surfaceSecretPromptUnavailable; use an unprotected wallet for that automation for now.WalletPasswordTooShort) to match the jargon-free, actionable sibling convention already used elsewhere in this codebase.hd_seed_for_shielded_spend's structural-misconfiguration guard returnedWalletLocked, telling the user to "unlock" a wallet that wasn't locked) — now returnsWalletStateInconsistent.ensure_data_dir_existsto create new data directories atomically at owner-only permissions (no create-then-chmod TOCTOU window) and to reject following a planted symlink on the existing-directory repair path.PR-comment triage follow-up (2026-07-23, round 2)
A pass over CodeRabbit/thepastaclaw's inline review threads on this PR found three more issues, all fixed:
delete_walletentry point, so despite the SEC-008/SEC-010 fix above, an automatic pre-delete backup snapshot still preserved a copy of the removed wallet's unencrypted shielded viewing key on disk. Switched todelete_wallet_skip_backup(the pinned no-backup entry point for exactly this case); regression test extended to assert no auto-backup directory is created.ctx.request_repaint(), an idle UI wouldn't paint it until an unrelated repaint happened to occur. Fixed; new unit test asserts a repaint is requested.bump_monitor_revisionover-firing:register_contact_accounts_in_managed_walletwas bumping the monitor revision of everyDashpayReceivingFundsaccount whenever any single one was new, contradicting its own doc comment and causing needless bloom-filter/rescan churn on already-covered contacts every time one new contact was added. Fixed to bump only the specific newly-inserted account; the existing regression test's assertions were corrected accordingly (it had been asserting the old, over-firing behavior) and strengthened to check per-account revisions individually.Deliberately left out of both follow-up rounds (documented, not silently dropped): the unmerged-upstream-pin risk (inherent to this PR chain, resolves once #3968 merges), unencrypted-at-rest FVKs (consistent with upstream's own "viewing-grade, not secret" classification), the Tier-2 lockout's error-message clarity, a dedicated test for the
broadcast_and_wait_for_affected_stateswitch, an upfront password-floor UI hint, one flaky test (concurrent_spv_start_failure_is_returned_to_every_caller, unrelated to this PR's content, needs separate concurrency diagnosis), and the pre-existing hardcoded test passwords noted above.Re-bump to 288a6cae + cold-boot regression fix + comment triage (2026-07-24)
Re-bumped the pin from
4a1ba64cto288a6cae(upstream mainlinev4.1-devmerges only — see "What was done" above). This surfaced one genuine regression and prompted a full pass over the PR's remaining open review comments.platform-wallet-storage'score_state::load_statenow validates thecore_transactions.txidcolumn's fixed 32-byte width on every row and fails the entire cold-boot wallet load hard on a malformed row, where the prior pin never read that column at load time at all (a corrupt row was silently untouched). Confirmed by bisection this is a genuine, intentional upstream tightening, not a DET-side bug. Decision: accept upstream's fail-hard design rather than work around it. Rewrote the now-stale regression test (cold_boot_keeps_wallet_visible_when_persisted_transaction_txid_is_corrupt→cold_boot_surfaces_typed_error_when_persisted_transaction_txid_is_corrupt) to assert the new behavior, and added a dedicatedTaskError::WalletLocalDataLoadFailedwith an accurate, actionable message ("Saved wallet data appears damaged and cannot be loaded. Restore the wallet from its recovery phrase to keep using it.") — the prior generic "please retry" wrapper was actively misleading here, since this corruption is permanent and retrying fails identically every time. Scoped narrowly to theFatalPersisterLoadkind only; every other persister-error classification is unchanged.forget_wallet_local_state: seed vault, session cache, shielded rows) now surfaces a failure via the same warning banner the async upstream-removal step already used, instead of only logging and reporting a clean removal.TaskError::UpdatedContractIdMismatchinstead of trusting the response blindly.docs/MCP.md,docs/CLI.md, andCHANGELOG.mdgeneralized the protected-wallet caveat from naming only shielded transfer/unshield/withdraw to all six operations that actually resolve the seed just-in-time (src/wallet_backend/shielded.rs'swith_secret_sessioncall sites): initialization, shield-from-Core, shield-from-Platform, transfer, unshield, withdraw.WalletPasswordTooShort/PasswordTooShortmessages and their pinned display tests reworded from "characters" to "UTF-8 bytes after trimming", matching the byte-based enforcement and the CHANGELOG/docs wording already in place."short"/"legacy-pass"in test fixtures) is disputed as a false positive — non-sensitive placeholders, not real secrets, replied on the thread; and the pre-existingbroadcast_and_wait_for_affected_statetest-coverage gap (already disclosed above) remains deferred — proper coverage needs SDK-mocking test infrastructure this file doesn't have yet, disproportionate scope for this round.Testing (2026-07-24 update)
cargo fmt --allandcargo fmt --all -- --checkclean at288a6cae.context::wallet_lifecycle(68 passed, including both the renamed cold-boot test and the new removal-warning test by name),backend_task::error(112 passed),backend_task::update_data_contract(2 passed, the new contract-id-guard tests), all green.cargo clippy --all-features --all-targets -- -D warningsclean.Cargo.lockdiff for the4a1ba64c→288a6caemove confirmed scoped to the same 28 platform git packages only (4.0.0→4.1.0-rc.1); no crates.io registry drift.Testing
cargo fmt --all, targetedwallet_backend(330 passed) andbackend_task(411 passed) lib scopes, andcargo clippy --lib --all-features -- -D warningsall clean against the4a1ba64cpin.contact_registration_bumps_monitor_revision_only_for_new_keys) confirmed executing (not a zero-match false pass) with an explicit... okin the verification log.Cargo.lockdiff for this update confirmed scoped to the platform package set only: 28 platform git packages moved to4a1ba64c, 11 nestedrust-dashcorepackages moved0091c4a→18c68d4; no crates.io registry drift.cargo fmt --all -- --check, scopedcargo clippy --lib --all-features -- -D warnings, and targeted test scopes (context::wallet_lifecycle65 passed,model::wallet99 passed,wallet_backend333 passed,app_dir4 passed,backend_task::error112 passed) all clean, independently re-verified by the coordinator with new/renamed test names confirmed present and passing in the logs (not just an aggregate count).wallet_backend(337 passed) andcontext::wallet_lifecycle(67 passed) re-run, withcontact_registration_bumps_monitor_revision_only_for_new_keysand the two new tests (wallet_data_removal_warning_requests_repaint, the extendedremove_wallet_reaps_persisted_shielded_viewing_keys) confirmed present and passing by name in the logs, independently re-verified by the coordinator.Breaking changes
None for DET consumers — the four crates' public API is additive-only at this pin.
Prior work
wallet_idauthentication gap, unrelated to this bump.Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation