fix(platform): zero cached platform-address balances absent from state#3855
Conversation
After a devnet chain reset, addresses proven absent from platform state kept their cached balances: the sync diff only consumed result.found, the provider never evicted absent addresses from its committed found map, and the in-memory managed-account balance was never zeroed. The stale balance survived re-sync and was offered as an identity-creation funding source. - sync_balances now emits a zeroed entry (balance 0, nonce 0) for every proven-absent address that previously carried cached funds, so the persister writes 0 through to SwiftData - sync_finished removes absent addresses from the committed found map so current_balances() stops re-seeding the stale value - on_address_absent zeroes the managed account's credit balance so spend-path enumeration no longer sees it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces explicit handling for proven-absent platform addresses. When an address is detected as absent, the system now persists a zeroed balance and removes that address from committed funded snapshots to prevent stale state from reappearing in current balance queries after chain resets. ChangesAbsent Address Zeroing and Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
|
✅ Review complete (commit fc96e59) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (1)
707-744:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
per_wallet_in_syncbefore each new sync pass.
sync_finishednow removes every staged absent address from committedfound, butprepare_for_sync()never resetsper_wallet_in_syncafter a failed pass. If one sync aborts after staging an address absent, the next successful pass will remove it here even when it was not absent in that pass, andcompute_address_balance_diff()will not persist that removal because it only sees the currentresult.absent. That leaves in-memory and persisted balance state out of sync.Suggested fix
pub(crate) async fn prepare_for_sync(&mut self) -> Result<(), PlatformWalletError> { + self.per_wallet_in_sync.clear(); let wallet_ids: Vec<WalletId> = self.per_wallet.keys().copied().collect();🤖 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/wallet/platform_addresses/provider.rs` around lines 707 - 744, sync_finished applies staged absent removals from per_wallet_in_sync even if the prior pass aborted, so ensure per_wallet_in_sync is cleared at the start of a new sync pass to avoid applying stale absent entries; add an explicit clear/reset of per_wallet_in_sync in prepare_for_sync() (and any sync-abort/cleanup path) so that prepare_for_sync(), sync_finished(), and compute_address_balance_diff() operate on the same per-pass state (use per_wallet_in_sync.clear() or replace with a fresh default map), referencing the per_wallet_in_sync field, the prepare_for_sync() entry point, and sync_finished()/compute_address_balance_diff() to locate the relevant logic.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs (1)
838-850: ⚡ Quick winAdd one test that calls
on_address_absentend-to-end.
stage_absent()only mutatesper_wallet_in_sync, so the newWalletManagerwrite-through on Lines 685-704 never runs in this suite. A regression in the in-memory zeroing path would still pass these tests.Also applies to: 856-933
🤖 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/wallet/platform_addresses/provider.rs` around lines 838 - 850, Tests currently call stage_absent which only mutates PlatformPaymentAddressProvider::per_wallet_in_sync and thus never exercises the WalletManager write-through performed by on_address_absent; add a single end-to-end test that calls PlatformPaymentAddressProvider::on_address_absent (not stage_absent) so the code path that invokes WalletManager (the write-through path that persists absent addresses for WALLET/ACCOUNT) runs and can catch regressions in the in-memory zeroing/persistence logic; ensure the test sets up a real or test WalletManager instance, uses the same WALLET and ACCOUNT keys, and asserts both the per_wallet_in_sync state and the WalletManager-persisted state after calling on_address_absent.
🤖 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.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs`:
- Around line 707-744: sync_finished applies staged absent removals from
per_wallet_in_sync even if the prior pass aborted, so ensure per_wallet_in_sync
is cleared at the start of a new sync pass to avoid applying stale absent
entries; add an explicit clear/reset of per_wallet_in_sync in prepare_for_sync()
(and any sync-abort/cleanup path) so that prepare_for_sync(), sync_finished(),
and compute_address_balance_diff() operate on the same per-pass state (use
per_wallet_in_sync.clear() or replace with a fresh default map), referencing the
per_wallet_in_sync field, the prepare_for_sync() entry point, and
sync_finished()/compute_address_balance_diff() to locate the relevant logic.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs`:
- Around line 838-850: Tests currently call stage_absent which only mutates
PlatformPaymentAddressProvider::per_wallet_in_sync and thus never exercises the
WalletManager write-through performed by on_address_absent; add a single
end-to-end test that calls PlatformPaymentAddressProvider::on_address_absent
(not stage_absent) so the code path that invokes WalletManager (the
write-through path that persists absent addresses for WALLET/ACCOUNT) runs and
can catch regressions in the in-memory zeroing/persistence logic; ensure the
test sets up a real or test WalletManager instance, uses the same WALLET and
ACCOUNT keys, and asserts both the per_wallet_in_sync state and the
WalletManager-persisted state after calling on_address_absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7c6db68-3d10-42e9-91af-7e701f0cea2c
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rspackages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The fix addresses the real chain-reset stale-balance bug, but the new sync_finished behavior assumes result.found and result.absent are disjoint per sync. The SDK's full-scan-then-incremental-catch-up flow can violate that: an address proven absent at the trunk checkpoint can be funded again before the tip and surface via on_address_found during catch-up. With the new extend-then-remove order, the legitimate new balance is removed from committed found and the diff helper also appends a zeroing persistence entry that wins last. The same exposure exists across syncs because per_wallet_in_sync is never cleared on a failed prior pass.
🔴 2 blocking | 🟡 1 suggestion(s)
2 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/wallet/platform_addresses/provider.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:707-744: Found/absent overlap within a sync silently wipes a freshly funded address
The new `sync_finished` extends committed `found` with scratch `found` and then unconditionally removes every scratch `absent` address from committed `found`. The doc comment justifies extend-then-remove on the premise that the SDK proves an address either present or absent per pass and the two sets are disjoint, but that does not hold for the full-scan-plus-incremental-catch-up flow. In `packages/rs-sdk/src/platform/address_sync/mod.rs`, `process_trunk_result` / `process_branch_result` insert into `result.absent` and call `on_address_absent` when an address is missing at the trunk/branch checkpoint, while `incremental_catch_up`'s compacted (line 646) and recent (line 704) phases later call `on_address_found` with the new balance whenever a credit op for that address shows up between the checkpoint and tip. Neither catch-up path removes the address from `result.absent` or from the provider's scratch `absent` set. The order trunk-absent then catchup-found is exactly what happens when an address is empty at the checkpoint and gets funded shortly afterwards (very plausible right after a chain reset, which is the regression this PR targets). With the new extend-then-remove, the freshly inserted `found` balance is deleted from committed `found`, so subsequent `current_balances()` and `from_persisted` no longer see it.
The symmetric defect is in `compute_address_balance_diff` (sync.rs lines 61-83): the absent loop checks only the pre-sync snapshot, so when `(tag, p2pkh)` is in both `result.found` and `result.absent` and the snapshot had non-default funds, the helper first emits the fresh balance entry and then appends a zero entry. `PlatformAddressChangeSet::addresses` is merged in order downstream, so the zero wins and the persister stores 0 for an address that is funded at the sync tip.
Fix: filter `account_scratch.absent` against `account_scratch.found` before the removal loop, and skip the zero-emission branch in `compute_address_balance_diff` for any `(tag, p2pkh)` that is also in `found`. The contract for the rest of the persistence path should be that an address can never appear in both buckets simultaneously.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:386-436: Clear `per_wallet_in_sync` at the start of each sync pass
`per_wallet_in_sync` is per-pass scratch but `prepare_for_sync` only refreshes `self.pending` and clears the committed `account_state.absent`; it never touches the scratch map. The SDK calls `provider.sync_finished()` only on the success path (`packages/rs-sdk/src/platform/address_sync/mod.rs` line 415), so if a prior `sync_address_balances` returns an error after `on_address_absent` has staged an address, that scratch absent entry survives into the next sync. With this PR the next successful `sync_finished` will now remove that address from committed `found` even if the new pass never re-proved it absent — and if the new pass legitimately found it via catch-up, the freshly committed balance is wiped. Before this PR the leftover scratch was harmless because `sync_finished` only overwrote committed `absent` wholesale; the new destructive removal raises the cost of any aborted sync.
Fix: `self.per_wallet_in_sync.clear();` at the top of `prepare_for_sync` before refreshing pending.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:685-704: New wallet-manager zeroing in `on_address_absent` has no unit test
The new `wallet_manager.write()` + `set_address_credit_balance(p2pkh, 0, None)` block in `on_address_absent` is the in-memory fix for the spend-path symptom (stale balance offered to identity-creation funding). None of the new tests exercise it: `sync_finished_removes_absent_from_committed_found` and `sync_finished_keeps_found_drops_absent` use `stage_absent` to write directly into `per_wallet_in_sync` precisely because the fixture has no wallet registered in the manager. Add a test that registers a `WalletManager` with a managed platform account, seeds a non-zero `address_balance`, calls `on_address_absent`, and asserts the account's `credit_balance` and `address_balances` map have been zeroed. Otherwise this mirror of `on_address_found` is unverified by the test suite.
Review follow-ups for the absent-address zeroing: - An address proven absent at the full-scan checkpoint can be re-found by incremental catch-up in the same pass (the SDK inserts into result.found without retracting the absent proof). The catch-up find reflects the chain tip, so it now wins: compute_address_balance_diff skips zero-emission for addresses also in found, and sync_finished drops them from the scratch absent set before the destructive removal - prepare_for_sync now clears per_wallet_in_sync so scratch staged by an aborted pass (sync_finished only runs on success) can't remove a committed balance the next pass never re-proved absent - build the changeset via struct literal (clippy field_reassign_with_default broke CI under -D warnings) - new tests: overlap at both layers, aborted-pass scratch clearing, and an end-to-end on_address_absent test against a real WalletManager asserting the managed account balance is zeroed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three prior blocking/suggestion findings are fixed at fc96e59: sync_finished now dedupes found-over-absent before applying removals; compute_address_balance_diff skips zero-emission for addresses also in found; prepare_for_sync clears per-pass scratch up front; and a new test exercises on_address_absent against a real wallet manager. Six agents (claude+codex across general/rust-quality/ffi) converge on no new in-scope defects in either the latest delta or the cumulative PR. Approving.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3855 +/- ##
=============================================
- Coverage 87.15% 70.73% -16.43%
=============================================
Files 2641 20 -2621
Lines 327793 2788 -325005
=============================================
- Hits 285701 1972 -283729
+ Misses 42092 816 -41276
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "03a42d74cda9b7583ac620580d6513a8547d07680541f30916e37bde0dbfe99a"
)Xcode manual integration:
|
2 participants
Issue being fixed or feature implemented
After the paloma devnet platform chain was reset, SwiftExampleApp kept displaying a stale platform-address balance (e.g. 294,627,247,940 credits / "2.94627248 DASH") from the old chain, and — worse — offered it as an identity-creation funding source that would fail at broadcast. Clearing Platform Sync Status and re-syncing against the new chain did not help: on the new chain
getAddressInfoproves the address absent from state (entry withoutbalance_and_nonce), but the balance refresh only overwrote cached rows on positive finds, so the stale balance was never zeroed.The SDK sync engine already reports proven-absent addresses (
AddressSyncResult.absent), butrs-platform-walletignored that signal in three places:PlatformAddressWallet::sync_balancesbuilt the persistence changeset only fromresult.found, so no zeroing entry ever reached the persister (→ FFI → SwiftData).PlatformPaymentAddressProvider::sync_finishedmerged scratchfoundentries but never removed proven-absent addresses from the committedfoundmap, socurrent_balances()kept re-seeding the stale balance into every subsequent sync.on_address_absentnever updated key-wallet's managed account, so the in-memory balance used by spend-path enumeration (identity-creation funding) kept the stale value.What was done?
All changes are in
rs-platform-wallet(per the project convention that the Swift layer only persists what the FFI hands it):compute_address_balance_diff(before, found, absent)inwallet/platform_addresses/sync.rs. It emits found-and-changed entries as before, plus a zeroed entry (balance 0, nonce 0) for every proven-absent address that previously carried non-default cached funds. An absent address has no balance and no nonce in state, so both are reset.sync_finishedinwallet/platform_addresses/provider.rsnow removes every address proven absent this pass from the committedfoundmap (extend-then-remove, so an absent proof wins in the impossible-by-contract overlap case).on_address_absentnow zeroes the managed account's credit balance viaset_address_credit_balance(p2pkh, 0, None), mirroringon_address_found's wallet-manager write. TheNonekey source is deliberate: gap-limit extension only fires on an unfunded→funded transition, which zeroing never is.No Swift changes were needed:
PlatformAddressChangeSet::is_empty, the FFI mapping inrs-platform-wallet-ffi, and Swift'sPlatformWalletPersistenceHandler.persistAddressBalancesall pass zero-balance entries through unconditionally, so thePersistentPlatformAddressrow is updated to 0 end-to-end.Note: the Platform Sync Status "Clear" button only resets in-memory display state and was left untouched — with this fix the stale balance is zeroed by the next sync pass itself, which is the layer that owns the decision.
How Has This Been Tested?
New unit tests in
rs-platform-wallet:sync::tests::absent_previously_funded_address_emits_zero_entry— the chain-reset case: cached balance + proven absent → single zeroed entry (balance 0, nonce 0)sync::tests::absent_never_funded_address_emits_nothingsync::tests::absent_zero_balance_nonzero_nonce_emits_zero_entrysync::tests::absent_default_funds_emits_nothingsync::tests::found_changed_emitted_unchanged_skippedsync::tests::found_and_absent_combineprovider::tests::sync_finished_removes_absent_from_committed_found— absent address evicted from the committedfoundmap and no longer yielded bycurrent_balances()provider::tests::sync_finished_keeps_found_drops_absentVerification:
cargo fmt --all;cargo check -p platform-wallet --all-features --tests; fullcargo test -p platform-walletsuite passing (228 lib + 8 integration); iOS framework build (./build_ios.sh --target sim) and SwiftExampleApp simulator build both succeed.Breaking Changes
None. Not consensus-breaking — client-side wallet sync/persistence behavior only.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit