fix(platform-wallet): reflect asset-lock top-up balance once, not doubled#4004
Merged
Merged
Conversation
…bled An "Top Up from Core" credited the recipient platform address twice in the UI until a manual Sync-tab Clear + Sync Now corrected it (QA ADDR-09). The on-chain balance was always correct — this was a client-side sync bug. AddressFundingFromAssetLockTransition records the recipient credit on-chain as AddBalanceToAddress, i.e. a delta (AddToCredits) in Drive's recent-address-balance-changes tree. The fund path reconciled the proof-attested absolute balance X into the provider's committed `found` seed but left the incremental sync watermark stale, so the next incremental BLAST pass seeded from the already-credited balance and then re-applied the recent delta on top -> X + X = 2X. A full scan (what the manual Clear forces) seeds absolutely from the tree, so Clear + resync fixed it. Add PlatformPaymentAddressProvider::invalidate_sync_watermark(), which zeroes the incremental watermark only (keeping the just-reconciled `found` seed so display and auto_select_inputs budgeting keep X visible), making last_sync_timestamp() return None so the next pass takes the full-scan branch and reconciles to X. Call it from fund_from_asset_lock after reconcile_address_infos; this covers both the fresh and resume top-up paths (one shared Rust fn). It is the automated equivalent of the manual Clear + Sync Now. Durability is in-memory only: the persisted watermark cannot be reset to 0 through the normal changeset (PlatformAddressChangeSet::merge uses .max() and the persister only fires on values > 0), so the next in-session pass (~15s) full-scans, reconciles, and persists a correct forward watermark. Adds a provider unit test and an ADDR-09 regression row to TEST_PLAN.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds an ADDR-09 fix in ChangesADDR-09 double-counting fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
|
✅ Review complete (commit 180da72) |
QuantumExplorer
added a commit
that referenced
this pull request
Jul 5, 2026
…nsfer credit Follow-up to the ADDR-09 asset-lock top-up double-credit fix (#4004). The same optimistic-absolute-seed vs. incremental-delta-re-apply mechanism also affects credit transfers whose OUTPUT is an address owned by the source wallet. A transfer records each output on-chain as `AddBalanceToAddress` (a DELTA / `AddToCredits` in Drive's recent-address-balance-changes tree), while inputs are recorded as `SetBalanceToAddress` (absolute). `reconcile_address_infos` writes the proof-attested ABSOLUTE post-balance into the provider's `found` seed but does not advance the incremental sync watermark, so the next incremental BLAST pass re-applies the recent `AddToCredits` delta on top of the already-absolute seed for that owned output → `X + delta` over-count, exactly the ADDR-09 double-count. The most concrete reachable path is a change output (always owned) or any transfer to one of the wallet's own addresses. Fix: after the transfer reconcile, when an owned OUTPUT was credited (`reconcile_credited_owned_output`), call `PlatformPaymentAddressProvider::invalidate_sync_watermark()` to force the next pass to full-scan-reconcile. Gated on owned outputs so the common send-to-someone-else transfer keeps the fast incremental cadence; inputs need no treatment because re-applying an absolute `SetCredits` op is idempotent. Scope notes (documented in code): - Cross-wallet transfers are NOT affected: each wallet's provider tracks only its own wallet (`from_wallets([self.wallet_id])`), so the recipient wallet never gets an optimistic absolute seed write; its own sync applies the delta once from a correct seed. - Withdrawals are NOT affected: the wallet-level `withdraw` always passes `None` for the change output, so owned addresses only ever receive absolute `SetBalanceToAddress` ops. A comment guards against a future change output silently reintroducing the bug. Adds unit tests for the owned-output gate; the provider-side effect is covered by `invalidate_sync_watermark_forces_full_scan_keeps_seed` (#4004). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks
thepastaclaw
reviewed
Jul 5, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Focused ADDR-09 fix: after reconcile_address_infos commits the proof-attested absolute balance, fund_from_asset_lock invalidates the incremental sync watermark so the next BLAST pass full-scans instead of re-applying the recent delta on top of the seeded balance. The mechanism is correct, well-documented, and covered by a targeted unit test. Reviewers converged on one substantive concern: the reconciliation commit and the watermark invalidation happen in two separate provider write-lock critical sections, leaving a narrow window in which a queued background sync_balances can grab the lock between them, take the incremental branch, and briefly reproduce the 2X reading — self-healing on the next full scan within ~15s.
🟡 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/wallet/platform_addresses/fund_from_asset_lock.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs:292-339: Watermark invalidation is not atomic with `reconcile_address_infos`
`reconcile_address_infos` at line 292-294 takes `self.provider.write()` internally, commits the reconciled `found = X` seed, and drops that guard before returning. The block at 334-339 then re-acquires `self.provider.write()` to call `invalidate_sync_watermark`. Meanwhile `sync_balances` in `sync.rs:115` also acquires `self.provider.write()` for the entire sync round — including the SDK network call at lines 133-136 — and calls `provider.last_sync_timestamp()` before seeding `before` from `current_balances(&*provider)`. tokio's `RwLock` is fair, so a background BLAST sync waiting on the lock during reconciliation can be granted the lock between the two acquisitions here. In that interleaving it will observe (a) the freshly committed `found = X` and (b) the still-non-zero watermark, take the incremental branch in `sync_address_balances`, re-apply the recent `AddToCredits(X)` delta, and persist `X + X = 2X` — the exact reading this PR claims to prevent. The subsequent invalidation still causes a full-scan reconcile within ~15s, so worst-case is a brief reappearance of the pre-PR bug rather than a regression, but the fix is not race-free the way the comment implies. A race-free variant would fold the watermark zeroing into the same critical section as the reconciliation commit — for example a fund-specific reconcile helper, or an `invalidate_watermark_after: bool` flag on `reconcile_address_infos` so transfer/withdrawal don't trigger it. Worth doing, since the whole point of this PR is to eliminate the ability to observe the double-count from a fresh top-up.
Source: reviewers opus (claude-general), gpt-5.5 (codex-general), opus (claude-security-auditor), gpt-5.5 (codex-security-auditor), opus (claude-rust-quality), gpt-5.5 (codex-rust-quality), opus (claude-ffi-engineer), gpt-5.5 (codex-ffi-engineer); verifier opus.
Comment on lines
293
to
+339
| .reconcile_address_infos(&address_infos, "fund from asset lock") | ||
| .await; | ||
|
|
||
| // ADDR-09: force the next BLAST sync to full-scan-reconcile | ||
| // instead of applying an incremental delta. | ||
| // | ||
| // `reconcile_address_infos` above set the provider's committed | ||
| // `found` seed to the proof-attested ABSOLUTE balance `X`, but the | ||
| // top-up is recorded on-chain as a DELTA (`AddBalanceToAddress` → | ||
| // `AddToCredits`) in Drive's recent-address-balance-changes tree, | ||
| // and we did NOT advance the incremental watermark. An incremental | ||
| // next pass would seed `result.found` from `current_balances()` | ||
| // (already `X`) and then re-apply that recent `AddToCredits(X)` | ||
| // delta from the stale watermark, landing at `X + X = 2X` — the | ||
| // ADDR-09 double-count. Zeroing the in-memory watermark makes | ||
| // `last_sync_timestamp()` return `None`, so the next pass full-scans | ||
| // (absolute seed from the tree, catch-up from the fresh checkpoint) | ||
| // and reconciles to the correct `X`. This is the automated | ||
| // equivalent of the manual Sync-tab "Clear" + "Sync Now". | ||
| // | ||
| // Unlike transfer/withdrawal (which also route through the seam), | ||
| // an asset-lock top-up credits its recipient with a pure additive | ||
| // delta and no offsetting input for that same address, so updating | ||
| // the `found` seed alone does not neutralize the re-applied delta — | ||
| // hence this path-specific watermark invalidation. | ||
| // | ||
| // DURABILITY: in-memory only. The persisted sync watermark cannot | ||
| // be reset to 0 through the normal changeset — | ||
| // `PlatformAddressChangeSet::merge` combines `sync_height` with | ||
| // `.max()` and the persister only fires `on_persist_sync_state_fn` | ||
| // when a component is `> 0` — so a durable zero would have to fight | ||
| // both the merge and the `> 0` gate. Instead we rely on the | ||
| // in-session BLAST cadence (~15s): the next pass full-scans, | ||
| // reconciles to `X`, and persists a correct FORWARD watermark, so | ||
| // durable state self-corrects within ~15s. The only residual gap is | ||
| // an app kill inside that ~15s window; a restart then resumes | ||
| // incremental sync from the stale persisted watermark and the | ||
| // double-count could briefly reappear until the next full rescan | ||
| // (or a manual Clear). That narrow window is accepted here rather | ||
| // than over-engineering a durable invalidation against the | ||
| // `.max()` merge / `> 0` gate. | ||
| { | ||
| let mut guard = self.provider.write().await; | ||
| if let Some(provider) = guard.as_mut() { | ||
| provider.invalidate_sync_watermark(); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Watermark invalidation is not atomic with reconcile_address_infos
reconcile_address_infos at line 292-294 takes self.provider.write() internally, commits the reconciled found = X seed, and drops that guard before returning. The block at 334-339 then re-acquires self.provider.write() to call invalidate_sync_watermark. Meanwhile sync_balances in sync.rs:115 also acquires self.provider.write() for the entire sync round — including the SDK network call at lines 133-136 — and calls provider.last_sync_timestamp() before seeding before from current_balances(&*provider). tokio's RwLock is fair, so a background BLAST sync waiting on the lock during reconciliation can be granted the lock between the two acquisitions here. In that interleaving it will observe (a) the freshly committed found = X and (b) the still-non-zero watermark, take the incremental branch in sync_address_balances, re-apply the recent AddToCredits(X) delta, and persist X + X = 2X — the exact reading this PR claims to prevent. The subsequent invalidation still causes a full-scan reconcile within ~15s, so worst-case is a brief reappearance of the pre-PR bug rather than a regression, but the fix is not race-free the way the comment implies. A race-free variant would fold the watermark zeroing into the same critical section as the reconciliation commit — for example a fund-specific reconcile helper, or an invalidate_watermark_after: bool flag on reconcile_address_infos so transfer/withdrawal don't trigger it. Worth doing, since the whole point of this PR is to eliminate the ability to observe the double-count from a fresh top-up.
source: ['claude', 'codex']
Comment on lines
520
to
+561
| } | ||
| } | ||
|
|
||
| /// Zero the incremental-sync watermark ONLY, so the next | ||
| /// `sync_balances` takes the full-scan branch — WITHOUT dropping the | ||
| /// cached `found` seed (unlike [`reset_sync_state`](Self::reset_sync_state), | ||
| /// which is the "Clear" flow). | ||
| /// | ||
| /// WHY (ADDR-09): the asset-lock top-up path reconciles the | ||
| /// proof-attested ABSOLUTE balance `X` into both the managed account | ||
| /// and the provider's committed `found` seed, but the on-chain credit | ||
| /// is recorded as a DELTA (`AddBalanceToAddress` → `AddToCredits`) in | ||
| /// Drive's recent-address-balance-changes tree. If the next pass ran | ||
| /// INCREMENTALLY it would seed `result.found` from `current_balances()` | ||
| /// (already `X`) and then re-apply that recent `AddToCredits(X)` delta | ||
| /// from the stale watermark, landing at `X + X = 2X` — the ADDR-09 | ||
| /// double-count. An optimistic absolute write is fundamentally | ||
| /// inconsistent with incremental delta re-application, so we force the | ||
| /// very next pass to full-scan-reconcile. | ||
| /// | ||
| /// With `sync_timestamp == 0`, [`last_sync_timestamp`](Self::last_sync_timestamp) | ||
| /// returns `None`, which makes `sync_address_balances` choose the | ||
| /// full-scan branch: `result.found` is re-seeded ABSOLUTELY from the | ||
| /// tree (the `found` seed is only consulted on the incremental branch, | ||
| /// which is skipped), and incremental catch-up runs from the fresh | ||
| /// full-scan checkpoint rather than the stale height, so no recent | ||
| /// delta is re-applied. `last_known_recent_block` is zeroed too since | ||
| /// catch-up reads it as its recent-tree boundary. | ||
| /// | ||
| /// The `found` seed is deliberately KEPT (not cleared): a full scan | ||
| /// ignores it as a seed, and preserving it means display and | ||
| /// `auto_select_inputs` budgeting keep the just-applied balance `X` | ||
| /// visible during the ~15s until the reconciling scan completes, | ||
| /// instead of the momentary zero `reset_sync_state` would show. | ||
| /// | ||
| /// This is the in-memory equivalent of the manual Sync-tab | ||
| /// "Clear" + "Sync Now" that also fixes the double-count. | ||
| pub(crate) fn invalidate_sync_watermark(&mut self) { | ||
| self.sync_height = 0; | ||
| self.sync_timestamp = 0; | ||
| self.last_known_recent_block = 0; | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: reset_sync_state and invalidate_sync_watermark duplicate the three-scalar zeroing
Both reset_sync_state (line 510) and the new invalidate_sync_watermark (line 557) zero the same three watermark fields (sync_height, sync_timestamp, last_known_recent_block). The two operations are semantically nested — Clear = invalidate watermark + drop found/absent + drop per_wallet_in_sync — so reset_sync_state could compose on top of invalidate_sync_watermark to keep the watermark reset single-sourced. If the watermark ever grows a fourth scalar, forgetting it in either function would silently reintroduce an ADDR-09-like bug or regress Clear. Not a blocker.
source: ['claude']
QuantumExplorer
added a commit
that referenced
this pull request
Jul 5, 2026
…reconcile seam Follow-up to the ADDR-09 asset-lock top-up double-credit fix (#4004), generalizing it to every flow that credits a wallet-owned address. Any transition that credits an address does so via an on-chain `AddBalanceToAddress` op — a DELTA (`AddToCredits`) in Drive's recent-address-balance-changes tree — while inputs are recorded as absolute `SetBalanceToAddress` ops. When `reconcile_address_infos` commits the proof-attested ABSOLUTE post-balance into the provider's `found` seed without advancing the incremental sync watermark, the next incremental BLAST pass re-applies the recent delta on top of the already-absolute seed → `X + delta` — the ADDR-09 double-count. #4004 patched this caller-side for the asset-lock top-up only; review of the first cut of this change found the same live bug in three more flows. Fix at the seam instead of per caller: - `reconcile_address_infos` now takes a `credited_outputs` set (built with the new `credited_outputs_set` helper) naming the addresses the transition credited via deltas. When a COMMITTED entry matches, the seam calls `invalidate_sync_watermark()` INSIDE its provider-lock critical section — so no sync pass can interleave between the seed commit and the invalidation with the stale watermark (closing the race window the caller-side pattern left open) — forcing the next pass to full-scan-reconcile. Entries skipped as stale/unchanged do not fire the gate: a sync already applied that credit and advanced the watermark past it. Callers audited and updated: - `transfer` — outputs are credited; a same-wallet output (change address or transfer-to-self) was the double-count shape (CONFIRMED). - `fund_from_asset_lock` — recipients are credited; replaces #4004's caller-side unconditional invalidation. - `transfer_credits_to_addresses` — recipients are credited; the primary use is consolidating identity credits into the wallet's OWN addresses, so this path double-counted too (CONFIRMED, unfixed before this change). - `register_from_addresses` — the optional refund output is credited (CONFIRMED mechanism; reachable when the output is owned). - `top_up_from_addresses` and `withdraw` — pass an empty set: they only drain inputs (absolute ops, idempotent under re-apply) and have no change output today; comments direct any future change output through the gate. Cross-wallet transfers remain unaffected: each wallet's provider tracks only its own wallet (`from_wallets([self.wallet_id])`), so the recipient wallet never receives an optimistic absolute seed write; its own sync applies the delta exactly once. Adds three seam-level tests (gate fires on committed credited output and keeps the seed; watermark preserved for input-only reconciliation; watermark preserved when the credited entry was skipped as already applied) alongside #4004's provider-level `invalidate_sync_watermark_forces_full_scan_keeps_seed`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bezibalazs
pushed a commit
that referenced
this pull request
Jul 5, 2026
…bled (#4004) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 5, 2026
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
2 participants
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.
Issue being fixed or feature implemented
QA ADDR-09: after a successful "Top Up from Core" (Core L1 asset lock → Platform credits on a DIP-17 platform-payment address), the SwiftExampleApp Platform Balance showed ~2× the topped-up amount until a manual Sync-tab Clear + Sync Now corrected it. The on-chain balance was always correct — this was a client-side balance-sync bug in
platform-wallet.What was done?
Root cause:
AddressFundingFromAssetLockTransitionrecords the recipient credit on-chain asAddBalanceToAddress, i.e. a delta (AddToCredits) in Drive's recent-address-balance-changes tree. The fund path reconciled the proof-attested absolute balanceXinto the provider's committedfoundseed but left the incremental sync watermark stale. The next incremental BLAST pass seededresult.foundfromcurrent_balances()(alreadyX) and then re-applied the recentAddToCredits(X)delta on top →X + X = 2X. A full scan (what the manual Clear forces) seeds absolutely from the tree, which is why Clear + resync fixed it.Fix:
PlatformPaymentAddressProvider::invalidate_sync_watermark()— zeroes the incremental watermark only (sync_height/sync_timestamp/last_known_recent_block), keeping the just-reconciledfoundseed so display andauto_select_inputsbudgeting keepXvisible. Withsync_timestamp == 0,last_sync_timestamp()returnsNone, so the next pass takes the full-scan branch and reconciles toX. This is the automated equivalent of the manual Clear + Sync Now.fund_from_asset_lockright afterreconcile_address_infos— covers both the fresh and resume top-up paths (they share one Rust fn).ADDR-09regression-guard row toSwiftExampleApp/TEST_PLAN.md.Durability is intentionally in-memory only: the persisted watermark can't be reset to 0 through the normal changeset (
PlatformAddressChangeSet::mergecombinessync_heightwith.max(), and the persister only fireson_persist_sync_state_fnwhen a component is> 0). The next in-session BLAST pass (~15s) full-scans, reconciles toX, and persists a correct forward watermark, so durable state self-corrects within ~15s. The only residual gap — an app kill inside that window — is documented in-code and accepted rather than over-engineering against the merge/gate.How Has This Been Tested?
invalidate_sync_watermark_forces_full_scan_keeps_seed— passes (asserts all three watermark scalars zero,last_sync_timestamp() == None(the full-scan trigger), and thefoundseed survives with balanceX).platform_addressesmodule suite green (78 tests);cargo fmt --all --checkandcargo clippy -p platform-wallet --all-targetsclean.build_ios.sh --target sim) + SwiftExampleApp build green; app launches with the fix and renders the Platform Balance single-counted.Breaking Changes
None. Client-side balance-sync fix only; no consensus or protocol change.
Related / follow-up
transfer.rsandwithdrawal.rsroute through the samereconcile_address_infosseam and also emitAddBalanceToAddressdeltas. Same-wallet is self-consistent (pairedSetBalanceToAddresson the source), but cross-wallet transfers are a plausible candidate for the same double-count — out of scope here, flagged for a separate investigation.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation