Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/app_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,24 @@ pub fn create_app_user_data_directory_if_not_exists() -> Result<(), std::io::Err
ensure_data_dir_exists(&app_data_dir)
}

/// Copy a data directory tree, permissions included. Cold-boot tests reopen
/// wallet state over a fresh path with identical bytes, sidestepping the
/// persister's single-open advisory lock a lingering subtask may still hold.
#[cfg(test)]
pub(crate) fn copy_dir_recursive(src: &Path, dst: &Path) {
ensure_data_dir_exists(dst).expect("create destination directory");
for entry in fs::read_dir(src).expect("read_dir") {
let entry = entry.expect("dir entry");
let from = entry.path();
let to = dst.join(entry.file_name());
if from.is_dir() {
copy_dir_recursive(&from, &to);
} else {
fs::copy(&from, &to).expect("copy file");
}
}
}

/// Creates the given data directory if it does not exist and verifies it is a directory.
pub fn ensure_data_dir_exists(data_dir: &Path) -> Result<(), std::io::Error> {
#[cfg(unix)]
Expand Down
27 changes: 27 additions & 0 deletions src/backend_task/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,18 @@ pub enum TaskError {
source: dash_sdk::dpp::key_wallet::Error,
},

/// An identity-funding account was derived but could not be saved. Saving
/// it is what lets a restart find the account again, so the operation is
/// stopped rather than left able to strand a funding lock the app could no
/// longer spend. The technical cause lives in `Debug` and the logs.
#[error(
"Your wallet could not save the account this payment needs. Check that your disk is not full, then try again."
)]
IdentityFundingAccountPersistFailed {
#[source]
source: Box<platform_wallet::changeset::PersistenceError>,
},

/// Single-key wallets are not supported in this version. Their data is
/// preserved; HD (recovery-phrase) wallets remain fully functional.
#[error(
Expand Down Expand Up @@ -303,6 +315,21 @@ pub enum TaskError {
wallet_index: u32,
},

/// Two identities on one wallet claim the same identity index. The wallet
/// backend keys its active set on that index, so admitting the second
/// would displace the first from memory while both keep their saved
/// records — and the next launch would reject the whole wallet's saved
/// data as damaged. Stopped before anything is written.
///
/// `index` is a numeric diagnostic for logs and the `Debug` view only.
#[error(
"Identity {occupant_id} on this wallet already uses this identity index, and every identity on a wallet needs its own. Pick an identity index that is not marked as used, or remove identity {occupant_id} first, then try again."
)]
IdentityIndexAlreadyTaken {
occupant_id: dash_sdk::platform::Identifier,
index: u32,
},

/// A wallet-funded top-up targeted an identity this wallet does not own
/// (it has no HD funding slot here). Funding it from this wallet would
/// derive an unrelated asset-lock account, so the op is stopped before any
Expand Down
2 changes: 1 addition & 1 deletion src/backend_task/identity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,7 @@ impl AppContext {
IdentityTask::RegisterDpnsName(input) => {
Ok(self.register_dpns_name(sdk, input).await?)
}
IdentityTask::RemoveIdentity { identity_id } => self.remove_identity(identity_id),
IdentityTask::RemoveIdentity { identity_id } => self.remove_identity(identity_id).await,
IdentityTask::RefreshIdentity(qualified_identity) => {
self.refresh_identity(sdk, qualified_identity, sender).await
}
Expand Down
27 changes: 26 additions & 1 deletion src/backend_task/identity/remove_identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use dash_sdk::dpp::identity::accessors::IdentityGettersV0;
use dash_sdk::platform::Identifier;

impl AppContext {
pub(super) fn remove_identity(
pub(super) async fn remove_identity(
&self,
identity_id: Identifier,
) -> Result<BackendTaskSuccessResult, TaskError> {
Expand All @@ -16,11 +16,13 @@ impl AppContext {
.map(|(voter_identity, _)| voter_identity.id())
});

self.release_identity_index(&identity_id).await;
self.delete_local_qualified_identity(&identity_id)?;
Comment on lines +19 to 20

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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 mut removed_identity_ids = vec![identity_id];
let mut associated_cleanup_failed = false;
if let Some(voter_id) = associated_voter_identity_id.filter(|id| *id != identity_id) {
self.release_identity_index(&voter_id).await;
match self.delete_local_qualified_identity(&voter_id) {
Ok(()) => removed_identity_ids.push(voter_id),
Err(error) => {
Expand All @@ -39,4 +41,27 @@ impl AppContext {
associated_cleanup_failed,
})
}

/// Release the identity index this identity holds in the wallet backend.
///
/// DET's own records are not the only ones holding it: an entry left behind
/// upstream keeps the index occupied by an identity the user believes is
/// gone, which blocks re-adding one there and lets a later registration
/// collide into the damaged-saved-data failure.
///
/// Best-effort — a wallet not registered this session has nothing to
/// release, and the user's removal must complete either way.
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"
);
}
Comment on lines +54 to +65

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
}
60 changes: 60 additions & 0 deletions src/backend_task/migration/v093_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1577,6 +1577,66 @@ async fn a_second_launch_after_an_unreadable_identity_preserves_user_edits_and_d
backend.shutdown().await;
}

/// A relaunch of an already-migrated install: a second `AppContext` over the
/// same on-disk state, reading the settings the first boot wrote.
fn reopen(dir: &std::path::Path) -> Arc<AppContext> {
let db = Arc::new(
Database::open_legacy_read_only(dir.join("data.db")).expect("open data.db read-only"),
);
let app_kv = AppContext::open_app_kv(dir).expect("open app k/v");
let settings = app_kv
.get::<AppSettings>(DetScope::Global, AppSettings::KV_KEY)
.expect("read settings blob")
.expect("the first boot must have written a settings blob");
let secret_store = AppContext::open_secret_store(dir).expect("open secret store");
AppContext::new(
dir.to_path_buf(),
settings.network,
db,
Default::default(),
Default::default(),
egui::Context::default(),
app_kv,
secret_store,
crate::model::user_role::UserRoleCell::default(),
)
.expect("AppContext")
}

/// The user's reproduction, minus the network: migrate a v0.9.3 install that
/// holds a password-protected wallet, close the app, open it again. The
/// relaunch must load the saved wallet state — a fatal persister load surfaces
/// as `WalletLocalDataLoadFailed` ("Saved wallet data appears damaged").
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn a_migrated_install_relaunches_without_damaged_wallet_data() {
let tmp = tempfile::tempdir().expect("tempdir");
write_v093_database(tmp.path());

let (ctx, _settings) = boot(tmp.path());
let backend = wire_backend(&ctx).await;
assert!(
run_migration_with_wallet_passwords(&ctx)
.await
.expect("migration"),
"precondition: the fixture has data to move",
);
backend.shutdown().await;
drop(backend);
drop(ctx);

let cold = tempfile::tempdir().expect("cold tempdir");
crate::app_dir::copy_dir_recursive(tmp.path(), cold.path());

let ctx2 = reopen(cold.path());
let (tx, _rx) = tokio::sync::mpsc::channel::<crate::app::TaskResult>(32);
let sender = crate::utils::egui_mpsc::SenderAsync::new(tx, ctx2.egui_ctx().clone());
ctx2.ensure_wallet_backend(sender)
.await
.expect("a relaunch after the storage update must load the saved wallet data");
let backend2 = ctx2.wallet_backend().expect("backend wired");
backend2.shutdown().await;
}

/// The identity import carries its own sentinel. Reusing the wallet drain's
/// would silently skip the import for every install that already drained its
/// wallets under a build that had no identity importer — i.e. exactly the
Expand Down
7 changes: 7 additions & 0 deletions src/context/wallet_lifecycle/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,13 @@ impl AppContext {
{
Ok(true) => added += 1,
Ok(false) => {}
// A taken index stays taken until the user re-indexes the
// identity, so this one is not deferred and never retries.
Err(error @ TaskError::IdentityIndexAlreadyTaken { .. }) => tracing::warn!(
identity = %qi.identity.id(),
%error,
"Identity shares its identity index with another on this wallet; left unregistered"
),
Err(error) => tracing::debug!(
identity = %qi.identity.id(),
%error,
Expand Down
Loading
Loading