From 542490dac31c73fa26f473dd3e274fa3586914db Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Sun, 5 Jul 2026 20:01:47 +0700 Subject: [PATCH] fix(platform-wallet): index-conflicting removal no longer orphans a restored address balance A reconcile removal (fully consumed input) can resolve to a pool index that collides with a different, funded address's true derivation index. commit_reconciliation leaves the in-memory bijection untouched in that case but still emits the zero-funds removal so the balance can't resurrect. That emitted entry carried the conflicting address_index all the way to durable storage: - Swift persistAddressBalances unconditionally overwrote the row's addressIndex with the conflicting value, even though the derivation index is authoritative from the address-emit path and this callback is documented to refresh only balance/nonce/isUsed. Two durable rows then claim one (accountIndex, addressIndex). - On restart insert_persisted_entry rebuilds the bijection with a BiBTreeMap insert per row; a colliding insert evicts the funded pairing, orphaning its balance from current_balances depending on fetch order. Fix both sides of the persister seam: - Swift: persistAddressBalances no longer writes accountIndex/addressIndex (volatile balance/nonce/isUsed only), so a removal can't rebind an index and the durable store stays a bijection. - Rust: insert_persisted_entry guards the bijection only. A zero row is indistinguishable from a legitimate never-funded derived address, so it still seeds found as before; it inserts into the bijection via insert_no_overwrite so it can never evict a funded pairing, while a funded row keeps overwrite semantics and wins its slot. For unique indices this is identical to the previous unconditional insert. Covers both the SwiftData/FFI and SQLite restore paths, which share this method. Tests: Rust round-trips the conflict through restore in both fetch orders (insert_persisted_entry_removal_remnant_never_orphans_funded_pairing), pins that a never-funded zero row still restores normally (insert_persisted_entry_unfunded_derived_address_restores_normally), and Swift round-trips through persist + reload (AddressBalancePersistTests). Co-Authored-By: Claude Fable 5 --- .../src/wallet/platform_addresses/provider.rs | 132 ++++++++++++- .../PlatformWalletPersistenceHandler.swift | 20 +- .../AddressBalancePersistTests.swift | 186 ++++++++++++++++++ 3 files changed, 334 insertions(+), 4 deletions(-) create mode 100644 packages/swift-sdk/SwiftTests/SwiftDashSDKTests/AddressBalancePersistTests.swift diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs index 8b1df4d02b9..c64bfaa11cd 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs @@ -109,14 +109,45 @@ impl PerAccountPlatformAddressState { } /// Seed one persisted address/funds entry into the account state. + /// + /// Guards the `index <-> address` bijection against an + /// index-conflicting *removal remnant*. + /// [`commit_reconciliation`](PlatformPaymentAddressProvider::commit_reconciliation) + /// can zero an address whose pool-resolved `address_index` equals a + /// *funded* address's true index; it leaves the in-memory bijection + /// untouched but still emits the zero, so the durable store can hold + /// two rows claiming one index. On restore those rows seed the account + /// one at a time, and a plain [`BiBTreeMap::insert`] drops conflicting + /// pairs — so, depending on fetch order, the zeroed row could evict the + /// funded `(index -> address)` pairing and orphan its balance from + /// [`current_balances`](AddressProvider::current_balances). + /// + /// A zero-balance/zero-nonce row can't be told apart from a legitimate + /// freshly-derived, never-funded address by its funds alone (both are + /// `{0, 0}`), and the latter must still restore into `found` and the + /// bijection. So `found` is seeded for *every* row exactly as before; + /// the guard is only on the bijection, and only bites on a collision: + /// a zeroed row inserts via `insert_no_overwrite` (it can never evict + /// an incumbent pairing), while a funded row keeps overwrite semantics + /// (it wins its slot, displacing a stale zero remnant that grabbed it + /// first — that remnant's dangling `found` entry is inert, since + /// `current_balances` only yields addresses still paired in the + /// bijection). For the common case of unique indices this is identical + /// to the previous unconditional insert. pub fn insert_persisted_entry( &mut self, address_index: AddressIndex, address: PlatformP2PKHAddress, funds: AddressFunds, ) { - self.addresses.insert(address_index, address); self.found.insert(address, funds); + if funds.balance == 0 && funds.nonce == 0 { + // Never evict an incumbent pairing (funded, or another zero). + let _ = self.addresses.insert_no_overwrite(address_index, address); + } else { + // Funded rows are authoritative for their index. + self.addresses.insert(address_index, address); + } } /// Read-only view of the persisted `(address, funds)` entries. @@ -2012,6 +2043,105 @@ mod tests { assert_eq!(state.found.get(&conflicting).copied(), Some(funds(200, 1))); } + /// Restore-side guard for the index-conflicting removal. The write + /// side is [`commit_reconciliation`](PlatformPaymentAddressProvider::commit_reconciliation): + /// when a reconcile removal pool-resolves to an index already owned by + /// a different, funded address it leaves the in-memory bijection + /// untouched but still emits the zero so the balance can't resurrect — + /// which persists a zeroed row that can collide with the funded row on + /// disk. A durable store can therefore hold a funded row and a zeroed + /// *removal remnant* that both claim the same derivation index. On + /// restart the rows load in arbitrary fetch order and seed the account + /// bijection one at a time; a naive `BiBTreeMap::insert` would let the + /// zero remnant evict the funded `(index -> address)` pairing, + /// orphaning its balance from `current_balances` for one of the two + /// orders. `insert_persisted_entry` must land on the same correct state + /// in BOTH orders: the funded pairing survives, so the balance the + /// engine reads back (the intersection of `found` and the bijection, + /// i.e. what `current_balances` yields) is exactly the funded one — the + /// remnant's inert zero `found` row is never paired, so it can't be + /// yielded. + #[test] + fn insert_persisted_entry_removal_remnant_never_orphans_funded_pairing() { + let funded = p2pkh(0x77); + let remnant = p2pkh(0x22); + const INDEX: AddressIndex = 5; + + for funded_first in [true, false] { + let mut state = PerAccountPlatformAddressState::from_persisted( + test_xpub(), + BiBTreeMap::new(), + BTreeMap::new(), + ); + + // Same two durable rows, opposite restore (fetch) order. + if funded_first { + state.insert_persisted_entry(INDEX, funded, funds(200, 3)); + state.insert_persisted_entry(INDEX, remnant, funds(0, 0)); + } else { + state.insert_persisted_entry(INDEX, remnant, funds(0, 0)); + state.insert_persisted_entry(INDEX, funded, funds(200, 3)); + } + + // The funded pairing survives in the bijection... + assert_eq!( + state.addresses.get_by_left(&INDEX).copied(), + Some(funded), + "funded (index -> address) pairing must survive (funded_first={funded_first})" + ); + // ...and the zero remnant never holds the slot it would have to + // evict the funded pairing to take. + assert!( + state.addresses.get_by_right(&remnant).is_none(), + "removal remnant must not hold a bijection slot (funded_first={funded_first})" + ); + + // What the engine actually reads back is the intersection of + // `found` and the bijection — precisely `current_balances`. It + // must be exactly the funded balance; the remnant's inert zero + // `found` row is unpaired and therefore never yielded. + let restored: Vec<_> = state + .found + .iter() + .filter_map(|(addr, &f)| state.addresses.get_by_right(addr).map(|_| (*addr, f))) + .collect(); + assert_eq!( + restored, + vec![(funded, funds(200, 3))], + "only the funded balance may round-trip (funded_first={funded_first})" + ); + } + } + + /// A zero-balance/zero-nonce row is indistinguishable from a legitimate + /// freshly-derived, never-funded address (both are `{0, 0}`). With a + /// free index it must restore *normally* — into both `found` and the + /// bijection — so the guard against index-conflicting removals never + /// swallows a real unfunded address. (The `platform-wallet-storage` + /// SQLite reconstruction test pins the same expectation end to end.) + #[test] + fn insert_persisted_entry_unfunded_derived_address_restores_normally() { + let unfunded = p2pkh(0x22); + let mut state = PerAccountPlatformAddressState::from_persisted( + test_xpub(), + BiBTreeMap::new(), + BTreeMap::new(), + ); + + state.insert_persisted_entry(7, unfunded, funds(0, 0)); + + assert_eq!( + state.found.get(&unfunded).copied(), + Some(funds(0, 0)), + "a never-funded derived address must still seed found (balance 0)" + ); + assert_eq!( + state.addresses.get_by_left(&7u32).copied(), + Some(unfunded), + "a free-slot row extends the bijection normally" + ); + } + /// An entry identical to the committed seed is a no-op and is dropped /// to avoid persister churn. #[test] diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift index 698f88af39d..320d451d2a8 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift @@ -85,12 +85,28 @@ public class PlatformWalletPersistenceHandler { /// cache wipe between runs), we skip it — the next /// address-emit pass will bring the row back and the next sync /// will fill in the balance. + /// + /// The entry also carries `accountIndex` / `addressIndex`, but this + /// callback deliberately does NOT write them: the derivation index is + /// authoritative from the address-emit path (the row's index is fixed + /// the moment its address is derived and never changes). A reconcile + /// *removal* can arrive here carrying a pool-resolved `addressIndex` + /// that conflicts with another address's true index (the Rust provider + /// still emits the zero so the balance can't resurrect — see + /// `commit_reconciliation`'s index-conflict removal path). Overwriting + /// the row's index with that value would make two durable rows claim + /// one index; on the next restore the bijection rebuild + /// (`insert_persisted_entry`) would then drop the funded pairing and + /// orphan its balance. So the balance path owns balance/nonce/`isUsed` + /// only; derivation metadata stays as the address-emit path set it. func persistAddressBalances( walletId: Data, entries: [(UInt8, Data, UInt64, UInt32, UInt32, UInt32)] ) { onQueue { - for (_, addressHash, balance, nonce, accountIndex, addressIndex) in entries { + // `accountIndex` / `addressIndex` (tuple slots 5 and 6) are + // intentionally ignored — see the note above. + for (_, addressHash, balance, nonce, _, _) in entries { // Scope by walletId + hash: a hash-only predicate can match // another wallet's row in a multi-wallet store (same seed // imported on coin-type-sharing networks, watch-only @@ -103,8 +119,6 @@ public class PlatformWalletPersistenceHandler { guard let existing = try? backgroundContext.fetch(descriptor).first else { continue } - existing.accountIndex = accountIndex - existing.addressIndex = addressIndex existing.balance = balance existing.nonce = nonce if balance > 0 || nonce > 0 { diff --git a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/AddressBalancePersistTests.swift b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/AddressBalancePersistTests.swift new file mode 100644 index 00000000000..1074cabb332 --- /dev/null +++ b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/AddressBalancePersistTests.swift @@ -0,0 +1,186 @@ +import XCTest +import SwiftData +@testable import SwiftDashSDK + +/// Coverage for `PlatformWalletPersistenceHandler.persistAddressBalances` +/// — the incremental BLAST / reconcile balance-update path. +/// +/// The regression these tests pin: a reconcile *removal* (a fully +/// consumed input) is emitted from Rust carrying a *pool-resolved* +/// `addressIndex` that can collide with a different, funded address's +/// true derivation index (`commit_reconciliation`'s index-conflict +/// removal path deliberately still emits the zero so the balance can't +/// resurrect). The balance-update callback must NOT let that conflicting +/// index overwrite the row's authoritative index — otherwise two durable +/// rows end up claiming one `(accountIndex, addressIndex)` slot, and on +/// the next restore the Rust bijection rebuild +/// (`PerAccountPlatformAddressState::insert_persisted_entry`) drops the +/// funded pairing and orphans its balance. +/// +/// The derivation index is owned by the address-emit path; this callback +/// only refreshes the volatile balance / nonce / `isUsed` fields. +@MainActor +final class AddressBalancePersistTests: XCTestCase { + + private let walletId = Data(repeating: 0x01, count: 32) + private let accountIndex: UInt32 = 0 + + // Two distinct addresses. `funded` legitimately owns derivation + // index 5; `removed` legitimately owns index 2. The reconcile + // removal for `removed` will (wrongly) carry index 5. + private let fundedHash = Data(repeating: 0x77, count: 20) + private let removedHash = Data(repeating: 0x22, count: 20) + private let fundedIndex: UInt32 = 5 + private let removedIndex: UInt32 = 2 + private let conflictingIndex: UInt32 = 5 + + private func makeHandler() throws -> (PlatformWalletPersistenceHandler, ModelContainer) { + let container = try DashModelContainer.createInMemory() + let handler = PlatformWalletPersistenceHandler(modelContainer: container, network: .testnet) + return (handler, container) + } + + /// Seed a funded `PersistentPlatformAddress` row through a sibling + /// context so the handler's own background context reads it back from + /// the shared in-memory store — mirroring how the address-emit path + /// seeds rows before balance updates arrive. + private func seedRow( + in container: ModelContainer, + addressLabel: String, + addressHash: Data, + addressIndex: UInt32, + balance: UInt64, + nonce: UInt32 + ) throws { + let context = ModelContext(container) + let row = PersistentPlatformAddress( + address: addressLabel, + addressType: 0, + addressHash: addressHash, + publicKey: Data(), + accountIndex: accountIndex, + addressIndex: addressIndex, + derivationPath: "m/9'/1'/17'/0'/\(accountIndex)'/\(addressIndex)", + isUsed: true, + balance: balance, + nonce: nonce, + walletId: walletId + ) + context.insert(row) + try context.save() + } + + private func loadedRow( + _ handler: PlatformWalletPersistenceHandler, + hashByte: UInt8 + ) -> (balance: UInt64, nonce: UInt32, accountIndex: UInt32, addressIndex: UInt32)? { + let rows = handler.loadCachedBalances(walletId: walletId) + for (_, hash, balance, nonce, accountIndex, addressIndex) in rows + where hash.allSatisfy({ $0 == hashByte }) && hash.count == 20 { + return (balance, nonce, accountIndex, addressIndex) + } + return nil + } + + /// The core regression: a zero-balance removal whose emitted + /// `addressIndex` conflicts with a funded address's index must zero + /// the removed row's balance WITHOUT stealing the funded address's + /// index. Both durable rows keep their own indices, so the store + /// stays a bijection and the Rust restore can't orphan the balance. + func testConflictingRemovalDoesNotStealFundedIndex() throws { + let (handler, container) = try makeHandler() + + // Address-emit seeded both rows at their true indices; both are + // currently funded. + try seedRow( + in: container, + addressLabel: "fixture-funded-77", + addressHash: fundedHash, + addressIndex: fundedIndex, + balance: 200, + nonce: 3 + ) + try seedRow( + in: container, + addressLabel: "fixture-removed-22", + addressHash: removedHash, + addressIndex: removedIndex, + balance: 500, + nonce: 4 + ) + + // Reconcile removal for `removed`: fully consumed, zero funds, + // and — the hazard — carrying `funded`'s index 5, not its own 2. + let removal: [(UInt8, Data, UInt64, UInt32, UInt32, UInt32)] = [ + (0, removedHash, 0, 0, accountIndex, conflictingIndex) + ] + handler.persistAddressBalances(walletId: walletId, entries: removal) + + let funded = try XCTUnwrap(loadedRow(handler, hashByte: 0x77)) + let removed = try XCTUnwrap(loadedRow(handler, hashByte: 0x22)) + + // The removed row is zeroed... + XCTAssertEqual(removed.balance, 0, "removal must zero the balance") + XCTAssertEqual(removed.nonce, 0) + // ...but keeps its OWN index — the conflicting index 5 was ignored. + XCTAssertEqual( + removed.addressIndex, removedIndex, + "the balance path must not overwrite the row's authoritative derivation index" + ) + + // The funded row is untouched: same index, same balance. + XCTAssertEqual(funded.addressIndex, fundedIndex) + XCTAssertEqual(funded.balance, 200) + + // The durable store is still a bijection: the two rows do not + // share an index, so the Rust restore rebuild can't evict either + // pairing. + XCTAssertNotEqual( + funded.addressIndex, removed.addressIndex, + "no two durable rows may claim the same derivation index" + ) + } + + /// A normal (non-conflicting) balance update still refreshes the + /// volatile fields and leaves the derivation index exactly as the + /// address-emit path set it. + func testBalanceUpdatePreservesDerivationIndex() throws { + let (handler, container) = try makeHandler() + + try seedRow( + in: container, + addressLabel: "fixture-funded-77", + addressHash: fundedHash, + addressIndex: fundedIndex, + balance: 0, + nonce: 0 + ) + + // BLAST reports a fresh balance; the entry echoes the true index. + let update: [(UInt8, Data, UInt64, UInt32, UInt32, UInt32)] = [ + (0, fundedHash, 1_000, 7, accountIndex, fundedIndex) + ] + handler.persistAddressBalances(walletId: walletId, entries: update) + + let funded = try XCTUnwrap(loadedRow(handler, hashByte: 0x77)) + XCTAssertEqual(funded.balance, 1_000) + XCTAssertEqual(funded.nonce, 7) + XCTAssertEqual(funded.addressIndex, fundedIndex) + } + + /// A balance update for an address that was never address-emitted + /// (no row exists) is skipped — no phantom row, no stray index. + func testBalanceUpdateForUnknownAddressIsSkipped() throws { + let (handler, _) = try makeHandler() + + let update: [(UInt8, Data, UInt64, UInt32, UInt32, UInt32)] = [ + (0, removedHash, 42, 1, accountIndex, conflictingIndex) + ] + handler.persistAddressBalances(walletId: walletId, entries: update) + + XCTAssertTrue( + handler.loadCachedBalances(walletId: walletId).isEmpty, + "no row should be created for a never-emitted address" + ) + } +}