diff --git a/packages/rs-platform-wallet-storage/SCHEMA.md b/packages/rs-platform-wallet-storage/SCHEMA.md index 1db74f30c76..769378695c1 100644 --- a/packages/rs-platform-wallet-storage/SCHEMA.md +++ b/packages/rs-platform-wallet-storage/SCHEMA.md @@ -29,6 +29,10 @@ Schema evolution is version-gated by refinery. Every read-write connection turns - **metadata-version rows** (`meta_data_versions`) carry a `wallet_id` column and are cleaned directly by `cascade_meta_data_versions_on_wallet_delete`. - **identity-scoped meta** (`meta_identity`, `meta_token`) carries no `wallet_id` — only `identity_id` (+ `token_id`). It is cleaned by `cascade_meta_on_identity_delete` (AFTER DELETE ON `identities`), which fires for the wallet's own identities when the FK cascade removes them on a wallet delete. +Deleting an `identities` row — on its own, or cascaded from a wallet delete — additionally fires `cascade_children_on_identity_delete` (V018), which brooms `identity_keys`, `contacts`, `ignored_senders`, and `pending_contact_crypto` by the deleted `identity_id`. That trigger exists because no live foreign key reaches those rows in every case: `identity_keys`' FK to `identities` is compound (`wallet_id, identity_id`), and SQLite's default MATCH SIMPLE skips FK enforcement entirely once ANY column of the child key is NULL, leaving it dormant for an out-of-wallet identity; `contacts` and `ignored_senders` use `owner_id`, while `pending_contact_crypto` uses `owner_identity_id`, with no FK to `identities`. Keying the broom on the identity id alone covers the wallet-owned case too, as an idempotent overlap with the live cascade. + +Manual flushes preserve removal/re-addition boundaries as ordered segments inside one transaction. Removal sweeps the previous incarnation before the later segment recreates the identity and its new children; mixed child writes in the removal's own segment are swept with it. + ### Orphan metadata and future garbage collection Any `meta_*` row whose parent object does not exist — because it was never created, or because it was removed via a path the cascade does not cover — may persist indefinitely. This is an accepted limitation that applies to all metadata types and scopes. Examples: @@ -128,7 +132,6 @@ erDiagram BLOB wallet_id FK "NULL = orphan identity (no parent wallet yet)" INTEGER identity_index "BIP-32 index; NULL for out-of-wallet identities" BLOB entry_blob "bincode-encoded IdentityEntry" - INTEGER tombstoned "0 | 1 (logical delete)" } IDENTITY_KEYS { @@ -357,6 +360,8 @@ SQL lookups without blob decoding. ### `pending_contact_crypto` +- Identity cleanup: `cascade_children_on_identity_delete` deletes rows by `owner_identity_id`, using `idx_pending_contact_crypto_owner(owner_identity_id)`. + Deferred, signer-dependent contact cryptography operations. The owner, contact, and operation kind form the deduplication key; `payload` carries the public-only ciphertext and key-index data needed when a signer becomes @@ -468,8 +473,18 @@ matching upstream's "no-op until a chainlock has been applied". Platform identities, wallet-parented or orphan. `wallet_id` is nullable: NULL means the identity was written before a parent wallet was registered -(orphan-to-parented promotion via COALESCE on upsert). `tombstoned = 1` -marks a logical delete; the row is retained for cascade integrity. +(orphan-to-parented promotion via COALESCE on upsert). + +Removal (`IdentityChangeSet.removed`) is a physical `DELETE`, scoped to +the flush wallet, and it is terminal: every dependent row goes with it +(see [How integrity is kept](#how-integrity-is-kept)), so re-adding the +same `identity_id` later starts from a blank identity rather than +inheriting the removed one's keys, contacts, or balances. The writer runs +in two halves for this reason — `apply_upserts` sits with the other +identity writers, `apply_removals` runs last in the transaction, after +every identity-scoped child writer, so a changeset carrying both an +upsert and a removal for one identity commits instead of pulling the FK +parent out from under its own child inserts. - PK: `identity_id`. - FK: `wallet_id → wallets(wallet_id) ON DELETE CASCADE` (nullable). @@ -508,6 +523,7 @@ trigger; an applied migration is never edited. - PK: `(identity_id, key_id)`. - FK: `wallet_id → wallets(wallet_id) ON DELETE CASCADE` (nullable; belt-and-braces — already implied by the compound FK below). - FK: `(wallet_id, identity_id) → identities(wallet_id, identity_id) ON DELETE CASCADE` (compound; a key may only be filed under the wallet that owns its identity). +- Trigger cleanup: `cascade_children_on_identity_delete` brooms by `identity_id`, closing the dormant case. - Index: `idx_identity_keys_wallet_identity(wallet_id, identity_id)`. ### `contacts` @@ -527,6 +543,8 @@ cleared on a superseding rotation. - PK: `(wallet_id, owner_id, contact_id)`. - FK: `wallet_id → wallets(wallet_id) ON DELETE CASCADE`. +- Trigger cleanup: `cascade_children_on_identity_delete` brooms by `owner_id` — there is no FK to `identities`. +- Index: `idx_contacts_owner(owner_id)`, the broom's access path (`owner_id` is not the leading PK column). - `state` CHECK: sourced from `sqlite::schema::contacts::CONTACT_STATE_LABELS`. ### `ignored_senders` @@ -537,6 +555,8 @@ deleted; `ignored_at` records when the mute was applied. - PK: `(wallet_id, owner_id, sender_id)`. - FK: `wallet_id → wallets(wallet_id) ON DELETE CASCADE`. +- Trigger cleanup: `cascade_children_on_identity_delete` brooms by `owner_id` — there is no FK to `identities`. +- Index: `idx_ignored_senders_owner(owner_id)`, the broom's access path. - No enum-domain CHECK column. ### `platform_addresses` @@ -826,3 +846,4 @@ table-rebuild migration, as V004 does. | V015 | `V015__purge_legacy_empty_script_spent_utxos.rs` | Deletes legacy `core_utxos` rows matching `spent = 1 AND length(script) = 0 AND is_sweep_placeholder = 0`, left by a producer that fabricated an empty script for a spend of an output the wallet never recorded. One such row rejects the load of the whole file, since `load_used_addresses` decodes every stored script with no load-policy escape hatch. Balance-neutral: the balance readers select `spent = 0` only. | | V016 | `V016__identity_keys_null_scope_requires_existing_identity.rs` | Recreates the `identity_keys` null-scope trigger pair (see Triggers above) to also reject a NULL-scoped key naming an identity that does not exist at all, closing the gap where V008's guard caught only the wallet-owned case. | | V017 | `V017__identity_scan_state.rs` | Adds `identity_scan_states` (one row per wallet: the last gap-limit identity-scan verdict — `complete`, `probed_from`/`probed_through`, `unlocated_gap`) and `identity_scan_failed_indices` (indices probed without an answer, cascading from the verdict row via `wallet_id`). Purely additive; an upgraded database reads back "no verdict recorded" for every wallet until the next scan (dashpay/platform#4365). | +| V018 | `V018__identity_hard_delete.rs` | Retires identity tombstoning. Adds `cascade_children_on_identity_delete` (brooms `identity_keys` / `contacts` / `ignored_senders` / `pending_contact_crypto` by the deleted identity id, covering the rows no live FK reaches) plus its access-path indexes `idx_contacts_owner`, `idx_ignored_senders_owner`, and `idx_pending_contact_crypto_owner`; purges every already-tombstoned identity and its dependents; drops `identities.tombstoned`. | diff --git a/packages/rs-platform-wallet-storage/migrations/V018__identity_hard_delete.rs b/packages/rs-platform-wallet-storage/migrations/V018__identity_hard_delete.rs new file mode 100644 index 00000000000..2f8510b37e5 --- /dev/null +++ b/packages/rs-platform-wallet-storage/migrations/V018__identity_hard_delete.rs @@ -0,0 +1,75 @@ +//! Retire identity tombstoning: a removed identity is deleted outright. +//! +//! The `tombstoned` flag kept a logically-deleted row on disk so its +//! dependents were not wiped, at the cost of a permanent divergence: the +//! in-memory `IdentityManager` drops the whole `ManagedIdentity` on +//! removal, so a re-added identity was empty in memory while the next +//! `load()` handed it the removed one's keys back. +//! +//! Deleting the row instead needs a broom for the dependents no foreign +//! key reaches: +//! +//! - `identity_keys`' FK to `identities` is compound +//! (`wallet_id, identity_id`), and SQLite's MATCH SIMPLE skips FK +//! enforcement entirely once ANY child key column is NULL — so for an +//! out-of-wallet identity (`wallet_id IS NULL` on both sides) the +//! cascade is dormant and its keys would survive the delete. +//! - `contacts`, `ignored_senders`, and `pending_contact_crypto` carry +//! identity owners but no FK to `identities`. Orphan contacts fail a +//! strict load; orphan ignored-sender rows are omitted by the loader, +//! and the deferred-crypto queue has no production reader. +//! +//! `token_balances`, `dashpay_profiles` and `dashpay_payments_overlay` +//! need nothing new: their FK column is `identity_id NOT NULL`, so it is +//! never dormant. `meta_identity` / `meta_token` keep riding V001's +//! `cascade_meta_on_identity_delete`, which this migration leaves alone. +//! V017's `identity_scan_states` / `identity_scan_failed_indices` are +//! wallet-scoped (FK to `wallets`, no `identity_id`), so a scan verdict +//! outliving one identity is the intended reading: it records how far the +//! wallet's index space was probed, not which identities came back. + +pub fn migration() -> String { + "\ +CREATE TRIGGER cascade_children_on_identity_delete +AFTER DELETE ON identities +FOR EACH ROW +BEGIN + DELETE FROM identity_keys WHERE identity_id = OLD.identity_id; + DELETE FROM contacts WHERE owner_id = OLD.identity_id; + DELETE FROM ignored_senders WHERE owner_id = OLD.identity_id; + DELETE FROM pending_contact_crypto WHERE owner_identity_id = OLD.identity_id; +END; + +-- Owner columns follow wallet_id in these primary keys. Owner-leading +-- indexes avoid scanning each child table once per cascaded identity. +CREATE INDEX idx_contacts_owner ON contacts(owner_id); +CREATE INDEX idx_ignored_senders_owner ON ignored_senders(owner_id); +CREATE INDEX idx_pending_contact_crypto_owner ON pending_contact_crypto(owner_identity_id); + +-- Purge what earlier schemas only flagged. Spelled out per table rather +-- than left to the cascade and the trigger above, so the outcome does +-- not depend on the migrating connection's `foreign_keys` pragma. +DELETE FROM identity_keys + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM contacts + WHERE owner_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM ignored_senders + WHERE owner_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM pending_contact_crypto + WHERE owner_identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM token_balances + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM dashpay_profiles + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM dashpay_payments_overlay + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM meta_identity + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM meta_token + WHERE identity_id IN (SELECT identity_id FROM identities WHERE tombstoned = 1); +DELETE FROM identities WHERE tombstoned = 1; + +ALTER TABLE identities DROP COLUMN tombstoned; +" + .to_string() +} diff --git a/packages/rs-platform-wallet-storage/src/sqlite/backup.rs b/packages/rs-platform-wallet-storage/src/sqlite/backup.rs index f83b1c6de66..e0f526d8b93 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/backup.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/backup.rs @@ -724,8 +724,8 @@ mod tests { conn.execute_batch("PRAGMA foreign_keys = OFF;").unwrap(); conn.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, NULL, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, NULL, ?3)", rusqlite::params![&[0x1Au8; 32][..], &[0x2Bu8; 32][..], vec![0u8; 4]], ) .unwrap(); @@ -770,8 +770,8 @@ mod tests { identity_id[..8].copy_from_slice(&(i as u64).to_le_bytes()); tx.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, NULL, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, NULL, ?3)", rusqlite::params![&identity_id[..], &[0x2Bu8; 32][..], vec![0u8; 4]], ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs b/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs index 3cf835019e1..918d484eac9 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/buffer.rs @@ -11,14 +11,103 @@ use std::collections::HashMap; use std::sync::Mutex; -use platform_wallet::changeset::{Merge, PlatformWalletChangeSet}; +use platform_wallet::changeset::{IdentityChangeSet, Merge, PlatformWalletChangeSet}; use platform_wallet::wallet::platform_wallet::WalletId; use crate::sqlite::error::WalletStorageError; #[derive(Default)] pub struct Buffer { - inner: Mutex>, + inner: Mutex>, +} + +/// Merged segments separated by identity removal/re-addition boundaries. +/// All segments commit in one transaction, preserving incarnation cleanup. +#[derive(Default)] +pub struct PendingWrites { + pub segments: Vec, +} + +impl PendingWrites { + fn push(&mut self, mut incoming: PlatformWalletChangeSet) { + if incoming.is_empty() { + return; + } + incoming.identities = incoming + .identities + .as_ref() + .map(|ids| self.normalize_identities(ids)); + if let Some(last) = self.segments.last_mut() { + let readds_removed = + readds_removed(last.identities.as_ref(), incoming.identities.as_ref()); + if !readds_removed { + last.merge(incoming); + return; + } + } + self.segments.push(incoming); + } + + /// Project the candidate batch's final identity slots for admission. + pub fn identities_with(&self, incoming: &PlatformWalletChangeSet) -> IdentityChangeSet { + let mut incoming = incoming + .identities + .as_ref() + .map(|ids| self.normalize_identities(ids)); + let mut effective = IdentityChangeSet::default(); + for (index, segment) in self.segments.iter().enumerate() { + let mut ids = segment.identities.clone().unwrap_or_default(); + if index + 1 == self.segments.len() && !readds_removed(Some(&ids), incoming.as_ref()) { + if let Some(new) = incoming.take() { + ids.merge(new); + } + } + project_identities(&mut effective, ids); + } + if let Some(incoming) = incoming { + project_identities(&mut effective, incoming); + } + effective + } + + // A boundary for one identity must not reset another's revision gate or + // snapshot collections. Removal ends the lookup before any older upsert. + fn normalize_identities(&self, incoming: &IdentityChangeSet) -> IdentityChangeSet { + let mut normalized = IdentityChangeSet::default(); + for id in incoming.identities.keys() { + for segment in self.segments.iter().rev() { + let Some(ids) = &segment.identities else { + continue; + }; + if ids.removed.contains(id) { + break; + } + if let Some(entry) = ids.identities.get(id) { + normalized.identities.insert(*id, entry.clone()); + break; + } + } + } + normalized.merge(incoming.clone()); + normalized + } +} + +fn readds_removed(old: Option<&IdentityChangeSet>, new: Option<&IdentityChangeSet>) -> bool { + old.is_some_and(|old| { + new.is_some_and(|new| new.identities.keys().any(|id| old.removed.contains(id))) + }) +} + +fn project_identities(effective: &mut IdentityChangeSet, next: IdentityChangeSet) { + for (id, entry) in next.identities { + effective.removed.remove(&id); + effective.identities.insert(id, entry); + } + for id in next.removed { + effective.identities.remove(&id); + effective.removed.insert(id); + } } impl Buffer { @@ -57,7 +146,7 @@ impl Buffer { ) -> Result<(), WalletStorageError> where F: FnOnce( - Option<&PlatformWalletChangeSet>, + Option<&PendingWrites>, &PlatformWalletChangeSet, ) -> Result<(), WalletStorageError>, { @@ -69,7 +158,7 @@ impl Buffer { .lock() .map_err(|_| WalletStorageError::LockPoisoned)?; check(guard.get(&wallet_id), &cs)?; - guard.entry(wallet_id).or_default().merge(cs); + guard.entry(wallet_id).or_default().push(cs); Ok(()) } @@ -80,12 +169,12 @@ impl Buffer { pub fn take_for_flush( &self, wallet_id: &WalletId, - ) -> Result, WalletStorageError> { + ) -> Result, WalletStorageError> { let mut guard = self .inner .lock() .map_err(|_| WalletStorageError::LockPoisoned)?; - Ok(guard.remove(wallet_id).filter(|cs| !cs.is_empty())) + Ok(guard.remove(wallet_id).filter(|cs| !cs.segments.is_empty())) } /// Re-merge a previously-taken changeset back into the buffer @@ -96,9 +185,9 @@ impl Buffer { pub fn restore( &self, wallet_id: WalletId, - cs: PlatformWalletChangeSet, + cs: PendingWrites, ) -> Result<(), WalletStorageError> { - if cs.is_empty() { + if cs.segments.is_empty() { return Ok(()); } let mut guard = self @@ -111,7 +200,9 @@ impl Buffer { let entry = guard.entry(wallet_id).or_default(); let newer = std::mem::take(entry); *entry = cs; - entry.merge(newer); + for segment in newer.segments { + entry.push(segment); + } Ok(()) } @@ -174,7 +265,13 @@ mod tests { .take_for_flush(&w) .unwrap() .expect("merged value present"); - let core = merged.core.expect("core present"); + let core = merged + .segments + .into_iter() + .next() + .unwrap() + .core + .expect("core present"); assert_eq!(core.synced_height, Some(20)); assert_eq!(core.last_processed_height, Some(10)); } @@ -188,7 +285,7 @@ mod tests { let seen = std::cell::Cell::new(None); buf.store_checked(w, cs_height(20, 20), |buffered, incoming| { seen.set(Some(( - buffered.and_then(|cs| cs.core.as_ref()?.synced_height), + buffered.and_then(|cs| cs.segments.last()?.core.as_ref()?.synced_height), incoming.core.as_ref().unwrap().synced_height, ))); Ok(()) @@ -212,7 +309,14 @@ mod tests { assert!(matches!(err, WalletStorageError::LockPoisoned)); let kept = buf.take_for_flush(&w).unwrap().expect("value still staged"); - assert_eq!(kept.core.expect("core present").synced_height, Some(10)); + assert_eq!( + kept.segments[0] + .core + .as_ref() + .expect("core present") + .synced_height, + Some(10) + ); } #[test] @@ -220,12 +324,24 @@ mod tests { let buf = Buffer::new(); let w = [0xBBu8; 32]; // Buffer has nothing for `w`; restore must seed the slot. - buf.restore(w, cs_height(7, 7)).unwrap(); + buf.restore( + w, + PendingWrites { + segments: vec![cs_height(7, 7)], + }, + ) + .unwrap(); let got = buf .take_for_flush(&w) .unwrap() .expect("restored value present"); - let core = got.core.expect("core present"); + let core = got + .segments + .into_iter() + .next() + .unwrap() + .core + .expect("core present"); assert_eq!(core.synced_height, Some(7)); assert_eq!(core.last_processed_height, Some(7)); } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/error.rs b/packages/rs-platform-wallet-storage/src/sqlite/error.rs index 1051e4a4b27..9f843edf7bb 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/error.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/error.rs @@ -292,12 +292,13 @@ pub enum WalletStorageError { }, /// A rehydration merge (`load_prekeyed`) found an `identity_keys` / - /// `contacts` entry whose owner identity is neither loaded nor - /// tombstoned for this wallet — an orphaned row a logical delete does - /// not explain. Hard-error rather than silently drop live key / contact - /// state; only a known-tombstoned owner's orphaned rows are safe to skip. + /// `contacts` entry whose owner identity is not loaded for this wallet. + /// Removing an identity sweeps its rows, so an absent owner is + /// corruption: hard-error rather than silently drop live key / contact + /// state. [`LoadPolicy::Recovery`](crate::LoadPolicy) downgrades it to a + /// counted skip. #[error( - "rehydration merge found an orphaned entry: owner {} is neither loaded nor tombstoned", + "rehydration merge found an orphaned entry: owner {} is not loaded", hex::encode(owner) )] OrphanedIdentityEntry { owner: [u8; 32] }, diff --git a/packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs b/packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs index e8a8a0875e1..025b15007ef 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/load_ctx.rs @@ -78,10 +78,10 @@ pub enum LoadSite { ContactRow, /// One wallet could not be rehydrated at all; the rest of the file was. WalletRehydration, - /// An `identity_keys` / `contacts` row's owner identity is tombstoned. + /// An `identity_keys` / `contacts` row's owner identity is absent. /// Counted per row, though `route_by_owner` decides once per collection /// after its walk, so one log line can carry many counts. - TombstonedIdentityOrphan, + MissingIdentityOwner, /// An identity owned by no wallet carries a registration index. UnownedIdentityHasRegistrationIndex, /// Two live `identities` rows of one wallet claim the same @@ -122,7 +122,7 @@ impl LoadSite { Self::UnresolvedUtxoAddress => "unresolved_utxo_address", Self::UndecodableAddressScript => "undecodable_address_script", Self::UsedAddressOwnerConflict => "used_address_owner_conflict", - Self::TombstonedIdentityOrphan => "tombstoned_identity_orphan", + Self::MissingIdentityOwner => "missing_identity_owner", Self::UnownedIdentityHasRegistrationIndex => "unowned_identity_has_registration_index", Self::IdentityIndexCollision => "identity_index_collision", Self::IdentityScanStateContradiction => "identity_scan_state_contradiction", @@ -154,8 +154,8 @@ impl LoadSite { Self::RehydrationMaintainGapLimit => { "recovery mode: leaving an address pool short after gap maintenance failed" } - Self::TombstonedIdentityOrphan => { - "recovery mode: skipping rows owned by a tombstoned identity" + Self::MissingIdentityOwner => { + "recovery mode: skipping rows whose owning identity is absent" } Self::WalletRehydration => { "recovery mode: dropping one wallet that could not be rebuilt, keeping the rest of the file" @@ -500,7 +500,7 @@ mod tests { fn tolerate_at_counts_every_occurrence_from_one_record() { let ctx = LoadCtx::recovery(); ctx.tolerate_at( - LoadSite::TombstonedIdentityOrphan, + LoadSite::MissingIdentityOwner, SiteCoords { wallet_id: Some([9u8; 32]), account_type: &"n/a", @@ -513,26 +513,26 @@ mod tests { let snapshot = ctx.degradation(); assert_eq!(snapshot.total, 5); assert_eq!( - snapshot.by_site.get(&LoadSite::TombstonedIdentityOrphan), + snapshot.by_site.get(&LoadSite::MissingIdentityOwner), Some(&5) ); } /// Invariant: `tolerate` must log the site's bespoke /// [`LoadSite::explanation`], not the old hard-coded generic literal — - /// `TombstonedIdentityOrphan` has bespoke prose that the previous + /// `MissingIdentityOwner` has bespoke prose that the previous /// literal could never surface. #[tracing_test::traced_test] #[test] fn tolerate_logs_the_site_explanation_as_the_message_field() { let ctx = LoadCtx::recovery(); ctx.tolerate( - LoadSite::TombstonedIdentityOrphan, + LoadSite::MissingIdentityOwner, WalletStorageError::blob_decode("orphaned row"), ) .expect("recovery must tolerate"); assert!(logs_contain( - "recovery mode: skipping rows owned by a tombstoned identity" + "recovery mode: skipping rows whose owning identity is absent" )); } diff --git a/packages/rs-platform-wallet-storage/src/sqlite/persister.rs b/packages/rs-platform-wallet-storage/src/sqlite/persister.rs index ad315d9bb86..03560ac2b89 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/persister.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/persister.rs @@ -8,14 +8,14 @@ use rusqlite::{Connection, OptionalExtension}; use dpp::prelude::Identifier; use platform_wallet::changeset::{ - ClientStartState, IdentityChangeSet, Merge, PersistenceCapabilities, PersistenceError, + ClientStartState, IdentityChangeSet, PersistenceCapabilities, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, }; use platform_wallet::wallet::identity::ManagedIdentity; use platform_wallet::wallet::platform_wallet::WalletId; use crate::sqlite::backup::{self, BackupKind}; -use crate::sqlite::buffer::Buffer; +use crate::sqlite::buffer::{Buffer, PendingWrites}; use crate::sqlite::config::{FlushMode, LoadPolicy, SqlitePersisterConfig, Synchronous}; use crate::sqlite::error::{AutoBackupOperation, WalletStorageError}; use crate::sqlite::load_ctx::{LoadCtx, LoadDegradation, LoadSite}; @@ -660,7 +660,7 @@ impl SqlitePersister { /// /// Keys are folded in exactly as `load()` does for wallet-owned /// identities, so a returned `ManagedIdentity` is usable without a - /// second call. Tombstoned identities are omitted. + /// second call. /// /// # Errors /// @@ -774,12 +774,11 @@ impl SqlitePersister { // `drained_slot` and consumed only after commit. let drained = self.buffer.take_for_flush(&wallet_id)?; let had_buffered = drained.is_some(); - let drained_slot: std::cell::Cell> = - std::cell::Cell::new(drained); + let drained_slot: std::cell::Cell> = std::cell::Cell::new(drained); // Any pre-commit failure must restore the changeset so a delete // that didn't happen doesn't lose pending writes. - let restore_buffer = |slot: &std::cell::Cell>| { + let restore_buffer = |slot: &std::cell::Cell>| { if let Some(cs) = slot.take() { if let Err(e) = self.buffer.restore(wallet_id, cs) { tracing::error!( @@ -823,7 +822,7 @@ impl SqlitePersister { // must precede the cascade's `BEGIN EXCLUSIVE` because // `Backup::new` deadlocks if the source holds an active write tx. // Applying a changeset outside `store()` is safe here because - // `identities::apply` re-runs the slot check inside this very tx. + // `identities::apply_upserts` re-runs the slot check in this very tx. if let Some(cs) = drained_slot.take() { #[cfg(any(test, feature = "__test-helpers"))] if let Some(primed) = primed_pre_flush_error { @@ -839,7 +838,7 @@ impl SqlitePersister { return Err(WalletStorageError::Sqlite(e)); } }; - match apply_changeset_to_tx(&pre_flush_tx, &wallet_id, &cs) { + match apply_pending_writes_to_tx(&pre_flush_tx, &wallet_id, &cs) { Ok(()) => { if let Err(e) = pre_flush_tx.commit() { drained_slot.set(Some(cs)); @@ -1116,7 +1115,7 @@ impl SqlitePersister { fn handle_flush_error( &self, wallet_id: &WalletId, - cs: PlatformWalletChangeSet, + cs: PendingWrites, err: WalletStorageError, ) -> Result<(), PersistenceError> { let field_count = populated_field_count(&cs); @@ -1661,10 +1660,13 @@ impl PlatformWalletPersistence for SqlitePersister { /// tracing fields. Computed /// from the public fields so no storage-only helper leaks into the /// `rs-platform-wallet` API. -fn populated_field_count(cs: &PlatformWalletChangeSet) -> usize { +fn populated_field_count(cs: &PendingWrites) -> usize { // Single source of truth with the version-domain mapping: each populated // field is exactly one touched domain. - schema::versions::touched_domains(cs).len() + cs.segments + .iter() + .map(|segment| schema::versions::touched_domains(segment).len()) + .sum() } /// Total rows sitting in the tables `load()` has no reader for. @@ -2110,15 +2112,15 @@ fn apply_pragmas( /// what `apply` will see and not an approximation of it. `None` when /// `incoming` carries no identities. fn merged_identities( - buffered: Option<&PlatformWalletChangeSet>, + buffered: Option<&PendingWrites>, incoming: &PlatformWalletChangeSet, ) -> Option { - let incoming = incoming.identities.clone()?; - let Some(mut merged) = buffered.and_then(|cs| cs.identities.clone()) else { - return Some(incoming); - }; - merged.merge(incoming); - Some(merged) + incoming.identities.as_ref()?; + Some( + buffered + .unwrap_or(&PendingWrites::default()) + .identities_with(incoming), + ) } /// Apply every populated sub-changeset under one transaction and @@ -2129,14 +2131,26 @@ fn merged_identities( fn write_changeset_in_one_tx( conn: &mut Connection, wallet_id: &WalletId, - cs: &PlatformWalletChangeSet, + cs: &PendingWrites, ) -> Result<(), WalletStorageError> { let tx = conn.transaction()?; - apply_changeset_to_tx(&tx, wallet_id, cs)?; + apply_pending_writes_to_tx(&tx, wallet_id, cs)?; tx.commit()?; Ok(()) } +/// Replay lifecycle segments atomically, including delete-wallet pre-flushes. +fn apply_pending_writes_to_tx( + tx: &rusqlite::Transaction<'_>, + wallet_id: &WalletId, + pending: &PendingWrites, +) -> Result<(), WalletStorageError> { + for cs in &pending.segments { + apply_changeset_to_tx(tx, wallet_id, cs)?; + } + Ok(()) +} + /// Apply every populated sub-changeset of `cs` against `tx` without /// committing (caller owns the tx). Separate from /// `write_changeset_in_one_tx` so `delete_wallet_inner` can flush a drained @@ -2182,7 +2196,7 @@ fn apply_changeset_to_tx( schema::shielded_viewing_keys::apply(tx, wallet_id, shielded)?; } if let Some(identities) = cs.identities.as_ref() { - schema::identities::apply(tx, wallet_id, identities)?; + schema::identities::apply_upserts(tx, wallet_id, identities)?; } if let Some(keys) = cs.identity_keys.as_ref() { schema::identity_keys::apply(tx, wallet_id, keys)?; @@ -2216,6 +2230,13 @@ fn apply_changeset_to_tx( cs.dashpay_payments_overlay.as_ref(), )?; } + // Identity removals land LAST: the delete cascades every identity-scoped + // child row, so running it before the writers above would instead pull + // the FK parent out from under their inserts and fail the whole flush. + // See `schema::identities::apply_removals`. + if let Some(identities) = cs.identities.as_ref() { + schema::identities::apply_removals(tx, wallet_id, identities)?; + } // Bump each touched domain's version inside this same tx so a domain's // cache-invalidation marker commits atomically with its data. schema::versions::bump_touched_domains(tx, wallet_id, cs)?; diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs index 0ff7f1fe848..b7f45a8d298 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs @@ -1,6 +1,6 @@ //! `identities` table writer. -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; use dpp::identity::accessors::IdentityGettersV0; use dpp::prelude::Identifier; @@ -22,7 +22,12 @@ use crate::sqlite::schema::blob::impl_persistable_blob; // PUBLIC material only: identity snapshot reaching the `entry_blob` column. impl_persistable_blob!(IdentityEntry); -pub fn apply( +/// Write the changeset's inserted / updated identities. +/// +/// The insert half of a two-part apply: [`apply_removals`] must run for +/// the same changeset, AFTER every identity-scoped child writer in the +/// same transaction. See that function for why the order is load-bearing. +pub fn apply_upserts( tx: &Transaction<'_>, wallet_id: &WalletId, cs: &IdentityChangeSet, @@ -41,16 +46,15 @@ pub fn apply( // A's row: it fires only when the on-disk row is unowned (orphan → // parented promotion) or already owned by the incoming scope. A // cross-wallet write becomes a no-op (SQLite skips a false-WHERE - // upsert without erroring), preserving the resident blob, index, and - // tombstone. `IS` is the NULL-safe match for the nullable column. + // upsert without erroring), preserving the resident blob and index. + // `IS` is the NULL-safe match for the nullable column. let mut stmt = tx.prepare_cached( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, ?3, ?4, 0) \ + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, ?3, ?4) \ ON CONFLICT(identity_id) DO UPDATE SET \ wallet_id = COALESCE(identities.wallet_id, excluded.wallet_id), \ identity_index = excluded.identity_index, \ - entry_blob = excluded.entry_blob, \ - tombstoned = 0 \ + entry_blob = excluded.entry_blob \ WHERE identities.wallet_id IS NULL OR identities.wallet_id IS excluded.wallet_id", )?; let wallet_id_param = wallet_id_to_param(wallet_id); @@ -105,17 +109,46 @@ pub fn apply( } } } - if !cs.removed.is_empty() { - // Scope the tombstone to the flush wallet (NULL-safe `IS`) so wallet - // A's `removed` set can't tombstone wallet B's identity; the sentinel - // scope maps to NULL and tombstones only orphan rows. - let wallet_id_param = wallet_id_to_param(wallet_id); - let mut stmt = tx.prepare_cached( - "UPDATE identities SET tombstoned = 1 WHERE identity_id = ?1 AND wallet_id IS ?2", - )?; - for id in &cs.removed { - stmt.execute(params![id.as_slice(), wallet_id_param])?; - } + Ok(()) +} + +/// Delete the changeset's removed identities, and with them every row +/// they own. +/// +/// # Ordering +/// +/// Must run AFTER every identity-scoped child writer in the same +/// transaction (`identity_keys`, `contacts`, `token_balances`, +/// `dashpay`). A merged buffer can carry `removed` for an identity +/// alongside upserts for that same identity — sync writes its keys, the +/// host then removes it, both land in one flush. Deleting first pulls +/// the foreign-key parent out from under those inserts and fails the +/// whole flush; deleting last lets them land and then sweeps them, which +/// is the same end state and the documented "removal wins" rule. +/// +/// Dependents go through three paths: the native `ON DELETE CASCADE` on +/// `identity_id`, V001's `cascade_meta_on_identity_delete`, and V018's +/// `cascade_children_on_identity_delete` for the rows no live foreign +/// key reaches (an out-of-wallet identity's `identity_keys`, whose +/// compound FK is dormant under MATCH SIMPLE, plus `contacts`, +/// `ignored_senders`, and `pending_contact_crypto`, which have no FK to +/// `identities` at all). +pub fn apply_removals( + tx: &Transaction<'_>, + wallet_id: &WalletId, + cs: &IdentityChangeSet, +) -> Result<(), WalletStorageError> { + if cs.removed.is_empty() { + return Ok(()); + } + // Scope the delete to the flush wallet (NULL-safe `IS`) so wallet A's + // `removed` set can't delete wallet B's identity; the sentinel scope + // maps to NULL and reaches only orphan rows. + let wallet_id_param = wallet_id_to_param(wallet_id); + let mut stmt = + tx.prepare_cached("DELETE FROM identities WHERE identity_id = ?1 AND wallet_id IS ?2")?; + for id in &cs.removed { + stmt.execute(params![id.as_slice(), wallet_id_param])?; } Ok(()) } @@ -131,21 +164,19 @@ pub fn apply( /// attributed to the caller that made it. /// /// Occupancy is keyed on the FLUSH SCOPE, not on the incoming row's -/// stored `wallet_id`: [`apply`]'s upsert promotes a NULL `wallet_id` +/// stored `wallet_id`: [`apply_upserts`] promotes a NULL `wallet_id` /// into the flush scope, so the scope is the slot the write actually -/// lands in. Ids in `cs.removed` hold no slot — [`apply`] inserts before -/// it tombstones, so "tombstone A@N + insert B@N" in one changeset has a -/// legal final state. Tombstoned rows are likewise transparent: the -/// tombstone `UPDATE` leaves `identity_index` populated, and counting -/// those would refuse legitimate slot reuse. +/// lands in. Ids in `cs.removed` hold no slot — [`apply_removals`] +/// deletes them later in the same transaction, so "remove A@N + insert +/// B@N" in one changeset has a legal final state. /// /// The judgement is on the state the changeset ENDS in, never the one it /// starts from: an on-disk occupant that `cs` itself rewrites to another /// index — or to none at all — has vacated the slot as surely as a -/// tombstoned one, so "A moves to 2, B takes 1" and a two-way swap are +/// removed one, so "A moves to 2, B takes 1" and a two-way swap are /// both legal. `identities` carries no `(wallet_id, identity_index)` -/// UNIQUE index, so [`apply`]'s row-at-a-time upserts pass straight -/// through the transient double-claim a swap goes through. +/// UNIQUE index, so the row-at-a-time upserts pass straight through the +/// transient double-claim a swap goes through. /// /// A pre-existing on-disk duplicate (written before this check existed) /// makes both of its slot-mates unwritable here. Refusing to extend the @@ -177,12 +208,11 @@ pub(crate) fn check_index_conflicts( let wallet_id_param = wallet_id_to_param(wallet_id); let mut stmt = conn.prepare_cached( "SELECT identity_id FROM identities \ - WHERE wallet_id IS ?1 AND identity_index = ?2 AND tombstoned = 0 \ - AND identity_id != ?3", + WHERE wallet_id IS ?1 AND identity_index = ?2 AND identity_id != ?3", )?; let mut claimed: BTreeMap = BTreeMap::new(); for (id, entry) in &cs.identities { - // A removed id is tombstoned by the end of the same `apply`, so + // A removed id is deleted by the end of the same transaction, so // whatever it claims here it does not keep. if cs.removed.contains(id) { continue; @@ -239,23 +269,18 @@ pub(crate) fn check_index_conflicts( Ok(()) } -/// Decode a single `identities` row into `(entry, tombstoned)`. -/// -/// Returns `Ok(None)` if no row matches. The `tombstoned` flag is -/// returned alongside the entry so the caller can decide whether to skip -/// a logically deleted identity rather than having to consult -/// [`load_state`] separately. +/// Decode a single `identities` row, or `Ok(None)` if no row matches. #[cfg(any(test, feature = "__test-helpers"))] pub fn fetch( conn: &Connection, wallet_id: &WalletId, identity_id: &[u8; 32], -) -> Result, WalletStorageError> { +) -> Result, WalletStorageError> { // Scope to the caller's wallet (NULL-safe `IS`) so a peer wallet sharing // the identity-id row can't leak through; sentinel matches orphan rows. let wallet_id_param = wallet_id_to_param(wallet_id); let mut stmt = conn.prepare( - "SELECT length(entry_blob), entry_blob, tombstoned FROM identities \ + "SELECT length(entry_blob), entry_blob FROM identities \ WHERE identity_id = ?1 AND wallet_id IS ?2", )?; let mut rows = stmt.query(params![&identity_id[..], wallet_id_param])?; @@ -264,15 +289,14 @@ pub fn fetch( Some(row) => { blob::check_size(row.get::<_, i64>(0)?)?; let payload: Vec = row.get(1)?; - let tombstoned: i64 = row.get(2)?; - Ok(Some((blob::decode(&payload)?, tombstoned != 0))) + Ok(Some(blob::decode(&payload)?)) } } } /// Build an [`IdentityManagerStartState`](platform_wallet::changeset::IdentityManagerStartState) -/// for one wallet. Tombstoned rows are skipped; a row that fails to decode is -/// a hard error (corruption is never silently dropped). Rows with +/// for one wallet. A row that fails to decode is a hard error (corruption is +/// never silently dropped). Rows with /// `identity_index = Some(_)` bucket into `wallet_identities`, `None` into /// `out_of_wallet_identities`. /// Strict-policy [`load_state_with_ctx`], for the tests that read identity @@ -312,9 +336,8 @@ pub fn load_state( /// silently missing credits is not. /// /// Do not convert them to `ctx.tolerate`. Beyond the balance, a skipped -/// identity orphans its keys and contacts, and the orphan is fatal in both -/// policies two functions later in `merge_contacts_and_keys` — so per-row -/// tolerance here would relocate a failure rather than remove one. +/// identity also disconnects its keys and contacts, so tolerating the identity +/// row would hand the caller an incomplete balance-bearing projection. pub fn load_state_with_ctx( conn: &Connection, wallet_id: &WalletId, @@ -328,7 +351,7 @@ pub fn load_state_with_ctx( // unowned bucket. A plain `=` could not express the second case at all. let wallet_id_param = wallet_id_to_param(wallet_id); let mut stmt = conn.prepare( - "SELECT identity_id, length(entry_blob), entry_blob, tombstoned, identity_index \ + "SELECT identity_id, length(entry_blob), entry_blob, identity_index \ FROM identities WHERE wallet_id IS ?1 ORDER BY identity_id", )?; // The ignored-senders TABLE is the authoritative ignore record (every @@ -342,11 +365,7 @@ pub fn load_state_with_ctx( let identity_id_bytes: Vec = row.get(0)?; blob::check_size(row.get::<_, i64>(1)?)?; let payload: Vec = row.get(2)?; - let tombstoned: i64 = row.get(3)?; - let typed_identity_index: Option = row.get(4)?; - if tombstoned != 0 { - continue; - } + let typed_identity_index: Option = row.get(3)?; let entry: IdentityEntry = blob::decode(&payload)?; // Cross-check the decoded blob against the typed columns it was // selected by (mirrors the accounts / identity_keys readers): the @@ -426,9 +445,8 @@ pub fn load_state_with_ctx( /// its own `public_keys` and contact maps at load time — no separate /// changeset layered on afterwards. Fail-hard on a corrupt row (inherited /// from the three underlying readers) and on any merged key / contact entry -/// whose owner is absent for a reason other than a known tombstone; a -/// tombstoned owner's orphaned rows are skipped with a summary log (see -/// [`merge_contacts_and_keys`]). +/// whose owner is absent, which [`LoadPolicy::Recovery`](crate::LoadPolicy) +/// downgrades to a counted skip (see [`merge_contacts_and_keys`]). pub fn load_prekeyed( conn: &Connection, wallet_id: &WalletId, @@ -446,15 +464,7 @@ pub fn load_prekeyed( established: records.established, ..Default::default() }; - let tombstoned = load_tombstoned_ids(conn, wallet_id)?; - merge_contacts_and_keys( - &mut state, - contacts, - identity_keys, - &tombstoned, - *wallet_id, - ctx, - )?; + merge_contacts_and_keys(&mut state, contacts, identity_keys, *wallet_id, ctx)?; // The scan verdict rides the same per-wallet start state the identities // do, because it is the fact the startup sequence weighs against them: // "we already have one" is not evidence we have them all unless the scan @@ -467,30 +477,6 @@ pub fn load_prekeyed( Ok(state) } -/// The set of identity ids tombstoned (logically deleted) for this wallet. -/// A rehydration-merge entry whose owner is in this set is an expected -/// logical-delete orphan — safe to skip; an owner absent for any other -/// reason is a hard error. -fn load_tombstoned_ids( - conn: &Connection, - wallet_id: &WalletId, -) -> Result, WalletStorageError> { - // NULL-safe `IS`, matching `load_state`: a tombstoned UNOWNED identity - // must be recognised as tombstoned too, else its leftover key rows are - // treated as inexplicable orphans and hard-error the read. - let wallet_id_param = wallet_id_to_param(wallet_id); - let mut stmt = conn - .prepare("SELECT identity_id FROM identities WHERE wallet_id IS ?1 AND tombstoned = 1")?; - let mut rows = stmt.query(params![wallet_id_param])?; - let mut out = HashSet::new(); - while let Some(row) = rows.next()? { - let id_bytes: Vec = row.get(0)?; - let id32 = super::id32("identities.identity_id", &id_bytes)?; - out.insert(Identifier::from(id32)); - } - Ok(out) -} - /// Reconstruct a [`ManagedIdentity`] from a persisted [`IdentityEntry`] /// using a freshly minted V0 [`Identity`] for `(id, balance, revision)`. /// Live runtime fields (contacts maps, public-key derivations) are @@ -586,9 +572,8 @@ pub fn ensure_exists( let payload = blob::encode(&stub)?; let wallet_id_param = wallet_id_to_param(wallet_id); conn.execute( - "INSERT OR IGNORE INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, NULL, ?3, 0)", + "INSERT OR IGNORE INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, NULL, ?3)", params![&identity_id[..], wallet_id_param, payload], )?; Ok(()) @@ -608,16 +593,22 @@ pub fn ensure_exists( /// # Errors /// /// [`WalletStorageError::OrphanedIdentityEntry`] when an entry's owner is -/// absent from the loaded set. A known-tombstoned owner is the one case -/// [`LoadPolicy::Recovery`](crate::LoadPolicy) forgives: its rows are -/// logical-delete leftovers, skipped and summarised once per collection. -/// Under `Strict` even those abort, because "the owner is gone" is exactly -/// the state that silently drops live key / contact material. +/// absent from the loaded set. Removing an identity sweeps its rows, so an +/// absent owner is corruption rather than routine bookkeeping. A `contacts` +/// row is the reachable shape: it keys on `owner_id` +/// with no foreign key to `identities`, so nothing rejects one naming an +/// identity that is not there. `identity_keys` cannot reach this state — +/// its wallet-scoped FK is live, and V016's trigger pair covers the +/// NULL-scoped case the compound FK leaves dormant. +/// [`LoadPolicy::Recovery`](crate::LoadPolicy) skips and counts those; +/// `Strict` aborts, because "the owner is gone" is exactly the state that +/// silently drops live key / contact material. +/// Ignored senders are restored separately; missing-owner rows are omitted +/// by that loader without a policy error or a recovery tally. pub fn merge_contacts_and_keys( state: &mut IdentityManagerStartState, contacts: ContactChangeSet, identity_keys: IdentityKeysChangeSet, - tombstoned: &HashSet, wallet_id: WalletId, ctx: &LoadCtx, ) -> Result<(), WalletStorageError> { @@ -640,7 +631,6 @@ pub fn merge_contacts_and_keys( .into_values() .map(|entry| (entry.identity_id, entry.public_key)), &mut by_id, - tombstoned, wallet_id, ctx, "identity_keys", @@ -652,7 +642,6 @@ pub fn merge_contacts_and_keys( .into_iter() .map(|(key, entry)| (key.owner_id, entry.request)), &mut by_id, - tombstoned, wallet_id, ctx, "sent_contact_requests", @@ -664,7 +653,6 @@ pub fn merge_contacts_and_keys( .into_iter() .map(|(key, entry)| (key.owner_id, entry.request)), &mut by_id, - tombstoned, wallet_id, ctx, "incoming_contact_requests", @@ -676,7 +664,6 @@ pub fn merge_contacts_and_keys( .into_iter() .map(|(key, established)| (key.owner_id, established)), &mut by_id, - tombstoned, wallet_id, ctx, "established_contacts", @@ -689,15 +676,13 @@ pub fn merge_contacts_and_keys( /// Apply one collection's entries to their owning identity. /// /// An entry whose owner is loaded is applied. An entry whose owner is -/// tombstoned is counted and, if the policy allows it, skipped — decided -/// once after the walk rather than per entry, so a wallet with thousands of -/// leftovers produces one log line, not thousands, while the per-site tally -/// still counts every skipped row. Any other missing owner is fatal in both -/// policies. +/// missing is counted and, under [`LoadPolicy::Recovery`](crate::LoadPolicy), +/// skipped — decided once after the walk rather than per entry, so a wallet +/// with thousands of orphans produces one log line, not thousands, while the +/// per-site tally still counts every skipped row. fn route_by_owner( entries: impl IntoIterator, by_id: &mut HashMap, - tombstoned: &HashSet, wallet_id: WalletId, ctx: &LoadCtx, collection: &'static str, @@ -708,20 +693,15 @@ fn route_by_owner( for (owner, payload) in entries { match by_id.get_mut(&owner) { Some(managed) => apply(managed, payload), - None if tombstoned.contains(&owner) => { + None => { skipped += 1; first_skipped_owner.get_or_insert(owner); } - None => { - return Err(WalletStorageError::OrphanedIdentityEntry { - owner: owner.to_buffer(), - }) - } } } if let Some(owner) = first_skipped_owner { ctx.tolerate_at( - LoadSite::TombstonedIdentityOrphan, + LoadSite::MissingIdentityOwner, SiteCoords { wallet_id: Some(wallet_id), account_type: &"identity", @@ -781,16 +761,18 @@ mod tests { } } + /// Both halves of the writer in one transaction, in the order + /// `apply_changeset_to_tx` runs them. fn apply_in_tx(conn: &mut Connection, scope: &[u8; 32], cs: &IdentityChangeSet) { let tx = conn.transaction().unwrap(); - apply(&tx, scope, cs).unwrap(); + apply_upserts(&tx, scope, cs).unwrap(); + apply_removals(&tx, scope, cs).unwrap(); tx.commit().unwrap(); } /// A wallet-B flush naming an identity already owned by wallet A must NOT - /// overwrite A's blob / index or clear A's tombstone — the DO UPDATE WHERE - /// scopes the overwrite to the owning wallet, so the cross-wallet write is - /// a no-op. + /// overwrite A's blob or index — the DO UPDATE WHERE scopes the overwrite + /// to the owning wallet, so the cross-wallet write is a no-op. #[test] fn cross_wallet_upsert_does_not_overwrite_resident_row() { let mut conn = migrated_conn(); @@ -800,14 +782,11 @@ mod tests { insert_wallet(&conn, &a); insert_wallet(&conn, &b); - // A registers X (balance 1000, index 5), then tombstones it. + // A registers X (balance 1000, index 5). let mut cs_a = IdentityChangeSet::default(); cs_a.identities .insert(Identifier::from(x), entry(x, Some(a), 1000, Some(5))); apply_in_tx(&mut conn, &a, &cs_a); - let mut cs_a_remove = IdentityChangeSet::default(); - cs_a_remove.removed.insert(Identifier::from(x)); - apply_in_tx(&mut conn, &a, &cs_a_remove); // B flushes X (balance 2000, index 9, unowned blob). Must be a no-op. let mut cs_b = IdentityChangeSet::default(); @@ -815,16 +794,42 @@ mod tests { .insert(Identifier::from(x), entry(x, None, 2000, Some(9))); apply_in_tx(&mut conn, &b, &cs_b); - let (resident, tombstoned) = fetch(&conn, &a, &x).unwrap().expect("A still owns the row"); + let resident = fetch(&conn, &a, &x).unwrap().expect("A still owns the row"); assert_eq!(resident.balance, 1000, "A's blob must survive B's write"); assert_eq!(resident.identity_index, Some(5), "A's index must survive"); - assert!(tombstoned, "A's tombstone must not be reset by B"); assert!( fetch(&conn, &b, &x).unwrap().is_none(), "B must not have taken ownership" ); } + /// A cross-wallet `removed` set is a no-op for the same reason the + /// cross-wallet upsert is: the DELETE carries the flush scope, so + /// wallet B cannot delete wallet A's identity out from under it. + #[test] + fn cross_wallet_removal_leaves_the_resident_row() { + let mut conn = migrated_conn(); + let a = [0xA1u8; 32]; + let b = [0xB2u8; 32]; + let x = [0x01u8; 32]; + insert_wallet(&conn, &a); + insert_wallet(&conn, &b); + + let mut cs_a = IdentityChangeSet::default(); + cs_a.identities + .insert(Identifier::from(x), entry(x, Some(a), 1000, Some(5))); + apply_in_tx(&mut conn, &a, &cs_a); + + let mut cs_b_remove = IdentityChangeSet::default(); + cs_b_remove.removed.insert(Identifier::from(x)); + apply_in_tx(&mut conn, &b, &cs_b_remove); + + assert!( + fetch(&conn, &a, &x).unwrap().is_some(), + "B's removed set must not reach A's identity" + ); + } + /// The WHERE still permits the orphan → parented promotion path: an /// unowned (NULL wallet_id) row is claimed by the first wallet to flush it. #[test] @@ -851,7 +856,7 @@ mod tests { .insert(Identifier::from(y), entry(y, Some(a), 500, Some(3))); apply_in_tx(&mut conn, &a, &cs_a); - let (claimed, _) = fetch(&conn, &a, &y).unwrap().expect("A claimed Y"); + let claimed = fetch(&conn, &a, &y).unwrap().expect("A claimed Y"); assert_eq!(claimed.balance, 500, "promotion applies the new blob"); assert_eq!(claimed.identity_index, Some(3)); } @@ -906,7 +911,7 @@ mod tests { .insert((out_of_wallet, 0), key(out_of_wallet, 0xB2)); let tx = conn.transaction().unwrap(); - apply(&tx, &w, &ids).unwrap(); + apply_upserts(&tx, &w, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &w, &keys).unwrap(); tx.commit().unwrap(); @@ -990,7 +995,7 @@ mod tests { /// guard only rejected keys whose identity was wallet-OWNED, so one /// naming no identity at all slipped through — MATCH SIMPLE leaves both /// foreign keys dormant on a NULL-scoped row, making the trigger the - /// only guard there is. Closed by V015. + /// only guard there is. Closed by V016. #[test] fn null_scoped_key_is_rejected_for_a_missing_identity() { use platform_wallet::changeset::IdentityKeysChangeSet; @@ -1026,8 +1031,8 @@ mod tests { let payload = blob::encode(&e).unwrap(); conn.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, 1, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, 1, ?3)", params![&[id_byte; 32][..], &wallet[..], payload], ) .unwrap(); @@ -1072,8 +1077,8 @@ mod tests { let payload = blob::encode(&e).unwrap(); conn.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, 9, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, 9, ?3)", params![&[0xC1u8; 32][..], &wallet[..], payload], ) .unwrap(); @@ -1183,7 +1188,7 @@ mod tests { .insert((identity, 0), sample_key_entry(identity, 0x11)); { let tx = conn.transaction().unwrap(); - apply(&tx, &owner, &ids).unwrap(); + apply_upserts(&tx, &owner, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &owner, &owner_keys).unwrap(); tx.commit().unwrap(); } @@ -1246,7 +1251,7 @@ mod tests { owner_keys.upserts.insert((x, 0), sample_key_entry(x, 0x11)); { let tx = conn.transaction().unwrap(); - apply(&tx, &owner, &ids).unwrap(); + apply_upserts(&tx, &owner, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &owner, &owner_keys).unwrap(); tx.commit().unwrap(); } @@ -1307,7 +1312,7 @@ mod tests { keys.upserts.insert((z, 0), sample_key_entry(z, 0x5B)); { let tx = conn.transaction().unwrap(); - apply(&tx, &unowned, &ids).unwrap(); + apply_upserts(&tx, &unowned, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &unowned, &keys) .expect("an unowned key on an unowned identity is legitimate"); tx.commit().unwrap(); @@ -1453,7 +1458,7 @@ mod tests { keys.upserts.insert((z, 0), sample_key_entry(z, 0x6B)); { let tx = conn.transaction().unwrap(); - apply(&tx, &unowned, &ids).unwrap(); + apply_upserts(&tx, &unowned, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &unowned, &keys).unwrap(); tx.commit().unwrap(); } @@ -1476,108 +1481,6 @@ mod tests { assert_eq!(remaining, 0, "the unowned key must actually be deleted"); } - /// `load_prekeyed` skips — never hard-errors on — an `identity_keys` - /// entry whose owner is a known-tombstoned identity: those orphaned rows - /// are the expected, self-explained fallout of a logical delete. - #[test] - fn load_prekeyed_skips_orphaned_keys_of_tombstoned_owner_in_recovery() { - use platform_wallet::changeset::IdentityKeysChangeSet; - - let mut conn = migrated_conn(); - let a = [0xA7u8; 32]; - insert_wallet(&conn, &a); - let y = Identifier::from([0x44u8; 32]); - - let mut ids = IdentityChangeSet::default(); - ids.identities - .insert(y, entry([0x44; 32], Some(a), 50, Some(0))); - let mut keys = IdentityKeysChangeSet::default(); - keys.upserts.insert((y, 0), sample_key_entry(y, 0xD4)); - { - let tx = conn.transaction().unwrap(); - apply(&tx, &a, &ids).unwrap(); - crate::sqlite::schema::identity_keys::apply(&tx, &a, &keys).unwrap(); - tx.commit().unwrap(); - } - // Tombstone Y; its key row survives as a logical-delete orphan. - let mut removed = IdentityChangeSet::default(); - removed.removed.insert(y); - apply_in_tx(&mut conn, &a, &removed); - - let strict = load_prekeyed(&conn, &a, &LoadCtx::strict()) - .expect_err("a tombstoned owner's leftover key must abort a strict load"); - assert!( - matches!(strict, WalletStorageError::OrphanedIdentityEntry { .. }), - "expected OrphanedIdentityEntry, got {strict:?}" - ); - - let state = load_prekeyed(&conn, &a, &LoadCtx::recovery()) - .expect("tombstoned-owner orphan must be skipped in recovery, not fatal"); - assert!( - state - .wallet_identities - .get(&a) - .map(|m| m.is_empty()) - .unwrap_or(true), - "tombstoned identity must not surface in the loaded state" - ); - } - - /// The same logical-delete skip, for a tombstoned UNOWNED identity. - /// - /// Not a duplicate of the wallet-owned case above: the skip depends on - /// `load_tombstoned_ids` recognising the owner as tombstoned, and that - /// query is scoped by `wallet_id`. Scoped with `=` it cannot match a - /// NULL, so an unowned tombstone is invisible, its surviving key rows - /// look like owners that vanished for no reason, and the read fails - /// with `OrphanedIdentityEntry` — the exact brick this whole line of - /// work exists to prevent, re-entering through the unowned door. The - /// NULL-safe `IS` is what closes it, and this test is what holds it - /// closed: revert that predicate to `= ?1` and this fails. - #[test] - fn load_prekeyed_skips_orphaned_keys_of_tombstoned_unowned_owner_in_recovery() { - use platform_wallet::changeset::IdentityKeysChangeSet; - - let mut conn = migrated_conn(); - let unowned = [0u8; 32]; - let z = Identifier::from([0x9Cu8; 32]); - - let mut ids = IdentityChangeSet::default(); - ids.identities.insert(z, entry([0x9C; 32], None, 30, None)); - let mut keys = IdentityKeysChangeSet::default(); - keys.upserts.insert((z, 0), sample_key_entry(z, 0x9D)); - { - let tx = conn.transaction().unwrap(); - apply(&tx, &unowned, &ids).unwrap(); - crate::sqlite::schema::identity_keys::apply(&tx, &unowned, &keys).unwrap(); - tx.commit().unwrap(); - } - - // Tombstone Z. Its key row survives — a logical delete tombstones - // the identity, it does not reap the children. - let mut removed = IdentityChangeSet::default(); - removed.removed.insert(z); - apply_in_tx(&mut conn, &unowned, &removed); - let surviving_keys: i64 = conn - .query_row( - "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1", - params![&z.to_buffer()[..]], - |row| row.get(0), - ) - .unwrap(); - assert_eq!( - surviving_keys, 1, - "the orphaned key row must actually exist, or this test proves nothing" - ); - - let state = load_prekeyed(&conn, &unowned, &LoadCtx::recovery()) - .expect("a tombstoned UNOWNED owner's orphan key must be skipped in recovery"); - assert!( - state.out_of_wallet_identities.is_empty(), - "the tombstoned identity must not surface in the loaded state" - ); - } - /// The delete's scope guard, which nothing else holds in place. /// /// `(identity_id, key_id)` is the primary key, so `wallet_id` in the @@ -1604,7 +1507,7 @@ mod tests { keys.upserts.insert((x, 0), sample_key_entry(x, 0xC4)); { let tx = conn.transaction().unwrap(); - apply(&tx, &owner, &ids).unwrap(); + apply_upserts(&tx, &owner, &ids).unwrap(); crate::sqlite::schema::identity_keys::apply(&tx, &owner, &keys).unwrap(); tx.commit().unwrap(); } @@ -1712,8 +1615,8 @@ mod tests { let blob_id = [0x02u8; 32]; // disagreeing blob let payload = blob::encode(&entry(blob_id, Some(a), 100, Some(1))).unwrap(); conn.execute( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, 1, ?3, 0)", + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, 1, ?3)", params![&typed_id[..], &a[..], payload], ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs b/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs index 4b75d740267..e985717c40b 100644 --- a/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs +++ b/packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs @@ -188,7 +188,7 @@ pub fn apply( // `(identity_id, key_id)` alone now identifies the row, so // `wallet_id = ?1` is no longer part of the key — it is kept // deliberately as a scope GUARD, mirroring the `identities` - // tombstone: one wallet's `removed` set must not delete another + // DELETE: one wallet's `removed` set must not delete another // wallet's key. Keeping it makes a cross-scope delete a no-op; // dropping it would make that delete succeed, which is a // destructive way to be permissive. diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs b/packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs index fb25a7af8c5..16d225c1813 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs @@ -110,12 +110,7 @@ const READ_ONLY_PREPARE_ALLOWED: &[(&str, &str)] = &[ // `load_state` (`SELECT identity_id, length(entry_blob)…`). ( "identities.rs", - "length(entry_blob), entry_blob, tombstoned", - ), - // load_tombstoned_ids supplies the merge's positive tombstone signal. - ( - "identities.rs", - "SELECT identity_id FROM identities WHERE", + "length(entry_blob), entry_blob", ), ("contacts.rs", "SELECT owner_id, contact_id, state"), ( diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs b/packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs index 4441f4027d1..d87421b924d 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs @@ -475,37 +475,37 @@ fn tc8_out_of_wallet_identity_loads_empty() { assert!(managed.dashpay().incoming_contact_requests().is_empty()); } -/// A tombstoned identity's orphaned key/contact rows are the one -/// orphan class recovery mode forgives. Strict refuses them: "the owner is -/// gone" is exactly the state that silently discards live key material, so -/// only an operator who asked for a best-effort load gets the old skip. +/// TC-9 — a `contacts` row whose owner identity is not loaded is the +/// orphan class recovery mode forgives. Strict refuses it: "the owner is +/// gone" is exactly the state that silently discards live contact +/// material, so only an operator who asked for a best-effort load gets +/// the skip. +/// +/// `contacts` is keyed by `owner_id` with no foreign key to `identities` +/// (only to `wallets`), which is what makes this row writable at all — a +/// removal sweeps such rows via the delete trigger, so one surviving here +/// means genuine corruption rather than routine bookkeeping. #[test] -fn tc9_tombstoned_identity_orphan_rows_load_only_in_recovery() { +fn tc9_orphaned_contact_row_loads_only_in_recovery() { let (persister, _tmp, path) = fresh_persister(); let w = wid(0xC9); ensure_wallet_meta(&persister, &w); - let id = Identifier::from([0x99; 32]); + let resident = Identifier::from([0x98; 32]); + let ghost = Identifier::from([0x99; 32]); let contact = Identifier::from([0xAB; 32]); let mut established_map = BTreeMap::new(); established_map.insert( SentContactRequestKey { - owner_id: id, + owner_id: ghost, recipient_id: contact, }, - established(id, contact), + established(ghost, contact), ); - // Store identity + key + contact. persister .store( w, PlatformWalletChangeSet { - identities: Some(id_changeset([wallet_identity_entry(id, w, 0)])), - identity_keys: Some(keys_changeset([key_entry( - id, - 0, - 0x99, - SecurityLevel::HIGH, - )])), + identities: Some(id_changeset([wallet_identity_entry(resident, w, 0)])), contacts: Some(ContactChangeSet { established: established_map, ..Default::default() @@ -514,23 +514,11 @@ fn tc9_tombstoned_identity_orphan_rows_load_only_in_recovery() { }, ) .unwrap(); - // Tombstone the identity only; its key/contact rows are left behind. - let mut removed = IdentityChangeSet::default(); - removed.removed.insert(id); - persister - .store( - w, - PlatformWalletChangeSet { - identities: Some(removed), - ..Default::default() - }, - ) - .unwrap(); drop(persister); let strict = reopen(&path) .load() - .expect_err("orphan rows of a tombstoned owner must abort a strict load"); + .expect_err("an orphaned contact row must abort a strict load"); let PersistenceError::Backend { source, .. } = strict else { panic!("expected a typed backend error, got {strict:?}"); }; @@ -554,57 +542,55 @@ fn tc9_tombstoned_identity_orphan_rows_load_only_in_recovery() { recovery .last_load_degradation() .by_site - .get(&platform_wallet_storage::LoadSite::TombstonedIdentityOrphan) + .get(&platform_wallet_storage::LoadSite::MissingIdentityOwner) .copied(), - // One per leftover row: the key row and the established-contact row. - Some(2), - "both leftover collections must be counted" + Some(1), + "the skipped row must be counted, never silently dropped" ); let im = &state.wallets[&w].identity_manager; let present = im .wallet_identities .values() .flat_map(|m| m.values()) - .any(|m| m.identity.id() == id) - || im.out_of_wallet_identities.contains_key(&id); + .any(|m| m.identity.id() == ghost) + || im.out_of_wallet_identities.contains_key(&ghost); assert!( !present, - "tombstoned identity must be absent from both buckets" + "the ghost owner must not be conjured into a bucket" ); } -/// Several leftover rows of one tombstoned owner in one collection must +/// Several orphaned rows of one missing owner in one collection must /// count several times: every other `LoadSite` counts occurrences, so a /// rescue operator reading `by_site` would otherwise be unable to tell -/// three lost keys from three lost collections. +/// three lost contacts from three lost collections. #[test] -fn tombstoned_identity_orphan_rows_are_counted_per_row() { +fn orphaned_rows_of_one_owner_are_counted_per_row() { let (persister, _tmp, path) = fresh_persister(); let w = wid(0xCA); ensure_wallet_meta(&persister, &w); - let id = Identifier::from([0x9A; 32]); - persister - .store( - w, - PlatformWalletChangeSet { - identities: Some(id_changeset([wallet_identity_entry(id, w, 0)])), - identity_keys: Some(keys_changeset([ - key_entry(id, 0, 0x91, SecurityLevel::HIGH), - key_entry(id, 1, 0x92, SecurityLevel::HIGH), - key_entry(id, 2, 0x93, SecurityLevel::HIGH), - ])), - ..Default::default() + let resident = Identifier::from([0x99; 32]); + let ghost = Identifier::from([0x9A; 32]); + let mut established_map = BTreeMap::new(); + for contact in [0xC1u8, 0xC2, 0xC3] { + let contact = Identifier::from([contact; 32]); + established_map.insert( + SentContactRequestKey { + owner_id: ghost, + recipient_id: contact, }, - ) - .unwrap(); - // Tombstone the owner; its three key rows are left behind. - let mut removed = IdentityChangeSet::default(); - removed.removed.insert(id); + established(ghost, contact), + ); + } persister .store( w, PlatformWalletChangeSet { - identities: Some(removed), + identities: Some(id_changeset([wallet_identity_entry(resident, w, 0)])), + contacts: Some(ContactChangeSet { + established: established_map, + ..Default::default() + }), ..Default::default() }, ) @@ -623,10 +609,10 @@ fn tombstoned_identity_orphan_rows_are_counted_per_row() { recovery .last_load_degradation() .by_site - .get(&platform_wallet_storage::LoadSite::TombstonedIdentityOrphan) + .get(&platform_wallet_storage::LoadSite::MissingIdentityOwner) .copied(), Some(3), - "each leftover key row must be counted" + "each orphaned contact row must be counted" ); } diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_identity_hard_delete.rs b/packages/rs-platform-wallet-storage/tests/sqlite_identity_hard_delete.rs new file mode 100644 index 00000000000..4dce03c6a06 --- /dev/null +++ b/packages/rs-platform-wallet-storage/tests/sqlite_identity_hard_delete.rs @@ -0,0 +1,1078 @@ +#![allow(clippy::field_reassign_with_default)] + +//! `IdentityChangeSet.removed` is a physical `DELETE FROM identities`. +//! +//! Every dependent row goes with it — through the native FK cascade +//! where one is live, and through `cascade_children_on_identity_delete` +//! where none is. Three shapes need the trigger rather than the FKs: +//! an out-of-wallet identity's `identity_keys` (the compound FK is +//! dormant once `wallet_id` is NULL), and `contacts` / `ignored_senders` +//! (keyed by `owner_id` with no FK to `identities` at all). + +mod common; + +use std::collections::{BTreeMap, BTreeSet}; + +use common::{ensure_wallet_meta, fresh_persister, wid}; +use common::{fresh_persister_with_mode, FlushMode}; +use dpp::identity::accessors::IdentityGettersV0; +use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; +use dpp::identity::{IdentityPublicKey, KeyType, Purpose, SecurityLevel}; +use dpp::platform_value::BinaryData; +use dpp::prelude::Identifier; +use platform_wallet::changeset::{ + ContactChangeSet, IdentityChangeSet, IdentityEntry, IdentityKeyEntry, IdentityKeysChangeSet, + PendingContactCrypto, PendingContactCryptoOp, PersistenceError, PersistenceErrorKind, + PlatformWalletChangeSet, PlatformWalletPersistence, SentContactRequestKey, + TokenBalanceChangeSet, +}; +use platform_wallet::wallet::identity::{ContactRequest, EstablishedContact, IdentityStatus}; +use platform_wallet::wallet::platform_wallet::WalletId; +use platform_wallet_storage::sqlite::migrations as mig; +use rusqlite::{params, Connection}; + +/// The all-zero scope every reader and writer maps to a NULL `wallet_id`. +const UNOWNED: WalletId = [0u8; 32]; + +fn reopen(path: &std::path::Path) -> platform_wallet_storage::SqlitePersister { + platform_wallet_storage::SqlitePersister::open( + platform_wallet_storage::SqlitePersisterConfig::new(path), + ) + .expect("reopen persister") +} + +fn iid(byte: u8) -> Identifier { + Identifier::from([byte; 32]) +} + +fn entry(id: Identifier, wallet_id: Option, index: Option) -> IdentityEntry { + IdentityEntry { + id, + balance: 1_000, + revision: 1, + identity_index: index, + last_updated_balance_block_time: None, + last_synced_keys_block_time: None, + dpns_names: Vec::new(), + contested_dpns_names: Vec::new(), + status: IdentityStatus::Active, + wallet_id, + dashpay_profile: None, + dashpay_payments: Default::default(), + contact_profiles: Default::default(), + ignored_senders: Default::default(), + } +} + +fn key_entry(id: Identifier, key_id: u32, byte: u8) -> IdentityKeyEntry { + IdentityKeyEntry { + identity_id: id, + key_id, + public_key: IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: key_id, + purpose: Purpose::AUTHENTICATION, + security_level: SecurityLevel::HIGH, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(vec![byte; 33]), + disabled_at: None, + }), + public_key_hash: [byte; 20], + wallet_id: None, + derivation_indices: None, + } +} + +fn keys_of(id: Identifier, key_id: u32, byte: u8) -> IdentityKeysChangeSet { + let mut cs = IdentityKeysChangeSet::default(); + cs.upserts.insert((id, key_id), key_entry(id, key_id, byte)); + cs +} + +fn contact_request(sender: Identifier, recipient: Identifier) -> ContactRequest { + ContactRequest { + sender_id: sender, + recipient_id: recipient, + sender_key_index: 1, + recipient_key_index: 2, + account_reference: 3, + encrypted_account_label: None, + encrypted_public_key: vec![9, 9, 9], + auto_accept_proof: None, + core_height_created_at: 42, + created_at: 7, + } +} + +/// An established pair plus an ignored sender, both owned by `owner` — +/// the two `owner_id`-keyed tables that carry no FK to `identities`. +fn contacts_of(owner: Identifier, contact: Identifier) -> ContactChangeSet { + let mut established = BTreeMap::new(); + established.insert( + SentContactRequestKey { + owner_id: owner, + recipient_id: contact, + }, + EstablishedContact { + contact_identity_id: contact, + outgoing_request: contact_request(owner, contact), + incoming_request: contact_request(contact, owner), + alias: Some("friend".into()), + note: None, + is_hidden: false, + accepted_accounts: vec![0], + payment_channel_broken: false, + contact_account_label: None, + external_account_reference: None, + }, + ); + ContactChangeSet { + established, + ignored: BTreeSet::from([(owner, contact)]), + ..Default::default() + } +} + +fn balances_of(owner: Identifier, token: Identifier) -> TokenBalanceChangeSet { + let mut cs = TokenBalanceChangeSet::default(); + cs.balances.insert((owner, token), 77); + cs +} + +fn removal_of(id: Identifier) -> IdentityChangeSet { + let mut cs = IdentityChangeSet::default(); + cs.removed.insert(id); + cs +} + +fn pending_crypto(owner: Identifier, contact: Identifier) -> PendingContactCrypto { + PendingContactCrypto { + owner_identity_id: owner, + contact_id: contact, + op: PendingContactCryptoOp::RegisterReceiving, + enqueued_at_ms: 1, + } +} + +#[test] +fn manual_re_add_preserves_new_incarnation_and_slot_reuse() { + for cycles in [1, 3] { + let (p, _tmp, path) = fresh_persister_with_mode(FlushMode::Manual); + let w = wid(0xD9); + ensure_wallet_meta(&p, &w); + let owner = iid(0x81); + let old_contact = iid(0x82); + let new_contact = iid(0x83); + let other = iid(0x84); + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + identity_keys: Some(keys_of(owner, 0, 0xAA)), + contacts: Some(contacts_of(owner, old_contact)), + pending_contact_crypto_added: vec![pending_crypto(owner, old_contact)], + ..Default::default() + }, + ) + .unwrap(); + p.flush(w).unwrap(); + + for _ in 0..cycles { + p.store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(owner)), + ..Default::default() + }, + ) + .unwrap(); + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(2)))), + identity_keys: Some(keys_of(owner, 1, 0xBB)), + contacts: Some(contacts_of(owner, new_contact)), + pending_contact_crypto_added: vec![pending_crypto(owner, new_contact)], + ..Default::default() + }, + ) + .unwrap(); + } + // The removed incarnation's old slot is available to another id. + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(other, Some(w), Some(1)))), + ..Default::default() + }, + ) + .unwrap(); + assert_eq!( + count_by_id( + &p.lock_conn_for_test(), + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1 AND key_id = 0", + owner + ), + 1, + "Manual stores must not delete the old incarnation before flush" + ); + p.flush(w).unwrap(); + drop(p); + let reopened = reopen(&path); + let state = reopened.load().unwrap(); + let identities = &state.wallets[&w].identity_manager.wallet_identities[&w]; + assert_eq!(identities[&1].identity.id(), other); + let restored = &identities[&2]; + assert_eq!(restored.identity.id(), owner); + assert_eq!( + restored + .identity + .public_keys() + .keys() + .copied() + .collect::>(), + vec![1] + ); + assert_eq!(restored.dashpay().established_contacts().len(), 1); + assert!(restored + .dashpay() + .established_contacts() + .contains_key(&new_contact)); + let conn = reopened.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM pending_contact_crypto WHERE owner_identity_id = ?1", + owner + ), + 1 + ); + let contact: Vec = conn + .query_row( + "SELECT contact_id FROM pending_contact_crypto WHERE owner_identity_id = ?1", + params![owner.as_slice()], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(contact, new_contact.as_slice()); + } +} + +#[test] +fn removal_sweeps_pending_crypto_without_touching_another_identity() { + let (p, _tmp, _path) = fresh_persister(); + let w = wid(0xDA); + ensure_wallet_meta(&p, &w); + for owner in [iid(0x91), iid(0x92)] { + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), None))), + pending_contact_crypto_added: vec![pending_crypto(owner, iid(0x93))], + ..Default::default() + }, + ) + .unwrap(); + } + p.store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(iid(0x91))), + pending_contact_crypto_added: vec![pending_crypto(iid(0x91), iid(0x94))], + ..Default::default() + }, + ) + .unwrap(); + let conn = p.lock_conn_for_test(); + let query = "SELECT COUNT(*) FROM pending_contact_crypto WHERE owner_identity_id = ?1"; + assert_eq!(count_by_id(&conn, query, iid(0x91)), 0); + assert_eq!(count_by_id(&conn, query, iid(0x92)), 1); +} + +#[test] +fn re_add_batch_rolls_back_atomically_and_survives_retry() { + for retryable in [false, true] { + let (p, _tmp, _path) = fresh_persister_with_mode(FlushMode::Manual); + let w = wid(0xDB); + let owner = iid(0x95); + ensure_wallet_meta(&p, &w); + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + identity_keys: Some(keys_of(owner, 0, 0xAA)), + ..Default::default() + }, + ) + .unwrap(); + p.flush(w).unwrap(); + p.store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(owner)), + ..Default::default() + }, + ) + .unwrap(); + p.store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(2)))), + identity_keys: Some(keys_of(owner, 1, 0xBB)), + ..Default::default() + }, + ) + .unwrap(); + if retryable { + p.force_next_flush_to_fail(platform_wallet_storage::WalletStorageError::Sqlite( + rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_BUSY), + None, + ), + )); + } else { + // Fail after the first segment has deleted the old incarnation. + p.lock_conn_for_test().execute_batch( + "CREATE TRIGGER reject_new_key BEFORE INSERT ON identity_keys WHEN NEW.key_id = 1 BEGIN SELECT RAISE(ABORT, 'injected later-segment failure'); END;" + ).unwrap(); + } + p.flush(w).expect_err("the injected error must surface"); + { + let conn = p.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1 AND identity_index = 1", + owner + ), + 1 + ); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1 AND key_id = 0", + owner + ), + 1 + ); + } + if retryable { + // New writes after restore still belong to the re-added identity. + p.store( + w, + PlatformWalletChangeSet { + identity_keys: Some(keys_of(owner, 2, 0xCC)), + ..Default::default() + }, + ) + .unwrap(); + p.flush(w).unwrap(); + let state = p.load().unwrap(); + let restored = &state.wallets[&w].identity_manager.wallet_identities[&w][&2]; + assert_eq!( + restored + .identity + .public_keys() + .keys() + .copied() + .collect::>(), + vec![1, 2] + ); + } + } +} + +#[test] +fn re_add_boundary_keeps_other_identity_snapshot_merge_semantics() { + let (p, _tmp, _path) = fresh_persister_with_mode(FlushMode::Manual); + let w = wid(0xDC); + let owner = iid(0x96); + let other = iid(0x97); + ensure_wallet_meta(&p, &w); + let mut old = entry(owner, Some(w), Some(1)); + old.revision = 100; + p.store(w, upsert_of(old).into()).unwrap(); + p.flush(w).unwrap(); + + let mut newer = entry(other, Some(w), Some(2)); + newer.revision = 100; + newer.balance = 2000; + newer.ignored_senders.insert(iid(0x98)); + p.store(w, upsert_of(newer).into()).unwrap(); + p.store(w, removal_of(owner).into()).unwrap(); + p.store(w, upsert_of(entry(owner, Some(w), Some(1))).into()) + .unwrap(); + let mut stale = entry(other, Some(w), Some(2)); + stale.revision = 99; + stale.balance = 100; + stale.ignored_senders.insert(iid(0x99)); + p.store(w, upsert_of(stale).into()).unwrap(); + p.flush(w).unwrap(); + + let conn = p.lock_conn_for_test(); + let restored = + platform_wallet_storage::sqlite::schema::identities::fetch(&conn, &w, &other.to_buffer()) + .unwrap() + .unwrap(); + assert_eq!(restored.revision, 100); + assert_eq!(restored.balance, 2000); + assert_eq!( + restored.ignored_senders, + BTreeSet::from([iid(0x98), iid(0x99)]) + ); + let readded = + platform_wallet_storage::sqlite::schema::identities::fetch(&conn, &w, &owner.to_buffer()) + .unwrap() + .unwrap(); + assert_eq!( + readded.revision, 1, + "a new incarnation does not retain the old revision gate" + ); +} + +fn upsert_of(e: IdentityEntry) -> IdentityChangeSet { + let mut cs = IdentityChangeSet::default(); + cs.identities.insert(e.id, e); + cs +} + +/// `SELECT COUNT(*)` with one 32-byte-identifier parameter. +fn count_by_id(conn: &Connection, sql: &str, id: Identifier) -> i64 { + conn.query_row(sql, params![id.as_slice()], |r| r.get(0)) + .expect("count query") +} + +// These write-only overlays are counted, not decoded, by the cleanup tests. +fn seed_dashpay_overlays(conn: &Connection, id: &[u8]) { + conn.execute( + "INSERT INTO dashpay_profiles (identity_id, profile_blob) VALUES (?1, X'00')", + params![id], + ) + .expect("seed DashPay profile"); + conn.execute( + "INSERT INTO dashpay_payments_overlay (identity_id, payment_id, overlay_blob) \ + VALUES (?1, 'payment', X'00')", + params![id], + ) + .expect("seed DashPay payment overlay"); +} + +/// Every row a removed identity owns, counted in one pass. +fn dependents_of(conn: &Connection, id: Identifier) -> Vec<(&'static str, i64)> { + [ + ( + "identities", + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", + ), + ( + "identity_keys", + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1", + ), + ( + "contacts", + "SELECT COUNT(*) FROM contacts WHERE owner_id = ?1", + ), + ( + "ignored_senders", + "SELECT COUNT(*) FROM ignored_senders WHERE owner_id = ?1", + ), + ( + "token_balances", + "SELECT COUNT(*) FROM token_balances WHERE identity_id = ?1", + ), + ( + "dashpay_profiles", + "SELECT COUNT(*) FROM dashpay_profiles WHERE identity_id = ?1", + ), + ( + "dashpay_payments_overlay", + "SELECT COUNT(*) FROM dashpay_payments_overlay WHERE identity_id = ?1", + ), + ( + "meta_identity", + "SELECT COUNT(*) FROM meta_identity WHERE identity_id = ?1", + ), + ( + "meta_token", + "SELECT COUNT(*) FROM meta_token WHERE identity_id = ?1", + ), + ] + .into_iter() + .map(|(table, sql)| (table, count_by_id(conn, sql, id))) + .collect() +} + +/// The base case: `removed` deletes the row rather than flagging it, and +/// leaves its wallet-mate alone. +#[test] +fn removed_identity_row_is_physically_deleted() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0xD0); + ensure_wallet_meta(&persister, &w); + let keep = iid(0x01); + let drop_me = iid(0x02); + + let mut both = IdentityChangeSet::default(); + both.identities.insert(keep, entry(keep, Some(w), Some(1))); + both.identities + .insert(drop_me, entry(drop_me, Some(w), Some(2))); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(both), + ..Default::default() + }, + ) + .expect("seed both identities"); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(drop_me)), + ..Default::default() + }, + ) + .expect("remove one"); + drop(persister); + + let p2 = reopen(&path); + let conn = p2.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", + drop_me + ), + 0, + "a removed identity leaves no row behind" + ); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", + keep + ), + 1, + "the wallet-mate must survive its sibling's removal" + ); +} + +/// A wallet-owned identity's whole dependent set goes with it: keys and +/// balances via live FKs, `contacts` / `ignored_senders` / `meta_*` via +/// the delete triggers. +#[test] +fn removing_a_wallet_identity_sweeps_every_dependent() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0xD1); + ensure_wallet_meta(&persister, &w); + let owner = iid(0x11); + let contact = iid(0x12); + let token = iid(0x13); + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + identity_keys: Some(keys_of(owner, 0, 0xAA)), + contacts: Some(contacts_of(owner, contact)), + token_balances: Some(balances_of(owner, token)), + ..Default::default() + }, + ) + .expect("seed identity and dependents"); + + // Metadata carries no FK at all, so seed it directly — the AFTER + // DELETE brooms are its only cleanup path. + { + let conn = persister.lock_conn_for_test(); + seed_dashpay_overlays(&conn, owner.as_slice()); + conn.execute( + "INSERT INTO meta_identity (identity_id, key, value) VALUES (?1, 'alias', X'00')", + params![owner.as_slice()], + ) + .expect("seed meta_identity"); + conn.execute( + "INSERT INTO meta_token (identity_id, token_id, key, value) \ + VALUES (?1, ?2, 'note', X'00')", + params![owner.as_slice(), token.as_slice()], + ) + .expect("seed meta_token"); + } + + { + let conn = persister.lock_conn_for_test(); + for (table, rows) in dependents_of(&conn, owner) { + assert_eq!(rows, 1, "`{table}` must be seeded before the removal"); + } + } + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(owner)), + ..Default::default() + }, + ) + .expect("remove the identity"); + drop(persister); + + let p2 = reopen(&path); + let conn = p2.lock_conn_for_test(); + for (table, rows) in dependents_of(&conn, owner) { + assert_eq!(rows, 0, "`{table}` must be swept by the identity removal"); + } +} + +/// The dormant-FK case. `identity_keys`' FK to `identities` is compound +/// (`wallet_id, identity_id`), and SQLite's MATCH SIMPLE skips +/// enforcement entirely once a child key column is NULL — so an +/// out-of-wallet identity's keys are reachable ONLY through the trigger. +#[test] +fn removing_an_out_of_wallet_identity_deletes_its_keys() { + let (persister, _tmp, path) = fresh_persister(); + let id = iid(0x7A); + + persister + .store( + UNOWNED, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(id, None, None))), + identity_keys: Some(keys_of(id, 0, 0xAB)), + ..Default::default() + }, + ) + .expect("seed unowned identity with a key"); + + { + let conn = persister.lock_conn_for_test(); + let null_scoped: i64 = conn + .query_row( + "SELECT COUNT(*) FROM identity_keys \ + WHERE identity_id = ?1 AND wallet_id IS NULL", + params![id.as_slice()], + |r| r.get(0), + ) + .expect("count null-scoped keys"); + assert_eq!( + null_scoped, 1, + "the key must be NULL-scoped, else this test is not exercising \ + the dormant FK at all" + ); + } + + persister + .store( + UNOWNED, + PlatformWalletChangeSet { + identities: Some(removal_of(id)), + ..Default::default() + }, + ) + .expect("remove the unowned identity"); + drop(persister); + + let p2 = reopen(&path); + let conn = p2.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", + id + ), + 0, + "the unowned identity row must be gone" + ); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1", + id + ), + 0, + "the dormant compound FK cannot cascade a NULL-scoped key; the \ + AFTER DELETE trigger must" + ); +} + +/// A merged buffer can carry `removed` for an identity alongside key / +/// contact / balance upserts for that same identity — sync writes its +/// keys, the host then removes it, both land in one flush. The removal +/// must run AFTER every identity-scoped child writer: doing it first +/// pulls the FK parent out from under those inserts and fails the whole +/// flush. +#[test] +fn removal_wins_over_child_writes_in_one_changeset() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0xD2); + ensure_wallet_meta(&persister, &w); + let owner = iid(0x21); + let contact = iid(0x22); + let token = iid(0x23); + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + ..Default::default() + }, + ) + .expect("seed the identity"); + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(owner)), + identity_keys: Some(keys_of(owner, 0, 0xCC)), + contacts: Some(contacts_of(owner, contact)), + token_balances: Some(balances_of(owner, token)), + ..Default::default() + }, + ) + .expect("a removal alongside child writes must commit, not FK-fail"); + drop(persister); + + let p2 = reopen(&path); + let conn = p2.lock_conn_for_test(); + for (table, rows) in dependents_of(&conn, owner) { + assert_eq!( + rows, 0, + "`{table}`: the removal must win over same-changeset child writes" + ); + } +} + +/// Removal is terminal, not reversible: re-adding the same identity id +/// gets a blank identity, never its old keys / contacts / balances back. +/// This is what the in-memory `IdentityManager` already does — it drops +/// the whole `ManagedIdentity` — so storage and memory agree. +#[test] +fn re_added_identity_starts_from_zero() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0xD3); + ensure_wallet_meta(&persister, &w); + let owner = iid(0x31); + let contact = iid(0x32); + let token = iid(0x33); + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + identity_keys: Some(keys_of(owner, 0, 0xAA)), + contacts: Some(contacts_of(owner, contact)), + token_balances: Some(balances_of(owner, token)), + ..Default::default() + }, + ) + .expect("seed identity and dependents"); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(owner)), + ..Default::default() + }, + ) + .expect("remove"); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(owner, Some(w), Some(1)))), + ..Default::default() + }, + ) + .expect("re-add the same identity id"); + drop(persister); + + let p2 = reopen(&path); + let state = p2.load().expect("load after re-add"); + let managed = &state.wallets[&w].identity_manager.wallet_identities[&w][&1]; + assert_eq!(managed.identity.id(), owner, "the identity is back"); + assert!( + managed.identity.public_keys().is_empty(), + "a re-added identity must not inherit the removed one's keys" + ); + assert!( + managed.dashpay().established_contacts().is_empty(), + "a re-added identity must not inherit the removed one's contacts" + ); + + let conn = p2.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM token_balances WHERE identity_id = ?1", + owner + ), + 0, + "a re-added identity must not inherit the removed one's balances" + ); +} + +/// The wallet stays loadable under the STRICT policy after a removal. +/// Leftover `identity_keys` / `contacts` rows would surface as +/// `OrphanedIdentityEntry` and take the whole wallet's load down with +/// them, turning an ordinary `remove_identity` into a bricked wallet. +#[test] +fn strict_load_survives_a_removed_identity() { + let (persister, _tmp, path) = fresh_persister(); + let w = wid(0xD4); + ensure_wallet_meta(&persister, &w); + let removed = iid(0x41); + let contact = iid(0x42); + let survivor = iid(0x43); + + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(removed, Some(w), Some(1)))), + identity_keys: Some(keys_of(removed, 0, 0x99)), + contacts: Some(contacts_of(removed, contact)), + ..Default::default() + }, + ) + .expect("seed the identity that will be removed"); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(survivor, Some(w), Some(2)))), + ..Default::default() + }, + ) + .expect("seed the survivor"); + persister + .store( + w, + PlatformWalletChangeSet { + identities: Some(removal_of(removed)), + ..Default::default() + }, + ) + .expect("remove"); + drop(persister); + + let p2 = reopen(&path); + let state = p2.load().expect("a strict load must survive a removal"); + assert!( + !p2.last_load_degradation().degraded, + "an ordinary removal is not a degraded load" + ); + let bucket = &state.wallets[&w].identity_manager.wallet_identities[&w]; + assert_eq!(bucket.len(), 1, "only the survivor remains"); + assert_eq!(bucket[&2].identity.id(), survivor); +} + +/// The seam between V016 and V018: an `identity_keys` orphan can be +/// neither written nor left behind. +/// +/// V016's NULL-scope trigger refuses a key naming an identity that is not +/// there, closing the only door MATCH SIMPLE's dormant compound FK left +/// open on the write side. V018's delete broom closes the read side, by +/// sweeping such keys when their identity goes. Either migration alone +/// leaves `identity_keys` able to orphan; this pins that together they +/// do not, so `MissingIdentityOwner` is reachable only through the +/// FK-less `contacts` / `ignored_senders` tables (covered by +/// `sqlite_contacts_keys_rehydration`). +#[test] +fn null_scoped_key_can_neither_name_a_missing_identity_nor_outlive_one() { + let (persister, _tmp, _path) = fresh_persister(); + let ghost = iid(0x6B); + + // Write side (V018): the key is refused, and nothing lands. + let err = persister + .store( + UNOWNED, + PlatformWalletChangeSet { + identity_keys: Some(keys_of(ghost, 0, 0x6C)), + ..Default::default() + }, + ) + .expect_err("a NULL-scoped key naming no identity must be refused"); + let PersistenceError::Backend { kind, .. } = &err else { + panic!("expected a typed backend error, got {err:?}"); + }; + assert_eq!( + *kind, + PersistenceErrorKind::Constraint, + "a guard trigger firing is an integrity violation, not an engine fault" + ); + { + let conn = persister.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1", + ghost + ), + 0, + "the refused key must not have reached disk" + ); + } + + // Read side (V018): the same row, written legitimately under a live + // identity, does not survive that identity's removal. + persister + .store( + UNOWNED, + PlatformWalletChangeSet { + identities: Some(upsert_of(entry(ghost, None, None))), + identity_keys: Some(keys_of(ghost, 0, 0x6C)), + ..Default::default() + }, + ) + .expect("the same key is legal once its identity exists"); + persister + .store( + UNOWNED, + PlatformWalletChangeSet { + identities: Some(removal_of(ghost)), + ..Default::default() + }, + ) + .expect("remove the unowned identity"); + + let conn = persister.lock_conn_for_test(); + assert_eq!( + count_by_id( + &conn, + "SELECT COUNT(*) FROM identity_keys WHERE identity_id = ?1", + ghost + ), + 0, + "the removal must sweep the key the dormant FK cannot cascade" + ); +} + +/// V018 retires the `tombstoned` column, and purges the rows and +/// dependents a pre-V018 database had already logically deleted. +#[test] +fn v018_purges_tombstoned_rows_and_retires_the_column() { + assert_v018_purges_tombstones(true); +} + +#[test] +fn v018_purges_tombstoned_rows_with_foreign_keys_disabled() { + assert_v018_purges_tombstones(false); +} + +fn assert_v018_purges_tombstones(foreign_keys: bool) { + let mut conn = Connection::open_in_memory().expect("open in-memory db"); + conn.pragma_update(None, "foreign_keys", foreign_keys) + .expect("configure foreign keys"); + + // Stand the database up at every migration BEFORE this one, so the + // fixture is written against the schema V018 has to upgrade. + mig::runner() + .set_target(refinery::Target::Version(17)) + .run(&mut conn) + .expect("migrate to the last pre-V018 schema"); + + let w = [0x5Au8; 32]; + let tombstoned = [0x51u8; 32]; + let live = [0x52u8; 32]; + let contact = [0x53u8; 32]; + let token = [0x54u8; 32]; + conn.execute( + "INSERT INTO wallets (wallet_id, network, birth_height) VALUES (?1, 'testnet', 0)", + params![w.as_slice()], + ) + .expect("insert wallet"); + for (id, flag) in [(tombstoned, 1), (live, 0)] { + conn.execute( + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ + VALUES (?1, ?2, NULL, X'00', ?3)", + params![id.as_slice(), w.as_slice(), flag], + ) + .expect("insert identity"); + seed_dashpay_overlays(&conn, id.as_slice()); + conn.execute( + "INSERT INTO identity_keys \ + (wallet_id, identity_id, key_id, public_key_blob, public_key_hash) \ + VALUES (?1, ?2, 0, X'00', X'00')", + params![w.as_slice(), id.as_slice()], + ) + .expect("insert key"); + conn.execute( + "INSERT INTO contacts (wallet_id, owner_id, contact_id, state) \ + VALUES (?1, ?2, ?3, 'established')", + params![w.as_slice(), id.as_slice(), contact.as_slice()], + ) + .expect("insert contact"); + conn.execute( + "INSERT INTO ignored_senders (wallet_id, owner_id, sender_id) VALUES (?1, ?2, ?3)", + params![w.as_slice(), id.as_slice(), contact.as_slice()], + ) + .expect("insert ignored sender"); + conn.execute( + "INSERT INTO pending_contact_crypto (wallet_id, owner_identity_id, contact_id, kind, payload, enqueued_at_ms) VALUES (?1, ?2, ?3, 'register_receiving', X'00', 1)", + params![w.as_slice(), id.as_slice(), contact.as_slice()], + ).expect("insert pending crypto"); + conn.execute( + "INSERT INTO token_balances (identity_id, token_id, balance, updated_at) \ + VALUES (?1, ?2, 1, 0)", + params![id.as_slice(), token.as_slice()], + ) + .expect("insert balance"); + conn.execute( + "INSERT INTO meta_identity (identity_id, key, value) VALUES (?1, 'alias', X'00')", + params![id.as_slice()], + ) + .expect("insert meta_identity"); + conn.execute( + "INSERT INTO meta_token (identity_id, token_id, key, value) \ + VALUES (?1, ?2, 'note', X'00')", + params![id.as_slice(), token.as_slice()], + ) + .expect("insert meta_token"); + } + + assert_eq!( + conn.pragma_query_value(None, "foreign_keys", |row| row.get::<_, bool>(0)) + .expect("read foreign key setting"), + foreign_keys, + "V018 must run with the requested foreign key setting" + ); + for id in [tombstoned, live] { + for (table, rows) in dependents_of(&conn, Identifier::from(id)) { + assert_eq!(rows, 1, "`{table}` must be seeded before migration"); + } + } + mig::run(&mut conn).expect("migrate to the newest version"); + + let columns: Vec = { + let mut stmt = conn + .prepare("PRAGMA table_info(identities)") + .expect("prepare table_info"); + let rows = stmt + .query_map([], |r| r.get::<_, String>(1)) + .expect("query table_info"); + rows.map(|r| r.expect("column name")).collect() + }; + assert!( + !columns.contains(&"tombstoned".to_string()), + "V018 must drop the tombstoned column, got {columns:?}" + ); + + for (table, purged) in dependents_of(&conn, Identifier::from(tombstoned)) { + assert_eq!( + purged, 0, + "`{table}` must be purged for a tombstoned identity" + ); + } + for (table, kept) in dependents_of(&conn, Identifier::from(live)) { + assert_eq!(kept, 1, "`{table}` must be untouched for a live identity"); + } + let sql = "SELECT COUNT(*) FROM pending_contact_crypto WHERE owner_identity_id = ?1"; + assert_eq!(count_by_id(&conn, sql, Identifier::from(tombstoned)), 0); + assert_eq!(count_by_id(&conn, sql, Identifier::from(live)), 1); +} diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs b/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs index ecf3b29fa85..18f125498bd 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_concurrency.rs @@ -93,7 +93,7 @@ fn live_occupant(p: &SqlitePersister, wallet_id: &WalletId, index: u32) -> Optio let conn = p.lock_conn_for_test(); conn.query_row( "SELECT identity_id FROM identities \ - WHERE wallet_id IS ?1 AND identity_index = ?2 AND tombstoned = 0", + WHERE wallet_id IS ?1 AND identity_index = ?2", params![wallet_id.as_slice(), i64::from(index)], |row| row.get::<_, Vec>(0), ) diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_uniqueness.rs b/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_uniqueness.rs index 8b113b45475..15ebc2fd157 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_uniqueness.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_identity_index_uniqueness.rs @@ -85,7 +85,7 @@ fn backend_kind(err: &PersistenceError) -> PersistenceErrorKind { } } -/// Live (non-tombstoned) occupant of `(wallet_id, index)`, if any. +/// Occupant of `(wallet_id, index)`, if any. fn live_occupant(p: &SqlitePersister, wallet_id: &WalletId, index: u32) -> Option<[u8; 32]> { let conn = p.lock_conn_for_test(); let wid_param: Option<&[u8]> = if *wallet_id == SENTINEL { @@ -95,7 +95,7 @@ fn live_occupant(p: &SqlitePersister, wallet_id: &WalletId, index: u32) -> Optio }; conn.query_row( "SELECT identity_id FROM identities \ - WHERE wallet_id IS ?1 AND identity_index = ?2 AND tombstoned = 0", + WHERE wallet_id IS ?1 AND identity_index = ?2", params![wid_param, i64::from(index)], |row| row.get::<_, Vec>(0), ) @@ -104,17 +104,18 @@ fn live_occupant(p: &SqlitePersister, wallet_id: &WalletId, index: u32) -> Optio .map(|raw| raw.try_into().expect("32-byte identity_id")) } -/// `Some(tombstoned)` when the row exists at all. -fn row_tombstoned(p: &SqlitePersister, id: &Identifier) -> Option { +/// Whether the identity has a row on disk. A removal deletes it, so +/// "removed" and "never written" are the same observable state. +fn row_exists(p: &SqlitePersister, id: &Identifier) -> bool { let conn = p.lock_conn_for_test(); conn.query_row( - "SELECT tombstoned FROM identities WHERE identity_id = ?1", + "SELECT 1 FROM identities WHERE identity_id = ?1", params![id.as_slice()], - |row| row.get::<_, i64>(0), + |_| Ok(()), ) .optional() .expect("query identity row") - .map(|t| t != 0) + .is_some() } /// Claim a slot by writing an `identities` row straight to the DB — @@ -126,8 +127,8 @@ fn row_tombstoned(p: &SqlitePersister, id: &Identifier) -> Option { fn peer_claims_slot(p: &SqlitePersister, wallet_id: &WalletId, id: &Identifier, index: u32) { let conn = p.lock_conn_for_test(); conn.execute( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, ?3, X'00', 0)", + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, ?3, X'00')", params![id.as_slice(), wallet_id.as_slice(), i64::from(index)], ) .expect("peer claims slot"); @@ -171,9 +172,8 @@ fn duplicate_index_is_rejected_at_write_time() { Some([0x01; 32]), "the resident identity must keep its slot" ); - assert_eq!( - row_tombstoned(&p, &iid(0x02)), - None, + assert!( + !row_exists(&p, &iid(0x02)), "the rejected identity must not reach disk at all" ); } @@ -238,7 +238,7 @@ fn rejected_duplicate_does_not_swallow_a_staged_changeset() { Some([0x03; 32]), "the staged write survived the rejection" ); - assert_eq!(row_tombstoned(&p, &iid(0x02)), None); + assert!(!row_exists(&p, &iid(0x02))); } /// The probe keys on the FLUSH SCOPE, not on the incoming identity's own @@ -263,10 +263,10 @@ fn probe_keys_on_the_flush_scope_not_the_stored_wallet_id() { assert_eq!(live_occupant(&p, &w, 1), Some([0x01; 32])); } -/// A tombstoned row holds no slot: remove-then-re-register at the same +/// A removed row holds no slot: remove-then-re-register at the same /// index is legitimate reuse, not a collision. #[test] -fn tombstoned_row_frees_its_slot_for_reuse() { +fn removed_row_frees_its_slot_for_reuse() { let (p, _tmp, _path) = fresh_persister(); let w = wid(0xF1); ensure_wallet_meta(&p, &w); @@ -278,15 +278,15 @@ fn tombstoned_row_frees_its_slot_for_reuse() { p.store(w, identity_cs([identity_entry(0x02, Some(1))], [])) .expect("the freed slot must be reusable"); - assert_eq!(row_tombstoned(&p, &iid(0x01)), Some(true)); + assert!(!row_exists(&p, &iid(0x01))); assert_eq!(live_occupant(&p, &w, 1), Some([0x02; 32])); } /// Removal and re-registration merged into ONE changeset has a legal -/// final state (the tombstone lands in the same transaction), so the -/// probe must treat a removed id as holding no slot. +/// final state (the delete lands in the same transaction), so the probe +/// must treat a removed id as holding no slot. #[test] -fn tombstone_and_reinsert_in_one_changeset_is_accepted() { +fn removal_and_reinsert_in_one_changeset_is_accepted() { let (p, _tmp, _path) = fresh_persister(); let w = wid(0xF2); ensure_wallet_meta(&p, &w); @@ -294,9 +294,9 @@ fn tombstone_and_reinsert_in_one_changeset_is_accepted() { .expect("first identity at index 1"); p.store(w, identity_cs([identity_entry(0x02, Some(1))], [iid(0x01)])) - .expect("tombstone + reinsert at the same index is legal"); + .expect("removal + reinsert at the same index is legal"); - assert_eq!(row_tombstoned(&p, &iid(0x01)), Some(true)); + assert!(!row_exists(&p, &iid(0x01))); assert_eq!(live_occupant(&p, &w, 1), Some([0x02; 32])); } @@ -319,8 +319,8 @@ fn colliding_entries_in_one_changeset_are_rejected_without_picking_a_winner() { .expect_err("two identities cannot share one slot"); assert_index_conflict(&err); - assert_eq!(row_tombstoned(&p, &iid(0x01)), None); - assert_eq!(row_tombstoned(&p, &iid(0x02)), None); + assert!(!row_exists(&p, &iid(0x01))); + assert!(!row_exists(&p, &iid(0x02))); assert_eq!(live_occupant(&p, &w, 1), None); } @@ -364,7 +364,7 @@ fn reupserting_the_same_identity_at_its_own_index_is_accepted() { /// An occupant that the SAME changeset moves to another index has /// vacated its old slot by the time the changeset lands, exactly like -/// one it tombstones. Judging the final state, not the starting one, is +/// one it removes. Judging the final state, not the starting one, is /// what makes the guard a uniqueness rule rather than a freeze. #[test] fn an_occupant_reindexed_in_the_same_changeset_frees_its_old_slot() { @@ -437,9 +437,8 @@ fn an_occupant_losing_its_index_in_the_same_changeset_frees_its_slot() { .expect("A gives up its index in the same changeset that B claims it"); assert_eq!(live_occupant(&p, &w, 1), Some([0x02; 32])); - assert_eq!( - row_tombstoned(&p, &iid(0x01)), - Some(false), + assert!( + row_exists(&p, &iid(0x01)), "A is still a live row, just no longer in a wallet slot" ); } @@ -462,7 +461,7 @@ fn walletless_identity_carrying_an_index_is_rejected() { ); assert!(!typed.is_transient()); assert_eq!(typed.persistence_kind(), PersistenceErrorKind::Constraint); - assert_eq!(row_tombstoned(&p, &iid(0x01)), None); + assert!(!row_exists(&p, &iid(0x01))); } /// The companion positive case: a wallet-less identity WITHOUT an index @@ -474,7 +473,7 @@ fn walletless_identity_without_an_index_is_accepted() { p.store(SENTINEL, identity_cs([identity_entry(0x01, None)], [])) .expect("wallet-less identities are first-class"); - assert_eq!(row_tombstoned(&p, &iid(0x01)), Some(false)); + assert!(row_exists(&p, &iid(0x01))); } /// A store checks disk and buffer as they are at that instant. A @@ -499,7 +498,7 @@ fn flush_rejects_a_duplicate_that_a_peer_created_under_the_buffer() { Some([0x01; 32]), "the whole transaction rolls back — the peer's row stands" ); - assert_eq!(row_tombstoned(&p, &iid(0x02)), None); + assert!(!row_exists(&p, &iid(0x02))); // A fatal flush failure drops that wallet's buffer rather than // restoring an unflushable changeset, so the retry is a no-op @@ -533,17 +532,16 @@ fn a_slot_held_by_a_buffered_write_refuses_a_second_claimant() { Some([0x01; 32]), "the first caller's write is untouched by the rejection" ); - assert_eq!( - row_tombstoned(&p, &iid(0x02)), - None, + assert!( + !row_exists(&p, &iid(0x02)), "the rejected changeset never reached the buffer" ); } /// A buffered removal frees the slot it names: the check reads the -/// merged view exactly as the flush will apply it, and `apply` inserts -/// before it tombstones. Reclaiming the slot in a later store is -/// legitimate reuse, not a collision. +/// merged view exactly as the flush will apply it, and the flush inserts +/// before it deletes. Reclaiming the slot in a later store is legitimate +/// reuse, not a collision. #[test] fn a_buffered_removal_frees_its_slot_for_a_later_store() { let (p, _tmp, _path) = fresh_persister_with_mode(FlushMode::Manual); @@ -557,7 +555,7 @@ fn a_buffered_removal_frees_its_slot_for_a_later_store() { p.flush(w).expect("the merged changeset is consistent"); assert_eq!(live_occupant(&p, &w, 1), Some([0x02; 32])); - assert_eq!(row_tombstoned(&p, &iid(0x01)), Some(true)); + assert!(!row_exists(&p, &iid(0x01))); } /// One wallet's rejected flush is one wallet's problem: `commit_writes` @@ -612,10 +610,9 @@ fn delete_wallet_proceeds_despite_unpersistable_pending_writes() { .expect("an unflushable buffer must not block the delete"); assert_eq!(report.wallet_id, w); - assert_eq!(row_tombstoned(&p, &iid(0x01)), None); - assert_eq!( - row_tombstoned(&p, &iid(0x02)), - None, + assert!(!row_exists(&p, &iid(0x01))); + assert!( + !row_exists(&p, &iid(0x02)), "the peer's row went with the wallet's cascade" ); let conn = p.lock_conn_for_test(); diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs b/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs index 211f9dbb186..e341aa0df0d 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs @@ -73,8 +73,8 @@ fn tc027_smoke_insert_every_table() { .unwrap(); let identity_id = [7u8; 32]; conn.execute( - "INSERT INTO identities (wallet_id, identity_index, identity_id, entry_blob, tombstoned) \ - VALUES (?1, NULL, ?2, X'01', 0)", + "INSERT INTO identities (wallet_id, identity_index, identity_id, entry_blob) \ + VALUES (?1, NULL, ?2, X'01')", params![wallet_id.as_slice(), identity_id.as_slice()], ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs b/packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs index 0d24aafe2b4..6cd617efe19 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs @@ -613,8 +613,8 @@ fn meta_identity_cleanup_fires_on_wallet_cascade() { ) .unwrap(); conn.execute( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, NULL, X'00', 0)", + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, NULL, X'00')", params![&idy[..], &w[..]], ) .unwrap(); @@ -682,8 +682,8 @@ fn meta_token_cleanup_fires_on_wallet_cascade_two_hops() { ) .unwrap(); conn.execute( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, NULL, X'00', 0)", + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, NULL, X'00')", params![&idy[..], &w[..]], ) .unwrap(); diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rs b/packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_removal.rs similarity index 67% rename from packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rs rename to packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_removal.rs index 98260e6e44a..4ac407883b2 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_qa_identity_removal.rs @@ -1,12 +1,12 @@ #![allow(clippy::field_reassign_with_default)] -//! Write-path coverage for the `IdentityChangeSet.removed` tombstone -//! branch. The tombstone runs a wallet-scoped, NULL-safe -//! `UPDATE identities SET tombstoned = 1 WHERE identity_id = ?1 AND -//! wallet_id IS ?2`, mirroring the upsert's per-entry wallet cross-check. -//! These tests pin that a tombstoned identity is excluded from the -//! per-wallet `load_state` and that a foreign wallet's `removed` set -//! cannot tombstone this wallet's identity. +//! Write-path coverage for the `IdentityChangeSet.removed` branch. The +//! removal runs a wallet-scoped, NULL-safe `DELETE FROM identities WHERE +//! identity_id = ?1 AND wallet_id IS ?2`, mirroring the upsert's +//! per-entry wallet cross-check. These tests pin that a removed identity +//! leaves nothing behind for `load_state` to see, that re-adding its id +//! starts from a blank identity, and that a foreign wallet's `removed` +//! set cannot reach this wallet's identity. mod common; @@ -53,11 +53,11 @@ fn entry_for(id: u8, wallet_id: [u8; 32]) -> IdentityEntry { } } -/// An identity routed through `IdentityChangeSet.removed` is tombstoned -/// and disappears from the per-wallet `load_state` while a sibling, +/// An identity routed through `IdentityChangeSet.removed` is deleted and +/// disappears from the per-wallet `load_state` while a sibling, /// non-removed identity survives. #[test] -fn qa_tomb1_removed_identity_excluded_from_load() { +fn qa_rm1_removed_identity_excluded_from_load() { let (persister, _tmp, path) = fresh_persister(); let w = wid(0xD0); ensure_wallet_meta(&persister, &w); @@ -82,7 +82,7 @@ fn qa_tomb1_removed_identity_excluded_from_load() { ) .unwrap(); - // Second flush: tombstone drop_me. + // Second flush: remove drop_me. let mut removed: BTreeSet = BTreeSet::new(); removed.insert(drop_me.id); persister @@ -102,7 +102,7 @@ fn qa_tomb1_removed_identity_excluded_from_load() { let p2 = reopen(&path); let conn = p2.lock_conn_for_test(); - // The tombstoned row is still physically present (logical delete). + // The removed row is physically gone; only the survivor remains. let total: i64 = conn .query_row( "SELECT COUNT(*) FROM identities WHERE wallet_id = ?1", @@ -110,25 +110,16 @@ fn qa_tomb1_removed_identity_excluded_from_load() { |r| r.get(0), ) .unwrap(); - assert_eq!(total, 2, "tombstone is a logical delete; row stays on disk"); + assert_eq!(total, 1, "a removal deletes the row, it does not flag it"); - let tombstoned: i64 = conn - .query_row( - "SELECT COUNT(*) FROM identities WHERE wallet_id = ?1 AND tombstoned = 1", - rusqlite::params![w.as_slice()], - |r| r.get(0), - ) - .unwrap(); - assert_eq!(tombstoned, 1, "exactly one identity tombstoned"); - - // load_state must skip the tombstoned identity and keep the other. + // load_state must surface only the survivor. let state = identities::load_state(&conn, &w).unwrap(); drop(conn); let wallet_idents = state.wallet_identities.get(&w).expect("wallet bucket"); assert_eq!( wallet_idents.len(), 1, - "load_state must surface only the non-tombstoned identity" + "load_state must surface only the surviving identity" ); let surviving_ids: Vec = wallet_idents.values().map(|m| m.identity.id()).collect(); assert!( @@ -137,14 +128,17 @@ fn qa_tomb1_removed_identity_excluded_from_load() { ); assert!( !surviving_ids.contains(&drop_me.id), - "tombstoned identity must NOT appear in load" + "removed identity must NOT appear in load" ); } -/// Re-upserting a tombstoned identity clears the tombstone (the upsert -/// sets `tombstoned = 0`) — the resurrection path the writer relies on. +/// Re-adding a removed identity id is legal and idempotent — the upsert +/// simply finds no conflicting row — but it starts from a blank +/// identity, never the removed one's state. The in-memory +/// `IdentityManager` drops the whole `ManagedIdentity` on removal, so +/// this is what keeps storage and memory agreeing. #[test] -fn qa_tomb2_reupsert_clears_tombstone() { +fn qa_rm2_re_add_after_removal_is_a_fresh_row() { let (persister, _tmp, path) = fresh_persister(); let w = wid(0xD1); ensure_wallet_meta(&persister, &w); @@ -180,13 +174,34 @@ fn qa_tomb2_reupsert_clears_tombstone() { ) .unwrap(); - // Re-upsert resurrects. + // The removal is observable between the two writes: nothing is left + // for the re-add to inherit. + { + let conn = persister.lock_conn_for_test(); + let rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", + rusqlite::params![e.id.as_slice()], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(rows, 0, "the removal deleted the row"); + } + + // Re-add the same id with a different balance: a plain insert now, + // not an update of a surviving row. + let re_added = IdentityEntry { + balance: 9_999, + ..e.clone() + }; + let mut re_add: BTreeMap = BTreeMap::new(); + re_add.insert(re_added.id, re_added); persister .store( w, PlatformWalletChangeSet { identities: Some(IdentityChangeSet { - identities: idents, + identities: re_add, removed: Default::default(), }), ..Default::default() @@ -197,34 +212,29 @@ fn qa_tomb2_reupsert_clears_tombstone() { let p2 = reopen(&path); let conn = p2.lock_conn_for_test(); - let tombstoned: i64 = conn - .query_row( - "SELECT tombstoned FROM identities WHERE identity_id = ?1", - rusqlite::params![e.id.as_slice()], - |r| r.get(0), - ) - .unwrap(); let state = identities::load_state(&conn, &w).unwrap(); drop(conn); - assert_eq!(tombstoned, 0, "re-upsert must clear the tombstone flag"); + let bucket = state.wallet_identities.get(&w).expect("wallet bucket"); + assert_eq!(bucket.len(), 1, "the re-added identity is loadable again"); assert_eq!( - state - .wallet_identities - .get(&w) - .map(|m| m.len()) - .unwrap_or(0), - 1, - "resurrected identity must reappear in load" + bucket + .values() + .next() + .expect("one identity") + .identity + .balance(), + 9_999, + "the re-added blob wins; no trace of the removed one survives" ); } -/// The tombstone UPDATE is scoped by `wallet_id`: a `removed` entry -/// naming an identity parented to a different wallet is a no-op against -/// that wallet's row (NULL-safe `wallet_id IS ?2` predicate). An -/// identity_id is globally unique to one wallet, so this is -/// defense-in-depth enforcing the isolation the data model assumes. +/// The removal DELETE is scoped by `wallet_id`: a `removed` entry naming +/// an identity parented to a different wallet is a no-op against that +/// wallet's row (NULL-safe `wallet_id IS ?2` predicate). An identity_id +/// is globally unique to one wallet, so this is defense-in-depth +/// enforcing the isolation the data model assumes. #[test] -fn qa_tomb3_tombstone_update_is_wallet_scoped() { +fn qa_rm3_removal_is_wallet_scoped() { let (persister, _tmp, path) = fresh_persister(); let wa = wid(0xE0); let wb = wid(0xE1); @@ -267,9 +277,9 @@ fn qa_tomb3_tombstone_update_is_wallet_scoped() { let p2 = reopen(&path); let conn = p2.lock_conn_for_test(); - let tombstoned: i64 = conn + let rows: i64 = conn .query_row( - "SELECT tombstoned FROM identities WHERE identity_id = ?1", + "SELECT COUNT(*) FROM identities WHERE identity_id = ?1", rusqlite::params![b_ident.id.as_slice()], |r| r.get(0), ) @@ -278,11 +288,11 @@ fn qa_tomb3_tombstone_update_is_wallet_scoped() { drop(conn); // Cross-wallet isolation: wallet A's `removed` set names wallet B's - // identity, but the wallet-scoped tombstone UPDATE leaves B's row - // untouched, so B's load still surfaces the identity. + // identity, but the wallet-scoped DELETE leaves B's row untouched, + // so B's load still surfaces the identity. assert_eq!( - tombstoned, 0, - "wallet-scoped tombstone: A's removed set must NOT affect B's identity" + rows, 1, + "wallet-scoped removal: A's removed set must NOT delete B's identity" ); assert_eq!( b_state @@ -291,6 +301,6 @@ fn qa_tomb3_tombstone_update_is_wallet_scoped() { .map(|m| m.len()) .unwrap_or(0), 1, - "B's identity must survive A's unrelated tombstone" + "B's identity must survive A's unrelated removal" ); } diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_recovery_mode.rs b/packages/rs-platform-wallet-storage/tests/sqlite_recovery_mode.rs index cd4587affd6..3bb4c2a5fea 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_recovery_mode.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_recovery_mode.rs @@ -214,8 +214,8 @@ fn seed_identity_index_collision(persister: &SqlitePersister, wallet_id: WalletI let entry = identity_entry(wallet_id, id, 7); conn.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, 7, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, 7, ?3)", params![ entry.id.as_slice(), wallet_id.as_slice(), @@ -899,8 +899,8 @@ fn seed_self_contradictory_unowned_identity(persister: &SqlitePersister, identit let payload = blob::encode(&entry).expect("encode unowned identity entry"); let conn = persister.lock_conn_for_test(); conn.execute( - "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, NULL, 4, ?2, 0)", + "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, NULL, 4, ?2)", params![id.as_slice(), payload], ) .expect("seed unowned identity carrying a registration index"); @@ -1546,8 +1546,8 @@ fn an_unreadable_identity_row_costs_its_wallet_not_just_the_identity() { let entry = identity_entry(sick, 0x99, 0); conn.execute( "INSERT INTO identities \ - (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ - VALUES (?1, ?2, 0, ?3, 0)", + (identity_id, wallet_id, identity_index, entry_blob) \ + VALUES (?1, ?2, 0, ?3)", params![ [0x58_u8; 32].as_slice(), sick.as_slice(), diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs b/packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs index f295b75e859..282a2f5abf1 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs @@ -15,13 +15,13 @@ use platform_wallet_storage::sqlite::{migrations as mig, schema::versions::Domai /// Golden `(version, name)` fingerprint of the frozen migration set. Bump /// deliberately only when adding/removing/renaming a migration file. const EXPECTED_ID_FINGERPRINT: &str = - "ee4e0c3efe48cab68bd25897267ae66892675fb4b2720cfc31abbbb6c8ca553f"; + "91f2fab573900a41066b8d237a29b83ca94b2fc730702389ab9c088271da522f"; /// Golden content-level fingerprint over every migration's rendered SQL. /// Bump it only when ADDING a migration file; a body change on an already /// applied migration is a defect, not a golden to refresh. const EXPECTED_SQL_FINGERPRINT: &str = - "7f74dadd8095b794d5ba5eb8aa74204666f028a4fea912c99e3f57f50b9d4bc8"; + "0dbcfb2ab8d8362a206c0ab948051028c7eafc640861a823c4c50f13b0602be8"; /// The migrations merged `v4.2-dev` already ships. Refinery keys /// `refinery_schema_history` by version and validates an applied migration's diff --git a/packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs b/packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs index e2c438f0649..5778b2694a0 100644 --- a/packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs +++ b/packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs @@ -27,8 +27,8 @@ fn native_fk_rejects_orphan_child() { let (persister, _tmp, _path) = fresh_persister(); let conn = persister.lock_conn_for_test(); let res = conn.execute( - "INSERT INTO identities (wallet_id, identity_index, identity_id, entry_blob, tombstoned) \ - VALUES (?1, NULL, ?2, X'00', 0)", + "INSERT INTO identities (wallet_id, identity_index, identity_id, entry_blob) \ + VALUES (?1, NULL, ?2, X'00')", params![[7u8; 32].as_slice(), [9u8; 32].as_slice()], ); let err = res.unwrap_err().to_string(); diff --git a/packages/rs-platform-wallet/src/wallet/apply.rs b/packages/rs-platform-wallet/src/wallet/apply.rs index f5271ed3152..10780172b4a 100644 --- a/packages/rs-platform-wallet/src/wallet/apply.rs +++ b/packages/rs-platform-wallet/src/wallet/apply.rs @@ -153,7 +153,7 @@ impl PlatformWalletInfo { for (_id, entry) in identities { self.identity_manager.apply_identity_entry(entry); } - // Best-effort tombstones across both buckets. Routed + // Best-effort removals across both buckets. Routed // through `remove_for_apply` so the manager's side-index // stays in lockstep with the buckets without us having to // reach in and touch the index from out here. diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs index c40ca540b42..f5af15abaf1 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs @@ -152,8 +152,12 @@ impl IdentityManager { /// Remove an identity from whichever bucket holds it. /// - /// Persists the tombstone changeset via `persister` and returns the - /// removed [`Identity`]. O(log n) via the side-index. + /// Persists the removal via `persister` and returns the removed + /// [`Identity`]. O(log n) via the side-index. + /// + /// Removal is terminal in storage as well as in memory: the persister + /// deletes the identity and every row it owns, so re-adding the same + /// id later starts from a blank identity. pub fn remove_identity( &mut self, identity_id: &Identifier,