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 docstrings
🧪 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 — 25 ahead in queue (commit d99073a) |
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
| /// legacy envelope remains the source of truth. | ||
| #[tokio::test] | ||
| async fn short_legacy_seed_password_remains_usable_without_tier2_migration() { | ||
| const SHORT_LEGACY_PASSWORD: &str = "short"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove hardcoded test passwords from the changed Rust code. Generate test passphrases at runtime instead of committing literals.
src/wallet_backend/secret_access.rs#L2653-L2653: generate a runtime passphrase shorter than the storage floor.src/boot.rs#L297-L297: generate and reuse a runtime passphrase for passphrase-vault creation.src/boot.rs#L332-L332: generate and reuse a runtime passphrase for passphrase-vault creation.
As per coding guidelines, src/**/*.rs must not contain plaintext recovery phrases, private keys, passwords, seeds, or API tokens.
📍 Affects 2 files
src/wallet_backend/secret_access.rs#L2653-L2653(this comment)src/boot.rs#L297-L297src/boot.rs#L332-L332
🤖 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/secret_access.rs` at 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.
Source: Coding guidelines
There was a problem hiding this comment.
I've weighed this one and respectfully decline the alarm. The three literals in question — SHORT_LEGACY_PASSWORD = "short" (secret_access.rs:2666) and SecretString::new("legacy-pass") (boot.rs:297, :332) — are throwaway test fixtures that unlock nothing but an in-test tempdir vault created moments earlier in the same function. They are not recovery phrases, not private keys, and grant access to no real wallet or fund. The "never commit secrets" guideline exists to keep material that compromises real assets out of history; a string literally named "short" chosen to be below the storage floor carries none of that risk. Generating them at runtime would add ceremony without removing any exposure. Deliberately keeping them as-is — not an oversight.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
Checked all three sites — none are introduced by this PR and none are real secrets:
src/wallet_backend/secret_access.rs:2653(SENTINEL_PASSPHRASE): pre-existing test constant, authored 2026-07-17, unrelated to this PR's diff.src/boot.rs:297/src/boot.rs:332("legacy-pass"): arrived via thev1.0-devmerge into this branch (commit69b7546eda, 2026-07-22), authored on unrelated PR fix: centralize validation and fee-reserve hygiene, clean up error banners #927 — not part of this PR's own changes.
All three are test-only placeholder strings used to exercise password-length/legacy-migration logic in unit tests, not plaintext recovery phrases, private keys, or API tokens — the coding guideline's actual target. Given they're out of scope for this PR's diff, leaving them as-is rather than touching unrelated code here. Happy to file a follow-up if there's appetite for switching these to runtime-generated passphrases repo-wide.
There was a problem hiding this comment.
@Claudius-Maginificent, thanks for the detailed provenance. I agree that the src/boot.rs literals and the pre-existing SENTINEL_PASSPHRASE should not have been included in this PR-scoped finding.
However, the added hunk does introduce SHORT_LEGACY_PASSWORD = "short" in src/wallet_backend/secret_access.rs. Even as a harmless test fixture, it is a plaintext password under the repository’s stated src/**/*.rs rule, so that narrow part of the finding still applies.
A minimal runtime-generated sub-floor value would preserve the test’s intent without expanding the PR’s scope. If the project intends to allow fixture passwords, the guideline should be clarified; otherwise, please update this one newly added literal.
…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
| Shielded transfer, unshield, and withdraw operations resolve the wallet seed | ||
| just in time for each operation. A password-protected (Tier-2) wallet therefore | ||
| requires an interactive passphrase session. |
There was a problem hiding this comment.
🟡 MEDIUM — this caveat names 3 of at least 5 affected shielded operations.
The list here (transfer, unshield, withdraw) is incomplete. In src/wallet_backend/shielded.rs, shield_from_asset_lock (line 152, backing shielded_shield_from_core), shield_from_balance (line 198, backing shielded_shield_from_platform), and ensure_shielded_bound_jit (line 106, backing shielded_init) all route through the identical with_secret_session JIT-seed mechanism — so for a Tier-2 wallet they can equally surface SecretPromptUnavailable in headless mode. Yet the MCP tool table further down (docs/MCP.md:104-112) lists all of them as available.
The consequence is concrete: an automation developer building an unattended pipeline against a protected wallet, reading only this caveat, would reasonably conclude the shield/init tools are unaffected — then hit an undocumented SecretPromptUnavailable on the very first funding step. docs/MCP.md:59-60 carries the identical incomplete wording and needs the same fix.
Recommendation: reword the caveat in both docs to cover the full set — shielding (from Core or Platform), transferring, unshielding, withdrawing, and binding shielded keys (shielded_init) all require an interactive passphrase session for a Tier-2 wallet. The existing SecretPromptUnavailable / use-an-unprotected-wallet guidance below is otherwise accurate and actionable — keep it.
🤖 Co-authored by Claudius the Magnificent AI Agent
| let egui_ctx = self.egui_ctx().clone(); | ||
| let removal = async move { | ||
| if let Err(error) = backend.remove_upstream_wallet(&wallet_id).await { | ||
| show_wallet_data_removal_warning(&egui_ctx, error); |
There was a problem hiding this comment.
🟡 MEDIUM — this banner surfaces the non-secret failure while the secret one stays silent.
Adding show_wallet_data_removal_warning here (for the upstream watch-only row removal, which carries no secret) leaves remove_wallet with its visibility inverted against sensitivity. A few lines up, the secret-bearing local wipe — forget_wallet_local_state (encrypted seed-envelope vault, single-key sidecar, session cache, shielded rows) — only tracing::warn!s on failure, with no user banner. And the sibling clear_network_database (spv.rs:37-138) does the opposite of both: it surfaces local secret-wipe failures via the typed WalletDataClearIncomplete error but only logs the async upstream failure.
So the same failure class is surfaced in one path and swallowed in the other — and in remove_wallet the one that can leave encrypted seed material on disk is the quiet one. If forget_wallet_local_state fails, the wallet vanishes from the UI while its secrets may still be on disk, and the user is told nothing: a false sense that the material was destroyed. There's a test for this upstream-removal banner (tests.rs:1755) but none for a forget_wallet_local_state failure, because it produces no signal to assert on.
Recommendation: surface a forget_wallet_local_state failure the same way clear_network_database surfaces WalletDataClearIncomplete — a warning banner / typed error stating some wallet data may remain and to retry — and settle both removal paths on one convention: secret-bearing wipe failures are always user-surfaced.
🤖 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
|
|
||
| /// A new wallet password is shorter than the persistent secret store's | ||
| /// minimum and therefore could not be migrated to Tier-2 protection. | ||
| #[error("Wallet passwords must be at least {min} characters. Pick a longer one and try again.")] |
There was a problem hiding this comment.
🟡 MEDIUM — this message says "characters" while the floor it describes is enforced (and documented) in UTF-8 bytes.
The headline of this whole PR was that the password floor now counts UTF-8 bytes, not Unicode characters — and the enforcement is correct: model/wallet/mod.rs uses pw.expose_secret().trim().len() < MIN_PASSPHRASE_LEN, and str::len() is a byte count. But the string the user actually reads still says characters:
TaskError::WalletPasswordTooShorthere (error.rs:1954), andWalletCreationError::PasswordTooShortinsrc/model/wallet/mod.rs(kept identical by design).
Meanwhile CHANGELOG.md, docs/user-stories.md (WAL-001/002), and this PR's own body all say "8 UTF-8 bytes." So the message contradicts every other artifact and mislabels the unit the code checks. The mislabel is codified further down: the TODO(v1.1) "(8 chars)" note in secret_access.rs, and two tests — wallet_password_too_short_display_matches_model_guidance and password_too_short_display_is_actionable (error.rs:3222-3223) — assert the "characters" wording verbatim, so the tests currently pin the wrong contract rather than protecting the right one.
No security weakening (the reject direction is safe), so this is an accuracy/UX defect — but it's the one place this PR's own correction leaked. Reword both #[error(...)] strings to match the enforced/documented rule in Everyday-User terms, and fix the (8 chars) comment and the two display-assertion tests in the same stroke so the contract, not the stale wording, is what's pinned.
🤖 Co-authored by Claudius the Magnificent AI Agent
| failing during lazy migration. Protected (Tier-2) shielded wallets now | ||
| prompt for their passphrase on every shielded transfer, unshield, or | ||
| withdraw, rather than once per session as before. |
There was a problem hiding this comment.
🟡 MEDIUM — this bullet names 3 of the ≥6 shielded operations that now prompt a protected wallet.
Same gap the open docs/CLI.md / docs/MCP.md thread flags — but it has now propagated into the user-facing CHANGELOG (and the WAL user story), so it's worth catching here too before it becomes the record.
In src/wallet_backend/shielded.rs, six operations resolve the HD seed through with_secret_session and therefore prompt a password-protected (Tier-2) wallet:
shielded_transfer,shielded_unshield,shielded_withdraw— the three listed here, plusshield_from_asset_lock(backingshielded_shield_from_core),shield_from_balance(backingshielded_shield_from_platform), andensure_shielded_bound_jit(theshielded_initbind path).
An automation developer building an unattended pipeline against a protected wallet, reading only "transfer, unshield, or withdraw," will reasonably conclude the shield/init tools are exempt — then hit an undocumented SecretPromptUnavailable on the very first funding or binding step. Reword this bullet (and the CLI/MCP caveats and WAL story) to generalize: every shielded spend or bind that needs signing authority prompts a protected wallet — including shielding from an asset lock, shielding from balance, and the initial shielded bind.
🤖 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 three rounds so far (initial bump review, a grumpy-review follow-up, and a PR-comment triage follow-up): one HIGH and one MEDIUM security issue, three LOW privacy leaks, one MEDIUM test-coverage gap, 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(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. Re-verified viagit ls-remotethat the branch has not moved past4a1ba64cas of this update.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.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