fix(wallet): stop a duplicate identity index from making a wallet unloadable - #951
fix(wallet): stop a duplicate identity index from making a wallet unloadable#951Claudius-Maginificent wants to merge 5 commits into
Conversation
…oadable A wallet whose saved state was migrated from an older install could come back on the next launch as "Saved wallet data appears damaged and cannot be loaded", with recovery-phrase restore as the only way forward. The wallet backend buckets a wallet's identities on `(wallet_id, identity_index)`. DET's identity index is user-entered (the "add existing identity" screen) and carries no uniqueness, so one wallet can hold two identities at the same index — the ordinary shape of an install upgraded from a build that never used the index as a key. Registering the second identity displaced the first from the active set while both kept their saved records. The next launch replayed the same collapse, could not reattach the displaced identity's saved keys, and failed the whole wallet's load with `OrphanedIdentityEntry` — surfaced as `TaskError::WalletLocalDataLoadFailed`. `ensure_identity_managed` is the single DET path into that registration, so it now refuses a second identity at a taken index with the dedicated `TaskError::IdentityIndexAlreadyTaken`, naming the identity that holds it. Nothing is written, the wallet stays loadable, and the identity reconciler logs the permanent condition as a warning instead of a retryable defer. Tests: `a_second_identity_at_a_taken_index_is_refused_and_the_wallet_still_reloads` (RED before the guard with the exact user-facing failure) and `a_migrated_install_relaunches_without_damaged_wallet_data`. The existing `reconcile_managed_identities_registers_only_wallet_owned` probed a taken index to prove an unrelated filter; it now probes a free one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A restart between an asset-lock broadcast and its consumption could strand the lock and the funds in it: `resume_asset_lock` fails to re-derive the credit output with "Funding account IdentityTopUp not found for re-derivation". `load()` rebuilds `Wallet.accounts` from `account_registrations` alone. `provision_identity_funding_account` created the account in both upstream in-memory collections and persisted nothing, so upstream's own creator — the only writer of that row — then hit its `contains_*` guards, concluded both sides already existed, and took its early return, skipping the registration and address-pool store its docs call load-bearing for crash recovery. The account was live in memory and absent from disk on every launch. DET's provisioning now writes the `AccountRegistrationEntry` through the persister and flushes it, and rolls both in-memory inserts back on a store failure so a retry re-creates and re-persists rather than short-circuiting on the presence guards. Failures surface as the dedicated `TaskError::IdentityFundingAccountPersistFailed`. Residual: the paired address-pool snapshot upstream writes alongside the registration is not reachable from here (`account_address_pool_entries` is crate-private), so pool depth for that account re-warms on the next sync instead of restoring. The account itself — what re-derivation needs — is restored. Test: `a_provisioned_identity_topup_account_survives_a_restart`, confirmed RED before the fix (0 persisted rows, expected 1). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change validates identity index ownership, persists identity funding accounts with rollback handling, releases indexes during identity removal, updates reconciliation behavior, and adds lifecycle and v0.9.3 relaunch regression coverage. ChangesIdentity lifecycle safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IdentityTask
participant WalletBackend
participant IdentityStore
IdentityTask->>WalletBackend: release identity indexes
WalletBackend-->>IdentityTask: release result
IdentityTask->>IdentityStore: delete local identity
IdentityStore-->>IdentityTask: removal result
Possibly related PRs
🚥 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 |
…wallet The occupancy guard added in dfcb906 covered only the reconcile and top-up paths. Two routes to the same fatal corruption stayed open, both found by independent review. Registration: `WalletBackend::register_identity` handed off to upstream `register_identity_with_funding`, whose Step 4 calls `add_identity` with no occupancy check and then deliberately swallows that add's failure so the spent asset lock is still consumed — a collision there is persisted, never reported. The UI gate that was assumed to cover this is inert: `wallet.identities` hydrates empty on every cold boot, so after a restart the index picker marks nothing as used and recommends index 0, the very index a first identity usually holds. A pre-flight check now refuses before the asset lock is built, so a collision costs the user nothing. Per CLAUDE.md the backend task is the authoritative enforcement layer; a combo box was never enough. Removal: `AppContext::remove_identity` only cleared DET's own k/v store and vault, so the wallet backend kept the identity and its index. The slot stayed occupied by an identity the user believes is gone — blocking re-use of that index with a phantom occupant they cannot act on, and leaving the index looking free to the picker while a registration there would still collide. Removal now releases the index upstream, which records a tombstone rather than deleting: the rows stay on disk and any orphaned key rows become safe to skip at load, which is what keeps a removal from becoming the damaged-data failure itself. Both call sites share one `index_occupant` helper so the invariant cannot drift between them, and the error message now names controls that exist ("pick an identity index that is not marked as used, or remove identity X first"). Test `removing_an_identity_frees_its_index_for_reuse` drives the real `BackendTask::RemoveIdentity` dispatch and was confirmed RED before the fix, failing with the exact phantom-occupant `IdentityIndexAlreadyTaken`. The registration pre-flight is not unit-testable offline — it needs funds and a live Platform — so it is verified by code trace against the pinned upstream revision, not by a run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
⛔ Blockers found — Sonnet deferred (commit ff26767) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/wallet_backend/identity_ops.rs (1)
356-441: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback clears an already-persisted account, not just the freshly-derived one.
in_walletistruewhenkw.accounts.*was rebuilt from the persisted manifest at wallet load (the cold-boot re-provision case this function's header comment describes at lines 328-335). In that branch, theif !in_walletblock at lines 373-396 is skipped, sopersist_account_registrationat line 422 is a redundant re-write of an already-durable row.If that redundant write fails, the
.inspect_errclosure at lines 423-440 still clearskw.accounts.identity_registration(or removes the top-up entry) unconditionally. This discards an in-memory account whose on-disk state never changed and was already confirmed durable, degrading funding-account availability for the rest of the session until the next call re-derives it. HD derivation is deterministic, so a subsequent call self-heals by re-deriving the same xpub, but the invariant assumed by the rollback comment ("so a retry re-creates and re-persists") only holds for the freshly-created (!in_wallet) case.Track whether the
kw-side account was newly derived this call, and only roll back that side when it was.🐛 Proposed fix: only clear the wallet-side entry that was actually created this call
if in_wallet && in_managed { return Ok(()); } + let newly_added_to_wallet = !in_wallet; if !in_wallet { let account_type = match funding { @@ self.persist_account_registration(&wallet_id, account_type, account_xpub) .inspect_err(|_| { // Roll back both sides so a retry re-creates and re-persists, // rather than the `in_wallet && in_managed` guard above short- // circuiting a persist that never happened. match funding { Funding::Registration => { - kw.accounts.identity_registration = None; + if newly_added_to_wallet { + kw.accounts.identity_registration = None; + } info.core_wallet.accounts.identity_registration = None; } Funding::TopUp(registration_index) => { - kw.accounts.identity_topup.remove(®istration_index); + if newly_added_to_wallet { + kw.accounts.identity_topup.remove(®istration_index); + } info.core_wallet .accounts .identity_topup .remove(®istration_index); } } })🤖 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/identity_ops.rs` around lines 356 - 441, Track whether the funding account was newly added in the !in_wallet branch, and update the persist_account_registration error rollback so the wallet-side entry is cleared only when that account was created during this call. Always preserve the managed-side rollback as appropriate, but leave an already-persisted kw account intact when persistence is merely a redundant rewrite.
🤖 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/wallet_backend/identity_ops.rs`:
- Around line 356-441: Track whether the funding account was newly added in the
!in_wallet branch, and update the persist_account_registration error rollback so
the wallet-side entry is cleared only when that account was created during this
call. Always preserve the managed-side rollback as appropriate, but leave an
already-persisted kw account intact when persistence is merely a redundant
rewrite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4ddc8d6-74f2-42a6-9608-f16844cff399
📒 Files selected for processing (6)
src/app_dir.rssrc/backend_task/error.rssrc/backend_task/migration/v093_upgrade.rssrc/context/wallet_lifecycle/bootstrap.rssrc/context/wallet_lifecycle/tests.rssrc/wallet_backend/identity_ops.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The focused duplicate-index and funding-account restart tests pass, but the production identity-registration seams still bypass the new slot guard and can spend funds before recreating the collision the PR is intended to prevent. The persistence rollback also mishandles transient store failures by discarding in-memory state while the changeset remains buffered; identity removal itself is intentionally DET-local and is not a defect in this PR. Source: reviewers gpt-5.6-sol (general, rust-quality); verifier gpt-5.6-sol.
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)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet_backend/identity_ops.rs`:
- [BLOCKING] src/wallet_backend/identity_ops.rs:69-76: Enforce identity-index occupancy before every paid registration
The new uniqueness check protects only `ensure_identity_managed`, while wallet-funded creation calls `register_identity_with_funding` directly after provisioning. At the pinned upstream revision, `IdentityManager::add_identity` checks only for a duplicate identity ID and then uses `BTreeMap::insert(identity_index, managed_identity)`, which replaces an existing occupant; the orchestrator performs that insertion only after Platform has accepted and consumed the registration funding. This path is reachable after a cold boot because DET's `Wallet.identities` map starts empty and is not hydrated from stored qualified identities, so the UI's used-index list can miss an existing upstream slot. Address-funded creation at `src/backend_task/identity/register_identity.rs:334-379` likewise submits payment before checking the slot, then saves an identity that reconciliation cannot manage alongside the occupant; add-existing at `src/backend_task/identity/load_identity_from_wallet.rs:250-267` also admits duplicate local ownership. Centralize an atomic occupancy reservation and use it before broadcasting, submitting address-funded creation, or saving an existing identity. A check that is released before the asynchronous registration is insufficient because concurrent registrations could both pass it.
- [SUGGESTION] src/wallet_backend/identity_ops.rs:462-465: Retry the retained changeset before rolling back memory
DET uses the default immediate-flush `SqlitePersister`. When its inline flush fails transiently, `store` restores the account-registration changeset to the per-wallet buffer and returns a transient `PersistenceError`, as required by `PlatformWalletPersistence`. The `and_then` here skips the subsequent bare `flush`, and the caller's `inspect_err` removes both in-memory account entries even though the registration remains queued. A later unrelated store can therefore commit that retained registration while the running wallet has no corresponding account in either in-memory collection. Handle `source.is_transient()` by retrying the existing buffer through `flush` without re-submitting the changeset; roll back the in-memory inserts only after a terminal failure for which the backend has dropped the buffered delta. Add an injected transient-failure test covering the two account maps, buffered state, durable row, and retry behavior.
| self.inner | ||
| .wallet_persister | ||
| .store(*wallet_id, changeset) | ||
| .and_then(|()| self.inner.wallet_persister.flush(*wallet_id)) |
There was a problem hiding this comment.
🟡 Suggestion: Retry the retained changeset before rolling back memory
DET uses the default immediate-flush SqlitePersister. When its inline flush fails transiently, store restores the account-registration changeset to the per-wallet buffer and returns a transient PersistenceError, as required by PlatformWalletPersistence. The and_then here skips the subsequent bare flush, and the caller's inspect_err removes both in-memory account entries even though the registration remains queued. A later unrelated store can therefore commit that retained registration while the running wallet has no corresponding account in either in-memory collection. Handle source.is_transient() by retrying the existing buffer through flush without re-submitting the changeset; roll back the in-memory inserts only after a terminal failure for which the backend has dropped the buffered delta. Add an injected transient-failure test covering the two account maps, buffered state, durable row, and retry behavior.
source: ['codex']
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/context/wallet_lifecycle/tests.rs (1)
4224-4248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve that the replacement identity is distinct and newly managed.
Line 4242 accepts
Ok(false). Iftest_identity_with_key()returns the same identity ID for both fixtures, the test can pass while the original manager entry remains. Assert that both IDs differ. Assert that the second call returnstrue.Proposed test strengthening
let removed = test_identity_with_key(); let replacement = test_identity_with_key(); +assert_ne!(removed.id(), replacement.id()); ... -backend - .ensure_identity_managed(&seed_hash, &replacement, 0) - .await - .expect(...); +assert!( + backend + .ensure_identity_managed(&seed_hash, &replacement, 0) + .await + .expect(...) +);🤖 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` around lines 4224 - 4248, Strengthen the identity replacement test around test_identity_with_key by asserting that removed.id and replacement.id are different, then capture the result of the second ensure_identity_managed call and assert it is true rather than merely accepting Ok(false). Preserve the existing removal-success assertion and failure context.
🤖 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/identity/remove_identity.rs`:
- Around line 54-65: The remove-identity flow must not delete the local identity
or report success when release_identity_index fails. Change
release_identity_index to return a typed Result using the backend task’s
TaskError conventions, propagate the forget_identity failure from the removal
operation, and perform local deletion only after successful index release.
In `@src/wallet_backend/identity_ops.rs`:
- Around line 88-96: The pre-flight check in reject_taken_identity_index is racy
because registration proceeds after the lock is released, allowing concurrent
tasks to claim the same wallet identity_index. Serialize registration for each
wallet/index or reserve the index before asset-lock creation, retain that
reservation through registration, and release it only on a terminal outcome; add
a regression test covering concurrent registration attempts.
---
Nitpick comments:
In `@src/context/wallet_lifecycle/tests.rs`:
- Around line 4224-4248: Strengthen the identity replacement test around
test_identity_with_key by asserting that removed.id and replacement.id are
different, then capture the result of the second ensure_identity_managed call
and assert it is true rather than merely accepting Ok(false). Preserve the
existing removal-success assertion and failure context.
🪄 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: 0eb9c30d-39d2-4e35-9c33-cacecb01cbd2
📒 Files selected for processing (5)
src/backend_task/error.rssrc/backend_task/identity/mod.rssrc/backend_task/identity/remove_identity.rssrc/context/wallet_lifecycle/tests.rssrc/wallet_backend/identity_ops.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend_task/error.rs
| async fn release_identity_index(&self, identity_id: &Identifier) { | ||
| let Ok(backend) = self.wallet_backend() else { | ||
| return; | ||
| }; | ||
| if let Err(error) = backend.forget_identity(identity_id).await { | ||
| tracing::warn!( | ||
| %identity_id, | ||
| %error, | ||
| "Identity index could not be released in the wallet backend; \ | ||
| adding an identity at that index may be refused" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report removal success when index release fails.
forget_identity can fail after the upstream identity remains persisted. Lines 58-64 only log that failure, and Line 20 then deletes the local identity. The wallet can retain a phantom index occupant with no local identity available to retry the tombstone.
Return the typed release error before local deletion, or persist a durable retry record before reporting successful removal.
As per coding guidelines, backend tasks are the authoritative enforcement layer and must return typed Result<T, TaskError> values.
🤖 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/identity/remove_identity.rs` around lines 54 - 65, The
remove-identity flow must not delete the local identity or report success when
release_identity_index fails. Change release_identity_index to return a typed
Result using the backend task’s TaskError conventions, propagate the
forget_identity failure from the removal operation, and perform local deletion
only after successful index release.
Source: Coding guidelines
There was a problem hiding this comment.
The finding is valid, but propagating forget_identity errors alone is insufficient. At the pinned upstream revision, IdentityManager::remove_identity swallows persister.store failures and returns success after mutating memory, so DET must establish a durable tombstone itself or persist a durable cleanup-retry record before deleting the local identity.
There was a problem hiding this comment.
@thepastaclaw, agreed. The current forget_identity result cannot prove that the upstream tombstone is durable.
DET must persist a durable tombstone or a durable cleanup-retry record before it deletes the local identity and returns RemovedIdentities. DET must also keep the local identity when that persistence step fails.
The review finding remains valid with this stronger requirement.
🐇🔍
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash-evo-tool PR: 951
File: src/backend_task/identity/remove_identity.rs:54-65
Timestamp: 2026-07-31T17:46:22.061Z
Learning: At the pinned upstream `rs-platform-wallet` revision used by Dash Evo Tool, `IdentityManager::remove_identity` can swallow `persister.store` failures after it mutates in-memory state and returns success. In `src/backend_task/identity/remove_identity.rs`, a successful `wallet_backend().forget_identity(identity_id).await` result does not prove that identity-index release is durable. Before `AppContext::remove_identity` deletes the local identity or returns removal success, DET must persist a durable upstream tombstone itself or persist a durable cleanup-retry record.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| // Refuse before the asset lock is built, so a collision costs nothing: | ||
| // upstream registers the identity at `identity_index` without an | ||
| // occupancy check and deliberately swallows that add's failure to | ||
| // guarantee the spent lock is consumed, so a collision there is | ||
| // persisted rather than reported. A pre-flight, not a mutex — DET | ||
| // cannot hold the manager lock across the network round-trip. | ||
| self.reject_taken_identity_index(seed_hash, identity_index) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the identity-index check atomic with registration.
Lines 88-95 release the read lock before asset-lock creation and network registration. Two registration tasks can both pass this check for the same wallet and index. Upstream can then accept the second registration and displace the first managed identity.
Serialize registration per wallet/index, or reserve the index before funding starts. Release the reservation only after the registration reaches a terminal outcome. Add a concurrent-registration regression test.
🤖 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/identity_ops.rs` around lines 88 - 96, The pre-flight
check in reject_taken_identity_index is racy because registration proceeds after
the lock is released, allowing concurrent tasks to claim the same wallet
identity_index. Serialize registration for each wallet/index or reserve the
index before asset-lock creation, retain that reservation through registration,
and release it only on a terminal outcome; add a regression test covering
concurrent registration attempts.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The focused duplicate-index, removal, and funding-account restart tests pass, and the new preflight closes the ordinary sequential wallet-funded collision path. The identity-index invariant is still bypassed by address-funded and local-load paths and remains non-atomic under concurrent registration or reconciliation; identity removal can also discard its only retry anchor before the upstream tombstone is durable. The transient account-registration persistence issue from the prior review is unchanged.
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)
🔴 2 blocking | 💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet_backend/identity_ops.rs`:
- [BLOCKING] src/wallet_backend/identity_ops.rs:88-95: Reserve the identity index across every registration path
The new preflight covers only wallet-funded registration and releases the manager read lock before account provisioning, asset-lock creation, and the Platform round trip. Backend tasks and unlock-time reconciliation can run concurrently, so two registrations—or registration racing `reconcile_managed_identities`—can observe a free slot before either records its identity. At the pinned upstream revision, `IdentityManager::add_identity` checks only for a duplicate identity ID and then uses `BTreeMap::insert(identity_index, ...)`, replacing an existing slot occupant after Platform has already accepted payment. The same invariant is bypassed entirely by `register_identity_from_platform_addresses`, which spends address credits at `src/backend_task/identity/register_identity.rs:334-356` before saving the wallet/index association, and by `load_user_identity_from_wallet`, which persists that association at `src/backend_task/identity/load_identity_from_wallet.rs:250-267` without consulting the guard. Use a shared per-`(wallet_id, identity_index)` reservation for wallet-funded registration, address-funded registration, reconciliation, and existing-identity attachment, and retain it until the operation reaches its terminal bookkeeping outcome.
In `src/backend_task/identity/remove_identity.rs`:
- [BLOCKING] src/backend_task/identity/remove_identity.rs:19-20: Keep the local identity until index release is durable
`release_identity_index` is best-effort, but the local identity is deleted unconditionally immediately afterward. If backend initialization was deferred, `wallet_backend()` fails and release becomes a no-op. Even with an initialized backend, the pinned upstream `IdentityManager::remove_identity` mutates memory, calls `persister.store`, logs and swallows any persistence error, and still returns success. A transient failure leaves the tombstone only in the persister buffer, while a terminal failure drops it; exiting before a retained write commits lets the old occupant reappear on restart after DET has deleted the identity and its only cleanup retry anchor. Reusing the apparently free slot can then recreate duplicate persisted identity rows or leave a paid replacement unmanaged. Require an observably durable tombstone, or save a durable cleanup-retry record, before deleting the local identity and reporting removal success.
| // Refuse before the asset lock is built, so a collision costs nothing: | ||
| // upstream registers the identity at `identity_index` without an | ||
| // occupancy check and deliberately swallows that add's failure to | ||
| // guarantee the spent lock is consumed, so a collision there is | ||
| // persisted rather than reported. A pre-flight, not a mutex — DET | ||
| // cannot hold the manager lock across the network round-trip. | ||
| self.reject_taken_identity_index(seed_hash, identity_index) | ||
| .await?; |
There was a problem hiding this comment.
🔴 Blocking: Reserve the identity index across every registration path
The new preflight covers only wallet-funded registration and releases the manager read lock before account provisioning, asset-lock creation, and the Platform round trip. Backend tasks and unlock-time reconciliation can run concurrently, so two registrations—or registration racing reconcile_managed_identities—can observe a free slot before either records its identity. At the pinned upstream revision, IdentityManager::add_identity checks only for a duplicate identity ID and then uses BTreeMap::insert(identity_index, ...), replacing an existing slot occupant after Platform has already accepted payment. The same invariant is bypassed entirely by register_identity_from_platform_addresses, which spends address credits at src/backend_task/identity/register_identity.rs:334-356 before saving the wallet/index association, and by load_user_identity_from_wallet, which persists that association at src/backend_task/identity/load_identity_from_wallet.rs:250-267 without consulting the guard. Use a shared per-(wallet_id, identity_index) reservation for wallet-funded registration, address-funded registration, reconciliation, and existing-identity attachment, and retain it until the operation reaches its terminal bookkeeping outcome.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Reserve the identity index across every registration path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Correction: the preceding auto-resolve reply was incorrect. This finding remains open at 4c1013a1672e7e8617f87731801e7eb28042aea7 and is one of the two blockers retained by the preliminary review: #951 (review)
No thread resolution occurred; please treat the earlier auto-resolve message as superseded by this correction.
| self.release_identity_index(&identity_id).await; | ||
| self.delete_local_qualified_identity(&identity_id)?; |
There was a problem hiding this comment.
🔴 Blocking: Keep the local identity until index release is durable
release_identity_index is best-effort, but the local identity is deleted unconditionally immediately afterward. If backend initialization was deferred, wallet_backend() fails and release becomes a no-op. Even with an initialized backend, the pinned upstream IdentityManager::remove_identity mutates memory, calls persister.store, logs and swallows any persistence error, and still returns success. A transient failure leaves the tombstone only in the persister buffer, while a terminal failure drops it; exiting before a retained write commits lets the old occupant reappear on restart after DET has deleted the identity and its only cleanup retry anchor. Reusing the apparently free slot can then recreate duplicate persisted identity rows or leave a paid replacement unmanaged. Require an observably durable tombstone, or save a durable cleanup-retry record, before deleting the local identity and reporting removal success.
source: ['codex']
| let removed = test_identity_with_key(); | ||
| let replacement = test_identity_with_key(); | ||
| assert!( | ||
| backend | ||
| .ensure_identity_managed(&seed_hash, &removed, 0) | ||
| .await | ||
| .expect("first identity registers at a free index"), | ||
| ); | ||
|
|
||
| ctx.run_backend_task( | ||
| BackendTask::IdentityTask(IdentityTask::RemoveIdentity { | ||
| identity_id: removed.id(), | ||
| }), | ||
| sender, | ||
| ) | ||
| .await | ||
| .expect("removing an identity must succeed"); | ||
|
|
||
| backend | ||
| .ensure_identity_managed(&seed_hash, &replacement, 0) | ||
| .await | ||
| .expect( | ||
| "the index of a removed identity must be free again; a phantom occupant \ | ||
| means the removal did not reach the wallet backend", | ||
| ); |
There was a problem hiding this comment.
💬 Nitpick: Assert that removal admits a distinct replacement identity
The test ignores the boolean returned by the second ensure_identity_managed call, so Ok(false) is accepted even though it means no replacement was newly registered. The fixtures currently generate random IDs, making accidental equality extremely unlikely, but the test should state both assumptions explicitly: the identities are distinct, and the replacement was newly admitted after removal.
| let removed = test_identity_with_key(); | |
| let replacement = test_identity_with_key(); | |
| assert!( | |
| backend | |
| .ensure_identity_managed(&seed_hash, &removed, 0) | |
| .await | |
| .expect("first identity registers at a free index"), | |
| ); | |
| ctx.run_backend_task( | |
| BackendTask::IdentityTask(IdentityTask::RemoveIdentity { | |
| identity_id: removed.id(), | |
| }), | |
| sender, | |
| ) | |
| .await | |
| .expect("removing an identity must succeed"); | |
| backend | |
| .ensure_identity_managed(&seed_hash, &replacement, 0) | |
| .await | |
| .expect( | |
| "the index of a removed identity must be free again; a phantom occupant \ | |
| means the removal did not reach the wallet backend", | |
| ); | |
| let removed = test_identity_with_key(); | |
| let replacement = test_identity_with_key(); | |
| assert_ne!(removed.id(), replacement.id()); | |
| assert!( | |
| backend | |
| .ensure_identity_managed(&seed_hash, &removed, 0) | |
| .await | |
| .expect("first identity registers at a free index"), | |
| ); | |
| ctx.run_backend_task( | |
| BackendTask::IdentityTask(IdentityTask::RemoveIdentity { | |
| identity_id: removed.id(), | |
| }), | |
| sender, | |
| ) | |
| .await | |
| .expect("removing an identity must succeed"); | |
| assert!( | |
| backend | |
| .ensure_identity_managed(&seed_hash, &replacement, 0) | |
| .await | |
| .expect( | |
| "the index of a removed identity must be free again; a phantom occupant \ | |
| means the removal did not reach the wallet backend", | |
| ), | |
| ); |
source: ['coderabbit']
|
Follow-up from a fresh repro against the reporting user's actual DB, done just now on the current The status-update note above is stale. Loading the user's real Querying the DB directly confirms it: two identity rows share However, merging this PR as-is will not unblock it. All three fixes here ( Logged the upstream side of this as a 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The dependency-only delta from the previously reviewed head updates empty-script UTXO handling; the identity lifecycle files are unchanged, and the relevant upstream identity-manager and persister code is identical across the old and current platform pins. Two blockers remain: identity indices are not reserved across concurrent and alternative entry paths, and local deletion can outrun a durable upstream tombstone; transient funding-account persistence handling and regression-test gaps also remain open.
Source: reviewers gpt-5.6-sol (general, rust-quality); verifier gpt-5.6-sol.
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)
🔴 2 blocking | 🟡 1 suggestion(s)
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/wallet_lifecycle/tests.rs`:
- [SUGGESTION] src/context/wallet_lifecycle/tests.rs:4294-4304: Exercise the paid registration guard rather than only the reconciler helper
The collision regression calls `ensure_identity_managed` directly for both identities, so it never executes the new `WalletBackend::register_identity` preflight. Removing the paid-path check at lines 94-95 leaves this test green. Contrary to the commit message, this path is testable offline: seed an occupied manager slot and call `register_identity`; the occupancy error is returned before the secret session, funding-account provisioning, asset-lock work, or any network request. Add that direct regression, plus barrier-controlled concurrency coverage and equivalent tests for the address-funded and existing-identity paths when the shared reservation is introduced.
| assert!( | ||
| backend | ||
| .ensure_identity_managed(&seed_hash, &resident, 0) | ||
| .await | ||
| .expect("the first identity at a free index must register"), | ||
| ); | ||
|
|
||
| let error = backend | ||
| .ensure_identity_managed(&seed_hash, &colliding, 0) | ||
| .await | ||
| .expect_err("a second identity at a taken index must be refused"); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise the paid registration guard rather than only the reconciler helper
The collision regression calls ensure_identity_managed directly for both identities, so it never executes the new WalletBackend::register_identity preflight. Removing the paid-path check at lines 94-95 leaves this test green. Contrary to the commit message, this path is testable offline: seed an occupied manager slot and call register_identity; the occupancy error is returned before the secret session, funding-account provisioning, asset-lock work, or any network request. Add that direct regression, plus barrier-controlled concurrency coverage and equivalent tests for the address-funded and existing-identity paths when the shared reservation is introduced.
source: ['codex']
Three resolutions, one textual and two silent: - src/wallet_backend/identity_ops.rs — both sides inserted after `ensure_identity_managed`. Kept #951's `reject_taken_identity_index` / `forget_identity` and #954's test-only `resolved_managed_identity_id`, plus #954's rewritten `top_up_identity` doc (#951 left that doc at the base text, so theirs is the current one). - The `Funding` rollback match in `provision_identity_funding_account` (#951) gained #954's `TopUpNotBound` variant. Auto-merge left it non-exhaustive; the arm clears both the key-wallet and managed copies, mirroring the other two. - `a_provisioned_identity_topup_account_survives_a_restart` (#951) opened the wallet database at its pre-#954 location inside the disposable SPV cache. It now goes through `wallet_database_path()`, as every other call site in the file already does. Side effect worth knowing: #954's index-less top-up funding account now persists its account registration, because it provisions through the path #951 taught to write the manifest row.
|
Merged `v1.0-dev` to resolve the mergeable-conflict state — it was pure adjacency (both this PR and #954 inserting a method at the same anchor line), now resolved with both additions kept. Two conflicts auto-merged without markers and needed a closer look:
Both fixed, verified: full `wallet_lifecycle` suite green (75/75), clippy clean, all four PRs' new tests confirmed passing by name. Flagging for review: the added `TopUpNotBound` rollback arm is untested — it only fires if `persist_account_registration` fails, and no existing test exercises that failure path. Worth a look before merge. 🤖 Co-authored by Claudius the Magnificent AI Agent |
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 (2)
src/context/wallet_lifecycle/tests.rs (2)
3781-3846: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDrain subtasks before copying the data directory.
This test calls
backend.shutdown().await;and then immediately callscopy_dir_recursive(source_dir.path(), cold_dir.path());. Sibling tests in this file that copy a directory after registering wallet state (for exampleissue7_fresh_persistor_bip44_xpub_matches_det_bridgeat line 754 andcold_boot_skips_corrupt_fvk_for_one_wallet_and_restores_healthy_walletat line 3942) additionally calllet _ = ctx.subtasks.shutdown_async().await;before the copy. Without draining subtasks here, a pending background write fromensure_identity_funding_accountscould still be in flight when the directory is copied, producing an inconsistent snapshot and a flaky assertion onpersisted_topup_rows.Add the missing drain to match the established pattern in this file.
🔧 Proposed fix
backend .ensure_identity_funding_accounts(&seed_hash, &seed, registration_index) .await .expect("provision identity funding accounts"); backend.shutdown().await; + let _ = ctx.subtasks.shutdown_async().await; seed_hash };🤖 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` around lines 3781 - 3846, In a_provisioned_identity_topup_account_survives_a_restart, drain the context’s background subtasks after backend.shutdown().await and before copy_dir_recursive. Use the established ctx.subtasks.shutdown_async() pattern so all persistence writes complete before the data directory snapshot is copied.
4746-4786: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the top-up error type before trusting the displacement check.
This test discards the dispatch error with
let _error = .... The sibling testcross_wallet_top_up_leaves_the_paying_wallets_identity_state_intact(lines 4713-4721) explicitly asserts the error is notTaskError::IdentityNotWalletOwnedorTaskError::IdentityIndexMismatch, to prove the operation reached funding rather than being rejected by an early routing guard.This test lacks that assertion. If a routing guard rejects the top-up before the code ever touches the identity manager,
own.identitywas never at risk, and the final assertion onresolved_managed_identity_idpasses without exercising the displacement path the test is meant to guard against. Add the same error-type assertion here so the test proves the displacement-prevention logic actually ran.🔧 Proposed fix
- let _error = dispatch_wallet_funded_top_up(&ctx, &sender, &foreign, &payer_arc, 0).await; + let error = dispatch_wallet_funded_top_up(&ctx, &sender, &foreign, &payer_arc, 0).await; + assert!( + !matches!( + error, + TaskError::IdentityNotWalletOwned { .. } | TaskError::IdentityIndexMismatch { .. } + ), + "the displacement risk must be tested against a real funding attempt, not an early \ + routing rejection: {error:?}" + );🤖 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` around lines 4746 - 4786, Update cross_wallet_top_up_never_displaces_the_paying_wallets_own_identity to inspect the result from dispatch_wallet_funded_top_up and assert its error is not TaskError::IdentityNotWalletOwned or TaskError::IdentityIndexMismatch, matching cross_wallet_top_up_leaves_the_paying_wallets_identity_state_intact. Retain the existing resolved_managed_identity_id assertion after confirming the operation reached the intended funding path.
🤖 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`:
- Around line 3781-3846: In
a_provisioned_identity_topup_account_survives_a_restart, drain the context’s
background subtasks after backend.shutdown().await and before
copy_dir_recursive. Use the established ctx.subtasks.shutdown_async() pattern so
all persistence writes complete before the data directory snapshot is copied.
- Around line 4746-4786: Update
cross_wallet_top_up_never_displaces_the_paying_wallets_own_identity to inspect
the result from dispatch_wallet_funded_top_up and assert its error is not
TaskError::IdentityNotWalletOwned or TaskError::IdentityIndexMismatch, matching
cross_wallet_top_up_leaves_the_paying_wallets_identity_state_intact. Retain the
existing resolved_managed_identity_id assertion after confirming the operation
reached the intended funding path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82cc11a6-871d-4676-8884-5ba81a678223
📒 Files selected for processing (4)
src/backend_task/error.rssrc/backend_task/identity/mod.rssrc/context/wallet_lifecycle/tests.rssrc/wallet_backend/identity_ops.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend_task/identity/mod.rs
- src/backend_task/error.rs
- src/wallet_backend/identity_ops.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The merged delta adds the index-less foreign-top-up funding branch, but it does not change the non-atomic identity-index preflight, best-effort removal ordering, or transient persistence chain. Two blocking data-integrity issues remain, and the paid-registration, unbound-account restart, and removal regressions still leave important behavior unproven.
Source: reviewers gpt-5.6-sol (general, rust-quality); verifier gpt-5.6-sol.
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)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/wallet_lifecycle/tests.rs`:
- [SUGGESTION] src/context/wallet_lifecycle/tests.rs:4574-4581: Prove the unbound top-up account survives a restart
The merged branch routes `Funding::TopUpNotBound` through this PR's account-registration persistence path, but this test calls `ensure_unbound_topup_funding_account` twice in the same process. That proves only that the two in-memory account collections were populated; it does not assert the `identity_topup_unbound` manifest row or verify that a cold boot reconstructs the account needed to resume a foreign-identity asset lock. The existing restart regression covers only the indexed `IdentityTopUp { registration_index }` variant. Add a cold-boot regression that provisions the unbound account, verifies its account-registration row, reloads the wallet, and confirms the account remains available for unbound top-up recovery.
3 participants
TL;DR: Harden identity-index handling on a wallet: two independent, real bugs found while investigating a wallet-load failure, neither of which turned out to be that failure's actual cause (see status note above).
User story
As a Dash Evo Tool user with more than one identity on the same wallet, I want the app to refuse a duplicate identity index instead of silently colliding, and to keep a top-up account's registration on disk, so a restart never strands my funds or my saved wallet state.
Scenario
Base flow
A user adds identities to a wallet over time — including via "add existing identity," which lets the user pick the identity index themselves — and tops up an identity's funding from the wallet.
Actual behavior
(wallet_id, index); a second identity at a taken index silently displaces the first from the active set while both remain persisted, and the next launch's reload finds the displaced identity's now-ownerless data and rejects the entire wallet's saved state as fatally corrupted ("Saved wallet data appears damaged and cannot be loaded. Restore the wallet from its recovery phrase to keep using it.").Expected behavior
A second identity at a taken index is refused up front on every path that can create one, not just some of them. A provisioned top-up account survives a restart.
Detailed discussion
What was NOT found here — the actual reported incident
The user's specific "Saved wallet data appears damaged" incident was traced (by loading their real, unmodified database) to an upstream bug in
rs-platform-wallet-storage(dashpay/platform, pinned git dependency, not vendored in this repo):derive_spent_utxosfabricates an emptyScriptBuffor a spent input it has no prior-output record for; contrary to its own doc comment ("informational only"), that row is persisted (INSERTed when no existing row matches);load_used_addressesthen scans every row's script on load, andAddress::from_scripton the empty script fails asUnrecognizedScript, which is treated as a fatal, whole-file-rejecting error. Fixed upstream and pulled in separately by #953 — not part of this PR.What was done
WalletBackend::ensure_identity_managednow checks whether the target identity index already has an occupant before registering a new identity, returning a new typedTaskError::IdentityIndexAlreadyTaken { occupant_id, index }instead of an ambiguous insert. Separately,provision_identity_funding_accountnow persists the new account'sAccountRegistrationEntrythrough the wallet persister (with rollback of the in-memory inserts on a persist failure) instead of leaving it memory-only.Regression tests:
a_second_identity_at_a_taken_index_is_refused_and_the_wallet_still_reloads,a_migrated_install_relaunches_without_damaged_wallet_data(confirmed RED before the fix against the duplicate-index mechanism specifically — this is not a reproduction of the user's actual incident, see status note),a_provisioned_identity_topup_account_survives_a_restart(RED before its fix).Gaps found by independent review, now closed on the branch (commit pending push):
WalletBackend::ensure_identity_managed). The primary identity-registration path (WalletBackend::register_identity→ upstreamregister_identity_with_funding) reached the same unguarded upstream insert — the UI's own "used index" gate can't backstop it either, since it hydrates empty on every cold boot (pre-existing TODO already flagged this UI-side unreliability).WalletBackend::register_identitynow pre-flights the same occupancy check before the asset lock is built.AppContext::remove_identitynow also releases the index upstream (tombstone, not delete — no user data removed).warnand silently leaves the second identity unregistered rather than surfacing a banner — no new collision can be created anymore, so this only affects pre-existing duplicates, and a full reconcile-time banner wasn't judged worth the UX cost for a shrinking, non-growable case.Testing
clippy --all-features --lib -- -D warnings— clean on every commit (implementer + independently reproduced by reviewer each time).test --lib --all-features— 2310+ passed, 0 failed; all new/changed tests confirmed passing by name (not just aggregate count), independently reproduced by reviewer.dfcb9065(identity-index guard): 1 HIGH + 2 MEDIUM found, both fixed indc547a14.72de5aeb(top-up persistence): ship recommended; minor non-blocking rollback-error-classification follow-up.dc547a14(closes the HIGH+MEDIUM gaps): ship-with-followup. Both corruption routes independently proven closed by execution (offline reproductions, not just reading the diff) — reviewer wrote and ran an offline test proving the registration-path guard fires, and independently reproduced RED-before on the removal-path test. 3 MEDIUM + 3 LOW non-blocking findings, tracked as follow-up (memcan TODO, project=dash-evo-tool-2): the HIGH fix itself has no regression test despite the commit claiming it's untestable offline (reviewer's own test disproves that); a narrow TOCTOU window where the boot-reconciler subtask can still slip into the same collision; the new error message's remediation wording doesn't match how the picker actually works; a few lower-severity gaps in logging/test coverage.Breaking changes
None. All three fixes only reject/complete previously-silent, already-broken behavior.
Checklist
cargo fmt --all,clippy --all-features --all-targets -- -D warningsclean on touched scopedc547a14)Prior work
dashpay/platformpin to the upstream fix.broadcast, block processing pins, and re-import cannot rebuild it #930 (identity/asset-lock persistence).Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit