refactor(platform-wallet)!: retire in-band pool snapshot; hardcode core UTXO account_index=0#3828
Open
Claudius-Maginificent wants to merge 26 commits into
Open
Conversation
…ools; contain flush blast radius On a genesis (birth_height=0) rescan, SPV can match a UTXO at a registered pool address before the live addresses_derived event for it lands. The UTXO writer's account lookup then missed, raised the fatal UtxoAddressNotDerived, and handle_flush_error dropped the WHOLE buffered changeset; core_bridge re-emitted it and the flush looped forever (173x in prod). core_derived_addresses (the per-address account_index map the UTXO writer joins) was fed only by live derive events and never from the account_address_pools snapshots that already hold every registered address. This wires the snapshot as the second source and stops one unresolvable UTXO from aborting an entire flush. C1 — apply_pools now mirrors every snapshot AddressInfo into core_derived_addresses in the same transaction, carrying the snapshot's real `used`. The row write is DRY'd into core_state::upsert_derived_address_row, called by both apply_pools and the live core_state::apply path through a shared core_state::derivation_path_label, so the two sources cannot drift. C2 — load() rehydrates core_derived_addresses from pools per wallet when the derived table is empty (already-persisted prod DBs), via core_state::rehydrate_derived_addresses_from_pools. It no-ops when rows already exist (no duplicates, no cost) and never touches ClientWalletStartState. No migration: V001 already defines both tables and the addr index. C3 — execute_upsert_utxo now SKIPS an unresolvable unspent UTXO (structured tracing::warn) instead of erroring, so the surrounding valid records, IS-locks, and sync-height still commit and the buffer drains. Funds-safe: the balance re-warms when the address later derives. The spent-only arm keeps its inert account-0 fallback. The UtxoAddressNotDerived variant and its fatal (non-transient) classification are retained for the exhaustive-match contract; it simply stops being raised. Tests: new sqlite_pool_derived_rehydration.rs covers genesis-rescan persist, pool/live row-shape parity, idempotent load rehydration, and blast-radius isolation. The structural-hardening contract test is rewritten to assert the skip semantics; the is_transient exhaustiveness gate still pins the variant as fatal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hydrate; review fixups The load-time reconcile fired only when core_derived_addresses was fully empty for a wallet, so a wallet with SOME live-derived rows plus a pool address that never derived was left un-repaired — its balance under-reported. Make the reconcile self-heal partial state, purely additively: - QA-003: drop the empty-only gate. The reconcile now upserts every pool-snapshot address via a dedicated INSERT ... ON CONFLICT DO NOTHING (INSERT_DERIVED_ADDRESS_IF_ABSENT_SQL), so it fills gaps without ever clobbering an existing live/mirrored row's account_index, derivation_path, or used flag. The live apply path keeps its DO UPDATE behavior unchanged. New test load_reconciles_partial_state_without_clobbering_live_rows seeds a live row (used=true, off-pool index) + a missing pool address and asserts the gap is filled while the live row is untouched (RED before, GREEN after). - PROJ-002: SCHEMA.md core_derived_addresses now documents all three population sources (live events, apply_pools mirror, load reconcile) routed through upsert_derived_address_row, and the skip (not reject) UTXO contract. - PROJ-003: rehydrate_derived_addresses_from_pools demoted to pub(crate). - QA-002: present-state comment at the upsert ON CONFLICT clause documenting write-once used (refreshes account_index/derivation_path, never used). - QA-004: reworded the C3 skip comment + warn — the balance re-warms only once the address is later derived (gap-limit dependent), not unconditionally. - QA-005: blast-radius test now mixes a live derive record with the skipped UTXO + sync-height bump and asserts the derive record committed — proving non-skipped records survive, not just sync-state. - QA-001 (deferred): TODO at the V001 core_derived_addresses PK noting it omits account_index/pool_type (ON CONFLICT collapse), practically unreachable under honest BIP32. PK unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemoves the ChangesSchema Simplification and account_index=0 Pivot
Background Sync Generation Counter Race Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…2-leaf PK + UNIQUE(address)
The core_derived_addresses read-index PK was (wallet_id, account_type,
address), which silently collapsed two addresses sharing that key via ON
CONFLICT. Re-key the table on the BIP32 leaf identity and demote `address`
to a UNIQUE-guarded derived attribute, so EVERY address collision —
within-pool or cross-pool — surfaces loud instead of corrupting the
address->account_index map.
PK is (wallet_id, account_type, pool_type, derivation_index): one row per
derived leaf, so a pool of N addresses persists N rows. `account_index`
stays a column (account-level context, the value the read returns) but is
not a uniqueness discriminator. UNIQUE(wallet_id, address) is the sole
arbiter of address uniqueness and its index backs
ACCOUNT_INDEX_BY_ADDRESS_SQL (the standalone idx_core_derived_addresses_addr
is dropped as redundant).
The free-text derivation_path column ("pool_type/index") is dropped: it
was a denormalised mirror of the now-typed pool_type + derivation_index
columns with no production reader. derivation_path_label is removed and
derivation_index is plumbed as a typed u32 through
upsert_derived_address_row (pub(crate)).
Authoritative upsert is ON CONFLICT(<leaf PK>) DO NOTHING — a same-leaf
re-derive is deterministic (address slot-derived, used write-once) so
there is nothing to update; a different leaf yielding the same address
trips UNIQUE(address). Reconcile stays INSERT OR IGNORE so a would-be
UNIQUE collision is skipped rather than aborting load().
V001 edited in place — no committed DB fixture exists. SCHEMA.md updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gaps Follow-up to the BIP32-leaf schema hardening. Test-only; no production changes. - Guard `used` write-once on the authoritative apply path: a same-leaf live re-derive (used=false) must not clear a stored used=true. The DO NOTHING clause enforces it; mutation-verified RED under DO UPDATE SET used = excluded.used, GREEN restored. - Tighten assert_unique_violation to assert SQLITE_CONSTRAINT_UNIQUE (2067), not the generic constraint bucket, matching its contract. - Anchor the new CHECK constraints directly: reject an invalid pool_type and account_type inserted into core_derived_addresses. - Exercise the FK ON DELETE CASCADE for core_derived_addresses: rows vanish when their wallet is deleted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
Collaborator
|
✅ Review complete (commit bde7060) |
thepastaclaw
suggested changes
Jun 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
One in-scope blocking issue: the new core_derived_addresses PK omits account_index, so two distinct accounts that share the same account_type label (e.g. Standard { index: 0 } and Standard { index: 1 }) collide on PK at the same pool/derivation_index and the second row is silently dropped via DO NOTHING. This breaks the PR's stated BIP32-leaf invariant for multi-account wallets and re-introduces the very missing-row class of bug the PR is meant to close. One supporting nitpick on a now-stale doc comment is also kept.
🔴 1 blocking | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:146-162: `core_derived_addresses` PK omits `account_index` and silently drops rows from secondary accounts
The PR documents the PK as the BIP32-leaf identity, but the leaf identity is `(wallet_id, account_type, account_index, pool_type, derivation_index)` — `account_index` is NOT redundant. `accounts::account_type_db_label` collapses every variant carrying a numeric discriminator to a single label (e.g. both `AccountType::Standard { index: 0 }` and `AccountType::Standard { index: 1 }` map to `"standard"`), and `accounts::account_index` stores the discriminator in a companion column. With the PK as written — `(wallet_id, account_type, pool_type, derivation_index)` — two distinct standard accounts deriving the same pool slot (e.g. external, derivation_index = 0) hit the same PK. Their addresses differ (different BIP32 leaves), so `UNIQUE(wallet_id, address)` does NOT fire; instead `upsert_derived_address_row`'s `ON CONFLICT(wallet_id, account_type, pool_type, derivation_index) DO NOTHING` silently drops the second account's row. The same `INSERT OR IGNORE` on the load-time reconcile path drops it again. A later UTXO landing on that second-account address fails the address→account lookup, gets the new "skip undeclared UTXO" treatment, and is silently absent from `core_utxos` — exactly the missing-money class of bug this PR is hardening against, just relocated from single-account to multi-account wallets. The CHECK-domain / UNIQUE / row-shape regression tests don't catch it because every test fixture is a single-account wallet. Add `account_index` to the PK and to the `ON CONFLICT(...)` target in `UPSERT_DERIVED_ADDRESS_SQL` and `INSERT_DERIVED_ADDRESS_IF_ABSENT_SQL`, update the schema comment, and add a regression test with two same-label accounts (e.g. `Standard { index: 0 }` and `Standard { index: 1 }`) deriving the same pool/derivation slot to assert both rows persist and both UTXOs resolve to the correct account_index.
…is missing from the derived index After the eager-mirror + load-time reconcile fix, every address in a wallet's persisted account_address_pools must also live in core_derived_addresses (declared => mapped). The unspent-UTXO writer used to warn-and-skip ANY address it could not resolve, which would silently swallow a regression in that mirror/reconcile and drop live money no one would notice. Discriminate the unspent miss by the pools: a declared-but-unmapped address is now a fatal DerivedIndexInvariantViolated (non-transient => Fatal at the trait boundary); a genuinely-undeclared address keeps the benign warn+skip (not-ours, or an SPV gap-limit edge). The pool-address set is decoded lazily at most once per apply, only on the first miss, and the spent-only synthetic path is left untouched (its inert account_index placeholder is excluded from reads). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…urviving UTXO in the over-fire test The skip-not-fatal test only checked the surviving declared-address UTXO's value, not its resolved account_index. Also assert the survivor is bucketed under the pool's account index (read from the derived map apply_pools mirrored), proving that skipping the undeclared row leaves the good row's attribution intact rather than mis-filing it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Both prior findings remain STILL VALID at head cccd217. The core_derived_addresses PK still omits account_index even though account_type_db_label (whose own docstring confirms it) collapses every indexed variant (Standard, CoinJoin, IdentityTopUp, DashpayReceivingFunds, DashpayExternalAccount, PlatformPayment) to a single label — two same-label sibling accounts deriving the same pool slot still PK-collide and silently drop via ON CONFLICT DO NOTHING. The new DerivedIndexInvariantViolated fatal guard now turns that dropped row into a fatal flush whenever a UTXO lands at the dropped address, replacing the prior emit-loop with a silent money-drop. The stale ACCOUNT_INDEX_BY_ADDRESS_SQL doc comment also still claims unreachable multi-row semantics under the new UNIQUE(wallet_id, address). No new latest-delta blocking issues; the invariant guard itself is well-shaped.
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:146-162: STILL VALID (carried-forward): `core_derived_addresses` PK omits `account_index`; sibling same-label accounts silently drop and now fatally trip the new invariant guard
The PK at line 159 is `(wallet_id, account_type, pool_type, derivation_index)`, omitting `account_index`. `accounts::account_type_db_label` (src/sqlite/schema/accounts.rs:238) collapses every variant carrying a numeric discriminator — `Standard{index}`, `CoinJoin{index}`, `IdentityTopUp{registration_index}`, `DashpayReceivingFunds{..}`, `DashpayExternalAccount{..}`, `PlatformPayment{account,..}` — to a single TEXT label. The function's own docstring explicitly says "Variants sharing a label are distinguished by the companion `account_index` column" — so the BIP32 leaf identity is `(wallet_id, account_type, account_index, pool_type, derivation_index)` and `account_index` is load-bearing, not "account-level context" as the new PK comment claims.
Consider a wallet with two Standard accounts (index 0 and 1). They derive into the same `(account_type='standard', pool_type='external', derivation_index=N)` slot but produce different addresses A and B. Both `apply_pools` and the live `addresses_derived` path go through `upsert_derived_address_row` (src/sqlite/schema/core_state.rs:243-246) which uses `ON CONFLICT(wallet_id, account_type, pool_type, derivation_index) DO NOTHING`. The first row (account_index=0, address A) inserts; the second (account_index=1, address B) collides on PK and is silently dropped. `UNIQUE(wallet_id, address)` does NOT fire because A ≠ B. The reconcile path (line 252-254) uses `OR IGNORE`, so the row keeps being re-dropped on every load — no recovery.
Under the new latest-delta guard (core_state.rs:200-216), this now manifests as a fatal regression: a later unspent UTXO at address B is pool-declared (both account snapshots are decoded by `pool_declared_addresses`) but unmapped, so it falls into the `DerivedIndexInvariantViolated` arm. `persistence_kind()` returns `Fatal`, so the buffered changeset is wiped. The prior emit-loop is gone, but every flush touching the dropped account's address is now a silent write-off, with the pool snapshot intact so the invariant violation re-triggers on every retry. Net effect on a multi-Standard-account wallet: the second account's funds become unflushable.
No test in `sqlite_pool_derived_rehydration.rs` exercises two same-label accounts (e.g. Standard {index: 0} and Standard {index: 1}) deriving overlapping pool slots; all fixtures hardcode `index: 0`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:243-246: STILL VALID (carried-forward, fix companion): `UPSERT_DERIVED_ADDRESS_SQL` conflict target must include `account_index`
Tied to the schema fix above: the authoritative writer's `ON CONFLICT(wallet_id, account_type, pool_type, derivation_index)` clause is the SQL site that silently drops the second same-label sub-account's row. The conflict target must match the corrected PK so a same-leaf re-derive remains a DO-NOTHING no-op while a different sub-account at the same pool slot persists its own row. Without aligning this target, the corrected PK would either reject the upsert (no unique index matching the target columns) or, if left at the current shape, re-introduce the silent-drop bug. The matching `INSERT_DERIVED_ADDRESS_IF_ABSENT_SQL` (line 252) uses `OR IGNORE` which auto-adapts to whatever PK the schema declares, so it follows automatically once the PK is updated.
In `packages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rs:38-56: Missing regression test for two same-label sub-accounts (Standard {index: 0} and {index: 1}) deriving the same pool slot
Every fixture in this test file uses a single Standard account with `index: 0`. The Design-Z thesis is that the PK encodes the BIP32-leaf identity and the schema fails loud on collisions — but neither claim is exercised against the realistic case of two same-label accounts deriving an overlapping pool slot. A test that seeds two Standard accounts ({index: 0} and {index: 1}) whose external pools both occupy derivation_index 0 should (a) persist BOTH derived rows, (b) keep each address's `account_index` lookup returning the correct sub-account, and (c) be RED today — the second row silently disappears, and the new fatal guard fires on that address when a UTXO lands. This is the load-bearing test that would have surfaced the PK bug.
…ount_address_pools stays complete The wallet-event adapter wrapped every post-registration CoreChangeSet with `account_address_pools` empty (`..Default::default()`), so the pool manifest was frozen at registration and never tracked gap-limit extensions. Those flowed only through the live `addresses_derived` delta, which has no `used` flag and is not a full pool — it can't rebuild the manifest. Attach a full pool snapshot in-band whenever the core delta derived new addresses: read the whole current pool for each affected account straight from the `WalletManager` the adapter holds, and ship it on the SAME changeset. The persister applies `account_address_pools` before the core UTXO delta in one tx, so a freshly-derived address is resolvable when that changeset's UTXOs are written — closing the gap-limit race in-band. Empty deltas emit nothing, so no write amplification on blocks that derive nothing. Additive only: the storage-side eager-mirror, load reconcile, and invariant guard are untouched this phase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… gate Adds Marvin's verification-gate scratch tests for the PR #3828 Phase-2 gate. They pin the single load-bearing claim the workaround-retirement plan rests on: a UTXO landing on a freshly-derived address resolves to the correct account_index when the pool snapshot rides the same PlatformWalletChangeSet as the UTXO, because apply_changeset_to_tx applies account_address_pools before the core UTXO delta inside one transaction. Three cases: in-band single changeset, adversarial merged-buffer arrival (UTXO stored before snapshot, Manual flush), and the non-fatal skip of a UTXO on a genuinely undeclared address. All pass on the current tree with the eager-mirror still present, demonstrating the manifest alone is a sufficient resolution source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… manifest fallback; relocate fatal guard to emitter-contract With the Phase-1 emitter making account_address_pools a complete, in-band manifest, the three storage workarounds collapse into one resolution rule. - Remove the eager-mirror: apply_pools writes only the snapshot blob; core_derived_addresses is fed exclusively by the live addresses_derived path, acting as an indexed read-cache in front of the manifest. - Remove the load-time reconcile (rehydrate_derived_addresses_from_pools and its load-path call); resolution-on-read needs no backfill. - UTXO resolution: derived-cache hit, else fall back to the pool manifest (the in-band snapshot is applied earlier in the same tx), else warn+skip a genuinely undeclared address. The old per-UTXO fatal branch becomes the manifest-fallback success path. - Relocate the fatal guard to the emitter contract: a live addresses_derived entry absent from the manifest aborts the flush (DerivedIndexInvariantViolated) at the storage trust boundary. Lag-safe — dropped events produce no changeset, so it can only fire on an emitter bug. No migration: the new resolution is a strict superset of the old read path. Tests reframed to the new model (manifest fallback, emitter-contract guard, old-DB back-compat); SCHEMA.md updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
changed the title
thepastaclaw
suggested changes
Jun 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Prior findings reconciliation: #1 STILL VALID, #2 STILL VALID, #3 STILL VALID, #4 STILL VALID. The latest delta (manifest-fallback + apply-time guard) cleanly retires the eager-mirror/reconcile path and meaningfully reduces blast radius of the schema bug (UTXO resolution falls back to the pool manifest), but the V001 core_derived_addresses PK and its companion UPSERT conflict target still omit account_index, so the BIP32-leaf identity the schema comment claims is not the one SQLite enforces. Sibling same-label accounts (e.g. Standard{index:0} and Standard{index:1}) at the same (account_type, pool_type, derivation_index) slot silently collide; the second derived-address row is dropped and the cache becomes permanently incomplete for that sibling, even though funds are no longer at risk thanks to the manifest fallback. No new latest-delta defects identified beyond the carried-forward issues.
🔴 2 carried-forward blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rs:32-76: [carried-forward STILL VALID] No regression test for two same-label sibling accounts on the same pool slot
The fixtures (`standard_external_pool`, `wallet_with_pools`) still only exercise a single `Standard{index:0}` account, and the new rich invariant tests added in this PR (within-pool/cross-pool collisions, manifest-fallback, reload-no-reconcile) never construct two same-label sub-accounts. The load-bearing regression for the PK / ON CONFLICT items above is: seed both `Standard{index:0}` and `Standard{index:1}`, register both pools, derive both External pools at `derivation_index=0`, persist via the in-band emitter, and assert (a) both derived rows persist in `core_derived_addresses`, and (b) UTXOs at each address resolve to their respective `account_index`. Today neither claim is exercised, so the silent row-drop is invisible to the suite.
thepastaclaw
suggested changes
Jun 15, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at SHA 925dfcb. The latest delta from ebb4b30 is dominated by a downstream v3.1-dev merge that does not touch the PR's in-scope surface; none of the four prior findings were addressed. All four are STILL VALID and carried forward. The schema-level defect (core_derived_addresses PK omits account_index) is the root cause: account_type_db_label collapses every index-bearing AccountType variant to one label, so sibling same-label accounts (Standard{index}, CoinJoin{index}, IdentityTopUp{registration_index}, Dashpay*, PlatformPayment{account}) sharing a pool slot collide on PK and the second derived row is silently dropped via ON CONFLICT DO NOTHING. The new in-band manifest fallback in execute_upsert_utxo mitigates functional fund-loss for the resolver path, but ACCOUNT_INDEX_BY_ADDRESS_SQL itself still returns incomplete/incorrect results for sibling accounts and the V001 schema bakes the wrong key in at migration zero. No new defects in the latest delta.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…sses PK; split standard label BIP32/BIP44
Closes the PK collision where Standard{0}/Standard{1} and BIP32/BIP44
standard accounts collapsed to one row, dropping UTXOs and tripping the
fatal flush guard into an unflushable loop.
- V001: extend core_derived_addresses PRIMARY KEY to include account_index
(was missing, causing Standard{idx:0} and Standard{idx:1} at the same
pool slot to silently clobber each other via ON CONFLICT DO NOTHING).
- accounts.rs: split AccountType::Standard arm in account_type_db_label
by StandardAccountType: BIP44Account -> "standard_bip44",
BIP32Account -> "standard_bip32". Removes the axis-2 collision where
a BIP32 depth-1 and BIP44 depth-3 standard acct-0 both resolved to
"standard" and the BIP32 row overwrote the BIP44 xpub in
account_registrations (todo 0e3ad26b).
- ACCOUNT_TYPE_LABELS: replaces "standard" with both new labels.
- UPSERT_DERIVED_ADDRESS_SQL ON CONFLICT target updated to match new PK.
- list_derived_addresses_for_test ORDER BY extended with account_index.
- All test fixtures using literal 'standard' updated to 'standard_bip44'.
- Three new regression tests: multi_account_index_same_slot_persists_both_rows,
bip32_and_bip44_standard_acct0_persist_both_rows,
account_registrations_bip32_and_bip44_both_survive.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… CHECK test and SCHEMA.md The "standard" -> "standard_bip44"/"standard_bip32" label split left two stale spots: - check_rejects_bad_pool_type seeded account_type="standard" as a valid placeholder so the pool_type CHECK fired on "not_a_pool". With "standard" no longer a valid label the INSERT tripped the account_type CHECK instead, so the test passed for the wrong reason and stopped exercising pool_type. Placeholder is now "standard_bip44" so pool_type is again what fails. - SCHEMA.md: refreshed the ACCOUNT_REGISTRATIONS account_type label list, marked CORE_DERIVED_ADDRESSES.account_index as a PK column with a corrected annotation, and updated the prose PK to the 5-column tuple (wallet_id, account_type, account_index, pool_type, derivation_index). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward verification at 925b109: the two blocking storage issues (core_derived_addresses PK omitting account_index and the matching UPSERT_DERIVED_ADDRESS_SQL conflict target) are FIXED — V001 PK is now (wallet_id, account_type, account_index, pool_type, derivation_index) and the UPSERT conflict target matches. The sibling/BIP32-vs-BIP44 regression coverage gap is FIXED (within_pool_address_collision_is_loud, cross_pool_address_collision_is_loud, plus standard_bip44/standard_bip32 label-split assertions in sqlite_pool_derived_rehydration.rs). One carried-forward nitpick remains STILL VALID: the doc comment on ACCOUNT_INDEX_BY_ADDRESS_SQL still describes a pre-UNIQUE(wallet_id, address) multi-row model. No new latest-delta defects found.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
…essSyncManager The exiting sync-loop thread cleared *background_cancel = None unconditionally; under restart-in-place a lagging old thread could clobber a freshly-installed cancel token, making the new loop uncancellable and letting a later start() spawn a duplicate loop. Gate the clear on a per-start generation counter, mirroring identity_sync.rs / shielded_sync.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ck-first-then-check) ShieldedSyncManager's exiting sync-loop thread checked background_generation before acquiring the background_cancel lock; a concurrent start() between the check and the lock could have the stale thread clobber the freshly-installed token. Reorder to lock-first-then-check, matching identity_sync.rs and platform_address_sync.rs (start() bumps the generation under the same lock). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens platform wallet persistence during genesis rescans and gap-limit extensions by ensuring the account_address_pools manifest is emitted in-band with derivation deltas, and by making SQLite persistence fall back to that manifest when the derived-address cache is missing—preventing fatal flush loops and “stuck at zero” balances. It also fixes restart-in-place races in background sync managers using generation guards.
Changes:
- Emit full
account_address_poolssnapshots in-band whenevercore.addresses_derivedis non-empty, so derived addresses are resolvable in the same persisted transaction as new UTXOs. - Update SQLite persistence to (a) validate the emitter contract (derived address must exist in manifest) and (b) resolve unspent UTXOs via derived-cache hit → manifest fallback → warn+skip (instead of aborting flush).
- Add/align background sync-loop generation guards to prevent stale threads from clobbering newly-installed cancel tokens.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-wallet/src/manager/shielded_sync.rs | Fix TOCTOU in generation-guarded cancel-token cleanup by locking before checking generation. |
| packages/rs-platform-wallet/src/manager/platform_address_sync.rs | Add background_generation guard so stale sync threads can’t clear a newer cancel token. |
| packages/rs-platform-wallet/src/changeset/core_bridge.rs | Build PlatformWalletChangeSet with in-band full pool snapshots when new addresses are derived; adds emitter-side tests. |
| packages/rs-platform-wallet/src/changeset/changeset.rs | Update documentation to reflect full-snapshot semantics and in-band emission tied to addresses_derived. |
| packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs | Enforce emitter contract at apply-time; add manifest fallback for UTXO resolution; skip undeclared unspent UTXOs instead of aborting flush. |
| packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs | Split Standard account labeling into standard_bip44 / standard_bip32 to prevent PK collisions across standard account variants. |
| packages/rs-platform-wallet-storage/src/sqlite/error.rs | Add DerivedIndexInvariantViolated error variant and update docs/classification for derived-address resolution behavior. |
| packages/rs-platform-wallet-storage/migrations/V001__initial.rs | Update core_derived_addresses schema (PK on leaf identity + UNIQUE(wallet_id, address)), add pool_type / derivation_index. |
| packages/rs-platform-wallet-storage/SCHEMA.md | Document new manifest-first model, derived-cache role, and updated core_derived_addresses constraints. |
| packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs | Adjust fixtures for new derived-address key shape; update expectations for undeclared unspent UTXO behavior. |
| packages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rs | New integration tests covering manifest fallback, invariant guard, blast-radius containment, and back-compat behavior. |
| packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs | Update hardcoded SQL inserts to new account-type labels and derived-address columns. |
| packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs | Update migration smoke inserts to match canonical account-type labels and derived-address schema. |
| packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs | Ensure derivations include matching in-band manifest entries to satisfy new invariant guard. |
| packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs | Add explicit cascade test for core_derived_addresses on wallet deletion. |
| packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs | Add new error variant to classification tables. |
| packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs | Update reader roundtrip tests to include in-band manifest entries. |
| packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs | Allow new read-only prepare (derived-address row shape) for compile-time SQL validation. |
| packages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rs | Extend CHECK-constraint tests to cover core_derived_addresses.pool_type and account_type. |
| packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs | Update account-type label usage in reader tests. |
| packages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rs | Add new fatal variant mapping coverage. |
| packages/rs-platform-wallet-storage/tests/marvin_gate_in_band_ordering.rs | New “gate” tests asserting in-band pool-before-core ordering resolves UTXOs and that undeclared UTXOs skip non-fatally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 569-571: The doc comment for the test helper starting at line 569
describing the `core_derived_addresses` row references outdated behavior
mentioning `apply_pools` mirroring and load-time rehydration, which have been
retired in this PR. Update this comment to accurately describe the current
manifest-fallback plus live-derive behavior instead of the stale implementation
details to prevent future confusion about how the address-to-account map is
populated.
- Around line 314-316: The map.insert() call in the loop over entry.addresses
silently overwrites existing address entries without checking for conflicts.
Modify the code to check if the address key already exists in the map before
insertion; if it exists with a different account_index value, return or
propagate an error instead of overwriting. This ensures manifest address
ownership conflicts are detected and fail loudly rather than silently corrupting
the UTXO attribution logic.
In `@packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs`:
- Around line 95-111: The manifest_for function is hardcoded to create an
AccountAddressPoolEntry with AccountType::Standard using BIP44Account, but the
test f2_no_bip44_wallet_nonzero_balance_survives_reopen is intended to be a
CoinJoin-only scenario. Refactor manifest_for to accept an AccountType parameter
instead of hardcoding the Standard BIP44Account variant, then update the call
sites (particularly in f2_no_bip44_wallet_nonzero_balance_survives_reopen and
the other location at lines 295-300) to pass a CoinJoin-specific AccountType to
keep the fixture semantics aligned with the test's intent and prevent masking
account-type attribution regressions.
In `@packages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rs`:
- Around line 136-137: The inline comment at line 136-137 incorrectly states
that account_index is not a key when in fact it is part of the composite primary
key in the updated schema. Update the comment wording to correctly reflect that
account_index is a component of the composite PK (along with derivation_index)
rather than claiming it is only account-level context that is not a key. This
will align the documentation with the actual schema contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ad95e29-3634-41ea-8bca-2992c5840f18
📒 Files selected for processing (22)
packages/rs-platform-wallet-storage/SCHEMA.mdpackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rspackages/rs-platform-wallet-storage/tests/marvin_gate_in_band_ordering.rspackages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rspackages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_error_classification.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rspackages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rspackages/rs-platform-wallet-storage/tests/sqlite_pool_derived_rehydration.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/manager/platform_address_sync.rspackages/rs-platform-wallet/src/manager/shielded_sync.rs
lklimek
reviewed
Jun 22, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward nitpick on ACCOUNT_INDEX_BY_ADDRESS_SQL is FIXED at ca68690 — the doc comment now cites UNIQUE(wallet_id, address) from V001 and frames ORDER BY/LIMIT as a defensive guard. The latest delta (doc/test polish, restart-in-place regression tests for both sync managers, CHECK-column count fix, contacts.state tests) is clean. One new latest-delta suggestion is preserved: pool_declared_address_indices silently overwrites duplicate manifest addresses, which contradicts the function's own documented 'fail-hard on corrupt snapshot' contract. The codex-general blocking claim about durable UTXO loss is dropped — the skip path is explicitly documented as a benign SPV gap-limit edge that re-warms on the next sync round, and 3/4 reviewers (including the rigorous Rust-quality pass) did not corroborate the data-loss framing.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:308-318: Fail hard on duplicate addresses in account_address_pools manifest
`pool_declared_address_indices` collapses every snapshot's addresses into a `HashMap<String, i64>` with plain `insert`, silently overwriting on collision. This map is now the authoritative fallback for UTXO account attribution when the live `core_derived_addresses` cache misses, and the function's own docstring states 'A corrupt snapshot is fail-hard (never skipped).' Silent overwrite contradicts that contract: a malformed manifest with the same address declared under two `(account_type, account_index)` snapshots would attribute UTXOs to whichever row SQLite returned last (the `SELECT` has no ORDER BY, so the winner is non-deterministic). Fail hard so a corrupt manifest cannot quietly mis-attribute balance.
…e in-band pool snapshot Replace the in-band `account_address_pools` snapshot + address->account lookup machinery (a full snapshot smuggled into a per-block diff) with a hardcoded `account_index = 0` at the storage writer, plus a fail-loud single-account guard in `core_bridge` (the only site with `record.account_type`). The product uses only the default account, and `core_utxos.account_index` has exactly one consumer (the per-account grouping reader), so this is correct with no public changeset-API change. Storage (platform-wallet-storage): - core_state::apply: drop the addresses_derived write loop, the emitter-contract guard, and the manifest-fallback lookup; execute_upsert_utxo now writes a const CORE_UTXO_ACCOUNT_INDEX (0). Deleted ACCOUNT_INDEX_BY_ADDRESS_SQL, UPSERT_DERIVED_ADDRESS_SQL, upsert_derived_address_row, pool_declared_address_indices, and the DerivedAddressRow/list_derived_addresses_for_test test helpers. - V001 (pre-release, edited in place): drop core_derived_addresses and account_address_pools tables; core_utxos unchanged (account_index column kept). Removed the now-unused pool_type CHECK interpolation. - accounts.rs: delete apply_pools; drop now-dead POOL_TYPE_LABELS / pool_type_db_label + their parity test. account_registrations and the account_index helper (used by apply_registrations) stay. - persister.rs: drop the apply_pools branch (account_address_pools field kept for API stability, no longer persisted). - error.rs: remove the now-unreachable DerivedIndexInvariantViolated and vestigial UtxoAddressNotDerived variants. Bridge (platform-wallet): - core_bridge: remove build_platform_changeset's snapshot block and snapshot_account_pools; build_core_changeset now returns Result and runs ensure_default_account on every UTXO-bearing record. A non-default funds account (AccountType::index() == Some(n != 0)) is a fail-loud CoreBridgeError::NonDefaultAccount, logged and skipped by the adapter so mis-attributed UTXOs are never persisted. addresses_derived is still forwarded (feeds the iOS registry via FFI); the FFI C ABI is unchanged. Tests: genesis-rescan regression rewritten (a UTXO on a fresh gap-limit address persists under account 0, no snapshot, no abort); single-account guard test (verified non-vacuous by neutering the guard); deleted the table-gone sqlite_pool_derived_rehydration suite; updated CHECK / migration / error-classification / object-metadata / FK / reader / structural-hardening tests to the new schema. Also fixes a batch of pre-existing stale WalletMetadataEntry initializers (missing wallet_group_id) that blocked the storage test build. BREAKING CHANGE: removes WalletStorageError::DerivedIndexInvariantViolated and WalletStorageError::UtxoAddressNotDerived (pub enum variants). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… (warn, don't skip) The single-account guard added in 8d8724e returned an Err that made the event adapter SKIP a non-default-account record's UTXOs. But a core UTXO can legitimately belong to a non-default account (multi-account / CoinJoin), and skipping it undercounts the wallet balance — i.e. loses funds. Since `core_utxos.account_index` only drives cosmetic per-account grouping (storage already writes a const 0 unconditionally), the safe behavior is to never skip and never fail: always persist the UTXO under index 0, and just warn on a non-default account so the approximate grouping is visible. - core_bridge: replace `ensure_default_account` (Err on non-default) with `warn_if_non_default_account` (tracing::warn! only, no value). Revert `build_core_changeset` to infallible (`-> CoreChangeSet`) and the adapter loop to the direct call. Delete the now-unused `CoreBridgeError` enum. - Tests: replace the skip-asserting guard test with end-to-end projections through `build_core_changeset` — `default_account_utxo_persists` and `non_default_account_utxo_persists_under_zero` (the fund-loss regression: a non-default-account UTXO is still projected, never dropped). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rs doc comments QA-002: the ACCOUNT_TYPE_LABELS and account_index() doc comments still named the account_address_pools and core_derived_addresses tables, both dropped in the hardcoded-account_index rearch. Both helpers now serve only account_registrations; update the wording to match the post-rearch schema. Comment-only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claudius-Maginificent
changed the title
Contributor
|
@coderabbitai review all |
Contributor
|
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai review 🤖 Co-authored by Claudius the Magnificent AI Agent |
Contributor
|
✅ Action performedReview finished.
|
… to retired pool-snapshot design Threads addressed: - 17 & 18 + PROJ-003 (SCHEMA.md): remove `core_derived_addresses` and `account_address_pools` from erDiagram, delete their dedicated table sections (including manifest-fallback / cache-hit prose), and revise the CHECK-constraint inventory from 8 columns / 5 domains down to 4 columns / 4 domains. Table count corrected from 23 → 21 throughout (intro paragraph, Diagram-1 description, and Migrations table). Upstream-enum coupling section updated: AddressPoolType removed, counts fixed (Three→Two, fourth→third, TODO updated accordingly). - 19 (changeset.rs ~966): `account_address_pools` field doc rewrote to describe current usage — registration-time seed only; incremental derivations via `core.addresses_derived` / FFI; storage persister explicitly ignores this field (UTXO attribution hardcoded to index 0). Reference to removed `core_bridge::build_platform_changeset` deleted. - 20 (core_state.rs ~123): CORE_UTXO_ACCOUNT_INDEX comment corrected from "rejected upstream by the core_bridge guard" to the actual behaviour: `warn_if_non_default_account` logs a `warn!` and still persists under 0. - 21 (sqlite_structural_hardening.rs ~119): same "rejected" wording fixed to warn-and-persist-under-0 in the test-function doc comment. No logic, SQL, or test code changed. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Draft
6 tasks
…r deep-index UTXOs After restart-in-place, watch-only rehydration eagerly derived only the gap-limit window (indices 0..=29) per chain, but persisted UTXOs can sit at deeper derivation indices (e.g. internal/change idx 300). The wallet total stayed exact, yet the per-address view undercounted because those deep addresses were never derived into their pools. `apply_persisted_core_state` now resolves each restored UTXO address back to its (chain, index) by forward-deriving from the keyless account xpub, then extends each chain's pool exactly to its own deepest restored index and refills the gap window beyond — mirroring the sync path's mark_used -> maintain_gap_limit shape. Throwaway probe pools drive the index search so the real pools are never over-derived across chains, and a bounded scan caps pathological/foreign addresses (re-warmed on the next full sync). The schema retains no derivation index (account_index is hardcoded 0, and the depth tables were retired), so the address is the only join key; the xpub is recovered from the keyless manifest by account type. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…log unresolved addresses QA-001/002: replace the flat 10k lockstep scan with independent per-chain loops. Each chain stops once no unresolved address resolves within a gap_limit-sized window past the deepest matched index (MAX_REHYDRATION_DERIVATION_INDEX = 10_000 remains the hard ceiling). Unresolved UTXO addresses (foreign key / multi-account mismatch) are counted and emitted via tracing::warn! instead of silently skipped. Total balance is unaffected; per-address visibility re-warms on next sync. QA-003: add rehydration_coinjoin_single_pool_deep_index test — verifies extend_pools_for_restored_utxos handles single-pool (CoinJoin/External) topology at a deep index (idx 30, just past the eager window). Narrows function doc claim to "tested with Standard BIP44 + CoinJoin". QA-004: add rehydration_unresolvable_address_is_deferred_not_panics test — asserts no panic/hang, total balance exact, foreign address deferred. QA-005: extend rehydration_extends_pools_to_cover_deep_index_utxos with an assertion that maintain_gap_limit fills beyond the deepest restored index (external.highest_generated >= deepest + gap_limit). Adjust test indices to work with the smarter scan bound (gap_limit=30): external 30 (anchor) + 50 (deep), internal 30. QA-006: document why chains with no resolved UTXOs skip mark_index_used and maintain_gap_limit (eager gap window already intact; no-op anyway). QA-007: soften "mirroring the sync path" to "following the … sequence". PROJ-003: rename manifest_of → manifest_for in sqlite_core_state_reader.rs for consistency with the same helper in rehydrate.rs unit tests. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…aram + depth bound PROJ-002: add # Parameters section documenting each argument including the manifest param (purpose + absent-account_type behaviour). Extend # Reconstructed with the address-pool-depth guarantee. Extend # Deferred with the MAX_REHYDRATION_DERIVATION_INDEX ceiling, the gap-limit scan bound, and the tracing::warn! re-warm behaviour. Remove the now-stale "This never logs" caveat since extend_pools_for_restored_utxos may emit a warn! for unresolved addresses. <sub>Co-authored by Claudius the Magnificent AI Agent</sub>
Contributor
|
@coderabbitai review all |
Contributor
|
✅ Action performedFull review finished. |
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
Collaborator
Author
There was a problem hiding this comment.
🔎 Claudius Grumpy Review — PR #3828
PR #3828 retires the in-band pool snapshot and moves core-UTXO rehydration to a keyless, account-0 model backed by the new sqlite storage layer. Discipline is high: all SQL is parameterized (no injection), changeset application stays atomic, SCHEMA.md was faithfully updated to the dropped tables, the call-tree is clean (no dangling callers after symbol removal), and removing the old fail-closed UtxoAddressNotDerived guard is a net fund-safety improvement (total balance now stays exact). No CRITICAL or HIGH issues. Three MEDIUM items warrant tracked follow-up: (1) the V001 'initial' migration was edited in place, which trips refinery's divergent-checksum abort for any wallet DB created against the prior schema — ship a V002 DROP instead of mutating V001; (2) the safety-critical RehydrationTopologyUnsupported fail-closed branch has no test; (3) the rehydration pool-extension scan anchors its first horizon at index 0, so a watch-only wallet holding its live balance on deep, sparse indices can silently fail to resolve those UTXOs into the per-address pool (balance total stays exact, per-address view goes blank). Remaining LOW items are doc drift, a phantom prepare() allow-list entry, committed ephemeral QA-00x IDs, and minor Rust hygiene.
Tally: 0 critical · 0 high · 3 medium · 13 low · 2 info (redundancy 0%).
Verdict: Approve with follow-ups. No blocking (CRITICAL/HIGH) issues. Before this reaches any environment with persisted wallet DBs, resolve the V001 migration packaging and add a RehydrationTopologyUnsupported test; confirm the deep-index horizon behavior against real watch-only usage.
Positives & non-line-anchored notes
- [LOW] SEC-002 — Persisted wallet blobs are (de)serialized with bincode 2.0.1, which carries the RUSTSEC-2025-0141 'unmaintained' advisory (at
packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs:1-160) - CALL-001 (positive) — Caller walk: the apply_persisted_core_state signature change and all removed symbols have consistent, non-dangling callers
- CODE-007 (positive) — Generation-guard TOCTOU fix is sound and consistent across both sync managers
15 findings posted inline below (3 pinned to the nearest in-diff line).
Co-authored by Claudius the Magnificent AI Agent
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Cumulative re-review at 4f83a07. Carried-forward prior finding from ca68690 (pool_declared_address_indices silent overwrite) is FIXED — the entire pool-snapshot/address-lookup machinery (pool_declared_address_indices, apply_pools, core_derived_addresses table, account_address_pools table, ACCOUNT_INDEX_BY_ADDRESS_SQL) has been removed; UTXO attribution now hardcodes account_index=0. New latest-delta findings center on the rehydration helper extend_pools_for_restored_utxos: a missed used-state restoration when restored addresses fall inside the initial eager window, an undocumented pool-ordering assumption between &/&mut accessors, swallowed ensure_derived failures on the real pool, and a diagnostic count mismatch in RehydrationTopologyUnsupported. The codex 'blocking' V001-migration-rewrite finding is dropped as a false positive: the rs-platform-wallet-storage crate is unreleased and V001 has been iterated in place 29 times under this active development series — there are no production V001 databases to migrate. No in-scope blocking issues remain; review_action is COMMENT.
🟡 3 suggestion(s) | 💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/rehydrate.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:259-359: Restored UTXO addresses inside the eager window never get their used-state restored
When every restored UTXO address is already present in the eager-derived pools, `unresolved` is empty and the function returns at line 268-269 before any `mark_index_used` call. The UTXOs themselves are inserted into `account.utxos`, so balance is correct, but `AddressInfo.used` / `used_indices` / `highest_used` remain unset for those slots. Receive-address selection paths (`next_unused`, `get_next_receive_address_index`) and the transaction-checker key set rely on this used-state, so between rehydrate and the first post-load sync a funded address inside the eager window can be handed out again as unused. The deep-index path beyond the eager window correctly calls `mark_index_used` at line 356, so this only affects the in-window case — but that is the common case for most wallets. Add a `pool.mark_used(&utxo.address)` pass over every restored UTXO regardless of whether deep-index discovery was needed.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:274-359: Probe-to-real-pool pairing relies on undocumented order parity between address_pools() and address_pools_mut()
`probes` is built from `account.managed_account_type().address_pools()` and then indexed by ordinal into `account.managed_account_type_mut().address_pools_mut()` at line 353 (`let pool = &mut *pools[i]`). The code silently assumes the two accessors return pools in the same order. If a future `ManagedAccountTrait` impl ever reorders pools between the `&` and `&mut` paths, `mark_index_used` / `maintain_gap_limit` will be applied to the wrong pool — a silent address-pool corruption bug invisible to today's tests because both BIP44 (External+Internal) and CoinJoin (single pool) topologies happen to have stable order. Either pair by an explicit key (e.g. `pool_type`) or pin the order invariant with a `debug_assert!`.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/rehydrate.rs:353-358: ensure_derived failure on the real pool is swallowed before mark_index_used
`ensure_derived` returns `Option<Address>` and yields `None` whenever `pool.generate_addresses(...).ok()?` fails — derivation errors are silently coerced. At line 354 the return value is discarded and execution unconditionally proceeds to `pool.mark_index_used(idx)` for every `idx` in `matched`. If real-pool derivation fails (it succeeded on the probe but probes carry separate state), the resolved index slots may not be generated on the real pool, after which `mark_index_used` / `maintain_gap_limit` operate on an undefined state. Either propagate the error (return `Result` and let the caller log+skip) or at minimum `tracing::warn!` and `continue` when `ensure_derived` returns `None`.
… docs Address open review threads on the sqlite wallet storage + rehydration PR. rehydrate.rs (extend_pools_for_restored_utxos): - Mark EVERY restored UTXO address used — not just deep-resolved ones. In-window addresses (inside the eager gap window) were never visited by the discovery scan, so a funded address kept used=false and could be handed out as a fresh receive address. Now a single mark_used pass covers in-window and deep-resolved alike; mark_used is a no-op for addresses a pool doesn't hold, so an underived index is never marked. - Replace bare pools[i] indexing: verify pools mirror the discovery probes 1:1 and fail closed with the new RehydrationPoolMismatch error on a structural break, then iterate via iter_mut().zip() with a pool_type debug_assert pinning chain-order parity. - Stop swallowing results: log on maintain_gap_limit Err and on apply-side ensure_derived returning None (deferring that address) instead of silently proceeding. - Add wallet_id + account_type structured fields to the unresolved warn, and emit a once-per-chain warn when discovery scans abnormally deep. Document why an explicit aggregate-derivation cap is unnecessary (chains x MAX is already bounded). - Document the horizon-walk limitation explicitly (chosen behavior): a legitimately-owned deep-and-sparse address — not only foreign keys — can land unresolved; the wallet total stays exact while the per-address view is incomplete until the next sync. Scope the AbsentHardened note: hardened derivation needs the private key, so those pools always defer under watch-only rehydration. core_bridge.rs: - Aggregate the per-record non-default-account warn in the BlockProcessed loop into one summary warn (count + sample txid) via a shared predicate; the single-record TransactionDetected path is unchanged. storage: - Drop the stale pool_type entry from the V001 migration header column list. - Reword the core_state addresses_derived comment to present-state. - Delete the phantom READ_ONLY_PREPARE_ALLOWED entry for a SELECT/columns that no longer exist. - Rename the genesis-rescan test to a behavioral sqlite_-prefixed name. Tests: in-window used-state is now asserted; a deep-and-sparse idx-45 UTXO is pinned as left-unresolved-but-balance-exact; the no-funds-account topology guard is covered (Err with the right count) plus its empty-UTXO Ok companion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental review at bde7060: latest delta (4f83a07..bde7060) is dominated by a merge of mb/cascade-work / origin/v3.1-dev, which brings in many unrelated changes outside this PR's scope. The PR's own in-scope file packages/rs-platform-wallet/src/manager/rehydrate.rs is byte-identical to the previously-reviewed revision. All five prior findings are STILL VALID and carried forward; no new in-scope defects were identified in the latest delta.
Carried-forward prior findings revalidated against this head:
- suggestion: [carried forward] Restored UTXO addresses inside the eager window never get marked used —
packages/rs-platform-wallet/src/manager/rehydrate.rs:259-270 - suggestion: [carried forward] Probe-to-real pool pairing depends on undocumented order parity between address_pools() and address_pools_mut() —
packages/rs-platform-wallet/src/manager/rehydrate.rs:274-358 - suggestion: [carried forward] ensure_derived failure on the real pool is swallowed before mark_index_used —
packages/rs-platform-wallet/src/manager/rehydrate.rs:353-358 - nitpick: [carried forward] RehydrationTopologyUnsupported.utxo_count reports unfiltered count —
packages/rs-platform-wallet/src/manager/rehydrate.rs:197-202 - nitpick: [carried forward] Unresolved-address warn lacks wallet_id / account_type context —
packages/rs-platform-wallet/src/manager/rehydrate.rs:333-340
New findings in the latest delta: none.
Posted as a top-level review because GitHub rejected the inline review payload for this non-current assigned SHA with HTTP 422; the full verifier output is preserved in review history.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
flush failed fatallyacross 7 wallets in one boot).core_derived_addressesmap. During a rescan, SPV matched historical UTXOs at registered addresses before the corresponding derive events landed → the lookup missed → the flush was classified fatal → the entire buffered changeset was dropped and re-emitted → infinite loop. Funded wallets showed 0 balance and never recovered.feat/platform-wallet-rehydration.What was done?
The core wallet uses only the default funds account (index 0) in the supported product (Platform, dash-evo-tool). Rather than resolve
address → account_indexat persist time (the race-prone lookup), the storage writer now records a constantcore_utxos.account_index = 0. With no lookup there is no race and no fatal loop — and a changeset goes back to carrying diffs, not a recurring full-pool snapshot.This retires the entire in-band pool-snapshot + address→account lookup machinery:
core_derived_addressesandaccount_address_poolstables;ACCOUNT_INDEX_BY_ADDRESS_SQL;apply_pools; the derived-address INSERT block incore_state::apply(); and the apply-time emitter-contract guard.account_address_poolschangeset field as the one-time registration seed for the iOS address registry (the only channel that can seed non-ECDSA pool addresses); and theaddresses_deriveddelta, which still feeds the SwiftPersistentCoreAddressrows before each UTXO changeset, so the cascade-delete chain stays intact.core_bridgeemits atracing::warn!but still persists the UTXO under index 0 — it never skips or fails. Worst case is approximate per-account grouping (cosmetic, multi-account-only); the total balance is always correct.Also in this PR — sync-manager restart safety:
PlatformAddressSyncManagergained abackground_generationguard (restart-in-place safety) andShieldedSyncManager's guard was reordered to lock-first-then-check (closes a TOCTOU), both mirroring the canonicalidentity_sync.rs, with new restart-in-place regression tests.Also in this PR — Core balance undercount on restart-in-place (deep-index UTXOs):
apply_persisted_core_staterestored all UTXOs + the full balance but never extended the watch-onlyAddressPoolpast the gap limit (0..=29), so UTXOs at deeper indices (e.g. internal idx 300) appeared inbalance.totalbut vanished from the per-address view — a regression of dash-evo-tool#829 Bug 2 (old-arch fix PR feat(dpp): add fees API to rust wasm bindings #830d10b805d).apply_persisted_core_statenow takes the account manifest and, after restoring UTXOs, reverse-maps each restored address to its(chain, index)by forward-deriving from the funds account's watch-only xpub, then extends each chain's pool to its deepest restored index and refills the gap (maintain_gap_limit). A per-chain bound stops once no unresolved address resolves within agap_limit-sized window past the deepest match (so foreign/multi-account UTXOs don't force a full scan);MAX_REHYDRATION_DERIVATION_INDEX = 10_000is the safety ceiling, and any still-unresolved addresses aretracing::warn!-logged (they re-warm on the next full sync). Total balance is unaffected in all cases.Fixed in passing: 6 pre-existing stale
WalletMetadataEntrytest initializers (missingwallet_group_id) that blocked the storage test build.How Has This Been Tested?
cargo test -p platform-wallet --lib --tests→ 205 passed, including the fund-loss regressionnon_default_account_utxo_persists_under_zeroand the deep-index rehydration regressions:rehydration_extends_pools_to_cover_deep_index_utxos(RED→GREEN, non-vacuity empirically confirmed — disabling the fix fails1000 vs 321000),rehydration_unresolvable_address_is_deferred_not_panics, andrehydration_coinjoin_single_pool_deep_index.account_index == 0and no pool snapshot present; rehydration (incl. a CoinJoin-only wallet), migrations, structural-hardening, check-constraints, and error-classification suites all pass.cargo fmt --all --checkclean;cargo clippy -p platform-wallet -p platform-wallet-storagezero warnings.Breaking Changes
WalletStorageErrorvariant —UtxoAddressNotDerived— unreachable after the lookup retirement. Downstream code matching on it must adapt.apply_persisted_core_stategained a requiredmanifestparameter (the account registration manifest); callers must pass it.!.)Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent