diff --git a/Cargo.toml b/Cargo.toml index 36365d591..60c3c21b2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,13 @@ serde = "1.0.219" serde_json = "1.0.140" serde_yaml_ng = { version = "0.10.0" } tokio = { version = "1.46.1", features = ["full"] } +# Pin is deliberate; do not "fix" the audit warning by bumping. bincode is flagged +# unmaintained (RUSTSEC-2025-0141, INFO-level — no known vulnerability). The advisory +# covers *all* versions, so no bump can clear it: 2.0.1 is the last functional release +# and 3.0.0 is a tombstone whose lib.rs is a bare `compile_error!`. This crate encodes +# the on-disk wallet-secret envelopes and QualifiedIdentity blobs, so switching encoders +# changes the wire format of data users already have on disk — that needs a designed +# read-old/write-new migration, not a dependency bump. bincode = { version = "=2.0.1", features = ["serde"] } hex = { version = "0.4.3" } async-trait = "0.1.89" diff --git a/docs/ai-design/2026-05-28-migration-tool/notes.md b/docs/ai-design/2026-05-28-migration-tool/notes.md index e045163f0..463e3ef56 100644 --- a/docs/ai-design/2026-05-28-migration-tool/notes.md +++ b/docs/ai-design/2026-05-28-migration-tool/notes.md @@ -203,19 +203,27 @@ idempotency is confirmed. ### `identity` (DET source file: `src/database/identities.rs`) -- **Source:** `identity` in `data.db` -- **Destination:** `identities.entry_blob` (typed BLOB column) in `platform-wallet-storage` -- **Mapping:** Deserialize DET's stored identity representation → serialize as - bincode-encoded `QualifiedIdentity` with a leading version byte prepended -- **Per-network split:** Yes -- **Gotchas:** Upstream schema uses a leading version byte in `entry_blob` for - forward/backward compatibility — this byte must be present and set correctly, or upstream - deserialization will silently produce garbage. Confirm the byte format with the - platform-wallet-storage author before implementing. This is the highest-risk table in the - migration. +> **SUPERSEDED (2026-07-13).** Design and task breakdown now live in +> `docs/ai-design/2026-07-13-legacy-identity-migration/design.md`. The entry below is kept +> for context; two of its claims are wrong. The destination is **not** upstream's +> `identities.entry_blob` — commit `b14bf32c` moved it to DET's own per-network k/v +> (`det:identity:v1` under `DetScope::Identity`, roster at `det:identity_index:v1`). And the +> version byte needs no agreement with the platform-wallet-storage author: `DetKv::put` +> prepends `kv::SCHEMA_VERSION` automatically, so an importer that goes through +> `AppContext::insert_local_qualified_identity` gets it for free. + +- **Source:** `identity` in `data.db` (`is_local = 1 AND data IS NOT NULL`) +- **Destination:** `StoredQualifiedIdentity` in DET's per-network k/v — see the design doc +- **Mapping:** `QualifiedIdentity::from_bytes(row.data)` → restore `status` from its column → + `AppContext::insert_local_qualified_identity(&qi, &wallet_link)`. The `data` BLOB carries + **all identity key material**; there is no second key table in v0.9.3. +- **Per-network split:** Yes — two-value filter (`mainnet` / legacy `dash`) +- **Gotchas:** The real risk is cross-version bincode compatibility of the `data` BLOB, not a + version byte. Every DET-side and dpp-side struct in the blob was verified unchanged between + v0.9.3 and HEAD; only the `bincode` rc.3 → 2.0.1 wire format is unproven, and a golden-blob + test closes it. Still the highest-risk table in the migration. - **Status:** DONE for new-install path — see commit `b14bf32c` (identities + tokens → - per-network k/v). Migration tool still needs to import legacy rows with the version-byte - contract correct. + per-network k/v). Migration tool still needs to import legacy rows. --- diff --git a/docs/ai-design/2026-07-13-legacy-identity-migration/design.md b/docs/ai-design/2026-07-13-legacy-identity-migration/design.md new file mode 100644 index 000000000..0d3375baf --- /dev/null +++ b/docs/ai-design/2026-07-13-legacy-identity-migration/design.md @@ -0,0 +1,547 @@ +# Legacy identity import (v0.9.3 → v1.0) + +**Status:** implemented and shipped — PR #885 (`feat/legacy-identity-migration`), +with QA follow-ups in PR #891. All tasks in §9 (T-ID-01 … T-ID-06) landed. This +document is retained as the design record: it describes shipped behaviour, not a +pending proposal. +**Scope:** carry the legacy `data.db` `identity` rows into the modern +`StoredQualifiedIdentity` k/v store during the cold-start migration. +**Known limitation:** a partially-loaded identity strands its legacy-only keys +(§7); an opt-in recovery flow is tracked in +[issue #889](https://github.com/dashpay/dash-evo-tool/issues/889). + +--- + +## 1. The gap + +The schema ladder preserves the v0.9.3 `identity` table intact — `v093_upgrade.rs` +asserts exactly that: `alias`, `wallet`, `wallet_index`, `identity_type` and +`network` all survive from schema v11 to current. Nothing then reads it. No +production code path imports those rows into the per-network k/v store that +`AppContext::load_local_qualified_identities` reads from, so a v0.9.3 user who +upgrades finds an empty Identities screen and an empty Masternodes screen. A +masternode owner loses the owner/voting/payout keys they had loaded and must +re-import them by hand. + +The `2026-05-28-migration-tool` notes already called `identity` "the highest-risk +table in the migration" and left it at "Migration tool still needs to import +legacy rows with the version-byte contract correct." This document settles what +that contract actually is, and it is not what the note assumed. + +--- + +## 2. What v0.9.3 actually persisted + +Source: `git show v0.9.3:src/database/identities.rs`, `…:src/database/initialization.rs`. + +```sql +CREATE TABLE identity ( + id BLOB PRIMARY KEY, -- 32-byte Identifier + data BLOB, -- QualifiedIdentity::to_bytes() (NULLABLE) + status INTEGER NOT NULL DEFAULT 0, + is_local INTEGER NOT NULL, -- 1 = user-owned, 0 = observed-only + alias TEXT, + info TEXT, -- never written by v0.9.3 + wallet BLOB, -- WalletSeedHash ([u8; 32]), nullable + wallet_index INTEGER, -- u32, NOT NULL iff wallet IS NOT NULL + identity_type TEXT, -- format!("{:?}") of IdentityType + network TEXT NOT NULL, + CHECK ((wallet IS NOT NULL AND wallet_index IS NOT NULL) + OR (wallet IS NULL AND wallet_index IS NULL)) +); +``` + +Three facts that decide the whole design: + +1. **`data` is the entire identity, keys included.** v0.9.3's + `insert_local_qualified_identity` writes `qualified_identity.to_bytes()` — + bincode of the full `QualifiedIdentity`, whose `private_keys: KeyStorage` + carries owner / voting / payout private-key material as `Clear`, + `AlwaysClear`, `Encrypted`, or `AtWalletDerivationPath`. There is **no second + table** holding identity keys in v0.9.3. Everything to migrate is in this one + BLOB plus five scalar columns. Key material is therefore *not* re-derivable + from the wallet seed in the general case: a masternode owner's owner/voting + key is typically an imported WIF stored as `Clear`, not an HD path. +2. **`data` is nullable, and `is_local` is not always 1.** v0.9.3's + `insert_identity_if_not_exists` writes observed (non-local) identities with + `is_local = 0` and a possibly-`NULL` blob. Every v0.9.3 read path filters + `WHERE is_local = 1 AND data IS NOT NULL`. The import must filter identically + — a non-local row is a cache entry, not user data. +3. **`identity_type` is `Debug`-formatted**, yielding exactly `"User"` / + `"Masternode"` / `"Evonode"` — byte-identical to the modern + `IdentityType::as_tag()`. **All three variants can appear.** The test's + masternode-only fixture is *not* representative: a v0.9.3 user with a DPNS + name has a `User` identity, and an evonode operator has `Evonode`. The import + must be type-agnostic (it is, if it round-trips the blob). + +--- + +## 3. The target shape + +`src/context/identity_db.rs`: + +```rust +struct StoredQualifiedIdentity { + qi_bytes: Vec, // QualifiedIdentity::to_bytes() + status: u8, + identity_type: String, // IdentityType::as_tag() + wallet_hash: Option<[u8; 32]>, + wallet_index: Option, +} +``` + +written at `DetScope::Identity(&id)` / key `det:identity:v1`, with the id also +registered in the Global roster `det:identity_index:v1` (there is no +cross-identity listing under `DetScope::Identity`, so an identity absent from +the roster is invisible to every load path). + +The production writer is `AppContext::insert_local_qualified_identity(&qi, &Some((seed_hash, index)))`. +It already does, in order: vault-first key extraction → blob encode → roster +insert → `DetKv::put`. **The import must call it and nothing lower.** + +--- + +## 4. The version-byte contract + +The 2026-05-28 note said the destination was upstream's `identities.entry_blob` +with a leading version byte "confirm the byte format with the +platform-wallet-storage author before implementing." That is **stale**. The +destination moved (commit `b14bf32c`) to DET's own per-network k/v. There are now +two byte layers, and neither needs a new author agreement: + +| Layer | Produced by | Format | +|---|---|---| +| **Outer** (k/v value) | `DetKv::put` — `src/wallet_backend/kv.rs:188` | `[ SCHEMA_VERSION (1B) = 1 ‖ bincode(StoredQualifiedIdentity) ]` | +| **Inner** (`qi_bytes`) | `QualifiedIdentity::to_bytes()` — `src/model/qualified_identity/mod.rs:510` | `bincode(QualifiedIdentity, config::standard())` — **no version byte** | + +The outer byte is prepended automatically and validated on read +(`KvAdapterError::SchemaVersion` on mismatch). The importer gets it for free by +going through `insert_local_qualified_identity`. **Hand-rolling the blob bytes is +the one way to get this wrong; the design forbids it.** + +The inner layer is where the real risk lives, and it is a *cross-version bincode +compatibility* question, not a version-byte question: + +### 4.1 Evidence that the legacy blob decodes on HEAD + +| Item | v0.9.3 | HEAD | Verdict | +|---|---|---|---| +| `QualifiedIdentity` manual `Encode` field order | `identity, associated_voter_identity, associated_operator_identity, associated_owner_key_id, identity_type, alias, private_keys, dpns_names` | identical | ✅ | +| `IdentityType` | `User, Masternode, Evonode` | identical | ✅ | +| `PrivateKeyTarget` | 3 variants, same order | identical | ✅ | +| `QualifiedIdentityPublicKey` | `{ identity_public_key, in_wallet_at_derivation_path }` | identical | ✅ | +| `KeyStorage` | `BTreeMap<(PrivateKeyTarget, KeyID), (QualifiedIdentityPublicKey, PrivateKeyData)>` | identical | ✅ | +| `PrivateKeyData` | `AlwaysClear, Clear, Encrypted, AtWalletDerivationPath` (0–3) | same 0–3, `InVault` **appended** at 4 | ✅ legacy discriminants unchanged | +| `WalletSeedHash` | `[u8; 32]` | identical | ✅ | +| dpp `IdentityV0` / `IdentityPublicKeyV0` fields (platform `29f7492` vs `44c20e3`) | `{id, public_keys, balance, revision}` / `{id, purpose, security_level, contract_bounds, key_type, read_only, data, disabled_at}` | identical — the inter-rev diff is serde attributes and added tests only | ✅ | +| dpp `Purpose` / `SecurityLevel` / `KeyType` / `ContractBounds` | — | no variant added or reordered | ✅ | +| bincode | `=2.0.0-rc.3` | `=2.0.1` | ⚠️ **unverified** | + +Everything structural checks out. The one open item is whether bincode's +`config::standard()` wire format is identical between `2.0.0-rc.3` and `2.0.1`. +It almost certainly is (varint + little-endian was frozen well before rc.3), but +"almost certainly" is not a contract for a blob that holds a masternode owner +key. **T-ID-06 (§9) closes it empirically with a golden blob.** Nothing else in +this plan depends on the answer; if the format *did* drift, the import would fail +loudly on decode (bincode errors, it does not silently produce garbage), the +sentinel would be withheld, and the legacy rows would still be there. + +--- + +## 5. Where the step plugs in + +A **sibling pass in `finish_unwire::run`**, after `drain_wallets` succeeds, under +its **own per-network sentinel**: + +``` +det:migration:identities::v1 +``` + +```rust +pub async fn run(app_context: &Arc) -> Result { + // Held, NOT unwrapped: DET's own rows may not gate the funds path. + let app_data = migrate_app_data(app_context); + + // Funds first. Nothing above may withhold a seed. + let wallet_moved = drain_wallets(app_context).await?; + + // Held too — the two DET-owned passes must not gate each other. + let identities = migrate_identities(app_context)?; + + if identities.unreadable > 0 { + // `app_data` is judged HERE, never before: unwrapping it first would + // return its (deterministic) error and mask the identity signal on this + // launch AND every retry, stranding a masternode owner's keys over a + // corrupt vote queue. Each arm publishes its own terminal state and + // returns `Ok` — a returned `Err` would be re-published as a plain + // `Failed`, dropping the identity count. + match app_data { + // SucceededWithUnreadableIdentities, or …AndVotes when the durable + // vote-warning record reads back non-empty. A failing *read* costs + // only the vote half — never the identity half, and never a `Failed`. + Ok(outcome) => { + let moved_data = + wallet_moved || outcome.moved_data() || identities.moved_data(); + … + return Ok(moved_data); + } + // The one true FailedWithUnreadableIdentities: a hard app-data + // failure. Both signals ride one retryable banner. + Err(app_data_error) => { + let moved_data = wallet_moved || identities.moved_data(); + … + return Ok(moved_data); + } + } + } + + // Every identity decoded, so app-data no longer masks anything: unwrap it. + let app_data = app_data?; + let moved_data = wallet_moved || app_data.moved_data() || identities.moved_data(); + … +} +``` + +**Held, then judged.** Each DET-owned pass runs unconditionally and its `Result` +is *held* — the order in which the two are **unwrapped** is the load-bearing part, +not the order in which they run. An app-data failure is deterministic (one +malformed vote-index blob is enough) and never writes its sentinel, so unwrapping +it ahead of the identity outcome would skip the identity import forever, not just +once. + +**Why after `drain_wallets`, not inside it.** The import needs three things the +drain produces: the wallet backend wired (`det_kv()` is `wallet_backend()?.kv()`), +the secret vault reachable (key material goes there), and `ctx.wallets` hydrated +(`register_migrated_wallets` calls `hydrate_context_wallets`) so that +`AtWalletDerivationPath` keys land against a wallet that actually exists. Running +it inside the drain, before registration, would import identities whose wallet +link points at nothing yet. + +**Why a new sentinel, not the drain's.** This is the same trap the app-data +sentinel was created to dodge, and the code says so at `migrate_app_data`: *"an +install that already completed the wallet drain under an earlier build (which had +no app-data import) still has those rows in `data.db`, and the wallet sentinel +would otherwise short-circuit the launch and strand them."* Every alpha/RC tester +who has already run the drain has `det:migration:finish_unwire::v1` written. +Reusing it would ship an identity importer that never runs for exactly the people +testing it. A third sentinel costs one constant. + +**Failure isolation.** A failing identity import must not withhold the wallet +sentinel — funds access is already restored and sentinel-guarded by the time this +runs. Conversely the identity sentinel is withheld on any failure so the next +launch retries. `MigrationStep::Identities` is added for the progress UI. + +--- + +## 6. Data mapping + +| Legacy column | Destination | Notes | +|---|---|---| +| `data` (BLOB) | `QualifiedIdentity::from_bytes` → `qi.` everything | Includes **all key material**. | +| `status` (u8) | `qi.status = IdentityStatus::from_u8(..)` | The encoder **skips** `status`, so a decoded blob carries the default `Unknown`. Restoring it from the column is mandatory or every migrated identity reads back as "unknown, refresh required". | +| `alias` | `qi.alias = column` (unconditional) | The SQL column is authoritative; the blob's copy is stale. In v0.9.3 `set_identity_alias` updated **only** the column, and every loader decoded the blob then unconditionally overwrote `alias` with the column value. A rename or removal left the blob stale, so the column always won. Migrating with a blob-first fallback would resurrect a renamed-away alias or reverse a removal — so the column always wins here, including a NULL column clearing a stale blob alias. | +| `identity_type` | already inside `data` | Column is `format!("{:?}")` of the same value. `insert_local_qualified_identity` re-derives the wrapper's tag from `qi.identity_type`. Do not read the column. | +| `wallet` + `wallet_index` | `wallet_and_identity_id_info: Option<(WalletSeedHash, u32)>` | Both-or-neither (enforced by the legacy `CHECK`). | +| `network` | row filter `WHERE network IN (?1, ?2)` | Two-value filter via `mainnet_alias_for()` — a pre-v29 DB spells mainnet `dash`. | +| `is_local` | filter `= 1` | Non-local rows are observed-identity cache. Skip. | +| `info` | dropped | Never written by v0.9.3. | +| `top_up` join | already imported | `migrate_app_data` writes `det:top_ups:v1` under the same `DetScope::Identity`. Independent key; no interaction. | + +`qi.network` is set by the read path (`decode_stored_identity`), not stored. +`qi.associated_wallets` / `qi.secret_access` / `qi.top_ups` are runtime wiring, +rehydrated on load. None of them need migrating. + +--- + +## 7. Edge cases + +| Case | Behaviour | Rationale | +|---|---|---| +| **Row already in the k/v store** | Skip the row wholesale (`skipped_existing`). The present record is never re-persisted, and legacy-only keys are **not** reconciled into it. **Known limitation** — see below. | Field *absence* cannot be told apart from a deliberate removal: an identity missing a key may have had it removed by the user ("Remove private key from DET", no tombstone), and a cleared alias persists as `None`; refilling from the stale legacy blob would resurrect either. A protected identity would additionally trip `encode_identity_blob_vault_first`'s `IdentityKeyProtectionDowngrade` guard if a plaintext legacy key were merged in, failing the whole pass. Provenance the model does not carry would be needed to reconcile safely, so the importer stays conservative and skips. | +| **Linked wallet failed to migrate / absent** | Import the identity anyway, **preserving `wallet_hash` + `wallet_index` verbatim**. Log at `warn`. | The link is what `load_local_qualified_identities_for_wallet` uses to re-attach the identity when the wallet is later restored or unlocked. Nulling it would orphan the identity permanently. A protected (locked) wallet is the *normal* case here — its seed is in the vault but it is not in `ctx.wallets` until the user unlocks. | +| **`data IS NULL` or `is_local = 0`** | Skip silently, do not count as failure. | Not user identity data; v0.9.3's own readers ignore these rows. | +| **`data` fails to decode** | Count `unreadable`, log at `warn` with the identity id, **withhold the sentinel**, publish a terminal warning state. Do **not** fail the pass. | Diverges from the scheduled-vote policy (which writes the sentinel anyway) on purpose: a corrupt vote row is unrecoverable, but an undecodable identity blob may be a *decoder* defect (bincode drift, §4.1) that a later build fixes. Withholding the sentinel keeps the retry door open at the cost of one cheap re-attempt per launch; the skip-if-present rule makes the retry a no-op for everything that already landed. Failing the pass instead would be wrong — it would gate nothing useful and shout at a user who cannot act. | +| **`id` not 32 bytes / `wallet` not 32 bytes** | Count `unreadable`, same as above. | Corruption. | +| **Identity type the fixture doesn't cover (`User`, `Evonode`)** | Handled with no extra code — the type lives in the blob and round-trips. | See §2.3. The *test* must cover it; the *code* need not branch on it. | +| **Second launch** | Sentinel short-circuits; nothing is read, nothing is written. | Mirrors `drain_wallets`. | +| **Legacy rows** | **Never deleted.** | Repo-wide migration rule: a migration that deletes its source can never be retried. | + +### Known limitation — a partially-loaded identity strands its legacy-only keys + +If a v0.9.3 identity was already loaded into the modern store *before* migration +but only partially — the canonical case is a masternode brought in from just its +ProTxHash, which persists a **bare** record (no private keys) plus, possibly, +missing owner/voting/payout associations — the importer skips it and does **not** +backfill the keys still sitting in the legacy blob. Those keys become +inaccessible through the current UI: the record shows as present but keyless. + +No data is destroyed. The legacy `data.db` is preserved verbatim (rows are never +deleted), so a future recovery flow can read those keys back. The conservative +skip is deliberate — as the edge-case rationale explains, field absence cannot be +distinguished from a deliberate user removal without provenance the model does +not carry, and merging a plaintext legacy key into a protected identity would +trip the vault-first downgrade guard. Rather than a heuristic that risks +resurrecting removed keys or failing the whole pass, the safe behaviour is to +skip and defer recovery to a dedicated, provenance-aware flow. + +The proper recovery flow — an interactive, opt-in re-import that reads the +preserved legacy blob and merges only genuinely-missing key material under the +identity password — is tracked as a follow-up in +[issue #889](https://github.com/dashpay/dash-evo-tool/issues/889). + +--- + +## 8. Security + +This step moves identity private keys. The repo rule (`CLAUDE.md`, DET Module +Placement Policy) is that all wallet/identity secret bytes enter and leave the +vault through the single `wallet_backend/secret_seam.rs` chokepoint. + +**The design satisfies this by not writing any secret-handling code at all.** +`AppContext::insert_local_qualified_identity` → +`encode_identity_blob_vault_first` → `IdentityKeyView::store_all` → +`SecretSeam`. The importer's only contact with key material is the +`QualifiedIdentity` value it holds between `from_bytes` and `insert_…`, which is +the same handling every existing load path already performs. + +Consequences the implementer must respect: + +- **Do not** construct `StoredQualifiedIdentity` directly (it is private to + `identity_db.rs` — keep it that way). +- **Do not** write `qi_bytes` from the legacy blob verbatim. It would persist + `Clear` / `AlwaysClear` plaintext keys into `det-app.sqlite`, bypassing the + vault — precisely the leak `encode_identity_blob_vault_first` exists to close. + The eager load-path migration would eventually repair it, but "eventually" is + not a security property. +- **Do not** log the blob, the decoded `qi`, or any `PrivateKeyData`. `Debug` is + already redacting on `StoredQualifiedIdentity` and `PrivateKeyData`; log the + hex identity id only. +- A `PrivateKeyData::Encrypted(_)` key (legacy per-identity password envelope) + round-trips untouched — `has_plaintext_for_vault` ignores it, so it is neither + vaulted nor decrypted. That is correct: the migration has no password. + +--- + +## 9. Implementation tasks + +Ordered; each is independently reviewable. **All six shipped** — see the status +header. + +- **T-ID-01 — `database/legacy_import.rs`: `read_identities`.** + `pub(crate) fn read_identities(conn: &Connection, network: Network) -> rusqlite::Result` + returning `{ identities: Vec, unreadable: u32 }` where + `LegacyIdentityRow { id: [u8; 32], qi: QualifiedIdentity, wallet: Option<([u8; 32], u32)> }`. + There is **no separate `status` field**: the reader restores `status` (and + `alias`) from their columns straight onto `qi` before the row is yielded, so the + caller receives one already-correct `QualifiedIdentity` and cannot forget to + apply them (the blob encodes neither — see §6). + SQL: `SELECT id, data, status, wallet, wallet_index, alias FROM identity WHERE is_local = 1 AND data IS NOT NULL AND network IN (?1, ?2)`, + params `(network.to_string(), mainnet_alias_for(network))`. `alias` is selected + because the column — not the blob's stale copy — is authoritative (§6). Missing + table ⇒ empty. Per-row decode failure ⇒ `unreadable += 1`, warn, continue; a + wrong SQLite storage class costs its own row, not the whole read. Mirrors + `read_scheduled_votes` exactly. + *Depends on:* nothing. + +- **T-ID-02 — `MigrationStep::Identities`** in `context/migration_status.rs`, + plus its progress-UI label. + *Depends on:* nothing. + +- **T-ID-03 — `finish_unwire::migrate_identities`.** Sentinel + `identities_sentinel_key_for(network)` → `det:migration:identities::v1`. + Pure body `migrate_identities_from_conn(conn, network, wallet_known, is_present, insert) -> Result` + with closure seams (matching `migrate_app_data_from_conn`) so it unit-tests + without an `AppContext`. `is_present` is the skip-if-already-imported check; a + present identity is skipped wholesale (see the §7 known limitation). Counters: + `imported`, `skipped_existing`, `unreadable`. Sentinel written **iff + `unreadable == 0`**. + *Depends on:* T-ID-01, T-ID-02. + +- **T-ID-04 — Wire into `run()`** after `drain_wallets`, before the terminal + state. Add `MigrationError::IdentityImportFailed { … }` for hard k/v-write + failures and a `MigrationState::SucceededWithUnreadableIdentities { count }` + terminal variant (sibling of the existing votes variant), or fold both counts + into one warning state — implementer's call, but the user must be told *which* + domain failed, since the remedies differ (re-schedule a vote vs. re-import a key). + *Depends on:* T-ID-03. + +- **T-ID-05 — Detection gate.** Add `"identity"` to `LEGACY_TABLES`. Note this + gate belongs to `drain_wallets`; `migrate_identities` needs its own cheap + "table has rows" probe so an identity-only install (a masternode voter with no + HD wallet) is not skipped. + *Depends on:* T-ID-03. + +- **T-ID-06 — Golden-blob test (the bincode contract).** A checked-in hex + constant: a `QualifiedIdentity::to_bytes()` produced by the **real v0.9.3 + binary** (see §11), asserted to decode on HEAD into the expected identity, keys + and type. This is the only test that proves §4.1's one open row. Small, permanent, + zero build cost after generation. + *Depends on:* the Part-2 generator (§11). + +--- + +## 10. Test plan (`v093_upgrade.rs`) + +Extend the existing fixture; keep its mutation-tested shape. + +**Fixture changes** — replace the placeholder `data = vec![0u8; 16]` blob (junk, +decodes to nothing) with real blobs, and widen coverage past masternode-only: + +| Row | Type | `is_local` | Wallet link | Key shape | Locks | +|---|---|---|---|---|---| +| A | `Masternode` | 1 | unprotected wallet, idx 0 | `Clear` owner + `Clear` voting | the reported bug | +| B | `User` | 1 | unprotected wallet, idx 1 | `AtWalletDerivationPath` | non-masternode variant; wallet-derived key path | +| C | `Evonode` | 1 | **none** (`NULL`/`NULL`) | `Clear` | wallet-less identity (masternode loaded by ProTxHash) | +| D | `User` | **0** | — | — | observed-identity cache row must be skipped | +| E | `User` | 1 | protected wallet | `AtWalletDerivationPath` | identity on a **locked** wallet still imports, link preserved | +| F | any | 1 | — | `data = NULL` | null-blob row must be skipped, not counted as failure | + +**Assertions in `v093_install_upgrades_…`** (replacing the current +"the identity row survives the ladder" block, which asserts the *precondition* +this design consumes — keep it, then add the outcome): + +1. `ctx.load_local_qualified_identities()` returns exactly A, B, C, E — not D, not F. +2. A's `alias == "my-masternode"`, `identity_type == Masternode`, + `wallet_index == Some(0)`, `status` equals the legacy column value **(not + `Unknown`)** — the status-restore contract from §6. +3. A's `masternode_key_presence()` reports `owner` and `voting` — the user's keys + came across, which is the whole point. +4. `ctx.load_local_masternode_identities()` returns A and C; + `load_local_user_identities()` returns B and E — the type-filtered views the + Masternodes and Identities screens actually call. +5. **No plaintext key on disk**: read the raw `det:identity:v1` value for A and + assert its decoded `private_keys` contain no `Clear`/`AlwaysClear` — every key + is `InVault`. This is the §8 security contract, and it must be asserted from + the *stored bytes*, not the in-memory `qi`. +6. The vault holds A's keys: `IdentityKeyView::new(secret_store, A).scheme(target, key_id)` is not `Absent`. +7. E imports with `wallet_hash == Some(protected_seed_hash)` even though the + protected wallet is locked and absent from `ctx.wallets`. +8. Top-up history still resolves for A — i.e. the identity blob and the + already-migrated `det:top_ups:v1` entry share a scope without colliding. + +**Assertions in `second_launch_after_a_v093_upgrade_changes_nothing`:** + +9. Identity count is unchanged; `run()` returns `false`. +10. The legacy `identity` rows are still in `data.db` (count unchanged). + +**Assertions in `a_retry_after_an_unreadable_identity_preserves_user_edits`:** + +11. Edit A's alias post-migration (`ctx.set_identity_alias`), re-run + `finish_unwire::run` ⇒ the alias survives. This is the skip-if-present rule + (§7): an identity already in the store is skipped wholesale — never + re-persisted with the stale legacy blob. It must go **RED** against a naive + implementation that re-inserts unconditionally. + + This assertion lives on the **retry** path, not the clean second launch, because + only the retry path actually reaches the check. A clean upgrade writes the + identity sentinel, and on the next launch that sentinel short-circuits the pass + before any row is examined — so `second_launch_after_a_v093_upgrade_changes_nothing` + would pass even against an importer with no skip-if-present rule at all, proving + nothing. Withholding the sentinel is what forces the re-run: the test seeds one + undecodable blob (`unreadable > 0` ⇒ sentinel not written), so the following + launch re-imports over identities that already landed — exactly the case where an + unconditional INSERT-OR-REPLACE would silently overwrite the user's rename. + +**Negative test (own `#[test]`, on `migrate_identities_from_conn`):** a row whose +`data` is garbage ⇒ `unreadable == 1`, the readable rows still import, and the +sentinel is **not** written. + +--- + +## 11. Part 2 — the bincode / standalone-crate question + +> **Settled.** This was a feasibility spike run during design; its verdict (§11.3) +> was adopted. The golden hex blob it recommended shipped as **T-ID-06** — +> `V093_MASTERNODE_BLOB_HEX` in `src/backend_task/migration/v093_upgrade.rs`, whose +> comment documents how to regenerate it. The section is kept for that rationale and +> for the regeneration recipe; the question itself is closed. + +### 11.1 Is bincode really the only blocker? + +**Yes.** Verified empirically. A standalone crate (`/data/tmp/v093-fixture-probe`, +own `Cargo.lock`, not a workspace member) depending only on + +```toml +dash-evo-tool = { git = "https://github.com/dashpay/dash-evo-tool", tag = "v0.9.3" } +rusqlite = "0.37.0" +libsqlite3-sys = { version = "0.35.0", features = ["bundled"] } +``` + +resolves cleanly: 928 packages, `bincode 2.0.0-rc.3` + `bincode_derive 2.0.0-rc.3` +selected with no conflict. Outside the workspace there is no unification pressure, +so the rc.3-vs-2.0.1 clash simply does not arise. The only other snag is a +`links = "sqlite3"` collision if the scratch crate pins a different `rusqlite` +major than v0.9.3's — matching v0.9.3's `rusqlite 0.37` / `libsqlite3-sys 0.35` +resolves it. Nothing else objects. + +It also **builds**: `cargo check --lib -p dash-evo-tool` against that lockfile +finishes green in 5m47s (warm shared cargo cache; a fully cold one is longer). + +### 11.2 Can v0.9.3's write paths be driven from `pub` API? + +Partly, and the parts that matter are reachable: + +- ✅ `Database::new(&path)` + `Database::initialize(&path)` — `pub`, no + `AppContext`. Produces the genuine v0.9.3 DDL and `database_version = 11`. +- ✅ `QualifiedIdentity { … }.to_bytes()` — `pub`, no `AppContext`. **This is the + only thing genuinely worth extracting**: it is the one artefact the current + fixture cannot honestly forge, and the one that closes §4.1. +- ✅ `model::wallet::encryption::encrypt_message` — `pub`. (Already used verbatim + by the current fixture from HEAD's own code, and byte-identical.) +- ❌ `Database::insert_local_qualified_identity(&self, qi, wallet_info, app_context: &AppContext)` + takes an `AppContext`, and v0.9.3's `AppContext::new` loads a `Config` from the + environment, builds an SDK, loads system data contracts and spawns a + `TaskManager`. Driving it is possible but heavy and fragile. + +The `AppContext` requirement is **avoidable and should be avoided**: the generator +does not need v0.9.3's row-writing SQL, only its *schema* and its *blob encoder*. +Rows go in with plain `rusqlite` — which is exactly what `v093_upgrade.rs` already +does, and which is already verified against `git show v0.9.3:`. + +### 11.3 Verdict — **qualified yes, scoped down** + +**Do not** build a fixture-regenerating tool that produces a whole v0.9.3 +`.sqlite` file, checked in or regenerated on demand. That is the rabbit hole: +the existing hand-rolled fixture is already source-verified line-by-line against +`git show v0.9.3:src/database/`, it reads clearly in the test, and a binary +`.sqlite` blob in the repo is opaque, unreviewable, and rots. It would replace a +good artefact with a worse one. + +**Do** build a one-shot, throwaway generator whose only output is a **golden hex +blob**: a real v0.9.3 `QualifiedIdentity::to_bytes()` for a masternode identity +with owner + voting `Clear` keys, printed as hex, pasted into `v093_upgrade.rs` +as a `const` and asserted to decode on HEAD (**T-ID-06**). That converts §4.1's +one unverified row — "bincode rc.3 and 2.0.1 agree on the wire format" — from an +assumption into a test, which is the single highest-value thing this whole +investigation can buy. The generator itself is scratch; it is never checked in, +never built in CI, and never maintained. Document how to regenerate it in a +comment above the constant. + +**Effort:** + +| | | +|---|---| +| Scratch crate + `main.rs` (construct `QualifiedIdentity`, print hex) | ~20 min | +| Build of the v0.9.3 dep graph (928 crates) — **measured**, warm cache | ~6 min wall clock, unattended | +| Paste constant + write the decode assertion | ~15 min | +| **Total attended** | **< 1 hour** | + +The build is long but hands-off, one-time, and off the critical path. The +artefact it produces is 3 lines in a test file with no ongoing cost. That is a +good trade. The full-`.sqlite`-fixture version of the same idea is not. + +--- + +## 12. Design-review findings (historical) + +> **Closed.** These are the defects and gaps the design investigation surfaced +> *before* implementation, kept as the record of why the shipped design looks the +> way it does. Every row below was addressed by the design in §§5–10 and shipped in +> PR #885. Nothing here is an open question or a live defect list. + +| Severity | Count | Items | +|---|---|---| +| **Critical** | 1 | Legacy identities (and all their key material) are silently dropped on the v0.9.3 → v1.0 upgrade. | +| **High** | 2 | Reusing the `finish_unwire` sentinel would strand every install that already ran the drain. Unconditional re-insert on retry would overwrite user edits with stale legacy blobs. | +| **Medium** | 3 | The `identity` `status` column is not in the bincode blob and would silently read back as `Unknown`. `identity` is absent from `LEGACY_TABLES`, so an identity-only install never trips detection. The rc.3-vs-2.0.1 bincode wire contract is unverified. | +| **Low** | 2 | The 2026-05-28 "version-byte contract" note is stale (destination moved to DET k/v). The test fixture's masternode-only, junk-blob identity row is not representative of the real column shape. | diff --git a/docs/user-stories.md b/docs/user-stories.md index 067c5602a..5cb12b203 100644 --- a/docs/user-stories.md +++ b/docs/user-stories.md @@ -621,6 +621,18 @@ As a user, I want my wallet's identities to be found and loaded automatically on - Already-loaded identities are refreshed (new keys, new DPNS names) while any alias the user assigned is preserved. - Locked, password-protected wallets are skipped without prompting; they are searched after the user unlocks them. +### IDN-016: Identities and their keys preserved across an app upgrade [Implemented] +**Persona:** Alex, Priya + +As a user, I want the identities I loaded before an upgrade — and the keys they hold, such as a masternode's owner and voting keys — to still be there after updating, so that I can keep signing and voting without re-importing anything. + +- Identities stored before the upgrade are imported from the previous version's storage on the first launch afterward, keeping each identity's keys, alias, and wallet link. Progress is shown as its own step. +- An identity that cannot be read is reported in a banner naming the recovery action (load it again), rather than dropped silently. The previous version's data is never deleted, so a later build can still import it. +- A single unreadable identity costs only itself: the readable identities in the same batch still import, and neither the wallet migration that restores access to funds nor the scheduled-vote import is blocked by it. +- The report of unreadable identities returns on every launch until it is explicitly acknowledged, so a user who stepped away cannot lose the only notice that some of their keys were not carried over. +- When identities and scheduled votes are both unreadable on the same launch, one banner names both remedies, and acknowledging it retires both reports — neither report can bury the other. +- An identity the user deletes after the upgrade stays deleted. The import runs once, so a later launch never restores a removed identity, its alias, or its keys. + --- ## DPNS (DPN) diff --git a/src/app.rs b/src/app.rs index 1ce14a9af..6a081672c 100644 --- a/src/app.rs +++ b/src/app.rs @@ -64,6 +64,22 @@ pub const MIGRATION_RETRY_ACTION_ID: &str = "migration:retry:finish_unwire"; /// still have a live deadline. Exposed for kittest coverage. pub const MIGRATION_VOTES_ACK_ACTION_ID: &str = "migration:ack:unreadable_votes"; +/// Banner action id pushed when the user acknowledges the unreadable-identity +/// warning. Until it fires, the warning is re-raised on every launch — a +/// dismissed banner is not an acknowledgement, because the identities it names +/// hold keys the user cannot sign with until they are loaded again. Exposed for +/// kittest coverage. +pub const MIGRATION_IDENTITIES_ACK_ACTION_ID: &str = "migration:ack:unreadable_identities"; + +/// Banner action id pushed when the user acknowledges the combined warning — the +/// launch where both unreadable identities and unreadable votes were left behind. +/// One banner names both problems, so its single acknowledgement retires both +/// records: re-raising either half after the user has read and dismissed the +/// sentence describing it would be a notice they have already acted on. Exposed +/// for kittest coverage. +pub const MIGRATION_UNREADABLE_ACK_ACTION_ID: &str = + "migration:ack:unreadable_identities_and_votes"; + /// Action id for the SPV-sync block's "Continue in the background" escape button. /// SPV sync is **unbounded** — with no peers it stays Connecting/Syncing forever /// with no terminal signal — so a button-less hard block would trap the user @@ -139,6 +155,7 @@ pub fn migration_running_text(step: MigrationStep) -> &'static str { MigrationStep::Shielded => "Verifying shielded balance.", MigrationStep::WalletSeeds => "Moving your wallets into the new vault.", MigrationStep::WalletMeta => "Updating wallet names.", + MigrationStep::Identities => "Restoring your identities and their keys.", MigrationStep::Finalize => "Finishing storage update.", } } @@ -156,6 +173,52 @@ pub fn migration_unreadable_votes_text(count: u32) -> String { ) } +/// User-facing banner copy for a migration that finished the wallet drain but +/// could not decode `count` identities. Their keys are therefore not loaded, so +/// the sentence names both the screen and the control that restore them — a user +/// who has never opened that flow cannot act on "load them again" alone. Kept +/// separate from the scheduled-votes copy because the remedy is different — load +/// an identity, not re-schedule a vote. The previous version's data is never +/// deleted, so the re-import is always possible. Exposed for kittest coverage. +pub fn migration_unreadable_identities_text(count: u32) -> String { + format!( + "Some identities from the previous version could not be read and were not carried over \ + ({count} in total). Your previous data is untouched. Choose Load Identity on the \ + Identities screen to load them again and restore their keys." + ) +} + +/// User-facing banner copy for the launch where both DET-owned passes left rows +/// behind: `identities` identities and `votes` scheduled votes could not be read. +/// One sentence per problem, each naming its own remedy — the remedies differ +/// (load an identity vs re-schedule a vote), and the identity warning recurs on +/// every launch, so it must never be the reason the deadline-critical vote notice +/// goes unseen. No "Retry now": neither corrupt row decodes better on a second +/// pass. Exposed for kittest coverage. +pub fn migration_unreadable_identities_and_votes_text(identities: u32, votes: u32) -> String { + format!( + "Some identities ({identities} in total) and some scheduled votes ({votes} in total) from \ + the previous version could not be read and were not carried over. Your previous data is \ + untouched. Choose Load Identity on the Identities screen to load the identities again, \ + and schedule the votes again on the Scheduled Votes screen." + ) +} + +/// User-facing banner copy for the rare launch where both DET-owned passes broke: +/// `count` identities could not be read AND updating the rest of the previous +/// version's data (such as scheduled votes) hit a hard error. Names each problem +/// in its own sentence and offers the retry the app-data half needs — the +/// identity half recovers by loading the identities again. The previous version's +/// data is never deleted, so both are recoverable. Exposed for kittest coverage. +pub fn migration_failed_with_unreadable_identities_text(count: u32) -> String { + format!( + "Some identities from the previous version could not be read and were not carried over \ + ({count} in total), and updating the rest of your previous data did not finish. Your \ + previous data is untouched. Choose Retry now to finish updating, then choose Load \ + Identity on the Identities screen to load them again and restore their keys." + ) +} + /// How long the cold-start readiness gate waits for the wallet backend to wire /// before it stops retrying silently and surfaces a visible, actionable banner. /// @@ -1850,6 +1913,7 @@ mod migration_banner_tests { MigrationStep::Shielded, MigrationStep::WalletSeeds, MigrationStep::WalletMeta, + MigrationStep::Identities, MigrationStep::Finalize, ] { let text = migration_running_text(step); @@ -1873,6 +1937,7 @@ mod migration_banner_tests { migration_running_text(MigrationStep::Shielded), migration_running_text(MigrationStep::WalletSeeds), migration_running_text(MigrationStep::WalletMeta), + migration_running_text(MigrationStep::Identities), migration_running_text(MigrationStep::Finalize), ]; let unique: std::collections::HashSet<&str> = labels.iter().copied().collect(); @@ -1891,6 +1956,51 @@ mod migration_banner_tests { assert_eq!(MIGRATION_RETRY_ACTION_ID, "migration:retry:finish_unwire"); } + /// Every banner action id is distinct. `drain_actions` dispatches on these + /// strings, so a collision would silently route one banner's acknowledgement + /// to another's task — retiring a warning the user was never shown. + #[test] + fn migration_action_ids_are_distinct() { + let ids = [ + MIGRATION_RETRY_ACTION_ID, + MIGRATION_VOTES_ACK_ACTION_ID, + MIGRATION_IDENTITIES_ACK_ACTION_ID, + MIGRATION_UNREADABLE_ACK_ACTION_ID, + ]; + let unique: std::collections::BTreeSet<_> = ids.iter().collect(); + assert_eq!( + unique.len(), + ids.len(), + "banner action ids must not collide" + ); + } + + /// The combined-failure banner must surface BOTH signals in one message: the + /// unreadable-identity count AND the app-data failure, plus the retry the + /// app-data half needs. If it named only one, the other would be silently + /// swallowed — exactly the bug this copy exists to prevent. + #[test] + fn migration_combined_failure_text_names_both_problems_and_the_retry() { + let text = migration_failed_with_unreadable_identities_text(3); + assert!( + text.contains("identities"), + "must name the identity problem" + ); + assert!( + text.contains('3'), + "must carry the unreadable-identity count" + ); + assert!( + text.contains("did not finish"), + "must name the app-data failure, not only the identities", + ); + assert!( + text.contains("Retry now"), + "must offer the retry the app-data half needs", + ); + assert!(text.ends_with('.'), "one complete sentence-shaped message"); + } + /// Cold-start dispatch gate (the startup-race fix): dispatch only when the /// network has NOT already been dispatched AND its wallet backend is wired. /// The not-ready row is the regression guard — a switched-to network whose diff --git a/src/app/reconcilers.rs b/src/app/reconcilers.rs index e0ef467f1..c24be4560 100644 --- a/src/app/reconcilers.rs +++ b/src/app/reconcilers.rs @@ -30,11 +30,13 @@ use crate::ui::components::{ }; use super::{ - COLD_START_BACKEND_READY_TIMEOUT, COLD_START_STUCK_MESSAGE, MIGRATION_RETRY_ACTION_ID, - MIGRATION_VOTES_ACK_ACTION_ID, SPV_CONNECTING_DESCRIPTION, SPV_CONTINUE_BACKGROUND_ACTION, - SPV_SYNCING_DESCRIPTION, SpvBlockStep, cold_start_backend_wait_timed_out, - migration_running_text, migration_unreadable_votes_text, should_dispatch_cold_start, - spv_block_step, + COLD_START_BACKEND_READY_TIMEOUT, COLD_START_STUCK_MESSAGE, MIGRATION_IDENTITIES_ACK_ACTION_ID, + MIGRATION_RETRY_ACTION_ID, MIGRATION_UNREADABLE_ACK_ACTION_ID, MIGRATION_VOTES_ACK_ACTION_ID, + SPV_CONNECTING_DESCRIPTION, SPV_CONTINUE_BACKGROUND_ACTION, SPV_SYNCING_DESCRIPTION, + SpvBlockStep, cold_start_backend_wait_timed_out, + migration_failed_with_unreadable_identities_text, migration_running_text, + migration_unreadable_identities_and_votes_text, migration_unreadable_identities_text, + migration_unreadable_votes_text, should_dispatch_cold_start, spv_block_step, }; /// Drives platform-level accessibility (AccessKit) activation on the first @@ -491,6 +493,63 @@ impl MigrationReconciler { handle.with_action("Got it", MIGRATION_VOTES_ACK_ACTION_ID); self.banner_handle = Some(handle); } + MigrationState::SucceededWithUnreadableIdentities { count } => { + // Same shape as the vote warning: the drain is done and the rows + // are still in the previous version's storage, so no retry action + // — but the keys those identities held are not loaded, so the user + // must be told even if they stepped away. Sticky, and re-raised on + // every launch until the "Got it" action retires the durable + // record: a dismissal is not an acknowledgement. + let handle = MessageBanner::set_global( + ctx, + migration_unreadable_identities_text(count), + MessageType::Warning, + ); + handle.disable_auto_dismiss(); + handle.with_action("Got it", MIGRATION_IDENTITIES_ACK_ACTION_ID); + self.banner_handle = Some(handle); + } + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { identities, votes } => { + // Both kinds of row were left behind. One Warning banner names both + // remedies — no retry, since neither corrupt row decodes better on a + // second pass. Sticky and acknowledgeable: because this single banner + // names both problems, its one "Got it" retires both durable records + // — re-raising either half afterwards would be a notice the user has + // already read and acted on. + let handle = MessageBanner::set_global( + ctx, + migration_unreadable_identities_and_votes_text(identities, votes), + MessageType::Warning, + ); + handle.disable_auto_dismiss(); + handle.with_action("Got it", MIGRATION_UNREADABLE_ACK_ACTION_ID); + self.banner_handle = Some(handle); + } + MigrationState::FailedWithUnreadableIdentities { count, error } => { + // Both DET-owned passes broke on the same launch. One Error banner + // (retryable, sticky) names both problems — the identities need + // reloading AND the app-data update must be retried — so neither + // silently hides the other. + if error.is_backend_not_ready() { + // Transient app-data backend-not-ready: reset to Idle so the + // frame loop re-dispatches once ready, no failure flash. + self.dispatched.remove(&app_context.network); + app_context + .migration_status() + .set_state(MigrationState::Idle); + self.last_state = Some(MigrationState::Idle); + return; + } + let handle = MessageBanner::set_global( + ctx, + migration_failed_with_unreadable_identities_text(count), + MessageType::Error, + ); + handle.disable_auto_dismiss(); + handle.with_details(error.as_ref()); + handle.with_action("Retry now", MIGRATION_RETRY_ACTION_ID); + self.banner_handle = Some(handle); + } MigrationState::Failed { error } => { if error.is_backend_not_ready() { // Transient: the wallet backend had not finished wiring when @@ -537,10 +596,12 @@ impl MigrationReconciler { } } - /// Drain pending banner-action clicks. Two actions are registered: the - /// migration Retry, which re-dispatches `FinishUnwire` after resetting the - /// cold-start guard, and the unreadable-vote acknowledgement, which clears the - /// durable warning. Both are returned for `AppState` to dispatch. + /// Drain pending banner-action clicks. Two kinds of action are registered: + /// the migration Retry, which re-dispatches `FinishUnwire` after resetting the + /// cold-start guard, and the three unreadable-row acknowledgements (votes, + /// identities, or the combined banner naming both), each of which clears the + /// durable warning records its banner named. All are returned for `AppState` + /// to dispatch. pub(super) fn drain_actions( &mut self, ctx: &egui::Context, @@ -569,6 +630,24 @@ impl MigrationReconciler { task = Some(BackendTask::MigrationTask( MigrationTask::AcknowledgeUnreadableVotes, )); + } else if action_id == MIGRATION_IDENTITIES_ACK_ACTION_ID { + tracing::info!( + target = "migration::cold_start", + ?network, + "User acknowledged the unreadable-identity warning", + ); + task = Some(BackendTask::MigrationTask( + MigrationTask::AcknowledgeUnreadableIdentities, + )); + } else if action_id == MIGRATION_UNREADABLE_ACK_ACTION_ID { + tracing::info!( + target = "migration::cold_start", + ?network, + "User acknowledged the combined unreadable-identity and unreadable-vote warning", + ); + task = Some(BackendTask::MigrationTask( + MigrationTask::AcknowledgeUnreadableIdentitiesAndVotes, + )); } else { tracing::warn!( target = "ui::banner", @@ -580,3 +659,112 @@ impl MigrationReconciler { task } } + +#[cfg(test)] +mod tests { + use super::*; + use egui_kittest::Harness; + use egui_kittest::kittest::Queryable; + + fn test_app_context(dir: &std::path::Path) -> Arc { + crate::app_dir::ensure_env_file(dir); + let db = Arc::new(crate::database::Database::new(dir.join("data.db")).expect("db")); + db.create_tables(true).expect("create tables"); + db.set_default_version().expect("set version"); + + let app_kv = AppContext::open_app_kv(dir).expect("open app k/v"); + let secret_store = AppContext::open_secret_store(dir).expect("open secret store"); + AppContext::new( + dir.to_path_buf(), + Network::Testnet, + db, + Default::default(), + Default::default(), + egui::Context::default(), + app_kv, + secret_store, + crate::model::user_role::UserRoleCell::default(), + ) + .expect("AppContext") + } + + /// Publish `state`, let the reconciler build its banner, click the named + /// button, and return whatever task the click routed to. Drives the real + /// `update_banner` → render → click → `drain_actions` chain, so a banner that + /// never wires its action fails here instead of passing a test that hand-rolls + /// the wiring it was supposed to prove. + fn click_banner_action(state: MigrationState, label: &str) -> Option { + let tmp = tempfile::tempdir().expect("tempdir"); + let app_context = test_app_context(tmp.path()); + app_context.migration_status().set_state(state); + + let mut reconciler = MigrationReconciler::new(); + let mut harness = Harness::builder() + .with_size(egui::vec2(600.0, 260.0)) + .build_ui(MessageBanner::show_global); + + reconciler.update_banner(&harness.ctx, &app_context); + harness.run(); + harness.get_by_label(label).click(); + harness.run(); + + reconciler.drain_actions(&harness.ctx, app_context.network) + } + + /// The unreadable-identity warning is acknowledgeable. It used to render as a + /// sticky banner with NO action button at all: the user was told their signing + /// keys had not come across and given no way to say "I understand", so the + /// warning returned on every launch with no gesture that could retire it. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn unreadable_identities_banner_acknowledgement_routes_to_its_task() { + let task = click_banner_action( + MigrationState::SucceededWithUnreadableIdentities { count: 2 }, + "Got it", + ); + assert_eq!( + task, + Some(BackendTask::MigrationTask( + MigrationTask::AcknowledgeUnreadableIdentities + )), + "the identity warning must offer an acknowledgement that reaches its backend task", + ); + } + + /// The combined banner names both problems in one message, so its single + /// acknowledgement must retire BOTH records. Routing it to either single-signal + /// task would leave the other half to re-raise on the next launch — a notice the + /// user has already read and acted on. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn combined_unreadable_banner_acknowledgement_routes_to_the_combined_task() { + let task = click_banner_action( + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: 2, + votes: 3, + }, + "Got it", + ); + assert_eq!( + task, + Some(BackendTask::MigrationTask( + MigrationTask::AcknowledgeUnreadableIdentitiesAndVotes + )), + "one banner naming both problems must retire both warnings on one click", + ); + } + + /// The vote warning keeps its own acknowledgement — the identity work above + /// must not have re-routed the sibling banner to the wrong task. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn unreadable_votes_banner_acknowledgement_routes_to_its_task() { + let task = click_banner_action( + MigrationState::SucceededWithUnreadableVotes { count: 2 }, + "Got it", + ); + assert_eq!( + task, + Some(BackendTask::MigrationTask( + MigrationTask::AcknowledgeUnreadableVotes + )), + ); + } +} diff --git a/src/backend_task/error.rs b/src/backend_task/error.rs index f9c0f6936..8f85fc46d 100644 --- a/src/backend_task/error.rs +++ b/src/backend_task/error.rs @@ -37,8 +37,13 @@ pub enum TaskError { /// [`Self::WalletBackendNotYetWired`]: the backend is ready, but this /// particular wallet is not yet registered with it (still loading, or /// skipped during load). User-actionable — waiting and retrying resolves it. - #[error("This wallet is still loading. Please wait a moment and try again.")] - WalletNotLoaded, + #[error("The wallet \"{wallet_label}\" is still loading. Please wait a moment and try again.")] + WalletNotLoaded { + /// Display alias for the affected wallet, or a fallback hex prefix of + /// the seed hash when no alias has been set. With several wallets + /// loaded, this is the only thing that says *which* one to wait for. + wallet_label: String, + }, /// An internal wallet-state inconsistency: the wallet backend's records /// disagree with each other in a way that should never happen (a wallet diff --git a/src/backend_task/identity/load_identity.rs b/src/backend_task/identity/load_identity.rs index c62efc5e1..2624de621 100644 --- a/src/backend_task/identity/load_identity.rs +++ b/src/backend_task/identity/load_identity.rs @@ -45,6 +45,11 @@ type WalletMatchResult = Option<(WalletSeedHash, u32, WalletKeyMap)>; /// win on collision; keys it omits (e.g. Owner/Payout on a voting-key-only /// update) are carried over from `existing` rather than lost. The existing /// alias and identity associations are kept only when the new build lacks them. +/// +/// Load-path only: this fills gaps from field *absence*, which is safe when a +/// user is actively re-loading (the missing field is genuinely being resupplied) +/// but is NOT valid for the background legacy migration, where an absent field +/// can be a deliberate removal (a cleared alias, a "Remove private key from DET"). fn merge_existing_keys_into(new: &mut QualifiedIdentity, existing: QualifiedIdentity) { for (key, value) in existing.private_keys.private_keys { new.private_keys.private_keys.entry(key).or_insert(value); diff --git a/src/backend_task/migration/finish_unwire.rs b/src/backend_task/migration/finish_unwire.rs index 6b94cbbc2..7de58af81 100644 --- a/src/backend_task/migration/finish_unwire.rs +++ b/src/backend_task/migration/finish_unwire.rs @@ -9,12 +9,16 @@ use std::sync::Arc; use dash_sdk::dpp::dashcore::Network; +use dash_sdk::dpp::identity::accessors::IdentityGettersV0; +use dash_sdk::platform::Identifier; use rusqlite::Connection; use serde::{Deserialize, Serialize}; use crate::backend_task::error::TaskError; use crate::context::AppContext; use crate::context::migration_status::{MigrationState, MigrationStep}; +use crate::model::qualified_identity::QualifiedIdentity; +use crate::model::wallet::WalletSeedHash; use crate::wallet_backend::{DetScope, KvAdapterError, network_prefix}; /// Sentinel key format string. The migration body filters every @@ -42,16 +46,18 @@ pub fn sentinel_key_for(network: Network) -> String { /// migration into the `Running` state. Ordered so the cheapest check /// (the single-row `wallet` table) runs first. /// -/// `scheduled_votes` and `top_up` are in the list because an install can -/// hold them with no wallet rows at all — a masternode voter who imported -/// identity keys directly has queued votes but no HD wallet. Omitting them -/// would leave the detection gate closed and drop those votes. +/// `scheduled_votes`, `top_up` and `identity` are in the list because an +/// install can hold them with no wallet rows at all — a masternode voter who +/// imported identity keys directly has identities and queued votes but no HD +/// wallet. Omitting them would leave the detection gate closed and drop that +/// user's keys. const LEGACY_TABLES: &[&str] = &[ "wallet", "single_key_wallet", "utxos", "scheduled_votes", "top_up", + "identity", ]; /// Persisted sentinel payload. Lives in `det-app.sqlite` under the @@ -166,6 +172,26 @@ pub enum MigrationError { source: Box, }, + /// The app-data pass failed with an error that did not already originate in + /// the migration layer (a k/v read while checking which votes exist, say). + /// Wrapped so the combined [`MigrationState::FailedWithUnreadableIdentities`] + /// banner keeps a typed chain when both DET-owned passes break together. + #[error("could not import the previous version's app data")] + AppDataImport { + #[source] + source: Box, + }, + + /// A decoded legacy identity could not be written into the identity store. + /// Hard failure: the identity sentinel stays unwritten so the next launch + /// retries the idempotent import. Never blocks the wallet drain — [`run`] + /// judges this only after the seeds are safe. + #[error("could not save an identity from the previous version")] + IdentityImportFailed { + #[source] + source: Box, + }, + /// The legacy top-up history could not be carried across — the legacy table /// is unreadable, or the k/v store rejected the write. Audit trail, not /// funds, so it never blocks the wallet drain; but the app-data sentinel @@ -187,6 +213,16 @@ pub enum MigrationError { source: KvAdapterError, }, + /// Could not read, write or clear the durable unreadable-identity warning. + /// That record is the only thing that survives the identity sentinel, so a + /// failure here is surfaced rather than dropped — a silently-lost warning is + /// a user who never learns their signing keys did not come across. + #[error("could not access the unreadable-identity warning")] + IdentityWarningRecord { + #[source] + source: KvAdapterError, + }, + /// The wallet backend was not yet wired when the migration ran. /// This is a hard configuration bug: the orchestrator runs after /// `ensure_wallet_backend`, so this should never fire in @@ -195,6 +231,17 @@ pub enum MigrationError { #[error("wallet backend not available during migration")] WalletBackendUnavailable, + /// A pass reported an error that is not a [`MigrationError`]. Every pass is + /// meant to report through one of the typed variants above; this catch-all + /// exists so a stray error still reaches a terminal banner. Leaving one + /// unpublished would strand the status on `Running`, which gates every + /// wallet-touching task behind `WalletStorageNotReady` and offers no retry. + #[error("could not finish updating the previous version's data")] + Unexpected { + #[source] + source: Box, + }, + /// Returned by [`guard_single_key_table_droppable`] when dropping the /// legacy single-key table would destroy a password-protected key that /// has no copy anywhere else. `remaining` is the un-restored row count. @@ -243,6 +290,24 @@ impl MigrationError { } } +/// Coerce any migration failure into the `Arc` chain the failure +/// banners render. Total by design: every error the orchestrator can produce must +/// end up publishable, or it leaves the status on `Running` — which gates every +/// wallet-touching task behind `WalletStorageNotReady`, with no retry to escape. +/// +/// A [`TaskError::MigrationFailed`] chain is reused verbatim, so the banner shows +/// the same typed source and a wrapped [`MigrationError::WalletBackendUnavailable`] +/// still classifies as backend-not-ready. Anything else is wrapped in +/// [`MigrationError::Unexpected`] rather than dropped. +pub(crate) fn migration_error_chain(error: TaskError) -> Arc { + match error { + TaskError::MigrationFailed { source } => source, + other => Arc::new(MigrationError::Unexpected { + source: Box::new(other), + }), + } +} + /// Run the FinishUnwire migration. Idempotent — completes a no-op when /// the sentinels are already present. /// @@ -252,31 +317,63 @@ impl MigrationError { /// decide whether to surface a "storage update complete" banner — a no-op /// launch must not show one. /// -/// Two independent passes, in this order: +/// Three independent passes, each under its own sentinel, in this order: /// /// 1. **App data** (scheduled votes, top-up history) — DET-owned rows the -/// wallet drain never touched, under their own sentinel. +/// wallet drain never touched. /// 2. **Wallet drain** (single keys, HD seeds, wallet metadata, upstream /// registration) — the pass that restores access to funds. +/// 3. **Identities** (identity rows and the keys they hold) — last, because it +/// needs the drain's output: a wired backend, a reachable vault, and a +/// hydrated `ctx.wallets` for wallet-derived identity keys to attach to. +/// +/// **Neither DET-owned pass gates the other.** The wallet drain runs regardless +/// of the app-data outcome, and the identity import runs regardless of it too — +/// both results are held, never propagated on the spot, and judged together at +/// the end. A legacy vote row that cannot be imported must never stand between +/// the user and their seeds, nor between the user and their identity keys: an +/// app-data failure is deterministic, so letting it short-circuit the identity +/// import would strand those keys outside the vault on every launch, not just +/// this one. /// -/// The wallet drain runs **regardless of the app-data outcome**. The two are -/// only coupled at the end, when the terminal state is published: a legacy vote -/// row that cannot be imported must never stand between the user and their -/// seeds. +/// The wallet drain is the one deliberate prerequisite: a drain failure returns +/// early, so the identity import does not run on that launch. It cannot — that +/// pass needs the drain's output (pass 3 above), and both retry on the next +/// launch, neither sentinel having been written. /// -/// A pending [`UnreadableVotesWarning`] is re-published on every launch — not -/// only the one that discovered it — until [`acknowledge_unreadable_votes`] -/// clears it. +/// Both DET-owned passes write their sentinel unconditionally, so their counters +/// exist for exactly one launch. What outlives them are the durable +/// [`UnreadableVotesWarning`] and [`UnreadableIdentitiesWarning`] records, and +/// those — never the counters — are what this function publishes: each is +/// re-raised on every launch, not only the one that discovered it, until +/// [`acknowledge_unreadable_votes`] / [`acknowledge_unreadable_identities`] +/// retires it. When both are pending they ride one +/// [`MigrationState::SucceededWithUnreadableIdentitiesAndVotes`], so a lone +/// identity warning can never outrank the vote warning and silently cost the +/// user a live vote deadline. If reading the vote-warning record fails, the vote +/// half is withheld until a later launch reads it successfully — the identity +/// half still reaches the user, and neither is reported as a failed migration — +/// a *hard* app-data failure (not merely a failed later read of this record) is +/// the only thing that publishes [`MigrationState::FailedWithUnreadableIdentities`] +/// instead; see the `# Errors` section below. /// /// # Errors /// /// [`TaskError::MigrationFailed`] when the wallet drain fails (the completion /// sentinel stays unwritten, so the next launch retries), or when the wallet -/// drain succeeded but the app-data pass hit a hard failure — an unreadable -/// legacy file, a k/v write error. Undecodable vote rows are *not* an error: -/// they are counted and reported on [`MigrationState::SucceededWithUnreadableVotes`], -/// because a retry cannot decode a corrupt row and failing here would wedge the -/// wallet drain on every launch. +/// drain succeeded but the app-data or identity pass hit a hard failure — an +/// unreadable legacy file, a k/v write error. Undecodable *rows* are not an +/// error: they are counted and reported on +/// [`MigrationState::SucceededWithUnreadableVotes`] / +/// [`MigrationState::SucceededWithUnreadableIdentities`], because failing here +/// would wedge the wallet drain behind a row the user cannot repair. +/// +/// One case returns `Ok` while publishing a failure banner itself: a hard +/// app-data failure that coincides with unreadable identities on the same +/// launch. Both must reach the user, so the terminal state is +/// [`MigrationState::FailedWithUnreadableIdentities`] (published here, carrying +/// both signals) rather than a returned `Err` that the caller would re-publish +/// as a plain `Failed`, dropping the identity count. pub async fn run(app_context: &Arc) -> Result { let status = app_context.migration_status(); @@ -308,12 +405,171 @@ pub async fn run(app_context: &Arc) -> Result { } }; - // Funds are reachable from here on, so the app-data outcome can now decide - // the terminal state. A hard failure leaves the app-data sentinel unwritten - // and reaches the user's "Retry now" banner; that retry re-runs the import - // alone, since the wallet drain short-circuits on its own sentinel. + // Funds are reachable from here on. The identity import runs next, and its + // result — like the app-data one — is held rather than propagated, because + // the two DET-owned passes must not gate each other. + // + // The identity pass needs the drain's output (backend wired, vault reachable, + // `ctx.wallets` hydrated) so a wallet-derived key lands against a wallet that + // exists. It must NOT wait on the app-data result: a hard app-data failure — + // one malformed vote-index blob is enough — is deterministic, so unwrapping + // it first would skip the identity import on this launch *and every retry* + // (the app-data sentinel is never written, so the failure recurs forever). + // That would strand a masternode owner's private keys outside the vault + // permanently, over a corrupt vote queue. + status.set_state(MigrationState::Running { + step: MigrationStep::Identities, + }); + let identities = migrate_identities(app_context); + + // Both DET-owned passes have now run. A hard failure in either still reaches + // the user's "Retry now" banner, but only after neither could block the + // other. The identity failure takes precedence when both fail: keys outrank + // votes. + let identities = match identities { + Ok(outcome) => outcome, + Err(identity_error) => { + if let Err(app_data_error) = &app_data { + tracing::warn!( + target = "migration::finish_unwire", + error = ?app_data_error, + "App-data import failed on the same launch as the identity import; both retry on the next launch", + ); + } + return Err(identity_error); + } + }; + + // The identity warning is read back from storage rather than taken from this + // pass's counters: the identity sentinel is written unconditionally, so every + // launch after the discovery run short-circuits the import and honestly + // reports zero unreadable rows — while the rows it could not decode still + // hold keys the user cannot sign with. Re-published until + // [`acknowledge_unreadable_identities`] retires it. + // + // A read failure here is surfaced, never dropped: this record is the only + // thing standing between the user and the news about their own keys. + let identities_warning = match read_identities_warning( + &app_context.app_kv(), + app_context.network, + ) { + Ok(warning) => warning, + Err(warning_error) => { + if let Err(app_data_error) = &app_data { + tracing::warn!( + target = "migration::finish_unwire", + error = ?app_data_error, + "App-data import failed on the launch where the identity-warning record could not be read; the app-data import retries on the next launch", + ); + } + return Err(warning_error.into()); + } + }; + + // Unreadable identities outrank a *readable* app-data pass: an identity that + // did not come across took its keys with it, so the user cannot sign — let + // alone vote — until it is loaded again. But a *hard* app-data failure on the + // same launch is not something the identity warning may swallow: it would + // leave the user no retry for their scheduled votes, silently and every + // launch (the app-data sentinel is never written, so it recurs). So when both + // break together, a single combined banner names each — the app-data half + // retryable — instead of one eating the other. + // + // This branch runs BEFORE `app_data` is unwrapped with `?`: unwrapping first + // would return the deterministic app-data error and permanently mask the + // identity outcome. `app_data` is moved here by value; the fall-through path + // below (no pending identity warning) still owns it because this branch always + // returns. + if let Some(identities_warning) = identities_warning { + let unreadable = identities_warning.count; + match app_data { + Ok(outcome) => { + let moved_data = wallet_moved || outcome.moved_data() || identities.moved_data(); + + // A pending vote warning must ride along, or the identity signal + // buries it forever: the identity warning is re-published on every + // launch until acknowledged, so this branch would otherwise return + // ahead of the `read_vote_warning` re-publish below. Both counts + // come from storage, not from this launch's counters: after the + // discovery run both passes short-circuit on their sentinels and + // honestly report zero — exactly on the launches where this branch + // is the only one the user ever sees. + // + // A k/v read that itself fails costs only the vote half of the + // banner, never the identity half: the record is durable and this + // branch re-runs on every launch (the identity sentinel stays + // unwritten), so the next successful read re-publishes it. Reporting + // the read error as a *failure* state instead would tell the user the + // app-data pass did not finish — it did, and wrote its sentinel — and + // offer a retry that re-runs nothing. + match read_vote_warning(&app_context.app_kv(), app_context.network) { + Ok(Some(warning)) => { + tracing::warn!( + target = "migration::finish_unwire", + unreadable, + imported = identities.imported, + votes_unreadable = warning.count, + network = ?app_context.network, + "Some legacy identities and some legacy scheduled votes could not be decoded; they stay in the previous version's data.db and must be loaded / scheduled again", + ); + status.set_state( + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: unreadable, + votes: warning.count, + }, + ); + } + Ok(None) => { + tracing::warn!( + target = "migration::finish_unwire", + unreadable, + imported = identities.imported, + network = ?app_context.network, + "Some legacy identities could not be decoded; they stay in the previous version's data.db and must be loaded again", + ); + status.set_state(MigrationState::SucceededWithUnreadableIdentities { + count: unreadable, + }); + } + Err(warning_error) => { + tracing::warn!( + target = "migration::finish_unwire", + unreadable, + imported = identities.imported, + error = ?warning_error, + network = ?app_context.network, + "Some legacy identities could not be decoded, and the pending vote-warning record could not be read so any vote notice is withheld until the next launch; the identity rows stay in the previous version's data.db and must be loaded again", + ); + status.set_state(MigrationState::SucceededWithUnreadableIdentities { + count: unreadable, + }); + } + } + return Ok(moved_data); + } + Err(app_data_error) => { + let moved_data = wallet_moved || identities.moved_data(); + tracing::warn!( + target = "migration::finish_unwire", + unreadable, + imported = identities.imported, + error = ?app_data_error, + network = ?app_context.network, + "Both DET-owned passes failed on the same launch: some legacy identities could not be decoded and the app-data import hit a hard error; both retry on the next launch", + ); + status.set_state(MigrationState::FailedWithUnreadableIdentities { + count: unreadable, + error: migration_error_chain(app_data_error), + }); + return Ok(moved_data); + } + } + } + + // No identity warning is pending, so app-data no longer masks anything + // critical: a hard failure here is the user's "Retry now" banner. let app_data = app_data?; - let moved_data = wallet_moved || app_data.moved_data(); + let moved_data = wallet_moved || app_data.moved_data() || identities.moved_data(); // The warning is read back from storage rather than taken from this pass's // counters: on every launch after the discovery run the import short-circuits @@ -565,6 +821,89 @@ pub fn acknowledge_unreadable_votes(app_context: &Arc) -> Result<(), Ok(()) } +/// Per-network key of the un-acknowledged unreadable-identity warning. Distinct +/// from the identity sentinel: the sentinel records that the import *ran*, this +/// record that the user has not yet been *told* what it could not carry across. +fn identities_warning_key_for(network: Network) -> String { + format!( + "det:migration:unreadable_identities:{}:v1", + network_prefix(network) + ) +} + +/// Durable "some identities could not be read" warning. +/// +/// The import runs once (the identity sentinel short-circuits every later +/// launch), so the pass counters exist for exactly one launch. A user who was +/// away, or who dismissed the banner without reading it, would otherwise never +/// hear that the keys those identities held — a masternode's owner / voting key, +/// say — are not loaded. This record outlives the pass: [`run`] re-publishes it +/// on every launch until [`acknowledge_unreadable_identities`] clears it. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct UnreadableIdentitiesWarning { + /// Legacy identity rows the import could not decode. Never `0` — a + /// zero-count warning is not written at all. + pub count: u32, +} + +/// The pending unreadable-identity warning for `network`, if the user has not +/// acknowledged it yet. +fn read_identities_warning( + app_kv: &crate::wallet_backend::DetKv, + network: Network, +) -> Result, MigrationError> { + app_kv + .get::(DetScope::Global, &identities_warning_key_for(network)) + .map_err(|source| MigrationError::IdentityWarningRecord { source }) +} + +/// Record `count` unreadable identity rows as a pending warning. A zero count +/// writes nothing — there is nothing to tell the user. +/// +/// Written BEFORE the identity sentinel: a crash between the two re-runs the +/// idempotent import, whereas the reverse order would lose the warning for good. +fn write_identities_warning( + app_kv: &crate::wallet_backend::DetKv, + network: Network, + count: u32, +) -> Result<(), MigrationError> { + if count == 0 { + return Ok(()); + } + app_kv + .put( + DetScope::Global, + &identities_warning_key_for(network), + &UnreadableIdentitiesWarning { count }, + ) + .map_err(|source| MigrationError::IdentityWarningRecord { source }) +} + +/// Retire the unreadable-identity warning for the active network: the user has +/// read it. Clears the durable record so later launches stay quiet, and drops +/// the banner. The legacy rows in `data.db` are untouched — only the notice is +/// retired, so a build with a better decoder can still recover the identities. +/// +/// This gesture does not gate the import loop: [`migrate_identities`] writes its +/// sentinel unconditionally, so the import is already a once-only event whether +/// or not the user ever clicks. Acknowledgement retires the notice, nothing more. +pub fn acknowledge_unreadable_identities(app_context: &Arc) -> Result<(), TaskError> { + let network = app_context.network; + app_context + .app_kv() + .delete(DetScope::Global, &identities_warning_key_for(network)) + .map_err(|source| MigrationError::IdentityWarningRecord { source })?; + tracing::info!( + target = "migration::finish_unwire", + network = ?network, + "User acknowledged the unreadable-identity warning", + ); + app_context + .migration_status() + .set_state(MigrationState::Idle); + Ok(()) +} + /// Import the DET-owned rows the wallet drain never touched: scheduled DPNS /// votes (deadline-critical) and top-up history (audit trail). /// @@ -612,7 +951,7 @@ fn migrate_app_data(app_context: &Arc) -> Result) -> Result = app_context - .get_scheduled_votes()? + .get_scheduled_votes() + .map_err(|source| MigrationError::AppDataImport { + source: Box::new(source), + })? .into_iter() .map(|v| (v.voter_id.to_buffer(), v.contested_name)) .collect(); @@ -660,21 +1006,27 @@ fn migrate_app_data(app_context: &Arc) -> Result Result<(), MigrationError> { let completion = MigrationCompletion { completed_at: now_epoch_seconds(), sha: env!("CARGO_PKG_VERSION").to_string(), - network_count: 1, + network_count, }; app_kv .put(DetScope::Global, sentinel_key, &completion) @@ -773,6 +1125,214 @@ where Ok(outcome) } +/// Per-network sentinel for the legacy identity import. Distinct from the +/// wallet-drain sentinel on purpose: every install that already drained its +/// wallets under an earlier build (which had no identity import) still has its +/// identities — and their owner / voting keys — sitting in `data.db`. Sharing +/// the drain's sentinel would declare exactly those installs "done" and drop +/// them. +pub fn identities_sentinel_key_for(network: Network) -> String { + format!("det:migration:identities:{}:v1", network_prefix(network)) +} + +/// Import the legacy `identity` rows — and the private keys they carry — into +/// the modern identity store. +/// +/// Runs after the wallet drain: the k/v store, the secret vault and a hydrated +/// `ctx.wallets` all have to exist first. Idempotent — an identity already in +/// the store is left alone, so a retry can never overwrite an alias the user has +/// since edited with the stale legacy copy. +/// +/// Key material is never handled here. Each decoded identity goes straight to +/// [`AppContext::insert_local_qualified_identity`], which routes the keys +/// through the vault seam and writes only `InVault` placeholders to disk. +/// +/// Runs exactly once: the sentinel is written even when rows were unreadable, so +/// a deletion the user makes afterwards is durable. Undecodable rows are counted +/// into a durable [`UnreadableIdentitiesWarning`] and left in `data.db`, never +/// deleted — recovering them after a decoder fix is an explicit user gesture +/// (dashpay/dash-evo-tool#889), not an automatic retry. +/// +/// # Errors +/// +/// [`TaskError::MigrationFailed`] when the legacy file cannot be opened or read, +/// or a decoded identity cannot be written to the store. Undecodable rows are +/// not an error — they are counted and reported to the user by [`run`]. +fn migrate_identities( + app_context: &Arc, +) -> Result { + let app_kv = app_context.app_kv(); + let network = app_context.network; + let sentinel_key = identities_sentinel_key_for(network); + + let done = app_kv + .get::(DetScope::Global, &sentinel_key) + .map_err(|source| MigrationError::Sentinel { source })? + .is_some(); + if done { + return Ok(IdentityMigrationOutcome::default()); + } + + let Some(path) = app_context.db.db_file_path() else { + // In-memory / headless: no legacy file to import from. + return Ok(IdentityMigrationOutcome::default()); + }; + if !path.exists() { + return Ok(IdentityMigrationOutcome::default()); + } + let conn = Connection::open(&path).map_err(|e| MigrationError::LegacyDbOpen { + path: path.to_string_lossy().to_string(), + source: e, + })?; + + // Own probe rather than the drain's [`detect_legacy_rows`] gate: an + // identity-only install (a masternode voter with no HD wallet) has to import + // its identities even though the wallet family is empty. Probing first also + // keeps a fresh install from needing the backend it has no data for. + if !table_has_rows(&conn, "identity")? { + write_completion_sentinel(&app_kv, &sentinel_key, 1)?; + return Ok(IdentityMigrationOutcome::default()); + } + + let backend = app_context + .wallet_backend() + .map_err(|_| MigrationError::WalletBackendUnavailable)?; + + let outcome = migrate_identities_from_conn( + &conn, + network, + |id| app_context.has_local_qualified_identity(id), + |qi, wallet| { + // Diagnostic only — the link is imported either way (see the pure + // body). An absent wallet means it failed to migrate or is locked. + if let Some((seed_hash, _)) = wallet + && backend.wallet_meta().get(network, seed_hash).is_none() + { + tracing::warn!( + target = "migration::finish_unwire", + identity = %hex::encode(qi.identity.id().to_buffer()), + "Importing an identity whose wallet is not present; the link is kept so it re-attaches when that wallet is restored", + ); + } + app_context.insert_local_qualified_identity(qi, wallet) + }, + )?; + + tracing::info!( + target = "migration::finish_unwire", + imported = outcome.imported, + skipped_existing = outcome.skipped_existing, + unreadable = outcome.unreadable, + network = ?network, + "Identity migration pass complete", + ); + + // Persist the warning before the sentinel: this pass counts the undecodable + // rows exactly once (the sentinel short-circuits every later launch), so the + // count has to outlive it or the user gets one banner and no second chance. + // Ordered before the sentinel so a crash in between re-runs the idempotent + // import rather than losing the warning. + write_identities_warning(&app_kv, network, outcome.unreadable)?; + + // The sentinel is written even when rows were unreadable — the same rule the + // app-data pass follows, for the same reason. Every *importable* row is now + // in the store, and a withheld sentinel would re-run this import on every + // launch: harmless for an identity the user has edited (skip-if-present), but + // it would resurrect one the user has DELETED, restoring its alias and + // re-writing its legacy plaintext keys into the vault, forever. The + // undecodable rows stay in `data.db` and are reported by the durable warning + // above; re-importing them after a decoder fix is an explicit user gesture + // (dashpay/dash-evo-tool#889), not an automatic retry that costs a deletion. + write_completion_sentinel(&app_kv, &sentinel_key, 1)?; + + Ok(outcome) +} + +/// Pure identity-import body (testable without an `AppContext`). +/// +/// `is_present` is the skip-if-already-imported check; `insert` is the +/// vault-routing writer. +fn migrate_identities_from_conn( + conn: &Connection, + network: Network, + mut is_present: P, + mut insert: I, +) -> Result +where + P: FnMut(&Identifier) -> Result, + I: FnMut(&QualifiedIdentity, &Option<(WalletSeedHash, u32)>) -> Result<(), TaskError>, +{ + let import_failed = |source: TaskError| MigrationError::IdentityImportFailed { + source: Box::new(source), + }; + + let legacy = + crate::database::legacy_import::read_identities(conn, network).map_err(|source| { + MigrationError::LegacyDbRead { + table: "identity", + source, + } + })?; + + let mut outcome = IdentityMigrationOutcome { + unreadable: legacy.unreadable, + ..Default::default() + }; + + for row in legacy.identities { + let id = Identifier::from(row.id); + + // Skip an identity already in the store, wholesale. Reconciling + // legacy-only keys into a present record is deliberately NOT attempted: + // field absence cannot be told apart from a deliberate removal (a + // cleared alias, "Remove private key from DET") without provenance the + // model does not carry, and a plaintext key merged into a protected + // identity trips the vault-first downgrade guard. The legacy `data.db` + // is preserved, so those keys are recoverable by a later build. See the + // known limitation in the design doc (§7) and the tracked follow-up. + // + // Check-and-insert needs no transaction despite `insert` being + // INSERT-OR-REPLACE: no other writer can interleave. Every production + // identity writer is a `BackendTask::IdentityTask`, and + // `run_backend_task` rejects all `is_wallet_touching` tasks with + // `WalletStorageNotReady` for as long as the migration holds + // `MigrationState::Running` — which spans this whole pass. + if is_present(&id).map_err(import_failed)? { + outcome.skipped_existing = outcome.skipped_existing.saturating_add(1); + continue; + } + + // The wallet link travels verbatim, never nulled — even when that wallet + // did not come across: it is what re-attaches the identity when the + // wallet is restored or unlocked later. + insert(&row.qi, &row.wallet).map_err(import_failed)?; + outcome.imported = outcome.imported.saturating_add(1); + } + + Ok(outcome) +} + +/// Outcome counters from one [`migrate_identities`] pass. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +struct IdentityMigrationOutcome { + /// Identities written into the per-network identity store, keys vaulted. + imported: u32, + /// Identities already in the store and therefore left untouched. + skipped_existing: u32, + /// Legacy rows that could not be decoded. Recorded as a durable + /// [`UnreadableIdentitiesWarning`] so the user still hears about them, but + /// never fails the pass — the identities that *did* decode must not be held + /// hostage by one that did not. + unreadable: u32, +} + +impl IdentityMigrationOutcome { + /// `true` when this pass actually moved an identity across. + fn moved_data(&self) -> bool { + self.imported > 0 + } +} + /// Returns `true` when any of the [`LEGACY_TABLES`] holds at least one /// row. Missing tables are treated as empty: a freshly-installed /// `data.db` already lacks the dropped tables, and that is correct. @@ -1768,21 +2328,14 @@ fn read_sentinel( .map_err(|e| MigrationError::Sentinel { source: e }) } -/// Write the completion sentinel for `network`, marking the migration +/// Write the wallet-drain completion sentinel for `network`, marking the drain /// as finished for this network on this install. fn write_sentinel( app_kv: &crate::wallet_backend::DetKv, network: Network, network_count: u32, ) -> Result<(), MigrationError> { - let completion = MigrationCompletion { - completed_at: now_epoch_seconds(), - sha: env!("CARGO_PKG_VERSION").to_string(), - network_count, - }; - app_kv - .put(DetScope::Global, &sentinel_key_for(network), &completion) - .map_err(|e| MigrationError::Sentinel { source: e }) + write_completion_sentinel(app_kv, &sentinel_key_for(network), network_count) } fn now_epoch_seconds() -> i64 { @@ -2066,52 +2619,265 @@ mod tests { } } - /// Round-trip: writing the sentinel and reading it back yields the - /// same payload. Guards the codec from accidental shape drift. - #[test] - fn sentinel_round_trip() { + // ── Identity import ────────────────────────────────────────────── + + mod identities { + use super::*; + use crate::database::test_helpers::{ + LegacyIdentityFixture, basic_legacy_identity_blob, create_legacy_identity_table, + }; use dash_sdk::dpp::dashcore::Network; + use dash_sdk::dpp::identity::accessors::IdentityGettersV0; + use std::cell::RefCell; - let kv = kv(); - write_sentinel(&kv, Network::Mainnet, 1).expect("write sentinel"); - let completion = read_sentinel(&kv, Network::Mainnet) - .expect("read") - .expect("present"); - assert_eq!(completion.network_count, 1); - assert!(completion.completed_at > 0); - assert_eq!(completion.sha, env!("CARGO_PKG_VERSION")); - } + const NETWORK: Network = Network::Testnet; - /// The sentinel is scoped per network — writing the mainnet sentinel - /// must not satisfy a subsequent testnet read, or a network switch - /// would leave testnet wallets permanently unmigrated behind a - /// stale-looking global sentinel. - #[test] - fn sentinel_is_per_network_mainnet_then_testnet() { - use dash_sdk::dpp::dashcore::Network; + fn create_identity_table(conn: &Connection) { + create_legacy_identity_table(conn).expect("create identity table"); + } - let kv = kv(); - // Step 1: simulate a successful mainnet migration. - write_sentinel(&kv, Network::Mainnet, 1).expect("write mainnet sentinel"); - assert!( - read_sentinel(&kv, Network::Mainnet) - .expect("read mainnet") - .is_some(), - "mainnet sentinel must be visible to a mainnet read", - ); - // Step 2: switching to testnet must NOT short-circuit. The - // testnet read returns `None` so the orchestrator proceeds to - // detect-and-drain legacy testnet rows. - assert!( - read_sentinel(&kv, Network::Testnet) - .expect("read testnet") - .is_none(), - "mainnet sentinel must not satisfy a testnet read — \ - per-network sentinel regression", - ); - // Step 3: a clean testnet migration writes its own sentinel - // without touching the mainnet one. Both then short-circuit - // their respective networks. + /// A minimal, genuinely-encodable identity blob. + pub(super) fn identity_blob(id: [u8; 32]) -> Vec { + basic_legacy_identity_blob(id, None, NETWORK) + } + + pub(super) fn insert_identity( + conn: &Connection, + id: [u8; 32], + data: Option>, + is_local: bool, + ) { + LegacyIdentityFixture::new(id, data, NETWORK.to_string()) + .with_is_local(is_local) + .insert(conn) + .expect("insert identity"); + } + + /// Collects what the importer tried to write, so a test can assert on the + /// identities that reached the (vault-routing) writer. + #[derive(Default)] + struct Recorder { + imported: RefCell>, + } + + /// A blob that will never decode must not take the identities around it + /// down with it: the readable rows still import, and the failure is + /// counted so the caller can record a durable warning for the user. + #[test] + fn an_undecodable_blob_never_blocks_the_identities_that_do_decode() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_identity_table(&conn); + let good = [0xAA; 32]; + let corrupt = [0xBB; 32]; + insert_identity(&conn, good, Some(identity_blob(good)), true); + insert_identity(&conn, corrupt, Some(vec![0xFF; 16]), true); + + let recorder = Recorder::default(); + let outcome = migrate_identities_from_conn( + &conn, + NETWORK, + |_| Ok(false), + |qi, _| { + recorder + .imported + .borrow_mut() + .push(qi.identity.id().to_buffer()); + Ok(()) + }, + ) + .expect("an undecodable row must not fail the pass"); + + assert_eq!(outcome.imported, 1, "the readable identity still imports"); + assert_eq!(outcome.unreadable, 1, "the corrupt row is reported"); + assert_eq!( + *recorder.imported.borrow(), + vec![good], + "only the decodable identity reaches the writer", + ); + assert!( + outcome.unreadable > 0, + "a non-zero `unreadable` is what raises the durable warning that tells \ + the user which keys did not come across", + ); + } + + /// An identity already in the store is left untouched — skipped + /// wholesale, never re-inserted. Re-writing would risk overwriting + /// whatever the user has done to it since (renamed it, removed a key) + /// with the stale legacy copy. + #[test] + fn an_identity_already_in_the_store_is_never_reimported() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_identity_table(&conn); + let existing = [0xAA; 32]; + let fresh = [0xBB; 32]; + insert_identity(&conn, existing, Some(identity_blob(existing)), true); + insert_identity(&conn, fresh, Some(identity_blob(fresh)), true); + + let recorder = Recorder::default(); + let outcome = migrate_identities_from_conn( + &conn, + NETWORK, + |id| Ok(id.to_buffer() == existing), + |qi, _| { + recorder + .imported + .borrow_mut() + .push(qi.identity.id().to_buffer()); + Ok(()) + }, + ) + .expect("import"); + + assert_eq!(outcome.imported, 1); + assert_eq!(outcome.skipped_existing, 1); + assert_eq!( + *recorder.imported.borrow(), + vec![fresh], + "the already-present identity must never reach the insert writer", + ); + } + + /// Observed (`is_local = 0`) rows are v0.9.3's lookup cache and NULL-blob + /// rows have nothing in them. Neither is user data: both are skipped, and + /// neither counts as unreadable — so neither raises a warning about keys + /// that were never there. + #[test] + fn observed_and_null_blob_rows_are_skipped_without_failing_the_pass() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_identity_table(&conn); + let observed = [0xAA; 32]; + let null_blob = [0xBB; 32]; + insert_identity(&conn, observed, Some(identity_blob(observed)), false); + insert_identity(&conn, null_blob, None, true); + + let recorder = Recorder::default(); + let outcome = migrate_identities_from_conn( + &conn, + NETWORK, + |_| Ok(false), + |qi, _| { + recorder + .imported + .borrow_mut() + .push(qi.identity.id().to_buffer()); + Ok(()) + }, + ) + .expect("import"); + + assert_eq!( + ( + outcome.imported, + outcome.unreadable, + outcome.skipped_existing + ), + (0, 0, 0), + "neither row is user data, and neither is a failure", + ); + assert!(recorder.imported.borrow().is_empty()); + } + + /// A link to a wallet that did not come across is kept verbatim, never + /// nulled: it is what re-attaches the identity when the user finally + /// unlocks (or restores) that wallet. + #[test] + fn a_link_to_an_absent_wallet_is_preserved_not_nulled() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_identity_table(&conn); + let id = [0xAA; 32]; + let orphan_wallet = [0x77; 32]; + LegacyIdentityFixture::new(id, Some(identity_blob(id)), NETWORK.to_string()) + .with_wallet(orphan_wallet.to_vec(), 4) + .insert(&conn) + .expect("insert identity"); + + let links: RefCell>> = RefCell::new(Vec::new()); + let outcome = migrate_identities_from_conn( + &conn, + NETWORK, + |_| Ok(false), + |_, wallet| { + links.borrow_mut().push(*wallet); + Ok(()) + }, + ) + .expect("import"); + + assert_eq!(outcome.imported, 1, "the identity imports anyway"); + assert_eq!( + *links.borrow(), + vec![Some((orphan_wallet, 4))], + "the wallet link must survive verbatim — nulling it would orphan the \ + identity from its keys permanently", + ); + } + + /// An identity-only install (a masternode voter with no HD wallet) must + /// still trip the detection gate, or its keys are never even looked at. + #[test] + fn detection_gate_covers_the_identity_table() { + assert!(LEGACY_TABLES.contains(&"identity")); + } + + /// The identity sentinel is per network and distinct from the wallet + /// drain's: reusing the latter would skip the import for every install + /// that already drained under a build without an identity importer. + #[test] + fn sentinel_is_per_network_and_distinct_from_the_other_sentinels() { + let testnet = identities_sentinel_key_for(Network::Testnet); + assert_ne!(testnet, identities_sentinel_key_for(Network::Mainnet)); + assert_ne!(testnet, sentinel_key_for(Network::Testnet)); + assert_ne!(testnet, app_data_sentinel_key_for(Network::Testnet)); + } + } + + /// Round-trip: writing the sentinel and reading it back yields the + /// same payload. Guards the codec from accidental shape drift. + #[test] + fn sentinel_round_trip() { + use dash_sdk::dpp::dashcore::Network; + + let kv = kv(); + write_sentinel(&kv, Network::Mainnet, 1).expect("write sentinel"); + let completion = read_sentinel(&kv, Network::Mainnet) + .expect("read") + .expect("present"); + assert_eq!(completion.network_count, 1); + assert!(completion.completed_at > 0); + assert_eq!(completion.sha, env!("CARGO_PKG_VERSION")); + } + + /// The sentinel is scoped per network — writing the mainnet sentinel + /// must not satisfy a subsequent testnet read, or a network switch + /// would leave testnet wallets permanently unmigrated behind a + /// stale-looking global sentinel. + #[test] + fn sentinel_is_per_network_mainnet_then_testnet() { + use dash_sdk::dpp::dashcore::Network; + + let kv = kv(); + // Step 1: simulate a successful mainnet migration. + write_sentinel(&kv, Network::Mainnet, 1).expect("write mainnet sentinel"); + assert!( + read_sentinel(&kv, Network::Mainnet) + .expect("read mainnet") + .is_some(), + "mainnet sentinel must be visible to a mainnet read", + ); + // Step 2: switching to testnet must NOT short-circuit. The + // testnet read returns `None` so the orchestrator proceeds to + // detect-and-drain legacy testnet rows. + assert!( + read_sentinel(&kv, Network::Testnet) + .expect("read testnet") + .is_none(), + "mainnet sentinel must not satisfy a testnet read — \ + per-network sentinel regression", + ); + // Step 3: a clean testnet migration writes its own sentinel + // without touching the mainnet one. Both then short-circuit + // their respective networks. write_sentinel(&kv, Network::Testnet, 1).expect("write testnet sentinel"); assert!(read_sentinel(&kv, Network::Mainnet).unwrap().is_some()); assert!(read_sentinel(&kv, Network::Testnet).unwrap().is_some()); @@ -3627,6 +4393,62 @@ mod tests { ); } + /// `migration_error_chain` reuses a `MigrationFailed` chain verbatim (so the + /// combined banner shows the same typed source, and a wrapped + /// `WalletBackendUnavailable` still classifies as backend-not-ready), and + /// wraps any stray non-migration `TaskError` rather than dropping it. + #[test] + fn migration_error_chain_preserves_migration_source_and_wraps_others() { + let inner = Arc::new(MigrationError::WalletBackendUnavailable); + let chain = migration_error_chain(TaskError::MigrationFailed { + source: Arc::clone(&inner), + }); + assert!( + Arc::ptr_eq(&chain, &inner), + "a MigrationFailed chain must be reused, not re-wrapped", + ); + assert!( + chain.is_backend_not_ready(), + "the reused chain keeps its backend-not-ready classification", + ); + + let wrapped = migration_error_chain(TaskError::WalletNotFound); + assert!( + matches!(*wrapped, MigrationError::Unexpected { .. }), + "a non-migration error is wrapped, never dropped", + ); + assert!( + !wrapped.is_backend_not_ready(), + "a wrapped stray error is terminal, offered with a retry", + ); + } + + /// The coercion is TOTAL — every `TaskError` yields a publishable chain that + /// still carries the original error as its `#[source]`. This is what keeps a + /// failed migration from stranding the status on `Running`: while it is + /// running, `run_backend_task` rejects every wallet-touching task with + /// `WalletStorageNotReady` and the banner offers no retry, so an error that + /// published nothing would wedge the wallet surface until the app restarts. + #[test] + fn every_task_error_yields_a_publishable_chain_with_its_source_intact() { + for stray in [ + TaskError::WalletNotFound, + TaskError::WalletStorageNotReady, + TaskError::IdentityNotFound, + ] { + let expected = stray.to_string(); + let chain = migration_error_chain(stray); + + let source = std::error::Error::source(&*chain) + .expect("the stray error must survive as the chain's source"); + assert_eq!( + source.to_string(), + expected, + "the original error must reach the details panel, not be replaced", + ); + } + } + /// Wire the real wallet seam onto a fixture context. Offline: the backend /// builds its sidecars and hydrates from them without touching the network, /// so an end-to-end `run()` can be driven to completion in a unit test. The @@ -3687,6 +4509,19 @@ mod tests { seed_hash } + /// Stage an identity row whose blob will never decode, so `read_identities` + /// counts it unreadable. Written into the *modern* `identity` table the app + /// context already created — not the legacy fixture shape — so the NULL wallet + /// link is what satisfies that table's both-or-neither `CHECK`. + fn insert_corrupt_identity_row(conn: &Connection, id: [u8; 32]) { + conn.execute( + "INSERT INTO identity (id, data, status, is_local, network) + VALUES (?1, ?2, 2, 1, 'testnet')", + rusqlite::params![id.as_slice(), vec![0xFFu8; 8]], + ) + .expect("corrupt identity row"); + } + /// QA-101 — the headline funds regression. A single undecodable legacy vote /// row must NOT stand between the user and their wallet: the drain runs to /// completion (seeds copied, wallet hydrated AND upstream-registered, the @@ -3873,6 +4708,167 @@ mod tests { backend.shutdown().await; } + /// Both DET-owned passes broken on the SAME launch must surface BOTH signals, + /// not have one silently eat the other. Here an undecodable identity row + /// (unreadable identities) and a structurally-damaged legacy `top_up` table + /// (a hard app-data failure) coincide. Before the fix the run published + /// `SucceededWithUnreadableIdentities` and returned `Ok`, so the app-data + /// failure never reached a banner — no retry, swallowed every launch. Now the + /// combined `FailedWithUnreadableIdentities` carries the identity count AND + /// the app-data error chain: the user sees both, with a retry for the + /// app-data half. Funds are still safe (the wallet drain ran regardless), and + /// neither DET-owned sentinel is written, so both retry on the next launch. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn both_app_data_and_identity_failures_surface_together() { + use crate::wallet_backend::poison::RwLockRecover; + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let network = Network::Testnet; + + // Funds: a normal legacy wallet, so the drain completes and `moved_data`. + let seed_hash = seed_legacy_wallet(&ctx, &[0xD6u8; 64], "funds", network); + + { + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + insert_corrupt_identity_row(&conn, [0x44u8; 32]); + // A structurally-damaged legacy `top_up` (no `amount` column) with a + // row → the app-data pass hard-fails when its reader's prepare fails. + conn.execute_batch( + "CREATE TABLE top_up ( + identity_id BLOB NOT NULL, + top_up_index INTEGER NOT NULL + );", + ) + .expect("broken legacy top-up schema"); + conn.execute( + "INSERT INTO top_up (identity_id, top_up_index) VALUES (?1, 0)", + rusqlite::params![[0x44u8; 32].as_slice()], + ) + .expect("top-up row"); + } + + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + + let did_work = run(&ctx) + .await + .expect("both failures are non-fatal to the run — it publishes its own terminal state"); + assert!(did_work, "the wallet drain moved data"); + + // Both signals in one terminal state: the identity count AND the app-data + // error chain. The pre-fix `SucceededWithUnreadableIdentities` would have + // hidden the app-data failure — `matches!` here would fail on it. + assert!( + matches!( + &*ctx.migration_status().state(), + MigrationState::FailedWithUnreadableIdentities { count: 1, .. } + ), + "both failures must surface together, got {:?}", + ctx.migration_status().state(), + ); + + // Funds stay safe: the drain ran despite both DET-owned passes breaking. + assert!( + ctx.wallets.read_recover().contains_key(&seed_hash), + "the migrated wallet must be visible — neither DET-owned failure may block funds", + ); + assert!( + backend.is_wallet_registered(&seed_hash), + "the migrated wallet must be reachable", + ); + + // The app-data pass hard-failed, so its sentinel stays unwritten and the + // import retries next launch. The identity pass did NOT fail — it imported + // what decoded and counted what did not — so it completes, and the + // undecodable row is carried forward by its durable warning instead of by + // an endlessly-retried import that would resurrect deleted identities. + assert!( + ctx.app_kv() + .get::(DetScope::Global, &app_data_sentinel_key_for(network)) + .expect("read app-data sentinel") + .is_none(), + "the app-data sentinel must stay unwritten so the failure retries", + ); + assert!( + ctx.app_kv() + .get::(DetScope::Global, &identities_sentinel_key_for(network)) + .expect("read identity sentinel") + .is_some(), + "an unreadable row is not a pass failure: the import completes, and the row is \ + reported by the durable warning rather than retried forever", + ); + + backend.shutdown().await; + } + + /// A failed *vote-warning read* is not an app-data failure, and the banner may + /// not say it is. Here the app-data pass SUCCEEDS (its sentinel is written) and + /// only the follow-up warning-record read fails, alongside an undecodable + /// identity row. `FailedWithUnreadableIdentities` tells the user "updating the + /// rest of your previous data did not finish" and offers a "Retry now" that + /// re-runs a pass which already completed — false on both counts. The identity + /// signal is the one the user must act on, so the run falls through to the + /// honest `SucceededWithUnreadableIdentities` and the unreadable notice record + /// is left for the next launch to re-read. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn an_unreadable_vote_warning_record_does_not_claim_the_app_data_pass_failed() { + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let network = Network::Testnet; + + // Funds: a normal legacy wallet, so the drain completes and `moved_data`. + seed_legacy_wallet(&ctx, &[0xD7u8; 64], "funds", network); + + { + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + // A corrupt identity blob → unreadable == 1, which is what steers the run + // into the branch under test. The legacy `top_up` table is left alone, so + // the app-data pass runs clean and writes its sentinel. + insert_corrupt_identity_row(&conn, [0x45u8; 32]); + } + + // Poison the warning record: a unit value encodes to an empty bincode body, + // so decoding it back as an `UnreadableVotesWarning` hits an unexpected end + // of input. The app-data pass decodes zero unreadable votes and therefore + // never overwrites it. + ctx.app_kv() + .put(DetScope::Global, &vote_warning_key_for(network), &()) + .expect("poison the vote-warning record"); + assert!( + read_vote_warning(&ctx.app_kv(), network).is_err(), + "precondition: the poisoned record must make the warning read fail, \ + otherwise this test would pass for the wrong reason", + ); + + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + + let did_work = run(&ctx).await.expect("a failed notice read is not fatal"); + assert!(did_work, "the wallet drain moved data"); + + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "an unreadable notice record must not be reported as an app-data failure", + ); + + // The fact the old banner denied: the app-data pass really did finish. + assert!( + ctx.app_kv() + .get::(DetScope::Global, &app_data_sentinel_key_for(network)) + .expect("read app-data sentinel") + .is_some(), + "the app-data sentinel proves the pass completed — a banner offering to \ + 'finish updating' it would be lying", + ); + + backend.shutdown().await; + } + /// Fix-8 — the unreadable-vote warning is durable. The discovery run records /// it; every later launch re-publishes it from that record even though both /// sentinels short-circuit the passes, so a user who was away when the @@ -3925,6 +4921,253 @@ mod tests { backend.shutdown().await; } + /// An unreadable identity must not permanently hide an unreadable *vote*. + /// Both damaged on the same launch is the trap: the identity branch returns + /// early, ahead of the durable vote-warning read, so a lone identity signal + /// would bury the vote half — and the user would silently miss a vote whose + /// deadline is still live. Both counts ride one terminal state instead, both + /// halves survive a restart (each re-read from its own durable record, since + /// both sentinels short-circuit their passes by then), and acknowledging the + /// vote half retires only that half. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn unreadable_identities_do_not_hide_the_unreadable_vote_warning() { + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let network = Network::Testnet; + + seed_legacy_wallet(&ctx, &[0xE7u8; 64], "funds", network); + // One vote decodes, one does not → the app-data pass succeeds AND records + // a durable unreadable-vote warning. + seed_legacy_votes( + &ctx, + &[0x55u8; 32], + &[("alice", "Lock"), ("corrupt", "Nonsense")], + network, + ); + { + // A corrupt identity blob → the identity pass counts it unreadable and + // records a durable warning, so the identity branch recurs on every launch. + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + insert_corrupt_identity_row(&conn, [0x66u8; 32]); + } + + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + + run(&ctx).await.expect("neither damaged row is fatal"); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: 1, + votes: 1, + }, + "the discovery run must name BOTH remedies — the identity warning may \ + not swallow a vote with a live deadline", + ); + + // The readable vote still came across; only the corrupt one did not. + let votes = ctx.get_scheduled_votes().expect("read scheduled votes"); + assert_eq!(votes.len(), 1, "the readable vote must still be imported"); + + // Both sentinels are now written, which is what makes the next launch a + // real test of the durable reads: both passes short-circuit and report + // zero unreadable rows, so *neither* half of the banner can come from a + // counter — each must be re-read from its own durable warning record. + assert!( + ctx.app_kv() + .get::(DetScope::Global, &app_data_sentinel_key_for(network)) + .expect("read app-data sentinel") + .is_some(), + "precondition: the app-data pass completed, so its counters go quiet from now on", + ); + assert!( + ctx.app_kv() + .get::(DetScope::Global, &identities_sentinel_key_for(network)) + .expect("read identity sentinel") + .is_some(), + "precondition: the identity pass completed too — a withheld sentinel would re-run \ + the import forever and resurrect identities the user has deleted", + ); + + // Later launch: both sentinels short-circuit their passes, so both halves + // of the combined banner must come from the durable records, not counters. + ctx.migration_status().set_state(MigrationState::Idle); + run(&ctx).await.expect("second launch"); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: 1, + votes: 1, + }, + "the vote warning must survive a restart even while identities stay unreadable", + ); + + // Acknowledging retires the vote half only: the identities are still + // unreadable, so their warning must keep coming back on its own. + acknowledge_unreadable_votes(&ctx).expect("acknowledge"); + ctx.migration_status().set_state(MigrationState::Idle); + run(&ctx).await.expect("third launch"); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "an acknowledged vote warning must not come back, but the identity one must", + ); + + backend.shutdown().await; + } + + /// The resurrection regression. One genuinely corrupt legacy row must not + /// make the identity import re-run forever: while it did, an identity the + /// user had deliberately DELETED — a routine security gesture, offered on + /// both the Identities and the Masternode screens — was silently re-imported + /// on the very next launch, its alias restored and its legacy plaintext keys + /// re-written into the vault. The skip-if-present rule cannot help here: a + /// deleted identity is, by construction, no longer present. + /// + /// The import now writes its sentinel unconditionally, exactly like the + /// app-data pass, so it is a once-only event and a deletion is durable. The + /// unreadable row is still reported — from a record that outlives the pass. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_deleted_identity_is_not_resurrected_by_an_unreadable_sibling_row() { + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let network = Network::Testnet; + + seed_legacy_wallet(&ctx, &[0x9Au8; 64], "funds", network); + + // One identity that decodes, and one genuinely corrupt row beside it — + // the row that used to hold the sentinel hostage forever. + let deleted = [0x11u8; 32]; + { + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + identities::insert_identity( + &conn, + deleted, + Some(identities::identity_blob(deleted)), + true, + ); + identities::insert_identity(&conn, [0x22u8; 32], Some(vec![0xFFu8; 8]), true); + } + + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + + run(&ctx).await.expect("first launch"); + + let deleted_id = Identifier::from(deleted); + assert!( + ctx.has_local_qualified_identity(&deleted_id) + .expect("read identity store"), + "precondition: the readable identity is imported by the discovery run", + ); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "precondition: the corrupt row is reported to the user", + ); + + // The user deliberately removes the identity — the gesture whose entire + // point is that those keys stop living in this install. + ctx.delete_local_qualified_identity(&deleted_id) + .expect("delete identity"); + assert!( + !ctx.has_local_qualified_identity(&deleted_id) + .expect("read identity store"), + "precondition: the delete took effect", + ); + + // The next launch, with the corrupt row still sitting in `data.db`. + ctx.migration_status().set_state(MigrationState::Idle); + run(&ctx).await.expect("second launch"); + + assert!( + !ctx.has_local_qualified_identity(&deleted_id) + .expect("read identity store"), + "a deleted identity must STAY deleted: re-importing it would restore the alias \ + the user cleared and re-write their legacy plaintext keys into the vault, on \ + every launch, with no banner to explain it and no way to stop it", + ); + + // Closing the retry door must not cost the user the notice about the row + // that never decoded. + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "the unreadable row is still reported once the import itself is complete", + ); + + backend.shutdown().await; + } + + /// The unreadable-identity warning is acknowledgeable, and durable until it + /// is. Before, it was a sticky banner with no action button: the user could + /// not retire it, and the only thing keeping it on screen was an import that + /// re-ran forever. The record now outlives the (once-only) import, is + /// re-published on every launch, and only an explicit acknowledgement stops + /// it — the legacy rows themselves are never touched. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn unreadable_identity_warning_is_republished_until_acknowledged() { + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + let network = Network::Testnet; + + seed_legacy_wallet(&ctx, &[0xB4u8; 64], "funds", network); + { + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + identities::insert_identity(&conn, [0x44u8; 32], Some(vec![0xFFu8; 8]), true); + } + + wire_backend(&ctx).await; + let backend = ctx.wallet_backend().expect("backend wired"); + + run(&ctx).await.expect("first launch"); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "the discovery run surfaces the warning", + ); + + assert!( + ctx.app_kv() + .get::(DetScope::Global, &identities_sentinel_key_for(network)) + .expect("read identity sentinel") + .is_some(), + "the import completes even with an unreadable row: a withheld sentinel would \ + re-run it on every launch and resurrect identities the user has deleted", + ); + + // The import is done, so the pass reports zero from here on — the warning + // has to come from storage or the user never hears it again. + ctx.migration_status().set_state(MigrationState::Idle); + run(&ctx).await.expect("second launch"); + assert_eq!( + *ctx.migration_status().state(), + MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + "a warning the user may have missed must survive a restart", + ); + + acknowledge_unreadable_identities(&ctx).expect("acknowledge"); + assert!( + matches!(*ctx.migration_status().state(), MigrationState::Idle), + "acknowledging clears the banner immediately", + ); + + ctx.migration_status().set_state(MigrationState::Idle); + run(&ctx).await.expect("third launch"); + assert!( + matches!(*ctx.migration_status().state(), MigrationState::Idle), + "an acknowledged warning must never come back", + ); + + backend.shutdown().await; + } + /// Funds-safety invariant (Fix #3): a run that cannot finish — here because /// no wallet backend is wired in the fixture — MUST return `Err` and MUST /// NOT write the completion sentinel, so the migration retries on a later @@ -3971,4 +5214,49 @@ mod tests { "the completion sentinel must not be written when the migration aborts", ); } + + /// A failed migration must always leave a terminal state behind. `Running` + /// makes `run_backend_task` reject every wallet-touching task with + /// `WalletStorageNotReady`, and the banner offers a retry only from + /// `Failed` — so a failure that published nothing would wedge wallets, + /// identities and sends until the app is restarted, with no way out. + #[tokio::test] + async fn a_failed_migration_never_strands_the_status_on_running() { + use crate::backend_task::migration::MigrationTask; + use dash_sdk::dpp::dashcore::Network; + + let tmp = tempfile::tempdir().expect("tempdir"); + let ctx = fresh_app_context(tmp.path()); + + // Same shape as the abort above: a legacy row trips detection, and the + // unwired backend fails the first backend-dependent step. + { + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + seed_legacy_row( + &conn, + &[7u8; 32], + &[1u8; 32], + &[], + &[], + "addr", + None, + false, + Network::Testnet, + ); + } + + let result = ctx.run_migration_task(MigrationTask::FinishUnwire).await; + assert!(result.is_err(), "the migration must report its failure"); + + let state = ctx.migration_status().state(); + assert!( + !state.is_running(), + "a failed migration left the status on `Running`, which gates every \ + wallet-touching task with no retry to escape it", + ); + assert!( + matches!(*state, MigrationState::Failed { .. }), + "the failure must reach the user as a retryable banner", + ); + } } diff --git a/src/backend_task/migration/mod.rs b/src/backend_task/migration/mod.rs index d66fb53c7..940f20427 100644 --- a/src/backend_task/migration/mod.rs +++ b/src/backend_task/migration/mod.rs @@ -40,6 +40,16 @@ pub enum MigrationTask { /// acknowledges it, so this is the one gesture that stops it — the legacy /// vote rows themselves are never touched. AcknowledgeUnreadableVotes, + /// Retire the "some identities could not be read" warning for the active + /// network. Same contract as [`Self::AcknowledgeUnreadableVotes`]: the + /// warning is re-raised on every launch until the user acknowledges it, and + /// the legacy identity rows themselves are never touched. + AcknowledgeUnreadableIdentities, + /// Retire both warnings at once — the acknowledgement of the single combined + /// banner that names both problems. Retiring only one half would re-raise the + /// other on the next launch, as a notice the user has already read and acted + /// on. + AcknowledgeUnreadableIdentitiesAndVotes, } impl AppContext { @@ -49,38 +59,52 @@ impl AppContext { /// failure, publishes [`MigrationState::Failed`] so the per-frame /// banner reconciliation in `AppState` can surface the error /// variant with a "Retry now" action — without it the banner - /// would be stuck in `Running` forever. + /// would be stuck in `Running` forever, and `run_backend_task` + /// would keep rejecting every wallet-touching task with + /// `WalletStorageNotReady` until the app is restarted. pub async fn run_migration_task( self: &Arc, task: MigrationTask, ) -> Result { match task { MigrationTask::FinishUnwire => match finish_unwire::run(self).await { - // `finish_unwire::run` already publishes the terminal state - // (`Success` only when it moved data, `Idle` for a no-op - // launch), so the banner is correct without anything here. + // Every `Ok` path of `finish_unwire::run` publishes its own + // terminal state — including the one that reports a *failure* + // the user must still act on + // (`FailedWithUnreadableIdentities`). Re-publishing here would + // overwrite it. Ok(_did_work) => Ok(BackendTaskSuccessResult::Refresh), Err(task_error) => { - // Publish a `Failed` state carrying the typed - // `MigrationError` chain so the UI banner can call - // `Display::fmt` at render time and surface the - // wrapped source via the details panel — no - // stringification on the writer side. - if let TaskError::MigrationFailed { source } = &task_error { - // `Arc::clone` is a cheap refcount bump — both - // the returned `Err` and the published `Failed` - // state observe the same typed error chain. - self.migration_status().set_state(MigrationState::Failed { - error: Arc::clone(source), - }); - } - Err(task_error) + // Publish `Failed` for *every* error, carrying the typed + // `MigrationError` chain so the banner can `Display::fmt` it + // at render time and surface the source in the details panel + // — no stringification on the writer side. Coercing rather + // than matching one variant is what makes this total: an + // error that published nothing would leave the status on + // `Running` and wedge the whole wallet surface. + let source = finish_unwire::migration_error_chain(task_error); + // `Arc::clone` is a cheap refcount bump — both the returned + // `Err` and the published `Failed` state observe the same + // typed error chain. + self.migration_status().set_state(MigrationState::Failed { + error: Arc::clone(&source), + }); + Err(TaskError::MigrationFailed { source }) } }, MigrationTask::AcknowledgeUnreadableVotes => { finish_unwire::acknowledge_unreadable_votes(self)?; Ok(BackendTaskSuccessResult::Refresh) } + MigrationTask::AcknowledgeUnreadableIdentities => { + finish_unwire::acknowledge_unreadable_identities(self)?; + Ok(BackendTaskSuccessResult::Refresh) + } + MigrationTask::AcknowledgeUnreadableIdentitiesAndVotes => { + finish_unwire::acknowledge_unreadable_votes(self)?; + finish_unwire::acknowledge_unreadable_identities(self)?; + Ok(BackendTaskSuccessResult::Refresh) + } } } } diff --git a/src/backend_task/migration/v093_upgrade.rs b/src/backend_task/migration/v093_upgrade.rs index 6932359b7..2439c325b 100644 --- a/src/backend_task/migration/v093_upgrade.rs +++ b/src/backend_task/migration/v093_upgrade.rs @@ -18,23 +18,44 @@ //! no `core_wallet_name` column, no `onboarding_completed` column, and seeds //! stored raw with empty salt/nonce. +use std::collections::BTreeMap; +use std::str::FromStr; use std::sync::Arc; use dash_sdk::dpp::dashcore::Network; +use dash_sdk::dpp::identity::accessors::IdentityGettersV0; +use dash_sdk::dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; +use dash_sdk::dpp::identity::v0::IdentityV0; +use dash_sdk::dpp::identity::{ + Identity, IdentityPublicKey, KeyID, KeyType, Purpose, SecurityLevel, +}; +use dash_sdk::dpp::key_wallet::bip32::DerivationPath; +use dash_sdk::dpp::platform_value::BinaryData; use dash_sdk::dpp::voting::vote_choices::resource_vote_choice::ResourceVoteChoice; use dash_sdk::platform::Identifier; use rusqlite::{Connection, params}; -use crate::backend_task::migration::finish_unwire::{self, MigrationCompletion, sentinel_key_for}; +use crate::backend_task::migration::finish_unwire::{ + self, MigrationCompletion, identities_sentinel_key_for, sentinel_key_for, +}; use crate::backend_task::migration::legacy_settings::{SettingsImport, import_legacy_settings}; use crate::context::AppContext; use crate::database::Database; -use crate::database::test_helpers::{create_database_at_path, legacy_master_epk_bytes}; +use crate::database::test_helpers::{ + LegacyIdentityFixture, create_database_at_path, legacy_master_epk_bytes, +}; +use crate::model::qualified_identity::encrypted_key_storage::{ + KeyStorage, PrivateKeyData, WalletDerivationPath, +}; +use crate::model::qualified_identity::qualified_identity_public_key::QualifiedIdentityPublicKey; +use crate::model::qualified_identity::{ + IdentityStatus, IdentityType, PrivateKeyTarget, QualifiedIdentity, +}; use crate::model::settings::{AppSettings, RootScreenType, ThemeMode}; use crate::model::wallet::encryption::encrypt_message; use crate::model::wallet::{ClosedKeyItem, WalletSeedHash}; use crate::wallet_backend::secret_seam::SecretScheme; -use crate::wallet_backend::{DetScope, WalletBackend}; +use crate::wallet_backend::{DetScope, IdentityKeyView, WalletBackend}; /// `DEFAULT_DB_VERSION` as shipped by v0.9.3. Every upgrading user's `data.db` /// enters the ladder here. @@ -47,10 +68,133 @@ const USER_NETWORK: Network = Network::Testnet; const UNPROTECTED_SEED: [u8; 64] = [0xA9; 64]; const PROTECTED_SEED: [u8; 64] = [0xC7; 64]; const PROTECTED_PASSWORD: &str = "correct horse battery staple"; -const IDENTITY_ID: [u8; 32] = [0xBB; 32]; const TOP_UP_AMOUNT: u64 = 123_456; const CONTESTED_NAME: &str = "quantum"; +/// Row A — the masternode identity whose owner + voting keys are the whole +/// point of the identity import. Its `data` blob is [`V093_MASTERNODE_BLOB_HEX`]. +const IDENTITY_ID: [u8; 32] = [0xBB; 32]; +/// The voter identity associated with row A (`associated_voter_identity`). +const VOTER_IDENTITY_ID: [u8; 32] = [0xBC; 32]; +/// Row B — a wallet-derived `User` identity on the unprotected wallet. +const USER_IDENTITY_ID: [u8; 32] = [0xCC; 32]; +/// Row C — an `Evonode` identity with no wallet at all (loaded by ProTxHash). +const EVONODE_IDENTITY_ID: [u8; 32] = [0xDD; 32]; +/// Row D — an observed (`is_local = 0`) identity: lookup cache, not user data. +const OBSERVED_IDENTITY_ID: [u8; 32] = [0xEE; 32]; +/// Row E — a `User` identity on the password-protected (locked) wallet. +const PROTECTED_IDENTITY_ID: [u8; 32] = [0xCD; 32]; +/// Row F — a local row whose `data` blob is NULL (v0.9.3 allowed it). +const NULL_BLOB_IDENTITY_ID: [u8; 32] = [0xEF; 32]; + +/// The masternode owner key held `Clear` in row A's legacy blob. After the +/// import it must exist only in the vault — never in `det-app.sqlite`. +const OWNER_PRIVATE_KEY: [u8; 32] = [0x11; 32]; +/// The masternode voting key held `Clear` in row A's legacy blob. +const VOTING_PRIVATE_KEY: [u8; 32] = [0x22; 32]; +/// The `Clear` key held by the wallet-less evonode identity (row C). +const EVONODE_PRIVATE_KEY: [u8; 32] = [0x33; 32]; + +/// Legacy `identity.status` column values. The bincode blob does **not** carry +/// status, so these are what prove the column is restored on import rather than +/// every identity reading back as `Unknown`. +const STATUS_ACTIVE: u8 = 2; +const STATUS_PENDING_CREATION: u8 = 1; +const STATUS_NOT_FOUND: u8 = 3; + +/// A **genuine v0.9.3** `QualifiedIdentity::to_bytes()` — the masternode +/// identity of row A, carrying `Clear` owner and voting keys. +/// +/// This is the wire-format contract between the two builds. v0.9.3 encodes with +/// bincode `2.0.0-rc.3`; this tree decodes with `2.0.1`. Everything else about +/// the import is reasoning about struct layout — this constant is the only proof +/// that the actual bytes a real user has on disk still decode here. +/// +/// To regenerate: build a throwaway crate (not a workspace member) depending on +/// `dash-evo-tool` tag `v0.9.3`, construct the identity below — id +/// [`IDENTITY_ID`], voter id [`VOTER_IDENTITY_ID`], type `Masternode`, alias +/// `my-masternode`, `associated_owner_key_id = Some(0)`, an `OWNER` key (id 0, +/// `Clear`([`OWNER_PRIVATE_KEY`]), `ECDSA_HASH160`) on +/// `PrivateKeyOnMainIdentity` and a `VOTING` key (id 1, +/// `Clear`([`VOTING_PRIVATE_KEY`]), `ECDSA_HASH160`) on +/// `PrivateKeyOnVoterIdentity` — and print `hex::encode(qi.to_bytes())`. +const V093_MASTERNODE_BLOB_HEX: &str = "00bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb02000000060000020014fc7250a211deddc70ee5a2738de5f07817351cef00010001050000020014531260aa2a199e228c537dfa42c82bea2c7c1f4d00fc40420f00010100bcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbcbc01010001050000020014531260aa2a199e228c537dfa42c82bea2c7c1f4d00fc40420f00010001050000020014531260aa2a199e228c537dfa42c82bea2c7c1f4d0000010001010d6d792d6d61737465726e6f64650200000000060000020014fc7250a211deddc70ee5a2738de5f07817351cef000001111111111111111111111111111111111111111111111111111111111111111101010001050000020014531260aa2a199e228c537dfa42c82bea2c7c1f4d000001222222222222222222222222222222222222222222222222222222222222222200"; + +/// The legacy blob of row A, exactly as a v0.9.3 install holds it on disk. +fn v093_masternode_blob() -> Vec { + hex::decode(V093_MASTERNODE_BLOB_HEX).expect("the golden blob is valid hex") +} + +fn identity_public_key(id: KeyID, purpose: Purpose) -> IdentityPublicKey { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose, + security_level: SecurityLevel::MASTER, + contract_bounds: None, + key_type: KeyType::ECDSA_HASH160, + read_only: false, + // 20 bytes: the shape of a hash160 key. The import never checks a key + // against its public data, so a stand-in value is enough here — row A's + // keys, the ones that matter, come from the real v0.9.3 blob instead. + data: BinaryData::new(vec![id as u8; 20]), + disabled_at: None, + }) +} + +/// Encode a `QualifiedIdentity` the way v0.9.3 did — same manual `Encode`, same +/// bincode config. Used for the fixture rows whose exact bytes do not matter; +/// row A instead carries the real v0.9.3 blob, which is what pins the wire format. +fn legacy_identity_blob( + id: [u8; 32], + identity_type: IdentityType, + alias: &str, + keys: Vec<(PrivateKeyTarget, KeyID, Purpose, PrivateKeyData)>, +) -> Vec { + let mut private_keys = BTreeMap::new(); + let mut public_keys = BTreeMap::new(); + for (target, key_id, purpose, data) in keys { + let public_key = identity_public_key(key_id, purpose); + public_keys.insert(key_id, public_key.clone()); + private_keys.insert( + (target, key_id), + (QualifiedIdentityPublicKey::from(public_key), data), + ); + } + + QualifiedIdentity { + identity: Identity::V0(IdentityV0 { + id: Identifier::from(id), + public_keys, + balance: 1_000_000, + revision: 1, + }), + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type, + alias: Some(alias.to_string()), + private_keys: KeyStorage::from(private_keys), + dpns_names: vec![], + associated_wallets: BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: BTreeMap::new(), + // Never encoded — the legacy `status` column is the only source, which is + // exactly what the import has to restore. + status: IdentityStatus::Unknown, + network: USER_NETWORK, + } + .to_bytes() +} + +/// A key the wallet derives on demand — no secret bytes in the blob at all. +fn wallet_derived_key(seed_hash: WalletSeedHash) -> PrivateKeyData { + PrivateKeyData::AtWalletDerivationPath(WalletDerivationPath { + wallet_seed_hash: seed_hash, + derivation_path: DerivationPath::from_str("m/9'/1'/5'/0'/0'").expect("derivation path"), + }) +} + /// The seed vault + metadata sidecar state of the two migrated wallets, plus /// the envelope bytes the fixture wrote, so a test can assert the protected /// envelope travelled byte-for-byte. @@ -64,6 +208,29 @@ struct Fixture { protected_nonce: Vec, } +/// Insert one row into the legacy `identity` table, in v0.9.3's column shape. +#[allow(clippy::too_many_arguments)] +fn insert_identity( + conn: &Connection, + id: [u8; 32], + data: Option>, + status: IdentityStatus, + is_local: bool, + alias: &str, + wallet: Option<(WalletSeedHash, u32)>, + identity_type: &str, +) { + let mut row = LegacyIdentityFixture::new(id, data, USER_NETWORK.to_string()) + .with_status(status) + .with_is_local(is_local) + .with_alias(alias) + .with_identity_type(identity_type); + if let Some((seed_hash, index)) = wallet { + row = row.with_wallet(seed_hash.to_vec(), index); + } + row.insert(conn).expect("insert identity row"); +} + /// Write a `data.db` in the exact shape v0.9.3 left on disk, then hand back the /// keys the assertions need. /// @@ -313,21 +480,120 @@ fn write_v093_database(dir: &std::path::Path) -> Fixture { ) .expect("insert protected wallet row"); - // A masternode identity owned by the unprotected wallet. `data` is the opaque - // bincode blob v0.9.3 wrote (`QualifiedIdentity::to_bytes`); no current code - // path decodes it — see the module note on the identity-import gap. - conn.execute( - "INSERT INTO identity - (id, data, status, is_local, alias, wallet, wallet_index, identity_type, network) - VALUES (?1, ?2, 0, 1, 'my-masternode', ?3, 0, 'Masternode', ?4)", - params![ - IDENTITY_ID.as_slice(), - vec![0u8; 16], - unprotected.as_slice(), - USER_NETWORK.to_string(), - ], - ) - .expect("insert identity row"); + // Row A — the masternode identity owned by the unprotected wallet, holding + // the user's owner + voting keys `Clear`. `data` is a REAL v0.9.3 blob, so + // this row also pins the cross-version bincode wire format. + insert_identity( + &conn, + IDENTITY_ID, + Some(v093_masternode_blob()), + IdentityStatus::Active, + true, + "my-masternode", + Some((unprotected, 0)), + "Masternode", + ); + + // Row B — a `User` identity whose key is wallet-derived: nothing secret in + // the blob, but the wallet link must survive or the key cannot be derived. + insert_identity( + &conn, + USER_IDENTITY_ID, + Some(legacy_identity_blob( + USER_IDENTITY_ID, + IdentityType::User, + "my-username", + vec![( + PrivateKeyTarget::PrivateKeyOnMainIdentity, + 0, + Purpose::AUTHENTICATION, + wallet_derived_key(unprotected), + )], + )), + IdentityStatus::PendingCreation, + true, + "my-username", + Some((unprotected, 1)), + "User", + ); + + // Row C — an evonode identity with no wallet at all (loaded by ProTxHash), + // holding a `Clear` key. A wallet-less identity must still import. + insert_identity( + &conn, + EVONODE_IDENTITY_ID, + Some(legacy_identity_blob( + EVONODE_IDENTITY_ID, + IdentityType::Evonode, + "my-evonode", + vec![( + PrivateKeyTarget::PrivateKeyOnMainIdentity, + 0, + Purpose::OWNER, + PrivateKeyData::Clear(EVONODE_PRIVATE_KEY), + )], + )), + IdentityStatus::NotFound, + true, + "my-evonode", + None, + "Evonode", + ); + + // Row D — an observed identity: v0.9.3's lookup cache, not the user's own. + // Importing it would put a stranger's identity on the Identities screen. + insert_identity( + &conn, + OBSERVED_IDENTITY_ID, + Some(legacy_identity_blob( + OBSERVED_IDENTITY_ID, + IdentityType::User, + "someone-else", + vec![], + )), + IdentityStatus::Active, + false, + "someone-else", + None, + "User", + ); + + // Row E — a `User` identity on the password-protected wallet. That wallet is + // still locked after the drain, so this is the row that proves a locked + // wallet does not cost the user the identity, nor its link to that wallet. + insert_identity( + &conn, + PROTECTED_IDENTITY_ID, + Some(legacy_identity_blob( + PROTECTED_IDENTITY_ID, + IdentityType::User, + "cold-username", + vec![( + PrivateKeyTarget::PrivateKeyOnMainIdentity, + 0, + Purpose::AUTHENTICATION, + wallet_derived_key(protected), + )], + )), + IdentityStatus::Active, + true, + "cold-username", + Some((protected, 0)), + "User", + ); + + // Row F — v0.9.3 allowed a local row with no blob. Nothing to import; it must + // be skipped silently, not counted as a failure. + insert_identity( + &conn, + NULL_BLOB_IDENTITY_ID, + None, + IdentityStatus::Active, + true, + "no-data", + None, + "User", + ); conn.execute( "INSERT INTO scheduled_votes @@ -441,6 +707,45 @@ fn fresh_install_schema_version() -> u16 { schema_version_at(&db_file) } +/// The persisted shape of an identity entry, mirroring the private +/// `context::identity_db::StoredQualifiedIdentity`. Lets the test read what +/// actually landed **on disk** rather than trusting the in-memory struct — the +/// only way to prove no plaintext key survived the import. Field order is the +/// bincode contract; a drift in the real struct surfaces here as a decode error. +#[derive(serde::Deserialize)] +struct StoredIdentityOnDisk { + qi_bytes: Vec, + status: u8, + identity_type: String, + wallet_hash: Option<[u8; 32]>, + wallet_index: Option, +} + +/// Read the raw stored entry for one identity out of the per-network k/v store. +fn stored_identity(ctx: &Arc, id: [u8; 32]) -> StoredIdentityOnDisk { + ctx.det_kv() + .expect("per-network k/v") + .get::(DetScope::Identity(&id), "det:identity:v1") + .expect("read stored identity") + .expect("the identity must be stored after the import") +} + +/// Every private key in a stored blob, as it sits on disk. +fn stored_key_data(stored: &StoredIdentityOnDisk) -> Vec { + QualifiedIdentity::from_bytes(&stored.qi_bytes) + .expect("the stored blob must decode") + .private_keys + .private_keys + .values() + .map(|(_, data)| data.clone()) + .collect() +} + +/// `true` if `needle` appears anywhere in `haystack`. +fn contains_bytes(haystack: &[u8], needle: &[u8]) -> bool { + haystack.windows(needle.len()).any(|w| w == needle) +} + fn top_up_history(ctx: &Arc) -> Option> { // `det:top_ups:v1` is `context::identity_db::TOP_UPS_KEY`, which is private to // that module. A key change surfaces here as a missing entry, not a silent pass. @@ -631,10 +936,9 @@ async fn v093_install_upgrades_with_wallets_settings_votes_and_history_intact() "the top-up audit trail must come across, scoped by its identity's network", ); - // ── Identity ───────────────────────────────────────────────────── - // The identity row survives the ladder with its wallet link intact. It is the - // join the top-up import scopes on, and the source a future identity importer - // reads (see the module note): the ladder must not drop or orphan it. + // ── Identity: the legacy row survives the ladder ───────────────── + // The precondition the import consumes: the ladder must not drop or orphan + // the row it reads from. let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); let (alias, wallet, wallet_index, identity_type, network): ( String, @@ -663,9 +967,176 @@ async fn v093_install_upgrades_with_wallets_settings_votes_and_history_intact() "a testnet identity must not be swept into mainnet by the v33 network rename", ); + // ── Identity import: the user's identities and their keys ──────── + let identities = ctx + .load_local_qualified_identities() + .expect("load imported identities"); + let ids: Vec<[u8; 32]> = identities + .iter() + .map(|qi| qi.identity.id().to_buffer()) + .collect(); + // Sorted by identity id — the documented order of `load_identities_filtered`. + assert_eq!( + ids, + vec![ + IDENTITY_ID, + USER_IDENTITY_ID, + PROTECTED_IDENTITY_ID, + EVONODE_IDENTITY_ID + ], + "every local identity must come across — and only those: an observed \ + (is_local = 0) row is a lookup cache, and a NULL-blob row has nothing to import", + ); + + let masternode = identities + .iter() + .find(|qi| qi.identity.id().to_buffer() == IDENTITY_ID) + .expect("the masternode identity must be imported"); + assert_eq!(masternode.alias.as_deref(), Some("my-masternode")); + assert_eq!(masternode.identity_type, IdentityType::Masternode); + assert_eq!(masternode.wallet_index, Some(0)); + assert_eq!( + masternode.status, + IdentityStatus::Active, + "status lives in a column, not the blob — dropping it would relabel every \ + imported identity as `Unknown, refresh required`", + ); + + // The whole point of the import: the masternode owner still controls the node. + let presence = masternode.masternode_key_presence(); + assert!( + presence.owner && presence.voting, + "the owner and voting keys the user had loaded must come across, not be lost \ + to a manual re-import: got {presence:?}", + ); + + // Type-filtered views — the Masternodes and Identities screens read these. + let masternode_ids: Vec<[u8; 32]> = ctx + .load_local_masternode_identities() + .expect("load masternode identities") + .iter() + .map(|qi| qi.identity.id().to_buffer()) + .collect(); + assert_eq!( + masternode_ids, + vec![IDENTITY_ID, EVONODE_IDENTITY_ID], + "the Masternodes screen must show the masternode and the evonode", + ); + let user_ids: Vec<[u8; 32]> = ctx + .load_local_user_identities() + .expect("load user identities") + .iter() + .map(|qi| qi.identity.id().to_buffer()) + .collect(); + assert_eq!( + user_ids, + vec![USER_IDENTITY_ID, PROTECTED_IDENTITY_ID], + "the Identities screen must show both user identities", + ); + + // ── Wallet links survive, including to a still-locked wallet ───── + let stored_masternode = stored_identity(&ctx, IDENTITY_ID); + let stored_protected = stored_identity(&ctx, PROTECTED_IDENTITY_ID); + assert_eq!( + (stored_protected.wallet_hash, stored_protected.wallet_index), + (Some(fixture.protected), Some(0)), + "an identity on a locked wallet keeps its link — that link is what re-attaches \ + it when the user unlocks the wallet", + ); + assert_eq!( + ( + stored_masternode.wallet_hash, + stored_masternode.wallet_index + ), + (Some(fixture.unprotected), Some(0)), + ); + assert_eq!( + stored_masternode.identity_type, "Masternode", + "the type tag backs the filtered loads without a full blob decode", + ); + let stored_evonode = stored_identity(&ctx, EVONODE_IDENTITY_ID); + assert_eq!( + (stored_evonode.wallet_hash, stored_evonode.wallet_index), + (None, None), + "a wallet-less identity must not acquire a wallet link", + ); + assert_eq!( + stored_evonode.status, STATUS_NOT_FOUND, + "each identity keeps its own status", + ); + assert_eq!( + stored_identity(&ctx, USER_IDENTITY_ID).status, + STATUS_PENDING_CREATION, + ); + assert_eq!( + stored_masternode.status, STATUS_ACTIVE, + "the status column reaches disk as the same discriminant v0.9.3 stored", + ); + + // The top-up history already imported under the same identity scope must + // still resolve — the two entries share a scope and must not collide. + assert_eq!( + top_up_history(&ctx), + Some(std::collections::BTreeMap::from([(0, TOP_UP_AMOUNT)])), + "the identity blob must not displace the top-up entry in the same scope", + ); + backend.shutdown().await; } +/// The cross-version wire contract, in isolation: a `QualifiedIdentity` encoded +/// by the **real v0.9.3 binary** (bincode `2.0.0-rc.3`) still decodes on this +/// tree (bincode `2.0.1`). +/// +/// Everything else about the identity import is reasoning about struct layout. +/// This is the one test that reads bytes a real user actually has on disk. If +/// bincode's wire format had drifted between the two versions, this fails — and +/// the import would be dropping masternode keys on the floor. +#[test] +fn a_real_v093_identity_blob_still_decodes() { + let qi = QualifiedIdentity::from_bytes(&v093_masternode_blob()) + .expect("a genuine v0.9.3 identity blob must decode on the current bincode"); + + assert_eq!(qi.identity.id().to_buffer(), IDENTITY_ID); + assert_eq!(qi.identity_type, IdentityType::Masternode); + assert_eq!(qi.alias.as_deref(), Some("my-masternode")); + assert_eq!(qi.associated_owner_key_id, Some(0)); + assert_eq!( + qi.associated_voter_identity + .as_ref() + .map(|(identity, _)| identity.id().to_buffer()), + Some(VOTER_IDENTITY_ID), + "the associated voter identity must survive the decode", + ); + + // The keys are the payload. v0.9.3 held them `Clear`, and they must decode + // to exactly the bytes it wrote — a shifted field or a changed varint would + // corrupt them silently. + let keys = &qi.private_keys.private_keys; + assert_eq!(keys.len(), 2, "both private keys must decode"); + assert_eq!( + keys.get(&(PrivateKeyTarget::PrivateKeyOnMainIdentity, 0)) + .map(|(_, data)| data.clone()), + Some(PrivateKeyData::Clear(OWNER_PRIVATE_KEY)), + "the masternode owner key must decode byte-for-byte", + ); + assert_eq!( + keys.get(&(PrivateKeyTarget::PrivateKeyOnVoterIdentity, 1)) + .map(|(_, data)| data.clone()), + Some(PrivateKeyData::Clear(VOTING_PRIVATE_KEY)), + "the masternode voting key must decode byte-for-byte", + ); + + // Status is not in the blob — the legacy column is its only source. This is + // the default the import has to overwrite. + assert_eq!( + qi.status, + IdentityStatus::Unknown, + "status is not encoded; a decoded blob must come back `Unknown` so the \ + importer's column restore is load-bearing", + ); +} + /// The second launch after an upgrade. Every step must short-circuit on its /// sentinel: no duplicated votes, no resurrected history, no rewritten sentinel, /// no clobbered preferences — and the legacy rows still in `data.db`, because a @@ -699,6 +1170,12 @@ async fn second_launch_after_a_v093_upgrade_changes_nothing() { ctx.clear_all_scheduled_votes() .expect("user casts the vote"); + // …and renames the imported masternode identity. The identity sentinel is what + // must protect the edit; the same guarantee under an undecodable row is covered + // by `a_second_launch_after_an_unreadable_identity_preserves_user_edits_and_deletions`. + ctx.set_identity_alias(&Identifier::from(IDENTITY_ID), Some("my-renamed-node")) + .expect("user renames the identity"); + // Second launch: the boot import runs again, then the migration. assert_eq!( import_legacy_settings(&app_kv, &ctx.db).expect("second settings import"), @@ -736,6 +1213,23 @@ async fn second_launch_after_a_v093_upgrade_changes_nothing() { "the migrated wallet must stay reachable across launches", ); + // The skip-if-present rule: a re-run must not push the legacy blob back over + // an identity the user has since edited. + assert_eq!( + ctx.get_identity_alias(&Identifier::from(IDENTITY_ID)) + .expect("read alias") + .as_deref(), + Some("my-renamed-node"), + "a re-run must not overwrite the user's edit with the stale legacy identity", + ); + assert_eq!( + ctx.load_local_qualified_identities() + .expect("load identities") + .len(), + 4, + "a second launch must not duplicate or drop an identity", + ); + // The migration never deletes its source, so a later build can re-read it. let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); let wallets: i64 = conn @@ -744,11 +1238,264 @@ async fn second_launch_after_a_v093_upgrade_changes_nothing() { let votes: i64 = conn .query_row("SELECT COUNT(*) FROM scheduled_votes", [], |r| r.get(0)) .expect("count vote rows"); + let identities: i64 = conn + .query_row("SELECT COUNT(*) FROM identity", [], |r| r.get(0)) + .expect("count identity rows"); assert_eq!( - (wallets, votes), - (2, 1), + (wallets, votes, identities), + (2, 1, 6), "legacy rows must survive untouched" ); backend.shutdown().await; } + +/// The security contract: the import itself must never write a plaintext private +/// key to disk. +/// +/// v0.9.3 stored masternode owner / voting keys as `Clear` — plaintext, inside +/// the identity blob. The import has to route them through the vault seam, so +/// the at-rest blob keeps only `InVault` placeholders. +/// +/// This reads the stored bytes **immediately after the migration, before any +/// load path runs**, and that ordering is the whole point: reading them after a +/// `load_local_qualified_identities()` would prove nothing, because the eager +/// load-path repair (`migrate_identity_keys_to_vault`) vaults resident plaintext +/// on read and would mask an importer that had written it. "Repaired on next +/// read" is not a security property — the bytes must never hit the disk at all. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn the_import_never_writes_a_plaintext_key_to_disk() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_v093_database(tmp.path()); + + let (ctx, _) = boot(tmp.path()); + let backend = wire_backend(&ctx).await; + finish_unwire::run(&ctx).await.expect("migration"); + + // Deliberately no `load_*` call before these reads. + for (id, label) in [ + (IDENTITY_ID, "the masternode's owner + voting keys"), + (EVONODE_IDENTITY_ID, "the wallet-less evonode's key"), + ] { + let stored = stored_identity(&ctx, id); + let key_data = stored_key_data(&stored); + assert!( + !key_data.is_empty(), + "{label}: the keys must be stored, not dropped", + ); + assert!( + key_data + .iter() + .all(|data| matches!(data, PrivateKeyData::InVault)), + "{label}: a `Clear`/`AlwaysClear` key must never survive the import to disk — \ + it must be vaulted: got {key_data:?}", + ); + for key in [OWNER_PRIVATE_KEY, VOTING_PRIVATE_KEY, EVONODE_PRIVATE_KEY] { + assert!( + !contains_bytes(&stored.qi_bytes, &key), + "{label}: raw private-key bytes must appear nowhere in the stored blob", + ); + } + } + + // Vaulted, not destroyed: the keys are readable back, byte-for-byte, so the + // masternode owner can still sign. Silently losing them would be as bad as + // leaking them. + let secret_store = ctx.secret_store(); + let vault = IdentityKeyView::new(&secret_store, IDENTITY_ID); + assert_eq!( + vault + .get(&PrivateKeyTarget::PrivateKeyOnMainIdentity, 0) + .expect("read owner key from the vault") + .expect("the owner key must be in the vault") + .as_slice(), + OWNER_PRIVATE_KEY.as_slice(), + "the owner key must arrive in the vault intact", + ); + assert_eq!( + vault + .get(&PrivateKeyTarget::PrivateKeyOnVoterIdentity, 1) + .expect("read voting key from the vault") + .expect("the voting key must be in the vault") + .as_slice(), + VOTING_PRIVATE_KEY.as_slice(), + "the voting key must arrive in the vault intact", + ); + + backend.shutdown().await; +} + +/// A corrupt vote queue must never cost the user their identity keys. +/// +/// The app-data pass (scheduled votes, top-up history) can fail hard — one +/// malformed `det:scheduled_vote_voters:v1` blob is enough. That failure is +/// deterministic: it recurs on every launch, and the app-data sentinel is never +/// written. So if the identity import waited on the app-data result, a masternode +/// owner's owner/voting keys would never reach the vault — not on this launch, +/// not on any retry — because of a broken vote queue they cannot even see. +/// +/// The two DET-owned passes are independent; neither may gate the other. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_corrupt_vote_index_never_strands_the_identity_keys() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_v093_database(tmp.path()); + + let (ctx, _) = boot(tmp.path()); + let backend = wire_backend(&ctx).await; + + // Poison the scheduled-vote roster with a blob that cannot decode into the + // voter list, so the app-data pass fails hard the way a corrupted entry would. + // (`det:scheduled_vote_voters:v1` is private to `context::identity_db`; a key + // rename surfaces here as an un-poisoned index, not a silent pass.) + ctx.det_kv() + .expect("per-network k/v") + .put( + DetScope::Global, + "det:scheduled_vote_voters:v1", + &vec![0xFFu8; 3], + ) + .expect("poison the vote index"); + + // The failure still reaches the user — it is not swallowed… + assert!( + finish_unwire::run(&ctx).await.is_err(), + "a hard app-data failure must still surface, so the user gets a retry", + ); + + // …but it must not have taken the identities down with it. + assert_eq!( + ctx.load_local_qualified_identities() + .expect("load identities") + .len(), + 4, + "a corrupt vote queue must not strand the user's identities", + ); + + let secret_store = ctx.secret_store(); + let vault = IdentityKeyView::new(&secret_store, IDENTITY_ID); + assert_eq!( + vault + .get(&PrivateKeyTarget::PrivateKeyOnMainIdentity, 0) + .expect("read owner key from the vault") + .expect("the owner key must be in the vault") + .as_slice(), + OWNER_PRIVATE_KEY.as_slice(), + "the masternode owner key must reach the vault even when the vote import fails — \ + otherwise a broken vote queue permanently costs the user control of their node", + ); + + backend.shutdown().await; +} + +/// A corrupt legacy row must not make the import re-run forever, on a real +/// v0.9.3 database. +/// +/// The import used to withhold its sentinel while any row was undecodable, so it +/// ran again on **every** launch. Skip-if-present made that harmless for an +/// identity the user had *edited* — and did nothing for one the user had +/// *deleted*: the next launch re-imported it, restored the alias the user had +/// cleared and re-wrote its legacy plaintext keys into the vault, forever. The +/// import now completes even with an undecodable row (the row is reported by a +/// durable warning instead), so a second launch changes nothing the user did. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_second_launch_after_an_unreadable_identity_preserves_user_edits_and_deletions() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_v093_database(tmp.path()); + + // One corrupt blob used to be enough to re-run the whole pass on every launch. + let corrupt_id = [0x0C; 32]; + let conn = Connection::open(tmp.path().join("data.db")).expect("open data.db"); + insert_identity( + &conn, + corrupt_id, + Some(vec![0xFF; 16]), + IdentityStatus::Active, + true, + "corrupt", + None, + "User", + ); + drop(conn); + + let (ctx, _) = boot(tmp.path()); + let backend = wire_backend(&ctx).await; + finish_unwire::run(&ctx).await.expect("first migration"); + + // The readable identities landed regardless of the corrupt row… + assert_eq!( + ctx.load_local_qualified_identities() + .expect("load identities") + .len(), + 4, + "an undecodable blob must not block the identities that do decode", + ); + // …the user is told… + assert_eq!( + *ctx.migration_status().state(), + crate::context::migration_status::MigrationState::SucceededWithUnreadableIdentities { + count: 1 + }, + "the user must learn that an identity did not come across", + ); + // …and the import is nonetheless COMPLETE: the corrupt row rides on the + // durable warning, not on an import that re-runs until it decodes. + assert!( + ctx.app_kv() + .get::( + DetScope::Global, + &identities_sentinel_key_for(USER_NETWORK) + ) + .expect("read identity sentinel") + .is_some(), + "an unreadable row must not withhold the sentinel: a re-running import resurrects \ + identities the user has deleted", + ); + + // Between launches the user renames the imported masternode and deletes one + // identity outright — the security gesture whose whole point is that those + // keys stop living in this install. + ctx.set_identity_alias(&Identifier::from(IDENTITY_ID), Some("my-renamed-node")) + .expect("rename identity"); + let deleted = Identifier::from(USER_IDENTITY_ID); + ctx.delete_local_qualified_identity(&deleted) + .expect("delete identity"); + + // Second launch. + finish_unwire::run(&ctx).await.expect("second migration"); + + assert_eq!( + ctx.get_identity_alias(&Identifier::from(IDENTITY_ID)) + .expect("read alias") + .as_deref(), + Some("my-renamed-node"), + "a second launch must not overwrite the user's edit with the stale legacy blob", + ); + assert!( + !ctx.has_local_qualified_identity(&deleted) + .expect("read identity store"), + "a deleted identity must STAY deleted: re-importing it would restore the alias the \ + user cleared and re-write their legacy plaintext keys into the vault, on every \ + launch, with no way to stop it", + ); + assert_eq!( + ctx.load_local_qualified_identities() + .expect("load identities") + .len(), + 3, + "the second launch must neither duplicate the identities it imported nor resurrect \ + the one the user removed", + ); + + backend.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 +/// installs this feature exists for. +#[test] +fn the_identity_sentinel_is_per_network_and_distinct_from_the_wallet_sentinel() { + let testnet = identities_sentinel_key_for(USER_NETWORK); + assert_ne!(testnet, identities_sentinel_key_for(Network::Mainnet)); + assert_ne!(testnet, sentinel_key_for(USER_NETWORK)); +} diff --git a/src/context/identity_db.rs b/src/context/identity_db.rs index 911b4b442..1a71ab844 100644 --- a/src/context/identity_db.rs +++ b/src/context/identity_db.rs @@ -717,6 +717,26 @@ impl AppContext { Ok(Some(qi)) } + /// `true` when an identity blob is already stored under `id`. + /// + /// Presence only: the blob is never decoded, so this cannot touch key + /// material. Backs the legacy-identity import's skip-if-present rule — + /// [`Self::insert_local_qualified_identity`] is INSERT-OR-REPLACE, so a + /// retry without this check would overwrite an identity the user has since + /// edited with the stale legacy copy. Keys on the blob, not the enumeration + /// index, so a dangling index entry still imports. + pub(crate) fn has_local_qualified_identity( + &self, + id: &Identifier, + ) -> std::result::Result { + let kv = self.det_kv()?; + let id_buf = id.to_buffer(); + Ok(kv + .get::(DetScope::Identity(&id_buf), IDENTITY_KEY) + .map_err(identity_err)? + .is_some()) + } + /// Internal: read every stored identity via the Global enumeration /// index, decode it, rehydrate the metadata kept outside the bincode /// blob, and apply `keep` as a pre-decode filter on the wrapper. diff --git a/src/context/migration_status.rs b/src/context/migration_status.rs index e09186233..c2a762b6a 100644 --- a/src/context/migration_status.rs +++ b/src/context/migration_status.rs @@ -38,6 +38,11 @@ pub enum MigrationStep { /// / master xpub) into the DET wallet-metadata sidecar in /// `det-app.sqlite`. WalletMeta, + /// Importing legacy `identity` rows — and the owner / voting / payout + /// keys they carry — into the modern identity store. Runs after the + /// wallet drain so each identity's key lands against a wallet that + /// already exists. + Identities, /// Writing the completion sentinel and cleaning up. Finalize, } @@ -65,6 +70,52 @@ pub enum MigrationState { /// the legacy rows are never deleted, and a retry cannot decode a corrupt /// row, so the user is told once rather than offered a futile retry. SucceededWithUnreadableVotes { count: u32 }, + /// The wallet drain completed, but `count` legacy identities could not be + /// decoded and did not come across — so the keys they held (a masternode's + /// owner / voting key, say) are not loaded. Terminal and non-fatal: the + /// legacy rows are never deleted, so they remain recoverable. Re-published + /// from a durable record on every launch until the user acknowledges it, and + /// separate from [`Self::SucceededWithUnreadableVotes`] because the remedy + /// differs — re-import a key, not re-schedule a vote. + SucceededWithUnreadableIdentities { count: u32 }, + /// The wallet drain completed, but both DET-owned passes left undecodable + /// rows behind on the same launch: `identities` legacy identities and + /// `votes` legacy scheduled votes did not come across. Terminal and + /// non-fatal — like the two single-signal variants it combines, and for the + /// same reason: a corrupt row decodes no better on a retry. + /// + /// It exists because neither signal may eat the other. Both warnings are + /// durable and re-published on every launch until acknowledged, so with only + /// the single-signal variants available the identity half would permanently + /// outrank the vote half and cost the user a vote whose deadline is still + /// live. One banner names both remedies instead: load the identities again, + /// re-schedule the votes. + /// + /// Because that single banner names both problems, its single acknowledge + /// action retires *both* durable records (see + /// [`acknowledge_unreadable_votes`](crate::backend_task::migration::finish_unwire::acknowledge_unreadable_votes) + /// and + /// [`acknowledge_unreadable_identities`](crate::backend_task::migration::finish_unwire::acknowledge_unreadable_identities)). + SucceededWithUnreadableIdentitiesAndVotes { identities: u32, votes: u32 }, + /// Both DET-owned passes are damaged on the same launch: the wallet drain + /// landed, but `count` legacy identities could not be decoded AND the + /// app-data import hit a hard failure. Rendered as a single retryable error + /// banner naming both problems, so neither masks the other — the plain + /// [`Self::SucceededWithUnreadableIdentities`] would have swallowed the + /// app-data failure and left the user no retry. The app-data sentinel stays + /// unwritten, so a retry (or the next launch) re-attempts that import; the + /// undecodable identity rows stay in the previous version's storage, named by + /// a durable warning. Terminal until then. + /// + /// A *hard* app-data failure is the only producer. An app-data pass that + /// completed and merely left a later notice-record read failing is not one: + /// its sentinel is written, so this state's "we did not finish updating the + /// rest of your data — retry" copy would be false and its retry a no-op. That + /// path publishes [`Self::SucceededWithUnreadableIdentities`] instead. + FailedWithUnreadableIdentities { + count: u32, + error: Arc, + }, /// Migration failed. The wrapped error is rendered for the user via /// its `Display` impl at banner-render time; the typed chain is /// preserved for the details panel and logs. @@ -89,7 +140,31 @@ impl PartialEq for MigrationState { MigrationState::SucceededWithUnreadableVotes { count: a }, MigrationState::SucceededWithUnreadableVotes { count: b }, ) => a == b, + ( + MigrationState::SucceededWithUnreadableIdentities { count: a }, + MigrationState::SucceededWithUnreadableIdentities { count: b }, + ) => a == b, + ( + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: ia, + votes: va, + }, + MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: ib, + votes: vb, + }, + ) => ia == ib && va == vb, (MigrationState::Running { step: a }, MigrationState::Running { step: b }) => a == b, + ( + MigrationState::FailedWithUnreadableIdentities { + count: a, + error: ea, + }, + MigrationState::FailedWithUnreadableIdentities { + count: b, + error: eb, + }, + ) => a == b && Arc::ptr_eq(ea, eb), (MigrationState::Failed { error: a }, MigrationState::Failed { error: b }) => { Arc::ptr_eq(a, b) } @@ -176,6 +251,7 @@ mod tests { MigrationStep::Shielded, MigrationStep::WalletSeeds, MigrationStep::WalletMeta, + MigrationStep::Identities, MigrationStep::Finalize, ] { status.set_state(MigrationState::Running { step }); @@ -203,4 +279,39 @@ mod tests { assert!(!status.state().is_running()); assert!(matches!(*status.state(), MigrationState::Failed { .. })); } + + /// The combined failure state compares by `count` AND error identity, like + /// `Failed`: two combined states with the same count but distinct error + /// `Arc`s are unequal, so the per-frame reconciler treats a fresh failure as + /// a transition and re-renders instead of suppressing it as a duplicate. + #[test] + fn combined_failure_state_compares_count_and_error_identity() { + use crate::backend_task::migration::MigrationError; + + let shared = Arc::new(MigrationError::WalletBackendUnavailable); + let a = MigrationState::FailedWithUnreadableIdentities { + count: 2, + error: Arc::clone(&shared), + }; + let same = MigrationState::FailedWithUnreadableIdentities { + count: 2, + error: Arc::clone(&shared), + }; + assert_eq!(a, same, "same count and same error Arc compare equal"); + + let different_error = MigrationState::FailedWithUnreadableIdentities { + count: 2, + error: Arc::new(MigrationError::WalletBackendUnavailable), + }; + assert_ne!( + a, different_error, + "a fresh error Arc is a new transition even at the same count", + ); + + let different_count = MigrationState::FailedWithUnreadableIdentities { + count: 3, + error: Arc::clone(&shared), + }; + assert_ne!(a, different_count, "a changed count is a transition"); + } } diff --git a/src/context/wallet_lifecycle/tests.rs b/src/context/wallet_lifecycle/tests.rs index 6d6dfdc4d..9c222a33f 100644 --- a/src/context/wallet_lifecycle/tests.rs +++ b/src/context/wallet_lifecycle/tests.rs @@ -2883,13 +2883,110 @@ async fn ensure_identity_managed_unregistered_wallet_is_wallet_not_loaded() { .await .expect_err("an unregistered wallet must not resolve"); assert!( - matches!(err, TaskError::WalletNotLoaded), + matches!(err, TaskError::WalletNotLoaded { .. }), "expected WalletNotLoaded, got: {err:?}" ); backend.shutdown().await; } +/// Both `WalletNotLoaded` sites must name the wallet they are about: the +/// typed field carries the label and the user-facing message repeats it. +fn assert_wallet_not_loaded_named(err: &TaskError, expected_label: &str) { + match err { + TaskError::WalletNotLoaded { wallet_label } => { + assert_eq!(wallet_label, expected_label, "wrong wallet named") + } + other => panic!("expected WalletNotLoaded, got {other:?}"), + } + assert!( + err.to_string().contains(expected_label), + "message must name the wallet ({expected_label}), got: {err}" + ); +} + +/// With several wallets loaded, a `WalletNotLoaded` from either construction +/// site (`resolve_wallet` async, `monitored_receive_addresses` sync) names the +/// affected wallet by its alias — the user can tell which wallet to wait for. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn wallet_not_loaded_names_the_wallet_by_alias() { + let (ctx, sender, _tmp) = offline_testnet_context(); + ctx.ensure_wallet_backend(sender) + .await + .expect("ensure_wallet_backend should succeed offline"); + let backend = ctx.wallet_backend().expect("backend wired"); + + // A loaded sibling wallet, so the error cannot name "the only wallet". + let loaded_seed = [0x11u8; 64]; + let loaded_hash = Wallet::new_from_seed(loaded_seed, Network::Testnet, None, None) + .expect("build wallet") + .seed_hash(); + backend + .register_wallet_from_seed(&loaded_hash, &loaded_seed, None) + .await + .expect("register the sibling wallet"); + + // The wallet under test: known by its meta sidecar, absent from `id_map`. + let pending_hash: WalletSeedHash = [0x7Bu8; 32]; + backend + .wallet_meta() + .set( + Network::Testnet, + &pending_hash, + &WalletMeta { + alias: "paycheque".into(), + ..Default::default() + }, + ) + .expect("persist wallet meta"); + + let err = backend + .ensure_identity_managed(&pending_hash, &basic_test_identity(), 0) + .await + .expect_err("a wallet missing from id_map must not resolve"); + assert_wallet_not_loaded_named(&err, "paycheque"); + + let err = backend + .monitored_receive_addresses(&pending_hash) + .expect_err("a wallet missing from id_map has no monitored addresses"); + assert_wallet_not_loaded_named(&err, "paycheque"); + + backend.shutdown().await; +} + +/// An unnamed wallet still gets identified: the label falls back to the same +/// truncated seed-hash hex `SeedLengthInvalid` uses (12 hex chars + ellipsis). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn wallet_not_loaded_falls_back_to_truncated_seed_hash_when_unnamed() { + let (ctx, sender, _tmp) = offline_testnet_context(); + ctx.ensure_wallet_backend(sender) + .await + .expect("ensure_wallet_backend should succeed offline"); + let backend = ctx.wallet_backend().expect("backend wired"); + + // No alias in the sidecar (and, for the second case below, no sidecar row + // at all) — both degrade to the hex label, never to an unnamed error. + let unnamed_hash: WalletSeedHash = [0x9Eu8; 32]; + backend + .wallet_meta() + .set(Network::Testnet, &unnamed_hash, &WalletMeta::default()) + .expect("persist wallet meta"); + + let err = backend + .ensure_identity_managed(&unnamed_hash, &basic_test_identity(), 0) + .await + .expect_err("a wallet missing from id_map must not resolve"); + assert_wallet_not_loaded_named(&err, "9e9e9e9e9e9e…"); + + let no_meta_hash: WalletSeedHash = [0xC4u8; 32]; + let err = backend + .monitored_receive_addresses(&no_meta_hash) + .expect_err("a wallet missing from id_map has no monitored addresses"); + assert_wallet_not_loaded_named(&err, "c4c4c4c4c4c4…"); + + backend.shutdown().await; +} + /// `ensure_identity_managed` registers a previously-unknown identity (→ /// `true`), then a second call is a no-op (→ `false`). Runs with no secret /// session promoted, proving the reconcile is seed-free / locked-safe. diff --git a/src/database/legacy_import.rs b/src/database/legacy_import.rs index f5997f0cd..b9ab85293 100644 --- a/src/database/legacy_import.rs +++ b/src/database/legacy_import.rs @@ -14,6 +14,7 @@ use std::collections::BTreeMap; use dash_sdk::dpp::dashcore::Network; +use dash_sdk::dpp::identity::accessors::IdentityGettersV0; use dash_sdk::dpp::platform_value::string_encoding::Encoding; use dash_sdk::dpp::voting::vote_choices::resource_vote_choice::ResourceVoteChoice; use dash_sdk::platform::Identifier; @@ -21,6 +22,7 @@ use rusqlite::Connection; use crate::backend_task::contested_names::ScheduledDPNSVote; use crate::database::{Database, column_exists, table_exists}; +use crate::model::qualified_identity::{IdentityStatus, QualifiedIdentity}; use crate::model::settings::{ AppSettings, RootScreenType, network_from_legacy_str, theme_mode_from_str, }; @@ -51,6 +53,39 @@ pub(crate) struct LegacyScheduledVotes { /// persists (`top_up_index -> amount`). pub(crate) type LegacyTopUps = Vec<([u8; 32], BTreeMap)>; +/// One decoded local identity from the legacy `identity` table. +/// +/// `qi` carries the identity's private keys, so it is moved straight into +/// [`AppContext::insert_local_qualified_identity`](crate::context::AppContext::insert_local_qualified_identity) +/// — the only writer that routes key material through the vault seam. +#[derive(Debug)] +pub(crate) struct LegacyIdentityRow { + /// 32-byte identity id, the k/v scope key. + pub id: [u8; 32], + /// The decoded identity, with `status` and `network` already restored + /// from their columns — the bincode blob carries neither. + pub qi: QualifiedIdentity, + /// `(wallet seed hash, account index)` of the owning wallet. `None` for a + /// wallet-less identity (a masternode loaded by ProTxHash). Both-or-neither, + /// enforced by the legacy `CHECK` constraint. + pub wallet: Option<([u8; 32], u32)>, +} + +/// Outcome of one legacy identity read. +/// +/// `unreadable` counts rows whose blob could not be decoded. The caller records +/// them as a durable warning and leaves them in the legacy file — never deleted, +/// so a build with a fixed decoder can still recover them on an explicit +/// re-import. The import itself completes: re-running it on every launch would +/// resurrect identities the user has deliberately deleted. +#[derive(Debug, Default)] +pub(crate) struct LegacyIdentities { + /// Rows decoded into the modern domain type. + pub identities: Vec, + /// Rows that failed to decode. Never silently dropped by the caller. + pub unreadable: u32, +} + /// Read the user preferences held in the legacy `settings` row. /// /// Returns `None` when the table or its singleton row is absent (a fresh @@ -241,6 +276,203 @@ pub(crate) fn read_scheduled_votes( Ok(out) } +/// Read the local identities — and the private keys they hold — for `network`. +/// +/// Only `is_local = 1` rows with a non-NULL `data` blob are user identities. +/// v0.9.3 also cached observed identities (`is_local = 0`, often a NULL blob); +/// every one of its own read paths filtered those out, and so does this one — +/// they are a lookup cache, not the user's data. +/// +/// A row whose blob will not decode is counted in +/// [`LegacyIdentities::unreadable`] and skipped, so one bad blob never blocks +/// the identities around it. +/// +/// A skipped row is logged by its identity id alone — the public, on-chain +/// handle, which is what lets the user tell which identity did not come across. +/// The `data` blob and everything it decodes to (private keys above all) are +/// never logged, at any level. +pub(crate) fn read_identities( + conn: &Connection, + network: Network, +) -> rusqlite::Result { + if !table_exists(conn, "identity")? { + return Ok(LegacyIdentities::default()); + } + + let mut stmt = conn.prepare( + "SELECT id, data, status, wallet, wallet_index, alias FROM identity \ + WHERE is_local = 1 AND data IS NOT NULL AND network IN (?1, ?2)", + )?; + let mut rows = stmt.query(rusqlite::params![ + network.to_string(), + mainnet_alias_for(network) + ])?; + + let mut out = LegacyIdentities::default(); + while let Some(row) = rows.next()? { + // A wrong SQLite storage class on any column is row-level corruption: + // decode through a `Result` so a bad column costs its own row, not the + // whole read. A bare `row.get::<_>?` here would escape `read_identities` + // and discard every identity already accumulated this pass — every other + // corruption case below is counted and skipped, and this must match. + let (id, data, status, wallet, wallet_index, alias) = match decode_identity_columns(row) { + Ok(columns) => columns, + Err(_) => { + // Not even the id is logged here: the failing column may be the id + // itself, so there is no trustworthy handle to name the row by. + tracing::warn!( + target = "database::legacy_import", + "Skipping legacy identity whose column types could not be read", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + } + }; + + let Ok(id) = <[u8; 32]>::try_from(id.as_slice()) else { + tracing::warn!( + target = "database::legacy_import", + blob_len = id.len(), + "Skipping legacy identity with a non-32-byte id", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + }; + + let Ok(status) = u8::try_from(status) else { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + "Skipping legacy identity with an out-of-range status value", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + }; + + let wallet_index = match wallet_index.map(u32::try_from) { + None => None, + Some(Ok(index)) => Some(index), + Some(Err(_)) => { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + "Skipping legacy identity with an out-of-range wallet index", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + } + }; + + // Both-or-neither: the legacy `CHECK` guarantees it, so a half-filled + // link is corruption. Dropping just the link would silently orphan the + // identity from the wallet that owns its keys, so the row is reported + // instead — the sentinel stays open and a later build can retry it. + let wallet = match (wallet, wallet_index) { + (None, None) => None, + (Some(seed_hash), Some(index)) => match <[u8; 32]>::try_from(seed_hash.as_slice()) { + Ok(seed_hash) => Some((seed_hash, index)), + Err(_) => { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + "Skipping legacy identity whose wallet link is not a 32-byte seed hash", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + } + }, + _ => { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + "Skipping legacy identity with a half-filled wallet link", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + } + }; + + let Ok(mut qi) = QualifiedIdentity::from_bytes(&data) else { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + "Skipping legacy identity whose stored data could not be decoded", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + }; + + // The vault key derives from the id inside the blob, not this row's `id` + // column (see `insert_local_qualified_identity`), while the migration's + // skip-if-present precheck keys off the column. A hand-edited row whose + // two ids disagree would pass that precheck and then silently overwrite a + // different, already-loaded identity. Treat the divergence as row-level + // corruption: count it and skip, like every other bad row here. + if qi.identity.id().to_buffer() != id { + tracing::warn!( + target = "database::legacy_import", + identity = %hex::encode(id), + embedded = %hex::encode(qi.identity.id().to_buffer()), + "Skipping legacy identity whose row id and stored id disagree", + ); + out.unreadable = out.unreadable.saturating_add(1); + continue; + } + + // Neither field is in the bincode blob — the legacy encoder skipped both + // and kept them in columns. Without this, every imported identity reads + // back as `Unknown` status on mainnet. + qi.status = IdentityStatus::from(status); + qi.network = network; + + // The SQL `alias` column is authoritative — the blob's copy is stale. + // In v0.9.3, `set_identity_alias` wrote ONLY the column, while every + // identity loader decoded the blob and then unconditionally overwrote + // `alias` with the column value (`identity.alias = alias;`). A rename or + // an alias removal therefore left the blob holding the old value, and the + // column always won at load time. Keeping the blob when populated would + // resurrect a renamed-away alias or reverse a removal during upgrade, so + // the column wins here exactly as it did in v0.9.3 — including a NULL + // column clearing a stale blob alias. + qi.alias = alias; + + out.identities.push(LegacyIdentityRow { id, qi, wallet }); + } + + Ok(out) +} + +/// Decode the six raw columns of one legacy `identity` row. Kept separate so a +/// malformed column storage class is a `Result` the row loop can count and skip, +/// rather than a `?` that escapes [`read_identities`] and discards every row +/// already accumulated. Same contract as [`decode_scheduled_vote_columns`]. +/// +/// `status` and `wallet_index` are read as signed 64-bit integers on purpose: +/// the legacy schema puts no `CHECK` on either, so a corrupted row can hold a +/// value past the modern `u8` / `u32` range. Widening here and converting in the +/// row loop turns that into a counted, skipped row rather than an +/// `IntegralValueOutOfRange` that a narrow `row.get` would raise through `?`. +#[allow(clippy::type_complexity)] +fn decode_identity_columns( + row: &rusqlite::Row<'_>, +) -> rusqlite::Result<( + Vec, + Vec, + i64, + Option>, + Option, + Option, +)> { + Ok(( + row.get(0)?, + row.get(1)?, + row.get(2)?, + row.get(3)?, + row.get(4)?, + row.get(5)?, + )) +} + /// Decode the five raw columns of one legacy `scheduled_votes` row. Kept /// separate so a conversion failure is a `Result` the row loop can count and /// skip, rather than a `?` that escapes [`read_scheduled_votes`]. @@ -376,6 +608,9 @@ impl Database { #[cfg(test)] mod tests { use super::*; + use crate::database::test_helpers::{ + LegacyIdentityFixture, basic_legacy_identity_blob, create_legacy_identity_table, + }; use crate::model::settings::ThemeMode; /// The v0.10-dev `settings` shape: every user-preference column the @@ -845,4 +1080,351 @@ mod tests { let conn = Connection::open_in_memory().unwrap(); assert!(read_top_ups(&conn, Network::Testnet).unwrap().is_empty()); } + + // ── Identities ─────────────────────────────────────────────────── + + fn create_identity_table(conn: &Connection) { + create_legacy_identity_table(conn).expect("create identity table"); + } + + /// A genuinely-encodable identity blob, in the legacy `to_bytes()` shape. + fn identity_blob(id: [u8; 32]) -> Vec { + identity_blob_with_alias(id, Some("alias")) + } + + /// Like [`identity_blob`], but with an explicit alias — `None` yields a blob + /// whose own alias is absent, which exercises the column fallback. + fn identity_blob_with_alias(id: [u8; 32], alias: Option<&str>) -> Vec { + basic_legacy_identity_blob(id, alias, Network::Testnet) + } + + /// Stage one local, `Active` legacy identity row on testnet. + fn insert_identity(conn: &Connection, id: [u8; 32], data: Option>) { + LegacyIdentityFixture::new(id, data, "testnet") + .insert(conn) + .expect("insert identity"); + } + + /// The status column is the only source of an identity's status — the + /// bincode blob does not carry it. Losing it relabels every migrated + /// identity as "Unknown, refresh required". + #[test] + fn identities_restore_status_from_its_column() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let id = [0xAA; 32]; + LegacyIdentityFixture::new(id, Some(identity_blob(id)), "testnet") + .with_status(IdentityStatus::Active) + .insert(&conn) + .expect("insert identity"); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + + assert_eq!(read.identities.len(), 1); + assert_eq!(read.identities[0].qi.status, IdentityStatus::Active); + assert_eq!(read.identities[0].qi.network, Network::Testnet); + assert_eq!(read.identities[0].id, id); + assert_eq!(read.unreadable, 0); + } + + /// v0.9.3 cached observed identities with `is_local = 0` and often a NULL + /// blob. They are a lookup cache, not the user's own identities — every + /// v0.9.3 read path filtered them out and so must this one. + #[test] + fn identities_skip_observed_and_null_blob_rows() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let mine = [0xAA; 32]; + let observed = [0xBB; 32]; + let null_blob = [0xCC; 32]; + insert_identity(&conn, mine, Some(identity_blob(mine))); + LegacyIdentityFixture::new(observed, Some(identity_blob(observed)), "testnet") + .with_is_local(false) + .insert(&conn) + .expect("insert identity"); + insert_identity(&conn, null_blob, None); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + + assert_eq!(read.identities.len(), 1, "only the local, non-null row"); + assert_eq!(read.identities[0].id, mine); + assert_eq!( + read.unreadable, 0, + "a skipped cache row is not a failure — counting it would withhold the \ + sentinel forever", + ); + } + + /// The wallet link is what re-attaches an identity to the wallet holding its + /// keys. It must survive the read exactly as stored. + #[test] + fn identities_carry_their_wallet_link() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let id = [0xAA; 32]; + let seed_hash = [0x77; 32]; + LegacyIdentityFixture::new(id, Some(identity_blob(id)), "testnet") + .with_wallet(seed_hash.to_vec(), 3) + .insert(&conn) + .expect("insert identity"); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + assert_eq!(read.identities[0].wallet, Some((seed_hash, 3))); + } + + /// A corrupt blob is counted, never silently dropped, and never blocks the + /// identities around it. + #[test] + fn identities_count_unreadable_rows() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let good = [0xAA; 32]; + let corrupt = [0xBB; 32]; + insert_identity(&conn, good, Some(identity_blob(good))); + insert_identity(&conn, corrupt, Some(vec![0xFF; 8])); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + + assert_eq!(read.identities.len(), 1, "the readable identity survives"); + assert_eq!(read.identities[0].id, good); + assert_eq!(read.unreadable, 1); + } + + /// Identities on another network must not leak into this network's import. + /// A pre-v29 `data.db` still spells mainnet `dash`. + #[test] + fn identities_filter_by_network_including_the_legacy_mainnet_spelling() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let testnet_id = [0xAA; 32]; + let legacy_mainnet_id = [0xBB; 32]; + insert_identity(&conn, testnet_id, Some(identity_blob(testnet_id))); + LegacyIdentityFixture::new( + legacy_mainnet_id, + Some(identity_blob(legacy_mainnet_id)), + LEGACY_MAINNET_ALIAS, + ) + .insert(&conn) + .expect("insert identity"); + + let testnet = read_identities(&conn, Network::Testnet).unwrap(); + assert_eq!(testnet.identities.len(), 1); + assert_eq!(testnet.identities[0].id, testnet_id); + + let mainnet = read_identities(&conn, Network::Mainnet).unwrap(); + assert_eq!( + mainnet.identities.len(), + 1, + "a pre-v29 mainnet identity must still be found on mainnet", + ); + assert_eq!(mainnet.identities[0].id, legacy_mainnet_id); + } + + /// An out-of-range `status` / `wallet_index` is row-level corruption like + /// any other: counted, skipped, and never allowed to take down the whole + /// read. SQLite puts no `CHECK` on either column, so a corrupted value + /// (300 does not fit a `u8`) is storable — and a narrow `row.get` would + /// raise `IntegralValueOutOfRange` through `?`, losing every readable + /// identity alongside it, keys included. + #[test] + fn identities_skip_out_of_range_column_values_without_failing_the_read() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + let good = [0xAA; 32]; + let bad_status = [0xBB; 32]; + let bad_index = [0xCC; 32]; + + insert_identity(&conn, good, Some(identity_blob(good))); + // `status` and `wallet_index` are bound as raw integers, past the u8/u32 + // range the modern types accept. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, network) + VALUES (?1, ?2, 300, 1, 'testnet')", + rusqlite::params![bad_status.as_slice(), identity_blob(bad_status)], + ) + .unwrap(); + conn.execute( + "INSERT INTO identity (id, data, status, is_local, wallet, wallet_index, network) + VALUES (?1, ?2, 2, 1, ?3, 4294967296, 'testnet')", + rusqlite::params![ + bad_index.as_slice(), + identity_blob(bad_index), + [0x77u8; 32].as_slice() + ], + ) + .unwrap(); + + let read = read_identities(&conn, Network::Testnet) + .expect("an out-of-range column must not fail the whole read"); + + assert_eq!( + read.identities.len(), + 1, + "the readable identity must still come across", + ); + assert_eq!(read.identities[0].id, good); + assert_eq!(read.unreadable, 2, "both corrupt rows are reported"); + } + + /// A wrong SQLite *storage class* on a column (an INTEGER where the blob is + /// expected, a BLOB where the `alias` text is expected) must cost only its + /// own row. Before the per-row decode, these raised `InvalidColumnType` + /// through `?` and discarded every identity already read this pass — keys + /// included. The good row, ordered first, proves the batch is not thrown + /// away when a later row is malformed. + #[test] + fn identities_skip_malformed_column_types_without_discarding_read_rows() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + + let good = [0xAA; 32]; + let bad_data = [0xBBu8; 32]; + let bad_alias = [0xCCu8; 32]; + + // Ordered first so a `?`-escape would take it down with the bad rows. + insert_identity(&conn, good, Some(identity_blob(good))); + // `data` holds an INTEGER — `row.get::>` rejects it. It passes the + // `data IS NOT NULL` filter, so the read reaches the decode and skips. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, network) + VALUES (?1, 42, 2, 1, 'testnet')", + rusqlite::params![bad_data.as_slice()], + ) + .unwrap(); + // `alias` holds a BLOB — a TEXT-affinity column keeps a blob as a blob, so + // `row.get::>` rejects it. This is the column added in the + // prior commit; a malformed one must not fail the whole read either. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, alias, network) + VALUES (?1, ?2, 2, 1, X'DEADBEEF', 'testnet')", + rusqlite::params![bad_alias.as_slice(), identity_blob(bad_alias)], + ) + .unwrap(); + + let read = read_identities(&conn, Network::Testnet) + .expect("a malformed column type must not fail the whole read"); + + assert_eq!( + read.identities.len(), + 1, + "the readable identity accumulated before the bad rows must survive", + ); + assert_eq!(read.identities[0].id, good); + assert_eq!(read.unreadable, 2, "both malformed-type rows are reported"); + } + + /// The SQL `alias` column is authoritative and always wins over the blob's + /// stale copy. In v0.9.3 `set_identity_alias` wrote only the column, and + /// every loader decoded the blob then unconditionally overwrote `alias` with + /// the column value — so a rename or a removal left the blob stale, and the + /// column always won at load time. The migration must reproduce that: a + /// populated column overrides a differing blob alias, and a NULL column + /// clears a stale blob alias (a rename-away / removal the user made in + /// v0.9.3). Getting this backwards would resurrect a renamed-away alias. + #[test] + fn identities_alias_always_takes_the_column() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + + let column_overrides_blob = [0xAA; 32]; + let column_fills_empty_blob = [0xBB; 32]; + let null_column_clears_blob = [0xCC; 32]; + + // Blob carries "blob-alias" but the column holds a newer "column-alias": + // the column value the user last set must win. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, alias, network) + VALUES (?1, ?2, 2, 1, 'column-alias', 'testnet')", + rusqlite::params![ + column_overrides_blob.as_slice(), + identity_blob_with_alias(column_overrides_blob, Some("blob-alias")) + ], + ) + .unwrap(); + // Blob has no alias — the column supplies the value. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, alias, network) + VALUES (?1, ?2, 2, 1, 'column-alias', 'testnet')", + rusqlite::params![ + column_fills_empty_blob.as_slice(), + identity_blob_with_alias(column_fills_empty_blob, None) + ], + ) + .unwrap(); + // Blob still holds a stale "blob-alias" but the column is NULL — the user + // removed the alias in v0.9.3, which wrote only the column. The NULL must + // win, or the migration resurrects the removed alias. + conn.execute( + "INSERT INTO identity (id, data, status, is_local, alias, network) + VALUES (?1, ?2, 2, 1, NULL, 'testnet')", + rusqlite::params![ + null_column_clears_blob.as_slice(), + identity_blob_with_alias(null_column_clears_blob, Some("blob-alias")) + ], + ) + .unwrap(); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + + let alias_of = |id: [u8; 32]| { + read.identities + .iter() + .find(|r| r.id == id) + .unwrap_or_else(|| panic!("row not found")) + .qi + .alias + .clone() + }; + assert_eq!( + alias_of(column_overrides_blob).as_deref(), + Some("column-alias"), + "the column value overrides a differing blob alias", + ); + assert_eq!( + alias_of(column_fills_empty_blob).as_deref(), + Some("column-alias"), + "the column supplies the alias when the blob has none", + ); + assert_eq!( + alias_of(null_column_clears_blob), + None, + "a NULL column clears a stale blob alias the user removed in v0.9.3", + ); + assert_eq!(read.unreadable, 0); + } + + /// The vault key comes from the id *inside* the blob, but the migration's + /// skip-if-present precheck keys off the row's `id` column. A row whose two + /// ids disagree is corruption: importing it would silently overwrite the + /// unrelated identity the blob names. It is counted and skipped, not imported. + #[test] + fn identities_skip_rows_whose_row_id_and_blob_id_disagree() { + let conn = Connection::open_in_memory().unwrap(); + create_identity_table(&conn); + + let good = [0xAA; 32]; + let row_id = [0xBB; 32]; + let blob_id = [0xCC; 32]; + + insert_identity(&conn, good, Some(identity_blob(good))); + // Row `id` column and the blob's embedded id disagree. + insert_identity(&conn, row_id, Some(identity_blob(blob_id))); + + let read = read_identities(&conn, Network::Testnet).unwrap(); + + assert_eq!(read.identities.len(), 1, "only the consistent row imports"); + assert_eq!(read.identities[0].id, good); + assert_eq!( + read.unreadable, 1, + "the divergent row is reported, never imported", + ); + } + + #[test] + fn identities_absent_table_reads_empty() { + let conn = Connection::open_in_memory().unwrap(); + let read = read_identities(&conn, Network::Testnet).unwrap(); + assert!(read.identities.is_empty()); + assert_eq!(read.unreadable, 0); + } } diff --git a/src/database/test_helpers.rs b/src/database/test_helpers.rs index af2cd115c..17dc6123c 100644 --- a/src/database/test_helpers.rs +++ b/src/database/test_helpers.rs @@ -4,6 +4,9 @@ //! in unit and integration tests throughout the codebase. use crate::database::Database; +use crate::model::qualified_identity::IdentityStatus; +use dash_sdk::dpp::dashcore::Network; +use rusqlite::Connection; use tempfile::TempDir; /// Creates an in-memory SQLite database for testing. @@ -220,10 +223,269 @@ pub fn seed_legacy_scheduled_vote_row( Ok(()) } +/// Create the legacy `identity` table in v0.9.3's column shape — what every +/// upgrading user's `data.db` actually holds. +/// +/// The v0.9.3 original also carries a `CHECK` tying `wallet` to `wallet_index`. +/// It is omitted here on purpose: the import must survive a half-filled wallet +/// link, and a test cannot stage that row if SQLite rejects it on insert. Tests +/// that assert the genuine v0.9.3 schema (constraint included) build their own +/// DDL instead. +pub fn create_legacy_identity_table(conn: &Connection) -> rusqlite::Result<()> { + conn.execute_batch( + "CREATE TABLE identity ( + id BLOB PRIMARY KEY, + data BLOB, + status INTEGER NOT NULL DEFAULT 0, + is_local INTEGER NOT NULL, + alias TEXT, + info TEXT, + wallet BLOB, + wallet_index INTEGER, + identity_type TEXT, + network TEXT NOT NULL + );", + ) +} + +/// A genuinely-decodable legacy `data` blob for a keyless identity, in the +/// `QualifiedIdentity::to_bytes()` shape v0.9.3 wrote. +/// +/// `alias` seeds the blob's own copy of the alias — pass `None` to stage a blob +/// whose alias is absent, which is what exercises the `alias` column fallback. +/// Tests that need real key material in the blob build their own. +pub fn basic_legacy_identity_blob(id: [u8; 32], alias: Option<&str>, network: Network) -> Vec { + use crate::model::qualified_identity::{IdentityType, QualifiedIdentity}; + use dash_sdk::dpp::version::PlatformVersion; + use dash_sdk::platform::Identifier; + + let identity = dash_sdk::dpp::identity::Identity::create_basic_identity( + Identifier::from(id), + PlatformVersion::latest(), + ) + .expect("basic identity"); + QualifiedIdentity { + identity, + associated_voter_identity: None, + associated_operator_identity: None, + associated_owner_key_id: None, + identity_type: IdentityType::User, + alias: alias.map(str::to_string), + private_keys: Default::default(), + dpns_names: vec![], + associated_wallets: std::collections::BTreeMap::new(), + secret_access: None, + wallet_index: None, + top_ups: Default::default(), + status: Default::default(), + network, + } + .to_bytes() +} + +/// One row of the legacy `identity` table, as v0.9.3 stored it. Defaults to the +/// common case — a local, `Active` identity with no alias, wallet link, or +/// identity type — so a test states only what it varies. +/// +/// Rows with a *malformed* column (a 300 in `status`, a BLOB in `alias`) are not +/// expressible here by design: the typed fields cannot hold them. Stage those +/// with raw SQL, which is the point of those tests. +pub struct LegacyIdentityFixture { + id: [u8; 32], + data: Option>, + status: u8, + is_local: bool, + alias: Option, + wallet: Option<(Vec, u32)>, + identity_type: Option, + network: String, +} + +impl LegacyIdentityFixture { + /// A local, `Active` identity on `network`. `data` is the encoded blob — + /// `None` stages the NULL-blob row the import must skip, and an undecodable + /// blob stages the corrupt row it must count. + /// + /// `network` is a string, not a [`Network`], so a test can write the pre-v29 + /// mainnet spelling (`dash`) that a real legacy database still carries. + pub fn new(id: [u8; 32], data: Option>, network: impl Into) -> Self { + Self { + id, + data, + status: u8::from(IdentityStatus::Active), + is_local: true, + alias: None, + wallet: None, + identity_type: None, + network: network.into(), + } + } + + /// Set the raw `status` column. v0.9.3 stored [`IdentityStatus`] as its + /// discriminant, and this column is the only source of an imported + /// identity's status — the blob never carried it. + pub fn with_status(mut self, status: IdentityStatus) -> Self { + self.status = u8::from(status); + self + } + + /// `false` stages an *observed* identity — v0.9.3's lookup cache, which every + /// one of its read paths filtered out and so must the import. + pub fn with_is_local(mut self, is_local: bool) -> Self { + self.is_local = is_local; + self + } + + /// Set the authoritative `alias` column. In v0.9.3 this always won over the + /// blob's own (often stale) copy. + pub fn with_alias(mut self, alias: impl Into) -> Self { + self.alias = Some(alias.into()); + self + } + + /// Link the identity to the wallet holding its keys. `seed_hash` is a plain + /// byte vector so a test can stage a corrupt, non-32-byte link. + pub fn with_wallet(mut self, seed_hash: Vec, wallet_index: u32) -> Self { + self.wallet = Some((seed_hash, wallet_index)); + self + } + + /// Set the `identity_type` column (`User`, `Masternode`, `Evonode`). + pub fn with_identity_type(mut self, identity_type: impl Into) -> Self { + self.identity_type = Some(identity_type.into()); + self + } + + /// Write the row. + pub fn insert(&self, conn: &Connection) -> rusqlite::Result<()> { + conn.execute( + "INSERT INTO identity + (id, data, status, is_local, alias, wallet, wallet_index, identity_type, network) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)", + rusqlite::params![ + self.id.as_slice(), + self.data, + self.status, + i64::from(self.is_local), + self.alias, + self.wallet.as_ref().map(|(seed_hash, _)| seed_hash.clone()), + self.wallet.as_ref().map(|(_, index)| *index), + self.identity_type, + self.network, + ], + )?; + Ok(()) + } +} + #[cfg(test)] mod tests { use super::*; + /// The staged `identity` row, read straight back out of SQLite. + #[derive(Debug, PartialEq)] + struct StagedRow { + id: Vec, + data: Option>, + status: i64, + is_local: i64, + alias: Option, + wallet: Option>, + wallet_index: Option, + identity_type: Option, + network: String, + } + + fn read_back(conn: &Connection) -> StagedRow { + conn.query_row( + "SELECT id, data, status, is_local, alias, wallet, wallet_index, identity_type, \ + network FROM identity", + [], + |row| { + Ok(StagedRow { + id: row.get(0)?, + data: row.get(1)?, + status: row.get(2)?, + is_local: row.get(3)?, + alias: row.get(4)?, + wallet: row.get(5)?, + wallet_index: row.get(6)?, + identity_type: row.get(7)?, + network: row.get(8)?, + }) + }, + ) + .expect("read back the staged row") + } + + /// The fixture's defaults are the ones every call site relies on: a local, + /// `Active` row whose optional columns are genuinely NULL. A drifted default + /// would silently re-target every test that omits the setter. + #[test] + fn legacy_identity_fixture_defaults_to_a_local_active_row() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_legacy_identity_table(&conn).expect("create identity table"); + let id = [0xAA; 32]; + let blob = basic_legacy_identity_blob(id, Some("alias"), Network::Testnet); + + LegacyIdentityFixture::new(id, Some(blob.clone()), "testnet") + .insert(&conn) + .expect("insert identity"); + + assert_eq!( + read_back(&conn), + StagedRow { + id: id.to_vec(), + data: Some(blob), + status: i64::from(u8::from(IdentityStatus::Active)), + is_local: 1, + alias: None, + wallet: None, + wallet_index: None, + identity_type: None, + network: "testnet".to_string(), + }, + "the default row is a local, Active identity whose optional columns are NULL", + ); + } + + /// Every setter must reach its own column — a fixture that silently drops the + /// wallet link would turn the import's wallet-reattachment tests green for the + /// wrong reason. + #[test] + fn legacy_identity_fixture_writes_every_column_it_is_given() { + let conn = Connection::open_in_memory().expect("in-memory db"); + create_legacy_identity_table(&conn).expect("create identity table"); + let id = [0xBB; 32]; + let seed_hash = vec![0x77; 32]; + + LegacyIdentityFixture::new(id, None, "dash") + .with_status(IdentityStatus::NotFound) + .with_is_local(false) + .with_alias("my-evonode") + .with_wallet(seed_hash.clone(), 3) + .with_identity_type("Evonode") + .insert(&conn) + .expect("insert identity"); + + assert_eq!( + read_back(&conn), + StagedRow { + id: id.to_vec(), + data: None, + status: i64::from(u8::from(IdentityStatus::NotFound)), + is_local: 0, + alias: Some("my-evonode".to_string()), + wallet: Some(seed_hash), + wallet_index: Some(3), + identity_type: Some("Evonode".to_string()), + network: "dash".to_string(), + }, + "every setter reaches its own column, and the pre-v29 mainnet spelling \ + survives verbatim", + ); + } + #[test] fn test_create_test_database() { let db = create_test_database(); diff --git a/src/model/qualified_identity/mod.rs b/src/model/qualified_identity/mod.rs index 4e972d719..6f0ce34e5 100644 --- a/src/model/qualified_identity/mod.rs +++ b/src/model/qualified_identity/mod.rs @@ -505,6 +505,32 @@ impl Signer for QualifiedIdentity { } } +/// Cap on any single allocation `from_bytes` will make while decoding a +/// `QualifiedIdentity` blob. A real identity — including its private keys, +/// DPNS names, and wallet links — is far under this. The cap exists only as a +/// decode-time safety net: bincode's default `NoLimit` config trusts a +/// length-prefixed field's claimed size and pre-allocates it *before* reading +/// anything, so a single flipped bit or a truncated blob can claim gigabytes +/// and abort the process (`handle_alloc_error`, uncatchable, not a +/// `Result::Err`) rather than fail the decode gracefully. `Limit` makes +/// bincode check the claimed size against this cap first and return +/// `DecodeError::LimitExceeded` instead — see +/// `a_length_inflated_collection_prefix_is_rejected_not_preallocated` below +/// for the regression coverage. Encoding is unaffected: this only bounds +/// decode-time allocation and does not change the wire format, so it stays +/// compatible with blobs `to_bytes` already wrote. +const IDENTITY_BLOB_DECODE_LIMIT: usize = 16 * 1024 * 1024; // 16 MiB + +/// The bincode configuration [`QualifiedIdentity::from_bytes`] decodes under. +/// Pulled into its own function (rather than inlined at the one call site) so +/// the decode-limit regression test below exercises the *exact* configuration +/// production uses — sharing this function, not a second hand-typed copy of +/// `.with_limit()` — so a future edit that weakens or drops the limit here is +/// caught by that test rather than silently diverging from it. +fn identity_blob_decode_config() -> impl bincode::config::Config { + bincode::config::standard().with_limit::<{ IDENTITY_BLOB_DECODE_LIMIT }>() +} + impl QualifiedIdentity { /// Serializes the QualifiedIdentity to a vector of bytes. pub fn to_bytes(&self) -> Vec { @@ -519,8 +545,12 @@ impl QualifiedIdentity { /// than skipping corrupted entries, because identities hold private keys /// and balance information — silently ignoring a corrupted identity could /// lead to loss of funds. + /// + /// Decodes under [`identity_blob_decode_config`] rather than bincode's + /// unbounded default, so a corrupted or length-inflated blob returns this + /// `Err` instead of aborting the process. pub fn from_bytes(bytes: &[u8]) -> Result { - bincode::decode_from_slice(bytes, bincode::config::standard()) + bincode::decode_from_slice(bytes, identity_blob_decode_config()) .map(|(identity, _)| identity) .map_err(|e| format!("Failed to decode QualifiedIdentity: {}", e)) } @@ -1169,3 +1199,81 @@ mod withdrawal_key_tests { assert_eq!(selected.identity_public_key.purpose(), Purpose::TRANSFER); } } + +/// Regression coverage for the `from_bytes` decode-limit fix (SEC-001 from the +/// PR #885 grumpy-review): a corrupted or length-inflated blob must decode to +/// a graceful `Err`, never abort the process. +/// +/// This deliberately does NOT decode a full `QualifiedIdentity` blob. Crafting +/// a byte-exact corruption of a real encoded identity is fragile — it would +/// tie the test to the current field order of a struct with many nested +/// types, and it does not need to succeed through `Identity`'s own encoding +/// to prove the point. `from_bytes`'s vulnerability lived entirely in its +/// bincode *configuration*, not in `QualifiedIdentity`'s shape: any +/// length-prefixed collection decoded under that configuration was exposed. +/// Exercising the exact same configuration directly against a minimal, +/// hand-built length-inflated prefix pins the actual fix (the config change) +/// precisely, and stays valid regardless of future changes to +/// `QualifiedIdentity`'s fields. +/// +/// The prefix construction mirrors the live reproduction from the review: a +/// `u64` varint length header (bincode's `U64_BYTE` marker, 253) claiming an +/// enormous element count, followed by only a couple of trailing bytes -- +/// exactly what a single flipped continuation bit or a truncated file +/// produces on a real blob. Before the fix (decoding under +/// `bincode::config::standard()`, i.e. `NoLimit`), decoding this buffer as +/// `Vec` pre-allocates the claimed length and aborts the process -- +/// confirmed by a standalone probe run outside the test harness during +/// review, since an in-process abort cannot be asserted as a normal test +/// failure (it takes the whole test binary down with it). After the fix +/// (decoding under `IDENTITY_BLOB_DECODE_LIMIT`), the same buffer must return +/// `DecodeError::LimitExceeded` instead. +#[cfg(test)] +mod decode_limit_tests { + use super::identity_blob_decode_config; + + #[test] + fn a_length_inflated_collection_prefix_is_rejected_not_preallocated() { + // bincode 2.0.1's varint scheme: 253 (`U64_BYTE`) marks "the next 8 + // bytes are a little-endian u64 length". Claim far more than the + // configured limit, then supply only 2 trailing bytes -- ordinary + // bit-flip/truncation corruption never has the claimed payload + // actually present. + const U64_VARINT_MARKER: u8 = 253; + let claimed_len: u64 = 1 << 40; // 1 TiB -- larger than any real identity blob + let mut corrupted = vec![U64_VARINT_MARKER]; + corrupted.extend_from_slice(&claimed_len.to_le_bytes()); + corrupted.extend_from_slice(&[0xAA, 0xBB]); + + // Uses the SAME config function `from_bytes` calls -- not a second + // hand-typed `.with_limit()` -- so a regression in that shared + // function is what this test actually catches. + let result: Result<(Vec, usize), bincode::error::DecodeError> = + bincode::decode_from_slice(&corrupted, identity_blob_decode_config()); + + match result { + Err(bincode::error::DecodeError::LimitExceeded) => {} + other => panic!( + "expected DecodeError::LimitExceeded for a length-inflated prefix, got \ + {other:?} -- if from_bytes's decode config regresses to NoLimit this \ + same buffer would instead pre-allocate 1 TiB and abort the process" + ), + } + } + + /// Sanity check that the limit is not so tight it rejects ordinary + /// legitimate data -- a real `QualifiedIdentity` with keys is far under + /// 16 MiB (the golden v0.9.3 fixture blob decoded elsewhere in this crate + /// is a few hundred bytes), so a plain in-bounds `Vec` must still + /// round-trip under the same limited config. + #[test] + fn an_ordinary_small_payload_still_decodes_under_the_limit() { + let payload = vec![0xABu8; 4096]; + let encoded = bincode::encode_to_vec(&payload, identity_blob_decode_config()) + .expect("encode under the limit"); + let (decoded, _): (Vec, usize) = + bincode::decode_from_slice(&encoded, identity_blob_decode_config()) + .expect("decode under the limit"); + assert_eq!(decoded, payload); + } +} diff --git a/src/model/wallet/meta.rs b/src/model/wallet/meta.rs index b7a8ef247..6c8e9e8c7 100644 --- a/src/model/wallet/meta.rs +++ b/src/model/wallet/meta.rs @@ -17,8 +17,33 @@ //! into the sidecar so an existing install keeps its names after //! the upgrade. +use super::WalletSeedHash; use serde::{Deserialize, Serialize}; +/// Seed-hash bytes rendered in a fallback wallet label — 6 bytes, 12 hex +/// chars: long enough to tell two wallets apart, short enough to read out. +const LABEL_HASH_PREFIX_BYTES: usize = 6; + +/// Human-readable label for a wallet: its alias when the user set one, +/// otherwise a truncated hex prefix of its seed hash. +/// +/// The single source of truth for how a wallet is named in user-facing +/// errors raised where only the seed hash is at hand (the wallet may not +/// even be loaded yet). +/// +/// ``` +/// # use dash_evo_tool::model::wallet::meta::wallet_label; +/// assert_eq!(wallet_label("paycheque", &[0x9E; 32]), "paycheque"); +/// assert_eq!(wallet_label("", &[0x9E; 32]), "9e9e9e9e9e9e…"); +/// ``` +pub fn wallet_label(alias: &str, seed_hash: &WalletSeedHash) -> String { + if alias.is_empty() { + format!("{}…", hex::encode(&seed_hash[..LABEL_HASH_PREFIX_BYTES])) + } else { + alias.to_string() + } +} + /// The original (pre-`uses_password`) [`WalletMeta`] on-disk shape, decode-only. /// /// `WalletMeta` is a positional-bincode `DetKv` value, so appending the @@ -202,6 +227,30 @@ mod tests { assert_eq!(decoded, v2); } + /// A named wallet is labelled by its alias verbatim, whatever its seed + /// hash — the alias is what the user recognises. + #[test] + fn wallet_label_uses_the_alias_when_set() { + assert_eq!(wallet_label("paycheque", &[0x9E; 32]), "paycheque"); + assert_eq!(wallet_label("a", &[0x00; 32]), "a"); + } + + /// An unnamed wallet degrades to 12 hex chars of its seed hash plus an + /// ellipsis — never to an anonymous "this wallet". Two distinct wallets + /// get two distinct labels. + #[test] + fn wallet_label_falls_back_to_truncated_seed_hash_hex() { + assert_eq!(wallet_label("", &[0x9E; 32]), "9e9e9e9e9e9e…"); + + let mut other = [0x9Eu8; 32]; + other[5] = 0x01; + assert_ne!( + wallet_label("", &other), + wallet_label("", &[0x9E; 32]), + "wallets differing inside the prefix must not share a label" + ); + } + /// TS-NOLEAK-02 (WalletMeta) — the encoded sidecar blob carries NO secret. /// `WalletMeta` structurally cannot hold a key (no secret field); this is /// canary coverage that a future field never smuggles one in. Asserted in diff --git a/src/ui/wallets/shielded_tab.rs b/src/ui/wallets/shielded_tab.rs index 9a4d24e00..51a679680 100644 --- a/src/ui/wallets/shielded_tab.rs +++ b/src/ui/wallets/shielded_tab.rs @@ -15,12 +15,12 @@ use eframe::egui::{self, Ui}; use egui::{Color32, Frame, Margin, RichText}; use std::sync::Arc; -/// J-3 indicator strings — single complete sentences so the i18n -/// extraction pass picks each one up as a discrete translation unit. -/// Exposed `pub` so kittest coverage (TC-A11Y-006) can assert against -/// the exact label the UI renders. +/// J-3 indicator strings — single complete sentences that name their own +/// subject, so the i18n pass picks each one up as a discrete translation unit +/// and no label depends on where it happens to be rendered. Exposed `pub` so +/// the tests (TC-A11Y-006) assert against the exact label the UI renders. pub const SHIELDED_VERIFYING_LABEL: &str = "Verifying shielded balance."; -pub const SHIELDED_VERIFIED_LABEL: &str = "Verified."; +pub const SHIELDED_VERIFIED_LABEL: &str = "Shielded balance verified."; pub const SHIELDED_SPEND_LOCKED_LABEL: &str = "Spending paused."; pub const SHIELDED_SPEND_LOCKED_TOOLTIP: &str = "Spending paused until shielded balance is verified."; @@ -75,11 +75,19 @@ pub fn derive_shielded_indicator(state: &MigrationState, skipped: bool) -> Shiel step: MigrationStep::Shielded, } => ShieldedIndicator::Verifying, MigrationState::Failed { .. } => ShieldedIndicator::Failed, - // Unreadable scheduled votes say nothing about shielded data: the wallet - // drain completed, so the balance is as authoritative as on `Success`. - MigrationState::Success | MigrationState::SucceededWithUnreadableVotes { .. } => { - ShieldedIndicator::Verified - } + // Every state below is reachable only after the wallet drain returned Ok, + // and what broke in them — undecodable vote or identity rows, a hard + // app-data failure — belongs to passes that run afterwards and never touch + // shielded storage. The balance is therefore as authoritative as on + // `Success`, even under an error banner: `FailedWithUnreadableIdentities` + // raises one, and the badge deliberately stays green beneath it. Mapping it + // to `Failed` instead would lock shielded spends over a corrupt vote row and + // offer a retry for a shielded migration that never failed. + MigrationState::Success + | MigrationState::SucceededWithUnreadableVotes { .. } + | MigrationState::SucceededWithUnreadableIdentities { .. } + | MigrationState::SucceededWithUnreadableIdentitiesAndVotes { .. } + | MigrationState::FailedWithUnreadableIdentities { .. } => ShieldedIndicator::Verified, // Idle / non-shielded running step → no badge. MigrationState::Idle | MigrationState::Running { .. } => ShieldedIndicator::Hidden, } @@ -827,6 +835,23 @@ mod tests { assert_ne!(SHIELDED_VERIFIED_ICON, SHIELDED_VERIFIED_LABEL); } + /// The badge stays green on the terminal states that failed *something else* + /// (unreadable identities, a hard app-data failure) because none of them + /// touch shielded data — so its copy must name the one thing it vouches for. + /// A badge whose subject comes from its position under the balance reads as + /// a blanket "all good" beside the migration error banner, and hands the + /// translator an adjective with no noun to agree with. + #[test] + fn verified_label_names_the_balance_it_vouches_for() { + assert!( + SHIELDED_VERIFIED_LABEL + .to_lowercase() + .contains("shielded balance"), + "the Verified badge must name its subject, not borrow it from the layout: \ + `{SHIELDED_VERIFIED_LABEL}`", + ); + } + /// `derive_shielded_indicator` maps every migration state onto the /// expected J-3 badge. Pure inputs / pure output — testable without /// a UI harness. @@ -878,6 +903,43 @@ mod tests { ShieldedIndicator::Verified, "an unreadable vote row says nothing about shielded data — the drain completed", ); + assert_eq!( + derive_shielded_indicator( + &MigrationState::SucceededWithUnreadableIdentities { count: 1 }, + false, + ), + ShieldedIndicator::Verified, + "an unreadable identity row costs the user keys, not shielded notes", + ); + assert_eq!( + derive_shielded_indicator( + &MigrationState::SucceededWithUnreadableIdentitiesAndVotes { + identities: 1, + votes: 2, + }, + false, + ), + ShieldedIndicator::Verified, + "two unreadable-row signals are still not a shielded-data signal", + ); + // The one state where the badge sits beside a red Error banner. It stays + // Verified on purpose: the error is the app-data pass, which runs after + // the drain and never touches shielded storage. Downgrading it would lock + // spends over a corrupt vote row and claim a shielded failure that did not + // happen. + assert_eq!( + derive_shielded_indicator( + &MigrationState::FailedWithUnreadableIdentities { + count: 1, + error: std::sync::Arc::new( + crate::backend_task::migration::MigrationError::WalletBackendUnavailable, + ), + }, + false, + ), + ShieldedIndicator::Verified, + "a failed app-data pass is not a failed shielded migration — spends stay open", + ); // Skip-for-now hides the indicator regardless of state — the // session-local override the UI uses to dismiss the retry // banner. diff --git a/src/wallet_backend/hydration.rs b/src/wallet_backend/hydration.rs index 895c1b513..b5c3aea53 100644 --- a/src/wallet_backend/hydration.rs +++ b/src/wallet_backend/hydration.rs @@ -23,7 +23,7 @@ use dash_sdk::dpp::dashcore::Network; use dash_sdk::dpp::key_wallet::bip32::ExtendedPubKey; use crate::backend_task::error::TaskError; -use crate::model::wallet::meta::WalletMeta; +use crate::model::wallet::meta::{WalletMeta, wallet_label}; use crate::model::wallet::seed_envelope::StoredSeedEnvelope; use crate::model::wallet::{ClosedKeyItem, OpenWalletSeed, Wallet, WalletSeed, WalletSeedHash}; use crate::wallet_backend::secret_seam::SecretScheme; @@ -268,14 +268,8 @@ fn wallet_from_envelope( // envelope is only length-checked to prove it is well-formed; the // open wallet parks no plaintext seed (R3). if encrypted_seed.len() != EXPECTED_SEED_LEN as usize { - let label = if meta.alias.is_empty() { - let hex_hash = hex::encode(seed_hash); - format!("{}…", &hex_hash[..hex_hash.len().min(12)]) - } else { - meta.alias.clone() - }; return Err(TaskError::SeedLengthInvalid { - wallet_label: label, + wallet_label: wallet_label(&meta.alias, &seed_hash), got: encrypted_seed.len() as u32, expected: EXPECTED_SEED_LEN, }); diff --git a/src/wallet_backend/mod.rs b/src/wallet_backend/mod.rs index 7bf8bf426..7c5170a94 100644 --- a/src/wallet_backend/mod.rs +++ b/src/wallet_backend/mod.rs @@ -123,6 +123,7 @@ use crate::context::AppContext; use crate::context::connection_status::ConnectionStatus; use crate::model::selected_identity::SelectedIdentity; use crate::model::selected_wallet::SelectedWallet; +use crate::model::wallet::meta::wallet_label; use crate::model::wallet::{PlatformAddressEntry, WalletSeedHash}; use crate::utils::egui_mpsc::SenderAsync; @@ -1576,17 +1577,29 @@ impl WalletBackend { } } + /// [`TaskError::WalletNotLoaded`] naming the wallet the caller asked for, + /// so a user with several wallets open knows which one to wait for. Reads + /// the alias from the meta sidecar — the wallet is by definition absent + /// from `id_map` here, so there is no live handle to ask. + fn wallet_not_loaded(&self, seed_hash: &WalletSeedHash) -> TaskError { + let alias = self + .wallet_meta() + .get(self.inner.network, seed_hash) + .map(|meta| meta.alias) + .unwrap_or_default(); + TaskError::WalletNotLoaded { + wallet_label: wallet_label(&alias, seed_hash), + } + } + /// Map a DET `WalletSeedHash` to the upstream wallet handle. async fn resolve_wallet( &self, seed_hash: &WalletSeedHash, ) -> Result, TaskError> { - let wallet_id = *self - .inner - .id_map - .read()? - .get(seed_hash) - .ok_or(TaskError::WalletNotLoaded)?; + // The guard drops before the error path reads the meta sidecar. + let wallet_id = self.inner.id_map.read()?.get(seed_hash).copied(); + let wallet_id = wallet_id.ok_or_else(|| self.wallet_not_loaded(seed_hash))?; self.inner .pwm .get_wallet(&wallet_id) @@ -1628,12 +1641,8 @@ impl WalletBackend { ) -> Result, TaskError> { use dash_sdk::dpp::key_wallet::account::{AccountType, StandardAccountType}; - let wallet_id = *self - .inner - .id_map - .read()? - .get(seed_hash) - .ok_or(TaskError::WalletNotLoaded)?; + let wallet_id = self.inner.id_map.read()?.get(seed_hash).copied(); + let wallet_id = wallet_id.ok_or_else(|| self.wallet_not_loaded(seed_hash))?; let standard = AccountType::Standard { index: DEFAULT_BIP44_ACCOUNT, standard_account_type: StandardAccountType::BIP44Account, diff --git a/tests/kittest/migration_banner.rs b/tests/kittest/migration_banner.rs index eafffa977..827bfcca5 100644 --- a/tests/kittest/migration_banner.rs +++ b/tests/kittest/migration_banner.rs @@ -6,7 +6,10 @@ //! needing a full `AppState` harness. use dash_evo_tool::app::{ - MIGRATION_RETRY_ACTION_ID, MIGRATION_VOTES_ACK_ACTION_ID, migration_running_text, + MIGRATION_IDENTITIES_ACK_ACTION_ID, MIGRATION_RETRY_ACTION_ID, + MIGRATION_UNREADABLE_ACK_ACTION_ID, MIGRATION_VOTES_ACK_ACTION_ID, + migration_failed_with_unreadable_identities_text, migration_running_text, + migration_unreadable_identities_and_votes_text, migration_unreadable_identities_text, migration_unreadable_votes_text, }; use dash_evo_tool::context::migration_status::MigrationStep; @@ -49,6 +52,7 @@ fn tc_mig_014_running_text_covers_every_step_with_sentence() { MigrationStep::Shielded, MigrationStep::WalletSeeds, MigrationStep::WalletMeta, + MigrationStep::Identities, MigrationStep::Finalize, ] { let text = migration_running_text(step); @@ -212,3 +216,138 @@ fn unreadable_votes_banner_acknowledgement_enqueues_action() { Some(MIGRATION_VOTES_ACK_ACTION_ID), ); } + +/// Every message that tells the user their identities did not come across must +/// also tell them WHERE to get them back. "Load these identities again" named no +/// screen and no control, which leaves the Everyday User hunting for a flow they +/// may never have opened — the repo's error-message rules require a concrete, +/// self-serviceable action. The remedy lives behind "Load Identity" on the +/// Identities screen, so all three variants name both, exactly as the vote copy +/// names the Scheduled Votes screen. +#[test] +fn every_unreadable_identity_message_names_where_to_load_them_again() { + for text in [ + migration_unreadable_identities_text(2), + migration_unreadable_identities_and_votes_text(2, 3), + migration_failed_with_unreadable_identities_text(1), + ] { + assert!( + text.contains("Identities screen"), + "the copy must name the screen that recovers the keys: `{text}`", + ); + assert!( + text.contains("Load Identity"), + "the copy must name the control the user has to press: `{text}`", + ); + assert!( + text.ends_with('.'), + "banner copy must be a complete sentence for i18n extraction: `{text}`", + ); + } +} + +/// The unreadable-identity warning is acknowledgeable, exactly like its vote +/// sibling. It used to render as a sticky, action-less banner: the user was told +/// their keys had not come across and given no way to say "I understand" — so the +/// warning returned on every launch with no gesture that could ever retire it. +/// Supersedes the pre-fix "warns without a retry action" coverage: it re-asserts +/// the same "no Retry now" fact (the drain is done; a corrupt row decodes no +/// better) and adds the acknowledgement round-trip the fix introduced. +#[test] +fn unreadable_identities_banner_acknowledgement_enqueues_action() { + let text = migration_unreadable_identities_text(2); + assert!( + text.ends_with('.'), + "banner copy must be a complete sentence for i18n extraction: `{text}`", + ); + + let label = text.clone(); + let mut harness = Harness::builder() + .with_size(egui::vec2(600.0, 220.0)) + .build_ui(move |ui| { + let handle = MessageBanner::set_global(ui.ctx(), label.clone(), MessageType::Warning); + handle.with_action("Got it", MIGRATION_IDENTITIES_ACK_ACTION_ID); + MessageBanner::show_global(ui); + }); + harness.run(); + + assert!( + harness.query_by_label(text.as_str()).is_some(), + "the warning banner must render the unreadable-identities copy verbatim", + ); + assert!( + harness.query_by_label("Retry now").is_none(), + "a completed drain must not offer a retry the user cannot benefit from", + ); + assert!(MessageBanner::take_action(&harness.ctx).is_none()); + + harness.get_by_label("Got it").click(); + harness.run(); + + assert_eq!( + MessageBanner::take_action(&harness.ctx).as_deref(), + Some(MIGRATION_IDENTITIES_ACK_ACTION_ID), + "the acknowledgement must reach the app loop, or the warning can never be retired", + ); +} + +/// The combined banner names both problems, so its single "Got it" must enqueue +/// the combined acknowledgement — the one that retires BOTH durable records. +/// Routing it to either single-signal ack would leave the other half to re-raise +/// on the next launch, as a notice the user has already read. Supersedes the +/// pre-fix coverage that routed this banner's "Got it" to the vote-only ack — +/// that routing no longer matches the combined-ack design this ships. +#[test] +fn combined_unreadable_banner_acknowledgement_enqueues_the_combined_action() { + let text = migration_unreadable_identities_and_votes_text(2, 3); + let label = text.clone(); + let mut harness = Harness::builder() + .with_size(egui::vec2(600.0, 260.0)) + .build_ui(move |ui| { + let handle = MessageBanner::set_global(ui.ctx(), label.clone(), MessageType::Warning); + handle.with_action("Got it", MIGRATION_UNREADABLE_ACK_ACTION_ID); + MessageBanner::show_global(ui); + }); + harness.run(); + + harness.get_by_label("Got it").click(); + harness.run(); + + assert_eq!( + MessageBanner::take_action(&harness.ctx).as_deref(), + Some(MIGRATION_UNREADABLE_ACK_ACTION_ID), + ); +} + +/// The one identity outcome that IS a failure: unreadable identities alongside a +/// hard app-data failure. Unlike its Warning siblings this renders as an Error with +/// a working "Retry now" — the app-data half genuinely did not finish, and its +/// sentinel is unwritten, so the retry re-runs it. +#[test] +fn failed_with_unreadable_identities_banner_offers_a_working_retry() { + let text = migration_failed_with_unreadable_identities_text(1); + let label = text.clone(); + let mut harness = Harness::builder() + .with_size(egui::vec2(600.0, 240.0)) + .build_ui(move |ui| { + let handle = MessageBanner::set_global(ui.ctx(), label.clone(), MessageType::Error); + handle.with_action("Retry now", MIGRATION_RETRY_ACTION_ID); + MessageBanner::show_global(ui); + }); + harness.run(); + + assert!( + harness.query_by_label(text.as_str()).is_some(), + "the error banner must render the combined-failure copy verbatim", + ); + assert!(MessageBanner::take_action(&harness.ctx).is_none()); + + harness.get_by_label("Retry now").click(); + harness.run(); + + assert_eq!( + MessageBanner::take_action(&harness.ctx).as_deref(), + Some(MIGRATION_RETRY_ACTION_ID), + "the app-data half is retryable, so its banner's retry must enqueue the action", + ); +}