fix(platform-wallet): persist address usage discovered during SPV block processing#4107
Conversation
…ck processing Upstream wallet_checker's mark_address_used calls (and the pools' highest_used bumps) were in-memory only: the WalletEvent bus carries neither, so a match found during block processing — a TXO on a BIP44 address, or a ProRegTx payload hitting a provider owner/voting key — never reached SwiftData. Every store kept is_used=false and PersistentAccount.externalHighestUsed=-1 forever (G1/G2 of the mainnet owner-key Absent investigation). - CoreChangeSet gains addresses_marked_used (authoritative post-mark AddressInfo per involved address) and account_highest_used (per-account external/internal pool watermarks), both rebuilt by the event bridge from the post-processing wallet state via the read-only ManagedAccountCollection::check_transaction matcher. Merge dedups marked-used on (account, pool, index) and folds watermarks monotonic-max. - The FFI persister fans marked-used entries out through the existing on_persist_account_address_pools_fn pipeline (after the derived- address emit, so a same-round derive-then-use lands used=true). - WalletChangeSetFFI::from_changeset now fills external/internal_highest_used + has_* flags from the changeset instead of hardcoding them absent; the Swift handler only overwrites its row when the flag is set, so no-usage batches never regress a stored watermark. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWallet event processing now records marked-used derived addresses and per-account highest-used derivation watermarks. Changesets merge these deltas safely, persistence forwards marked-used address pools, and FFI conversion exposes watermark values and presence flags. ChangesAddress usage delta persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletEvent
participant CoreBridge
participant TransactionChecker
participant CoreChangeSet
participant FFIPersister
WalletEvent->>CoreBridge: process transaction or block records
CoreBridge->>TransactionChecker: re-check records for address usage
TransactionChecker-->>CoreBridge: matched addresses and highest-used indexes
CoreBridge->>CoreChangeSet: populate usage deltas
CoreChangeSet->>FFIPersister: persist merged changeset
FFIPersister-->>FFIPersister: emit marked-used address-pool snapshots
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
✅ Review complete (commit 03dfc48) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/changeset/core_bridge.rs (1)
337-371: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHighest-used snapshot is recomputed for every involved address, not once per touched account.
For each involved address that lands in an owner's pool, the inner
for p in owner...address_pools()loop rebuilds the fullHighestUsedIndexessnapshot andmerge_maxes it intohighest_used. Since the snapshot for a givenowner_typeis invariant across the enclosing loops (it reads the same live pool state), this repeats identical work O(matches × involved-addresses) times per account. It's idempotent (merge_max), so purely a wasted-work concern, not a correctness one; the read lock is also held for the whole walk. Consider snapshotting each touched account's watermarks once (e.g. collect the set of touchedowner_types, then snapshot after the address walk).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs` around lines 337 - 371, The highest-used watermark snapshot is redundantly recomputed for every matching involved address. In the owning-account traversal around `highest_used`, first collect each touched `owner_type`, then compute its `HighestUsedIndexes` from `address_pools()` once per account after the address walk and merge that single snapshot, preserving the existing internal/external aggregation.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/changeset/core_bridge.rs`:
- Around line 337-371: The highest-used watermark snapshot is redundantly
recomputed for every matching involved address. In the owning-account traversal
around `highest_used`, first collect each touched `owner_type`, then compute its
`HighestUsedIndexes` from `address_pools()` once per account after the address
walk and merge that single snapshot, preserving the existing internal/external
aggregation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a132fde0-0452-4c2c-9b90-9a951fe58e85
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/changeset/mod.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR fixes a real, previously-reported bug where SPV-discovered address usage never reached SwiftData because WalletEvent carries no mark-used/highest-used deltas. The re-derivation approach (replaying ManagedAccountCollection::check_transaction after the fact) is fundamentally sound for output-side and payload-based matches, and I verified against the pinned key-wallet rev (234e3c4) that the addresses_marked_used path (which drives the actual is_used flag) is wired correctly and independently of the buggy bucketing described below. However, two verified gaps remain in scope: replaying check_transaction after record_transaction has already removed spent UTXOs from the live account state means input-side usage (an address later spent as a transaction input) is never captured, and WalletChangeSetFFI::from_changeset buckets accounts strictly by cs.records, silently dropping account_highest_used watermarks for any account whose own record isn't in the current batch — a shape the PR's own test suite already constructs and asserts is meant to be persisted.
Source: reviewers gpt-5.6-sol (Sol reviewer) and claude-sonnet-5 (Sonnet reviewer), general lane; verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(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/src/changeset/core_bridge.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/changeset/core_bridge.rs:308-328: Post-event replay can never observe spent-input addresses — UTXOs are already removed by the time this code runs
`collect_usage_deltas` re-runs `accounts.check_transaction(&record.transaction, &[type_to_check])` from inside the wallet-event subscriber, which only fires *after* `record_transaction`/`confirm_transaction` has already mutated the account (see `wallet_checker.rs:150-179` in the pinned key-wallet rev 234e3c4: `record_transaction`/`confirm_transaction` runs first, `mark_address_used` second, using the `account_match` computed *before* either mutation). Input matching inside `check_transaction_for_match` depends entirely on the live UTXO set: `managed_core_funds_account.rs:671` does `if let Some(utxo) = self.utxos.get(&input.previous_output)`. But `record_transaction`'s `update_utxos` removes spent outpoints from `self.utxos` before returning (`managed_core_funds_account.rs:263-279`), and that removal always precedes the `WalletEvent` this bridge reacts to. So for any transaction that spends a wallet-owned input, this bridge's replay will never find that input's UTXO and will silently omit that address from both `marked_used` and `highest_used` — even though the original in-wallet processing correctly called `mark_address_used` on it via the pre-mutation `account_match`.
This isn't just a theoretical edge case: the PR's own motivation is that real mainnet wallets currently have addresses persisted with `is_used=false` due to the exact bug this PR fixes. When one of those already-broken addresses is later spent as an input, this bridge's replay-based recovery cannot repair it, because by construction it only ever sees post-mutation state. The fix needs to source spent-input addresses from `TransactionRecord.input_details` (which key-wallet already populates *before* `update_utxos` runs, per the comment at `managed_core_funds_account.rs:393`) rather than relying solely on re-running `check_transaction` after the fact.
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/core_bridge.rs:278-377: `collect_usage_deltas` has no direct test coverage
This is the function that actually fixes the reported bug — re-running `ManagedAccountCollection::check_transaction`, resolving an involved address back to its owning `(account_type, pool_type)` by scanning every account's pools, and snapshotting `highest_used`. There is no `#[cfg(test)]` module in `core_bridge.rs` at all, and no other file references `collect_usage_deltas`. Every existing test in this PR exercises pure data-structure logic downstream of it (`CoreChangeSet::merge` in `changeset.rs`, `group_marked_used_into_pool_entries` in `persistence.rs`), none of which would catch a regression in the matching/resolution logic itself. A regression test using a `ManagedAccountCollection` fixture covering a ProReg-payload owner-key match and a standard BIP44 output match would directly protect the scenario described in the PR (mainnet txid `5076f9ed...`) rather than only its downstream plumbing.
In `packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:293-350: `from_changeset` drops `account_highest_used` watermarks for any account not also present in `cs.records`
`ffi_accounts` (and therefore every `has_external_highest_used`/`has_internal_highest_used` flag) is built exclusively by iterating `by_account`, which is populated only from `cs.records` (lines 297-306). `cs.account_highest_used` is looked up per-bucket at line 348 — but only for buckets that already exist, i.e. only for account types that have a record in `cs.records` for this specific `store()` call. `collect_usage_deltas` (core_bridge.rs:308-374) does not guarantee that alignment: it resolves highest-used watermarks for *any* account whose pool owns a matched address, using a `type_to_check` scoped re-check that (for indexed categories like `StandardBIP44`) sweeps every account index in that category, not just the index of the record(s) passed in. Combined with key-wallet's `confirm_transaction` returning `None` (no record emitted) for already-finalized/idempotent re-confirmations while still unconditionally calling `mark_address_used` (`wallet_checker.rs:158-179`), a `BlockProcessed` batch can produce an `account_highest_used` entry for an account whose own record isn't in `inserted`/`updated`/`matured` this round — and that entry is then silently dropped by `from_changeset`.
This is not just a hypothetical: `changeset.rs`'s `marked_used_and_highest_used_mark_core_changeset_non_empty` test constructs exactly this shape — a `CoreChangeSet` with `account_highest_used` populated and `records` empty — and asserts the changeset must be treated as non-empty/persistable. But no test exercises `from_changeset` with that same shape, and reading the conversion confirms it produces `accounts_count == 0`, so the watermark never reaches Swift. Note this specifically affects the `external_highest_used`/`internal_highest_used` fields (gap-limit bookkeeping); the `is_used` flag itself is safe since `addresses_marked_used` is fanned out through a separate, unfiltered callback path (`persistence.rs:848-869`).
- Source spent-input addresses from the record's own input_details (and Received/Change output_details) in addition to the replayed check_transaction match: record_transaction removes spent outpoints from the live UTXO set before the wallet event fires, so the replay alone can never observe input-side usage. - Emit an (otherwise empty) AccountChangeSetFFI bucket for watermark-only accounts in from_changeset — a batch can carry a highest-used advance for an account with no record of its own this round, which was silently dropped. - Hoist the per-account address_pools() allocation and snapshot highest-used watermarks once per touched account instead of per involved address. - Split the derivation into a sync collect_usage_deltas_from_accounts and cover it with wallet-fixture tests: BIP44 receive marks External #0 used + advances the watermark, and a spend captures the spent address after UTXO removal; plus from_changeset watermark-only regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd-example-app Conflicts (both test-region/import collisions with base #4107, "persist address usage discovered during SPV block processing"): - rs-platform-wallet-ffi/src/persistence.rs: kept BOTH sides — the branch's #4069 round-serialization probe tests and base's marked-used grouping tests (148 crate tests green together). - rs-platform-wallet/src/changeset/core_bridge.rs: import union (HashMap + atomics from the branch, BTreeMap + HashSet from base). Verified: cargo fmt, platform-wallet-ffi lib tests (148), core_bridge tests (9), workspace clippy --all-features -D warnings, JNI check (shielded). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This commit fixes both prior blocking data-loss paths. Spent-input addresses are now sourced from TransactionRecord.input_details, which key-wallet populates before update_utxos removes the spent outpoints, and watermark-only accounts now emit an AccountChangeSetFFI row. Direct regression tests cover both fixes, address-pool snapshots eliminate the prior repeated allocation, and the full platform-wallet and platform-wallet-ffi test suites, clippy, and formatting checks are clean. No new issue was found in the latest delta or cumulative diff.
Source: Sol reviewer gpt-5.6-sol (general), Sonnet reviewer claude-sonnet-5 (general), sampled Opus reviewer claude-opus-4-8 (general); final verifier claude-sonnet-5. The Sol orchestrator openai/gpt-5.6-sol was orchestration-only and is not reviewer or verifier evidence.
Prior reconciliation: prior-1 FIXED; prior-2 FIXED; prior-3 FIXED; prior-4 FIXED.
Carried-forward prior findings: None.
New findings in the latest delta: None.
2 participants
Issue being fixed or feature implemented
On a mainnet wallet whose seed owns masternode provider keys, SPV block processing correctly detects a ProRegTx payload match on the Provider Owner Keys address (verified with txid
5076f9ed731bcb7b5ddf47e21bf1262d22d99665ca89bb6329e6e706d605da85at height 1030673) andkey-walletmarks the address used in memory — but nothing ever reaches the persistence layer. TheWalletEventbus carries neither themark_address_usedresults nor the pools'highest_usedbumps, so:is_used = falseon matched addresses (provider keys and regular BIP44 alike), andWalletChangeSetFFI::from_changesethardcodedhas_external_highest_used/has_internal_highest_usedtofalse, soPersistentAccount.externalHighestUsedstays-1on every account in every store.What was done?
CoreChangeSetgains two deltas (rs-platform-wallet):addresses_marked_used(the authoritative post-markAddressInfoper involved address, asDerivedAddressInfo) andaccount_highest_used(per-account external/internal pool watermarks via a newHighestUsedIndexestype). Merge dedups marked-used entries on(account_type, pool_type, index)— same discipline asaddresses_derived— and folds watermarks monotonic-max per slot; both fields gateis_emptyso a usage-only event still persists.core_bridge.rs):collect_usage_deltasre-runs the same matcher the wallet used during processing — the read-onlyManagedAccountCollection::check_transaction— against each record's transaction after processing, then resolves every involved address back to its owning pool. That recovers the pool type (which the match doesn't carry) plus the authoritativeAddressInfoandhighest_used, covering both TXO matches (BIP44) and special-tx payload matches (provider owner/voting keys). Single-pool accounts (provider keys, identity funding) surface their watermark in the external slot, matching the account row's two persisted fields.rs-platform-wallet-ffi): marked-used entries are bucketed per(account_type, pool_type)and fanned out through the existingon_persist_account_address_pools_fnpipeline, deliberately after the derived-address emit — the Swift persister overwritesisUsedwith the latest emit, so derive-then-use within one round landstrue.from_changesetnow fillsexternal/internal_highest_used+has_*flags from the changeset; values are authoritative pool state (not batch maxima) and the flags stayfalseon no-usage batches, so stored watermarks can never regress.No Swift changes are required —
persistAccountAddressesand the account-changeset handler already consume both surfaces.How Has This Been Tested?
Nonenever regresses a slot), usage-only changesets are non-empty (rs-platform-wallet), and(account, pool)grouping incl. aProviderOwnerKeys/Absent-pool case withused == truepreserved (rs-platform-wallet-ffi).cargo test -p platform-wallet --lib(415 passed) andcargo test -p platform-wallet-ffi --lib(141 passed).cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targetsclean;cargo check --all-features --testsclean on both crates;cargo fmt --all.XkxD3qhMb7uaQg7rvm9ZNCzyYhZ7tLPQc2) as used with Highest Used 0.Breaking Changes
None. New changeset fields are additive (
serde(skip)breadcrumbs,Default-constructed), and the FFI struct layout is unchanged — only previously-hardcoded fields are now populated.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit