fix(sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025)#3650
fix(sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025)#3650lklimek wants to merge 11 commits into
Conversation
… for post-snapshot addresses (Found-025) `incremental_catch_up` built its `key_to_tag` lookup once from a single pre-RPC `provider.pending_addresses()` snapshot and passed it by immutable reference into both apply loops. The `if let Some(..) = address_lookup.get(..)` predicate had no `else`, so any balance change the platform returned for an address derived *after* the snapshot was dropped with no log, metric, or error — `result.found` never got it and `on_address_found` was never called. Under concurrent multi-identity funding the derive-fund-sync interleave is routine, which is why e2e gates TK-001/007/013/014 and id_005 flaked here. Extract the two inline apply loops into a pure `pub(crate) apply_address_changes` seam (no Sdk, no network, no async) that returns applied updates plus the addresses absent from the snapshot. The new `apply_block_changes` re-polls `pending_addresses()` when an unknown address appears (mirroring the tree-scan refresh) and replays only the previously-unknown subset, so a fresh receive address is recovered and known-address `AddToCredits` deltas are never double-counted. An address still unknown after the refresh is logged at `warn` — observable, never silently dropped. Known-address behavior is byte-for-byte identical. Adds three deterministic `#[cfg(test)]` regression guards on the pure seam (no proof/Sdk needed): unknown-address surfacing, post-snapshot recovery through the refresh, and delta double-count safety. All three fail on the pre-fix silent-discard logic and pass post-fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughBuffers unknown address balance changes during incremental catch-up, applies known entries via a shared seam, re-polls ChangesAddress Balance Change Recovery with Post-Snapshot Resolution
Sequence DiagramsequenceDiagram
participant Caller as incremental_catch_up
participant ApplyBlock as apply_block_changes
participant AddressLookup as key_to_tag_lookup
participant Provider as provider
Caller->>ApplyBlock: pass change entry + current_height + result + pending_unknown
ApplyBlock->>AddressLookup: lookup key -> known? compute applied delta
AddressLookup-->>ApplyBlock: known (tag) or miss
ApplyBlock->>Provider: on_address_found for newly-known keys
Note over ApplyBlock,Provider: misses serialized into pending_unknown
Caller->>Provider: refresh_and_replay_unknown -> pending_addresses() (bounded loop)
Provider-->>Caller: reported pending addresses
Caller->>AddressLookup: extend lookup with reported addresses (extras)
Caller->>ApplyBlock: replay buffered changes for resolved keys
ApplyBlock->>Provider: on_address_found for replay-resolved keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
Review GateCommit:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3650 +/- ##
=========================================
Coverage 87.16% 87.16%
=========================================
Files 2607 2607
Lines 319420 319420
=========================================
Hits 278413 278413
Misses 41007 41007
🚀 New features to boost your workflow:
|
lklimek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-sdk/src/platform/address_sync/mod.rs`:
- Around line 779-786: When you insert or update a found entry in result.found
(using result_key and AddressFunds with nonce and new_balance) also remove any
stale entry for that same key from result.absent so the final AddressSyncResult
cannot contain the same (tag, address) in both collections; specifically, after
creating funds and calling result.found.insert(result_key, funds) (the same
place where applied.push((tag, address, funds)) is invoked), call
result.absent.remove(&result_key) to clear the absent marker for that address.
🪄 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: ac0a1e08-9fbf-438e-bce0-5574ebcf9959
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/address_sync/mod.rs
|
✅ 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: "9189a85d34df70641664318044487ea885bfee666d563365839225f4bb3a811f"
)Xcode manual integration:
|
…-001) The tree scan can prove an address absent (`result.absent.insert(...)` at L171 / L223) and a later chain-confirmed balance change in the catch-up phase can legitimately re-discover that same address. The catch-up path only wrote to `result.found`, leaving the stale `absent` marker in place — the same `(tag, address)` then appeared in both sets and the final `AddressSyncResult` was internally inconsistent. Drop the stale `absent` entry before inserting into `found`. Symmetric direction (`found → absent`) doesn't exist: no `result.found.remove` call is reachable from any code path, and "absent" means proven-not-in- tree (not "balance is 0"), so a found-with-balance-0 stays in `found`. Adds two regression tests: one at the `apply_address_changes` seam and one at the `apply_block_changes` level, both asserting `found` and `absent` are disjoint after the catch-up rediscovers a previously-absent address. Addresses CodeRabbit #3650 (comment) 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly fixes a real silent-discard bug in incremental_catch_up, and the pure apply_address_changes seam is well-structured. However, one blocking issue: the server-side gRPC returns address balance changes for the entire chain (no per-wallet filter), so every other wallet's address triggers the new 'unknown' branch — producing per-block pending_addresses() refresh storms and warn-level log spam on any multi-user chain. The new tests never stage this dominant real-world case. A secondary concern: newly-discovered addresses are persisted with nonce=0 since the incremental RPCs don't carry nonces; not a new bug, but the Found-025 path now exposes more addresses to it.
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
7 additional finding(s)
blocking: Per-block refresh + warn log fires for every other wallet's address on a multi-user chain
packages/rs-sdk/src/platform/address_sync/mod.rs (line 833)
GetRecentAddressBalanceChangesRequest (platform.proto:2765) and GetRecentCompactedAddressBalanceChangesRequest (platform.proto:2824) carry no per-wallet filter — only start_height, prove, and start_height_exclusive. The server (rs-drive-abci/src/query/address_funds/recent_address_balance_changes/v0/mod.rs) returns every address balance change in the block range for every wallet on the chain. The pre-fix code's if let Some(..) = address_lookup.get(..) predicate, with no else, was effectively the client-side wallet filter: 'not in my snapshot → not my address → ignore'.
The new apply_block_changes treats every server-returned address absent from the snapshot as a Found-025 candidate. On a populated chain, every block with cross-wallet activity:
- Marks those addresses as
outcome.unknown, - Triggers a
provider.pending_addresses()re-poll plus aHashSet<&[u8]>build, - Hits the
address_lookup.len() == beforebranch (the wallet rarely derives mid-pass) and emits awarn!per block claiming addresses 'will be resolved on the next full sync (Found-025)' — falsely suggesting missing data when the addresses simply belong to other wallets.
The Found-025 race is real, but the discriminator 'not in my snapshot' is exactly the same predicate that legitimately discards every other wallet's activity. Needed: a discriminator that distinguishes 'this is an address I have derived but isn't in my entry-time snapshot' from 'this is some other wallet's address'. Options: (a) accumulate unknown across the whole pass and do a single end-of-pass refresh, (b) extend AddressProvider with a cheap tracks_key(&[u8]) -> bool to filter foreign addresses before counting them as unknown, or (c) only treat an address as Found-025 if it appears in the post-refresh pending_addresses().
suggestion: New regression guards never exercise non-wallet addresses in the server response
packages/rs-sdk/src/platform/address_sync/mod.rs (line 1538)
All three Found-025 guards stage scenarios where every unknown address is a tracked-but-late wallet address the next pending_addresses() call will surface. None feed apply_block_changes a change set containing addresses the provider never has and never will (i.e., another wallet's address on a shared chain) — the dominant case in production once the SDK talks to a populated address_funds tree. A guard for that case — provider snapshot stable, change set contains a non-wallet address, expect no per-block refresh storm and no warn-level logging — would have caught the issue above and would pin the desired filter behavior going forward.
suggestion: Newly-discovered addresses persisted with nonce=0 may stale-overwrite real on-chain nonce
packages/rs-sdk/src/platform/address_sync/mod.rs (line 769)
When apply_block_changes replays an address that was missing from the entry-time snapshot, result.found has no prior entry for that (tag, address), so result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0) synthesizes nonce=0. The incremental RPCs (GetRecentAddressBalanceChanges{,Compacted}) carry only balance deltas, never nonces. Scenario: wallet derives address A, funds it (nonce stays 0), spends from it (nonce becomes 1 on-chain), then this sync persists A with the correct balance but nonce=0. If the wallet writes result.found into a long-lived PlatformAddressChangeSet, a subsequent address-signed transition can fail with InvalidAddressNonce.
This is not a regression strictly introduced by this PR — the same nonce=0 fallback applies to any newly-active address whose first appearance is through incremental sync — but the Found-025 path now exposes more addresses to it. Pre-fix, those addresses were silently dropped; persisting them with a synthetic nonce is on balance better, but consider either (a) fetching authoritative AddressFunds for addresses recovered through the refresh, or (b) leaving them out of result.found and flagging them for full-sync resolution.
suggestion: Per-block Vec materialization runs even when there are zero unknown addresses
packages/rs-sdk/src/platform/address_sync/mod.rs (line 821)
apply_block_changes unconditionally materializes changes into a Vec<(&PlatformAddress, AddressBalanceChange<'_>)> (line 821) so the rare replay-after-refresh path can re-iterate. On the dominant hot path — every block where the platform reports only known addresses — the replay branch is never taken and the Vec allocation is pure overhead added by this PR. This function runs once per compacted entry and once per recent entry per catch-up, so on long catch-ups it's many allocations the pre-fix loop did not pay. Run the first pass with the borrowed iterator directly, and only buffer the small (address, change) subset for unknown items inside the rare unknown branch.
suggestion: Unconditional clone of key_to_tag even when no refresh runs
packages/rs-sdk/src/platform/address_sync/mod.rs (line 466)
let mut address_lookup: HashMap<Vec<u8>, (P::Tag, P::Address)> = key_to_tag.clone(); clones the full snapshot at entry on every incremental_catch_up call so the apply loops can mutate it. For wallets with thousands of pending addresses this is a per-sync constant tax even though the Found-025 refresh path is rare. A Cow<'_, HashMap<...>> pattern (clone-on-first-refresh) would let the common path use the borrowed map directly and only pay the clone when an unknown address triggers a refresh.
suggestion: `address_lookup.len() == before` is an imprecise proxy for 'unknown addresses now resolvable'
packages/rs-sdk/src/platform/address_sync/mod.rs (line 842)
If provider.pending_addresses() adds new addresses on refresh but none match the bytes in outcome.unknown, address_lookup.len() > before is true and the code proceeds into the replay path; the replay surfaces the unchanged unknown set, fires the second warn! at lines 873–880, and the first warn is skipped — net: redundant alloc + filter, two warn paths to maintain. Tighter check: outcome.unknown.iter().any(|k| address_lookup.contains_key(k)) skips the replay entirely when the refresh added only unrelated entries.
nitpick: Warn message phrasing implies the next sync will fix it, but it may not
packages/rs-sdk/src/platform/address_sync/mod.rs (line 849)
Both warns claim 'they will be resolved on the next full sync (Found-025)', implying a normally-scheduled later sync resolves the gap. If the address is truly unknown to the provider (foreign address, or a key the wallet never derived), no future sync will resolve it — only the application deriving/registering the address will. Distinguish 'refresh produced no new candidates' from 'will be resolved next sync' to avoid misleading operators chasing nonexistent missing data.
🤖 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.
- [BLOCKING] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:833-881: Per-block refresh + warn log fires for every other wallet's address on a multi-user chain
`GetRecentAddressBalanceChangesRequest` (platform.proto:2765) and `GetRecentCompactedAddressBalanceChangesRequest` (platform.proto:2824) carry no per-wallet filter — only `start_height`, `prove`, and `start_height_exclusive`. The server (`rs-drive-abci/src/query/address_funds/recent_address_balance_changes/v0/mod.rs`) returns every address balance change in the block range for every wallet on the chain. The pre-fix code's `if let Some(..) = address_lookup.get(..)` predicate, with no `else`, was effectively the client-side wallet filter: 'not in my snapshot → not my address → ignore'.
The new `apply_block_changes` treats every server-returned address absent from the snapshot as a Found-025 candidate. On a populated chain, every block with cross-wallet activity:
1. Marks those addresses as `outcome.unknown`,
2. Triggers a `provider.pending_addresses()` re-poll plus a `HashSet<&[u8]>` build,
3. Hits the `address_lookup.len() == before` branch (the wallet rarely derives mid-pass) and emits a `warn!` per block claiming addresses 'will be resolved on the next full sync (Found-025)' — falsely suggesting missing data when the addresses simply belong to other wallets.
The Found-025 race is real, but the discriminator 'not in my snapshot' is exactly the same predicate that legitimately discards every other wallet's activity. Needed: a discriminator that distinguishes 'this is an address I have derived but isn't in my entry-time snapshot' from 'this is some other wallet's address'. Options: (a) accumulate `unknown` across the whole pass and do a single end-of-pass refresh, (b) extend `AddressProvider` with a cheap `tracks_key(&[u8]) -> bool` to filter foreign addresses before counting them as unknown, or (c) only treat an address as Found-025 if it appears in the post-refresh `pending_addresses()`.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:1538-1806: New regression guards never exercise non-wallet addresses in the server response
All three Found-025 guards stage scenarios where every unknown address *is* a tracked-but-late wallet address the next `pending_addresses()` call will surface. None feed `apply_block_changes` a change set containing addresses the provider never has and never will (i.e., another wallet's address on a shared chain) — the dominant case in production once the SDK talks to a populated `address_funds` tree. A guard for that case — provider snapshot stable, change set contains a non-wallet address, expect no per-block refresh storm and no warn-level logging — would have caught the issue above and would pin the desired filter behavior going forward.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:769-787: Newly-discovered addresses persisted with nonce=0 may stale-overwrite real on-chain nonce
When `apply_block_changes` replays an address that was missing from the entry-time snapshot, `result.found` has no prior entry for that `(tag, address)`, so `result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0)` synthesizes nonce=0. The incremental RPCs (`GetRecentAddressBalanceChanges{,Compacted}`) carry only balance deltas, never nonces. Scenario: wallet derives address A, funds it (nonce stays 0), spends from it (nonce becomes 1 on-chain), then this sync persists A with the correct balance but nonce=0. If the wallet writes `result.found` into a long-lived `PlatformAddressChangeSet`, a subsequent address-signed transition can fail with `InvalidAddressNonce`.
This is not a regression strictly introduced by this PR — the same nonce=0 fallback applies to any newly-active address whose first appearance is through incremental sync — but the Found-025 path now exposes more addresses to it. Pre-fix, those addresses were silently dropped; persisting them with a synthetic nonce is on balance better, but consider either (a) fetching authoritative `AddressFunds` for addresses recovered through the refresh, or (b) leaving them out of `result.found` and flagging them for full-sync resolution.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:821-832: Per-block Vec materialization runs even when there are zero unknown addresses
`apply_block_changes` unconditionally materializes `changes` into a `Vec<(&PlatformAddress, AddressBalanceChange<'_>)>` (line 821) so the rare replay-after-refresh path can re-iterate. On the dominant hot path — every block where the platform reports only known addresses — the replay branch is never taken and the Vec allocation is pure overhead added by this PR. This function runs once per compacted entry and once per recent entry per catch-up, so on long catch-ups it's many allocations the pre-fix loop did not pay. Run the first pass with the borrowed iterator directly, and only buffer the small `(address, change)` subset for unknown items inside the rare unknown branch.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:466-466: Unconditional clone of key_to_tag even when no refresh runs
`let mut address_lookup: HashMap<Vec<u8>, (P::Tag, P::Address)> = key_to_tag.clone();` clones the full snapshot at entry on every `incremental_catch_up` call so the apply loops can mutate it. For wallets with thousands of pending addresses this is a per-sync constant tax even though the Found-025 refresh path is rare. A `Cow<'_, HashMap<...>>` pattern (clone-on-first-refresh) would let the common path use the borrowed map directly and only pay the clone when an unknown address triggers a refresh.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:842-857: `address_lookup.len() == before` is an imprecise proxy for 'unknown addresses now resolvable'
If `provider.pending_addresses()` adds new addresses on refresh but none match the bytes in `outcome.unknown`, `address_lookup.len() > before` is true and the code proceeds into the replay path; the replay surfaces the unchanged unknown set, fires the second `warn!` at lines 873–880, and the first warn is skipped — net: redundant alloc + filter, two warn paths to maintain. Tighter check: `outcome.unknown.iter().any(|k| address_lookup.contains_key(k))` skips the replay entirely when the refresh added only unrelated entries.
- [NITPICK] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:849-880: Warn message phrasing implies the next sync will fix it, but it may not
Both warns claim 'they will be resolved on the next full sync (Found-025)', implying a normally-scheduled later sync resolves the gap. If the address is truly unknown to the provider (foreign address, or a key the wallet never derived), no future sync will resolve it — only the application deriving/registering the address will. Distinguish 'refresh produced no new candidates' from 'will be resolved next sync' to avoid misleading operators chasing nonexistent missing data.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new push correctly resolves CodeRabbit's CMT-001 (added result.absent.remove(&result_key) at line 789 plus two regression tests, keeping found and absent disjoint). However, the prior blocking concern — apply_block_changes treating every server-returned address absent from the entry-time snapshot as a Found-025 candidate — is unchanged. Because GetRecentAddressBalanceChangesRequestV0 and GetRecentCompactedAddressBalanceChangesRequestV0 carry no per-wallet filter (verified in platform.proto:2766–2828), foreign addresses on any populated multi-wallet chain will trigger per-block pending_addresses() re-polls and warn-level log spam. All other prior findings remain valid against aef52d9.
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
7 additional finding(s)
blocking: Per-block refresh + warn log fires for every other wallet's address on a multi-user chain
packages/rs-sdk/src/platform/address_sync/mod.rs (line 838)
Verified at aef52d9 lines 838–886 (unchanged by this delta). GetRecentAddressBalanceChangesRequestV0 (platform.proto:2766) and GetRecentCompactedAddressBalanceChangesRequestV0 (platform.proto:2825) carry only start_height/start_block_height, prove, and start_height_exclusive — no per-wallet filter. The server returns every address balance change in the block range for every wallet on the chain. Pre-fix, the missing else on if let Some(..) = address_lookup.get(..) was the de facto client-side wallet filter: not in my snapshot → not my address → ignore.
apply_block_changes now treats every server-returned address absent from the snapshot as a Found-025 candidate. On a populated chain, every block with cross-wallet activity:
- Adds those addresses to
outcome.unknown. - Re-polls
provider.pending_addresses()and rebuilds aHashSet<&[u8]>. - Almost always lands in the
address_lookup.len() == beforebranch and emits awarn!claiming addresses 'will be resolved on the next full sync (Found-025)' — falsely suggesting missing data when the addresses simply belong to other wallets.
The Found-025 race is real, but not in my snapshot is exactly the predicate that legitimately discards every other wallet's activity. A distinct discriminator is needed: e.g. (a) accumulate unknown across the whole pass and refresh once at the end; (b) extend AddressProvider with a cheap tracks_key(&[u8]) -> bool; or (c) only treat an address as Found-025 if it appears in the post-refresh pending_addresses() set.
suggestion: Regression guards never exercise non-wallet (foreign) addresses in the server response
packages/rs-sdk/src/platform/address_sync/mod.rs (line 1718)
Verified at aef52d9: the three Found-025 guards and the two new CMT-001 guards all stage scenarios where every address the server reports is one the provider does or will surface. The new cmt_001_apply_block_changes_keeps_found_and_absent_disjoint (1782–1846) uses a NoopProvider whose pending_addresses() is empty and pre-inserts the address into lookup, so the unknown/replay path is never exercised. None of the tests feed apply_block_changes a change set containing addresses the provider never has and never will — the dominant production case on a shared chain. A test that pins this invariant (provider snapshot stable, change set contains a non-wallet address, expect no per-block refresh storm and no warn-level logging) would catch the blocking issue above and lock in the desired filter behavior.
suggestion: Newly-discovered addresses persisted with nonce=0 may stale-overwrite real on-chain nonce
packages/rs-sdk/src/platform/address_sync/mod.rs (line 769)
Verified at aef52d9 line 780. When apply_address_changes applies a balance change for an address with no prior result.found entry, result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0) synthesizes nonce=0. The incremental RPCs (GetRecentAddressBalanceChanges{,Compacted}) carry only balance deltas, never nonces. Scenario: wallet derives address A, funds it (nonce stays 0), spends from it (nonce becomes 1 on-chain), then this sync persists A with the correct balance but nonce=0. If the wallet writes result.found into a long-lived PlatformAddressChangeSet, a subsequent address-signed transition can fail with InvalidAddressNonce. The new absent.remove at line 789 widens the persistence surface slightly without addressing the nonce-source gap. Options: (a) fetch authoritative AddressFunds for addresses recovered through the refresh; (b) leave them out of result.found and flag for full-sync resolution; or (c) mark the nonce as unknown at the type level.
suggestion: Per-block Vec materialization runs even when there are zero unknown addresses
packages/rs-sdk/src/platform/address_sync/mod.rs (line 821)
Verified at aef52d9 line 826: apply_block_changes unconditionally materializes changes into a Vec<(&PlatformAddress, AddressBalanceChange<'_>)> so the rare replay-after-refresh path can re-iterate. On the dominant hot path — every block where the platform reports only known addresses — the function returns at line 838 and the allocation is pure overhead added by this PR. This runs once per compacted entry and once per recent entry per catch-up, so on long catch-ups it accumulates many allocations. Run the first pass with the borrowed iterator directly, and only buffer the small (address, change) subset for unknown items inside the rare unknown branch.
suggestion: Unconditional clone of key_to_tag even when no refresh runs
packages/rs-sdk/src/platform/address_sync/mod.rs (line 466)
Verified at aef52d9 line 466: let mut address_lookup: HashMap<Vec<u8>, (P::Tag, P::Address)> = key_to_tag.clone(); clones the full snapshot at entry on every incremental_catch_up call so the apply loops can mutate it. For wallets with thousands of pending addresses this is a full per-sync HashMap<Vec<u8>, ...> copy that duplicates every key buffer and (tag, address) payload, even though the Found-025 refresh path is rare. A Cow<'_, HashMap<...>> or clone-on-first-refresh pattern would let the common path use the borrowed map directly and only pay the clone when an unknown address triggers a refresh.
suggestion: `address_lookup.len() == before` is an imprecise proxy for 'unknown addresses now resolvable'
packages/rs-sdk/src/platform/address_sync/mod.rs (line 847)
Verified at aef52d9 lines 847–862. If provider.pending_addresses() adds new addresses on refresh but none match the bytes in outcome.unknown, address_lookup.len() > before is true and the code proceeds into the replay path; the replay surfaces the unchanged unknown set, fires the second warn! at lines 878–885, and the first warn is skipped — net: redundant allocation + filter, two warn paths to maintain. A tighter check — outcome.unknown.iter().any(|k| address_lookup.contains_key(k)) after the refresh — skips the replay entirely when the refresh added only unrelated entries and pins the algorithm's intent.
nitpick: Warn message phrasing implies the next sync will fix it, but it may not
packages/rs-sdk/src/platform/address_sync/mod.rs (line 854)
Verified at aef52d9 lines 855–860 and 878–885. Both warns claim 'they will be resolved on the next full sync (Found-025)', implying a normally-scheduled later sync resolves the gap. If the address is truly unknown to the provider (foreign address, or a key the wallet never derived), no future sync will resolve it — only the application deriving/registering the address will. Distinguish 'refresh produced no new candidates' from 'will be resolved next sync' to avoid misleading operators chasing nonexistent missing data.
🤖 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.
- [BLOCKING] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:838-886: Per-block refresh + warn log fires for every other wallet's address on a multi-user chain
Verified at aef52d95 lines 838–886 (unchanged by this delta). `GetRecentAddressBalanceChangesRequestV0` (platform.proto:2766) and `GetRecentCompactedAddressBalanceChangesRequestV0` (platform.proto:2825) carry only `start_height`/`start_block_height`, `prove`, and `start_height_exclusive` — no per-wallet filter. The server returns every address balance change in the block range for every wallet on the chain. Pre-fix, the missing `else` on `if let Some(..) = address_lookup.get(..)` was the de facto client-side wallet filter: `not in my snapshot → not my address → ignore`.
`apply_block_changes` now treats every server-returned address absent from the snapshot as a Found-025 candidate. On a populated chain, every block with cross-wallet activity:
1. Adds those addresses to `outcome.unknown`.
2. Re-polls `provider.pending_addresses()` and rebuilds a `HashSet<&[u8]>`.
3. Almost always lands in the `address_lookup.len() == before` branch and emits a `warn!` claiming addresses 'will be resolved on the next full sync (Found-025)' — falsely suggesting missing data when the addresses simply belong to other wallets.
The Found-025 race is real, but `not in my snapshot` is exactly the predicate that legitimately discards every other wallet's activity. A distinct discriminator is needed: e.g. (a) accumulate `unknown` across the whole pass and refresh once at the end; (b) extend `AddressProvider` with a cheap `tracks_key(&[u8]) -> bool`; or (c) only treat an address as Found-025 if it appears in the post-refresh `pending_addresses()` set.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:1718-1846: Regression guards never exercise non-wallet (foreign) addresses in the server response
Verified at aef52d95: the three Found-025 guards and the two new CMT-001 guards all stage scenarios where every address the server reports is one the provider does or will surface. The new `cmt_001_apply_block_changes_keeps_found_and_absent_disjoint` (1782–1846) uses a `NoopProvider` whose `pending_addresses()` is empty and pre-inserts the address into `lookup`, so the unknown/replay path is never exercised. None of the tests feed `apply_block_changes` a change set containing addresses the provider never has and never will — the dominant production case on a shared chain. A test that pins this invariant (provider snapshot stable, change set contains a non-wallet address, expect no per-block refresh storm and no warn-level logging) would catch the blocking issue above and lock in the desired filter behavior.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:769-791: Newly-discovered addresses persisted with nonce=0 may stale-overwrite real on-chain nonce
Verified at aef52d95 line 780. When `apply_address_changes` applies a balance change for an address with no prior `result.found` entry, `result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0)` synthesizes nonce=0. The incremental RPCs (`GetRecentAddressBalanceChanges{,Compacted}`) carry only balance deltas, never nonces. Scenario: wallet derives address A, funds it (nonce stays 0), spends from it (nonce becomes 1 on-chain), then this sync persists A with the correct balance but nonce=0. If the wallet writes `result.found` into a long-lived `PlatformAddressChangeSet`, a subsequent address-signed transition can fail with `InvalidAddressNonce`. The new `absent.remove` at line 789 widens the persistence surface slightly without addressing the nonce-source gap. Options: (a) fetch authoritative `AddressFunds` for addresses recovered through the refresh; (b) leave them out of `result.found` and flag for full-sync resolution; or (c) mark the nonce as unknown at the type level.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:821-836: Per-block Vec materialization runs even when there are zero unknown addresses
Verified at aef52d95 line 826: `apply_block_changes` unconditionally materializes `changes` into a `Vec<(&PlatformAddress, AddressBalanceChange<'_>)>` so the rare replay-after-refresh path can re-iterate. On the dominant hot path — every block where the platform reports only known addresses — the function returns at line 838 and the allocation is pure overhead added by this PR. This runs once per compacted entry and once per recent entry per catch-up, so on long catch-ups it accumulates many allocations. Run the first pass with the borrowed iterator directly, and only buffer the small `(address, change)` subset for unknown items inside the rare unknown branch.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:466-466: Unconditional clone of key_to_tag even when no refresh runs
Verified at aef52d95 line 466: `let mut address_lookup: HashMap<Vec<u8>, (P::Tag, P::Address)> = key_to_tag.clone();` clones the full snapshot at entry on every `incremental_catch_up` call so the apply loops can mutate it. For wallets with thousands of pending addresses this is a full per-sync `HashMap<Vec<u8>, ...>` copy that duplicates every key buffer and `(tag, address)` payload, even though the Found-025 refresh path is rare. A `Cow<'_, HashMap<...>>` or clone-on-first-refresh pattern would let the common path use the borrowed map directly and only pay the clone when an unknown address triggers a refresh.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:847-862: `address_lookup.len() == before` is an imprecise proxy for 'unknown addresses now resolvable'
Verified at aef52d95 lines 847–862. If `provider.pending_addresses()` adds new addresses on refresh but none match the bytes in `outcome.unknown`, `address_lookup.len() > before` is true and the code proceeds into the replay path; the replay surfaces the unchanged unknown set, fires the second `warn!` at lines 878–885, and the first warn is skipped — net: redundant allocation + filter, two warn paths to maintain. A tighter check — `outcome.unknown.iter().any(|k| address_lookup.contains_key(k))` after the refresh — skips the replay entirely when the refresh added only unrelated entries and pins the algorithm's intent.
- [NITPICK] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:854-885: Warn message phrasing implies the next sync will fix it, but it may not
Verified at aef52d95 lines 855–860 and 878–885. Both warns claim 'they will be resolved on the next full sync (Found-025)', implying a normally-scheduled later sync resolves the gap. If the address is truly unknown to the provider (foreign address, or a key the wallet never derived), no future sync will resolve it — only the application deriving/registering the address will. Distinguish 'refresh produced no new candidates' from 'will be resolved next sync' to avoid misleading operators chasing nonexistent missing data.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Re: @thepastaclaw review at sha Status per finding (no fix landed yet — surfacing scope before acting):
Nothing else inline on the PR — only thread (CodeRabbit CMT-001) is resolved. Awaiting direction on whether the HIGH item should land here or in a separate PR; the rest are LOW/MEDIUM and reasonable to defer. 🤖 Co-authored by Claudius the Magnificent AI Agent |
|
Thanks for verifying and surfacing the scope. Direction: the HIGH multi-wallet false-positive should land in this PR before merge, not as a separate follow-up. It is a behavioral regression introduced by the Found-025 recovery path: previously foreign chain activity stayed quiet; this branch can turn it into refreshes + warnings on every populated-chain block. I agree with option (b), with one small shape preference: make the provider API return the tracked entry if possible, not just a bool, e.g.
A default implementation can be based on Please also add the regression guard for the dominant foreign-address case: stable provider, server response contains a never-tracked address, expect no For the rest:
|
…s (CMT-001/CMT-004/CMT-005/CMT-006/CMT-007)
Pre-fix `apply_block_changes` refreshed `provider.pending_addresses()`
once per block and emitted a `warn!` claiming "addresses will be
resolved on the next full sync (Found-025)" for every snapshot miss.
`GetRecentAddressBalanceChanges{,Compacted}RequestV0` carry no
per-wallet filter, so on a populated multi-wallet chain every other
wallet's address tripped that branch — refresh storm + warn-log flood
on the operator's box for legitimately-not-mine addresses.
This rewrites the catch-up flow to do exactly one refresh per call:
* `apply_block_changes` now takes a borrowed `&HashMap<...>` lookup and
appends each miss to a caller-provided `Vec<PendingUnknownChange>`.
It still applies all known-address hits synchronously so the per-block
cursor / delta semantics are preserved.
* `incremental_catch_up` threads one `pending_unknown` buffer through
both phases (compacted then recent) and calls a new
`refresh_and_replay_unknown` exactly once at end-of-pass.
* `refresh_and_replay_unknown` polls `pending_addresses()` once, keeps
only those whose key actually matches a buffered miss (CMT-006: a
precise intersection check replaces the imprecise `len() == before`
proxy), and replays only those entries. Foreign addresses fall out at
the intersection step — no allocation beyond the buffered misses
themselves.
* Foreign-address logging is demoted from `warn!` to `debug!` and the
message no longer claims "next full sync will resolve it" (CMT-007 —
no sync resolves an address the wallet never derived).
* The unconditional `address_lookup = key_to_tag.clone()` is gone
(CMT-005), as is the unconditional per-block `Vec` materialization
(CMT-004) — only buffered misses allocate, and `key_to_tag` is shared
read-only through both apply phases.
Also adds a `TODO(CMT-002)` at the two nonce=0 synthesis sites
(per-block apply + replay): the incremental RPCs carry no nonces, so
addresses first surfaced through this path persist with `nonce=0` and
clients must rely on `AddressInvalidNonceError.expected_nonce` to
recover. Resolution wants either authoritative `AddressFunds` fetch for
recovered addresses or modelling `nonce` as `Option<u32>` — deferred
out of this PR (cross-cutting change).
The pre-refactor `pub(crate) fn apply_address_changes` /
`AppliedAddressChanges` seam is removed — the only callers were two
unit tests with redundant invariants now covered by the end-to-end
`apply_block_changes` + `refresh_and_replay_unknown` regression guards.
Existing CMT-001 / Found-025 regression tests (recovers post-snapshot
address, no double-counted delta, found ∩ absent disjoint) updated to
call the new two-step API; all pass. CMT-003 regression guard (foreign
addresses ignored, no refresh storm) follows in the next commit.
Addresses thepastaclaw review at #3650 (review)
🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
…block_changes (CMT-003) Pins the CMT-001 fix from the previous commit at the test surface: four "blocks" worth of changes — one for a known wallet address, three for foreign-wallet addresses — must result in (a) zero per-block `pending_addresses()` polls (the refresh is end-of-pass only), (b) exactly one `pending_addresses()` poll from `refresh_and_replay_unknown`, (c) no foreign address in `result.found`, (d) no foreign address in `result.absent`, (e) `on_address_found` fired exactly once (for the known address), and (f) `found` and `absent` still globally disjoint. Pre-refactor this test would have failed on the poll count (per-block refresh storm) and would have stuffed each foreign address through the `unknown` channel + warn-log path. Now it pins the corrected shape on the dominant production case (multi-wallet shared chain). Addresses thepastaclaw #3650 (review) 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
|
Triage of @thepastaclaw review (id 4335687567) landed in two commits:
Per-finding status
One small calibration on the briefThe brief mentioned "at the two pre-existing sites — find them via Quality gate
🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Verified the follow-up commits (84518fc + 9eede09) against my previous review: the high multi-wallet/foreign-address refresh issue is fixed by buffering unknowns and doing one end-of-pass replay, the hot-path allocation/replay/warn nits are addressed, and CMT-002 is documented as out-of-scope/cross-cutting.\n\nLocal validation: cargo test -p dash-sdk --lib address_sync — 31 passed.\n\nNon-blocking nit: the first TODO(CMT-002) comment still references the old apply_address_changes name, but that is comment-only and not worth holding this PR.
`CMT-NNN` and `Found-025` were Claudius-internal review tracking labels, not public issue identifiers. External readers won't recognize them, and per project conventions code should describe present-state behaviour directly — the legitimate place for review-thread references is the commit history, not the source. This pass renames test functions, rewrites inline / doc comments, drops the labels from `debug!` log strings, and de-prefixes the two `TODO(CMT-002)` markers (the technical content is preserved; the ticket label is gone). Test names now describe what they assert: * `found_025_apply_block_changes_recovers_post_snapshot_address` → `apply_block_changes_recovers_post_snapshot_address` * `cmt_001_apply_block_changes_keeps_found_and_absent_disjoint` → `apply_block_changes_keeps_found_and_absent_disjoint_on_catch_up` * `found_025_known_delta_not_double_counted_on_refresh` → `refresh_does_not_double_count_known_address_delta` * `cmt_003_foreign_address_is_ignored_without_refresh_storm` → `apply_block_changes_ignores_foreign_address_without_refresh_storm` Verified by `grep -rnE 'CMT-[0-9]+|Found-025|Found025|found_025|cmt_[0-9]+' packages/rs-sdk/src/` — 28 hits before, 0 after. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
lklimek
added
the
ready for final review
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-sdk/src/platform/address_sync/mod.rs (1)
855-916:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSingle refresh can still drop second-wave post-snapshot addresses.
Line 859 snapshots
pending_addresses()before any replay callbacks run. If replaying one recovered address causeson_address_foundto derive the next pending address, any buffered change for that next address never entersextras, falls intostill_unknown, and is dropped at the end of the pass. This still loses balances when multiple consecutive post-snapshot addresses become active in the same sync window; the refresh/replay step needs to iterate until no new tracked unknowns appear.🤖 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-sdk/src/platform/address_sync/mod.rs` around lines 855 - 916, The snapshot of provider.pending_addresses() taken once allows newly discovered post-snapshot addresses (derived by provider.on_address_found) to be missed; change the refresh/replay step to repeat: loop re-reading provider.pending_addresses(), rebuild extras, replay pending_unknown and call provider.on_address_found until a full pass yields no newly tracked unknowns (i.e., no decrease in still_unknown or no new entries moved from unknown to found), preserving the existing logic that populates extras, computes new_balance and updates result/found/absent and replay_applied; ensure the loop awaits provider.on_address_found calls each iteration and breaks when extras.is_empty() or no progress is made to avoid livelock.
🤖 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-sdk/src/platform/address_sync/mod.rs`:
- Around line 855-916: The snapshot of provider.pending_addresses() taken once
allows newly discovered post-snapshot addresses (derived by
provider.on_address_found) to be missed; change the refresh/replay step to
repeat: loop re-reading provider.pending_addresses(), rebuild extras, replay
pending_unknown and call provider.on_address_found until a full pass yields no
newly tracked unknowns (i.e., no decrease in still_unknown or no new entries
moved from unknown to found), preserving the existing logic that populates
extras, computes new_balance and updates result/found/absent and replay_applied;
ensure the loop awaits provider.on_address_found calls each iteration and breaks
when extras.is_empty() or no progress is made to avoid livelock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa9dde1d-f1ff-47e6-92c6-084b4efd4b32
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/address_sync/mod.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three carried-forward findings re-validate at head 64c1979. The delta from 9eede09 is comment/test cleanup only (stripping internal CMT-NNN/Found-025 triage tokens) with no semantic change. Both Claude and Codex independently re-confirmed the same three issues: a nonce=0 synthesis at two write sites (intentionally deferred per new TODOs), a single pre-replay pending_addresses() snapshot in refresh_and_replay_unknown that can miss same-pass gap-extension addresses, and an unbounded pending_unknown owned-clone buffer across the catch-up pass. No new delta-only findings.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s)
suggestion: Recovered addresses persisted with synthesized nonce=0 at both write sites (intentionally deferred)
packages/rs-sdk/src/platform/address_sync/mod.rs (line 807)
Both apply paths synthesize let nonce = result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0); before constructing AddressFunds { nonce, balance }: forward apply at line 812, replay at line 903. The incremental RPCs (GetRecentAddressBalanceChanges{,Compacted}) carry only balance deltas, never nonces, so any address first surfaced via the incremental path — by construction every address that lands in the replay path has no prior result.found entry — gets persisted with nonce = 0 even when the on-chain nonce is non-zero (e.g. address already spent before this sync window). The AddressFunds type makes nonce look as authoritative as balance, but only the balance is proven; downstream wallet code treating result.found/on_address_found as canonical can then build an address-signed transition with a stale nonce and fail with InvalidAddressNonce.
The latest delta updated the TODO comments (lines 807-811 and 902) to explicitly defer this, noting clients can recover via AddressInvalidNonceError.expected_nonce. The author has chosen to defer — flagging so the gap is not lost. Proper fixes when picked up: (a) fetch authoritative AddressFunds for first-seen-via-incremental addresses, (b) model nonce as Option<u32> so the unknown state is encoded in the type, or (c) exclude unknown-path recoveries from result.found and surface them as a follow-up full-sync trigger.
suggestion: Single pre-replay `pending_addresses()` snapshot can drop same-pass gap-extension addresses
packages/rs-sdk/src/platform/address_sync/mod.rs (line 851)
refresh_and_replay_unknown calls provider.pending_addresses() exactly once at line 859 to build extras, then invokes provider.on_address_found(...) only after all matching buffered changes have been applied (line 915). The AddressProvider contract at provider.rs:81-86 explicitly documents that the pending set may grow when on_address_found triggers gap extension. Concretely: if pending_unknown contains both address A and A+1, and the provider only exposes A+1 after on_address_found(A) extends the gap, this function recovers A but leaves A+1 in still_unknown for the current pass — its buffered change is silently dropped until the next sync.
This is an API-contract mismatch, not a borrow-safety issue: the function snapshots provider state too early and never re-observes the mutation its own awaited callbacks are allowed to perform. The pre-refactor per-block refresh over-refreshed (the bug fixed in the parent commit); this replacement under-refreshes by making replay non-iterative. A small bounded loop — repeat the extras build + replay while the prior iteration recovered something AND the pending set grew, capped at ~3 iterations — would preserve the new multi-wallet behavior without dropping second-order recoveries. No existing test pins the gap-extension case.
nitpick: `pending_unknown` retains owned change clones for the entire catch-up pass
packages/rs-sdk/src/platform/address_sync/mod.rs (line 465)
pending_unknown: Vec<PendingUnknownChange> is allocated once at line 465 and survives until the end-of-pass refresh_and_replay_unknown at line 684. Every foreign-wallet address change encountered during both the compacted loop (lines 619-636) and the recent-entries loop (lines 662-678) pushes a PendingUnknownChange { key: Vec<u8>, change: OwnedAddressBalanceChange, current_height } via change.into_owned() at line 824. For Compacted entries, the clone copies the inner BlockAwareCreditOperation, whose AddToCreditsOperations variant is a BTreeMap<BlockHeight, Credits> of arbitrary size.
On a heavily populated multi-wallet chain doing a multi-batch compacted catch-up, this buffer grows to O(foreign_address_changes × catch_up_blocks) heap allocations that are ultimately discarded at the empty-extras early return (line 866-874) — the now-common case on multi-wallet chains, which is precisely when the buffer is largest. Not a correctness bug, but worth measuring before this path sees larger production loads. Mitigations: (a) store only Vec<u8> keys and re-derive replay changes after the refresh resolves them, (b) cap the buffer with a size threshold beyond which buffering is skipped (fall back to a follow-up sync), or (c) drain pending_unknown opportunistically between the compacted and recent phases.
🤖 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.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:807-916: Recovered addresses persisted with synthesized nonce=0 at both write sites (intentionally deferred)
Both apply paths synthesize `let nonce = result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0);` before constructing `AddressFunds { nonce, balance }`: forward apply at line 812, replay at line 903. The incremental RPCs (`GetRecentAddressBalanceChanges{,Compacted}`) carry only balance deltas, never nonces, so any address first surfaced via the incremental path — by construction every address that lands in the replay path has no prior `result.found` entry — gets persisted with `nonce = 0` even when the on-chain nonce is non-zero (e.g. address already spent before this sync window). The `AddressFunds` type makes `nonce` look as authoritative as `balance`, but only the balance is proven; downstream wallet code treating `result.found`/`on_address_found` as canonical can then build an address-signed transition with a stale nonce and fail with `InvalidAddressNonce`.
The latest delta updated the TODO comments (lines 807-811 and 902) to explicitly defer this, noting clients can recover via `AddressInvalidNonceError.expected_nonce`. The author has chosen to defer — flagging so the gap is not lost. Proper fixes when picked up: (a) fetch authoritative `AddressFunds` for first-seen-via-incremental addresses, (b) model `nonce` as `Option<u32>` so the unknown state is encoded in the type, or (c) exclude unknown-path recoveries from `result.found` and surface them as a follow-up full-sync trigger.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:851-916: Single pre-replay `pending_addresses()` snapshot can drop same-pass gap-extension addresses
`refresh_and_replay_unknown` calls `provider.pending_addresses()` exactly once at line 859 to build `extras`, then invokes `provider.on_address_found(...)` only after all matching buffered changes have been applied (line 915). The `AddressProvider` contract at `provider.rs:81-86` explicitly documents that the pending set may grow when `on_address_found` triggers gap extension. Concretely: if `pending_unknown` contains both address A and A+1, and the provider only exposes A+1 after `on_address_found(A)` extends the gap, this function recovers A but leaves A+1 in `still_unknown` for the current pass — its buffered change is silently dropped until the next sync.
This is an API-contract mismatch, not a borrow-safety issue: the function snapshots provider state too early and never re-observes the mutation its own awaited callbacks are allowed to perform. The pre-refactor per-block refresh over-refreshed (the bug fixed in the parent commit); this replacement under-refreshes by making replay non-iterative. A small bounded loop — repeat the `extras` build + replay while the prior iteration recovered something AND the pending set grew, capped at ~3 iterations — would preserve the new multi-wallet behavior without dropping second-order recoveries. No existing test pins the gap-extension case.
- [NITPICK] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:465-684: `pending_unknown` retains owned change clones for the entire catch-up pass
`pending_unknown: Vec<PendingUnknownChange>` is allocated once at line 465 and survives until the end-of-pass `refresh_and_replay_unknown` at line 684. Every foreign-wallet address change encountered during both the compacted loop (lines 619-636) and the recent-entries loop (lines 662-678) pushes a `PendingUnknownChange { key: Vec<u8>, change: OwnedAddressBalanceChange, current_height }` via `change.into_owned()` at line 824. For `Compacted` entries, the clone copies the inner `BlockAwareCreditOperation`, whose `AddToCreditsOperations` variant is a `BTreeMap<BlockHeight, Credits>` of arbitrary size.
On a heavily populated multi-wallet chain doing a multi-batch compacted catch-up, this buffer grows to O(foreign_address_changes × catch_up_blocks) heap allocations that are ultimately discarded at the empty-`extras` early return (line 866-874) — the now-common case on multi-wallet chains, which is precisely when the buffer is largest. Not a correctness bug, but worth measuring before this path sees larger production loads. Mitigations: (a) store only `Vec<u8>` keys and re-derive replay changes after the refresh resolves them, (b) cap the buffer with a size threshold beyond which buffering is skipped (fall back to a follow-up sync), or (c) drain `pending_unknown` opportunistically between the compacted and recent phases.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Verified the new chained post-snapshot replay finding: the refresh path only polled I pushed a focused fix branch here (couldn't push to the PR branch directly): https://github.com/thepastaclaw/platform/tree/tracker-1325-fix Commit: Validation: |
|
PR head moved to Updated branch: https://github.com/thepastaclaw/platform/tree/tracker-1325-fix Validation: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the review targets against bd96afe0e0ac56fadf41a42faa9e6083de1a20fb. Two previously reported issues are still present in address_sync: recovered addresses can still be surfaced with a fabricated nonce, and the one-shot unknown-address replay still does not honor the provider contract when on_address_found extends the pending set mid-replay. The remaining performance note is a real tradeoff in the new design, but I would not post it as a review finding without evidence that the buffering cost is material.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Recovered addresses are still persisted with a synthesized nonce of 0
packages/rs-sdk/src/platform/address_sync/mod.rs (line 807)
Both incremental write paths still reuse an existing nonce if one is already in result.found and otherwise write 0 (apply_block_changes here and refresh_and_replay_unknown at lines 902-906). That is not just a placeholder internal detail: AddressSyncResult and AddressProvider::on_address_found both expose AddressFunds as a (nonce, balance) pair, so a post-snapshot address recovered through incremental replay is surfaced as if its nonce were authoritative even though the incremental RPCs only prove balance deltas. The new TODO acknowledges this, but the behavior is unchanged and can still hand downstream code a stale nonce for an address whose on-chain nonce is already higher.
suggestion: One-shot unknown-address replay still misses addresses revealed by same-pass gap extension
packages/rs-sdk/src/platform/address_sync/mod.rs (line 858)
refresh_and_replay_unknown snapshots provider.pending_addresses() once into extras before it replays any buffered changes, then calls provider.on_address_found only after the replay loop finishes. That still conflicts with the provider contract in provider.rs:81-83, which says the pending set may grow when on_address_found extends the gap. In the concrete case where replay recovers address A and that callback makes A+1 pending, any buffered change for A+1 remains in still_unknown until the next sync because this function never re-polls after the mutation. The outer sync flow does not add another incremental replay pass, so the missed recovery is real for the current call.
🤖 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.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:807-815: Recovered addresses are still persisted with a synthesized nonce of 0
Both incremental write paths still reuse an existing nonce if one is already in `result.found` and otherwise write `0` (`apply_block_changes` here and `refresh_and_replay_unknown` at lines 902-906). That is not just a placeholder internal detail: `AddressSyncResult` and `AddressProvider::on_address_found` both expose `AddressFunds` as a `(nonce, balance)` pair, so a post-snapshot address recovered through incremental replay is surfaced as if its nonce were authoritative even though the incremental RPCs only prove balance deltas. The new TODO acknowledges this, but the behavior is unchanged and can still hand downstream code a stale nonce for an address whose on-chain nonce is already higher.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:858-915: One-shot unknown-address replay still misses addresses revealed by same-pass gap extension
`refresh_and_replay_unknown` snapshots `provider.pending_addresses()` once into `extras` before it replays any buffered changes, then calls `provider.on_address_found` only after the replay loop finishes. That still conflicts with the provider contract in `provider.rs:81-83`, which says the pending set may grow when `on_address_found` extends the gap. In the concrete case where replay recovers address A and that callback makes A+1 pending, any buffered change for A+1 remains in `still_unknown` until the next sync because this function never re-polls after the mutation. The outer sync flow does not add another incremental replay pass, so the missed recovery is real for the current call.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…3650 CMT-003) Crikey, foreign-wallet changes on a busy shared chain can pile up in the end-of-pass replay buffer without anyone noticing. We're not chasing premature optimization here — just dropping a one-shot tracing::warn the first time the buffer crosses 1000 entries in a pass, so a future operator can actually see whether this path needs the reviewer's mitigation (a) treatment (keys-only buffer + re-derive on refresh). Additive only — no logic changes, no behavioural shift, no new test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dresses (PR #3650 CMT-001) Streuth — the single-pass refresh was leaving freshly-gap-extended addresses on the cutting room floor. If `pending_unknown` buffered both A and A+1, and the provider only exposed A+1 *after* `on_address_found(A, …)` triggered gap extension, the previous implementation polled `pending_addresses()` once, missed A+1, and silently dropped its buffered balance until the next sync. Wrap the refresh+replay in a bounded for-loop (cap 3 iterations, debug! when the cap fires). Each iteration: 1. Re-reads `pending_addresses()`, builds `extras` from the still-unresolved intersection. 2. Replays only matching unresolved buffered entries. 3. Fires `on_address_found` callbacks BEFORE the next refresh so gap extension actually has a chance to surface follow-on addresses to the next poll. 4. Tracks resolved keys so subsequent iterations skip already-applied entries (no double-counted deltas). Adds a regression test (`refresh_loops_until_gap_extension_recovers_follow_on_address`) that pins the gap-extension scenario with a mock provider whose pending set grows from `[A]` to `[A, B]` mid-replay; the test asserts both A and B land in `result.found` with `on_address_found` firing for each. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@thepastaclaw — addressing your 2026-05-21T11:38 post-approval re-validation. Two commits on top of `bd96afe0`: CMT-001 — bounded-iteration replay (`875a55d8`) New regression test `refresh_loops_until_gap_extension_recovers_follow_on_address` — `GapExtendingProvider` mock returns `[(10, A)]` until `on_address_found(A, …)` flips `extended = true`, after which it returns `[(10, A), (11, B)]`. Asserts both A and B end up in `result.found` and trigger callbacks. CMT-003 — pending_unknown size warn (`a1e456fc`) CMT-002 — nonce=0 synthesis: intentionally deferred on this PR. The TODO comments at lines 807-811 and 902 already document the gap and the documented client-recovery path via `AddressInvalidNonceError.expected_nonce`. We'll file a follow-up issue tracking the three proper-fix options you outlined. Validations: `cargo fmt` clean, `clippy --tests -- -D warnings` clean, `cargo test --lib address_sync` 32 passed. 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the review set against 875a55d8fb0ca0c9ec879bc00b66c44b87e578b0. One carried-forward issue is still real: incremental replay still fabricates nonce = 0 for first-seen addresses and that value is staged, returned, diffed, and persisted as if authoritative. The latest delta does fix the prior one-shot gap-extension miss, but it introduces one new truncation issue: the new three-round replay cap can still leave valid same-pass recoveries pending until a later sync.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Incremental replay still emits an invented nonce for first-seen addresses
packages/rs-sdk/src/platform/address_sync/mod.rs (line 810)
When an address is first discovered through incremental balance deltas, both incremental write paths synthesize AddressFunds.nonce from prior local state and fall back to 0 if no prior entry exists (mod.rs:810-819 and mod.rs:952-957). That value is not confined to an internal cache: AddressSyncResult.found exposes it as a concrete nonce, AddressProvider::on_address_found receives it as if authoritative, the platform wallet stages it into per_wallet_in_sync and commits it in sync_finished() (packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:565-573 and :667-683), and sync_balances() diffs and persists changed AddressFunds entries (packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:39-64). The TODO acknowledges the gap, but the current API still serializes unknown nonce data as a real nonce, so a post-snapshot address can overwrite persisted state with nonce = 0 even when the chain nonce is higher.
suggestion: The three-iteration replay cap breaks the provider contract for longer gap-extension chains
packages/rs-sdk/src/platform/address_sync/mod.rs (line 853)
refresh_and_replay_unknown() now re-polls pending_addresses() after each replay wave, which fixes the earlier one-shot bug, but the hardcoded REPLAY_REFRESH_MAX_ITERATIONS: usize = 3 means sync no longer runs until all newly revealed pending addresses are resolved. The trait documentation still promises that pending_addresses() may grow after on_address_found() and that sync continues until all pending addresses are resolved (packages/rs-sdk/src/platform/address_sync/provider.rs:28-35 and :81-85). In the current implementation, a provider that reveals one additional address per callback wave will stop making progress after three rounds, log that the rest will surface on the next sync, and then return to sync_address_balances(), which immediately calls sync_finished() and ends the pass. That is a real behavioral truncation, not just a livelock guard, and the new regression test only covers a two-wave chain, not the longer case that still fails.
🤖 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.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:810-819: Incremental replay still emits an invented nonce for first-seen addresses
When an address is first discovered through incremental balance deltas, both incremental write paths synthesize `AddressFunds.nonce` from prior local state and fall back to `0` if no prior entry exists (`mod.rs:810-819` and `mod.rs:952-957`). That value is not confined to an internal cache: `AddressSyncResult.found` exposes it as a concrete nonce, `AddressProvider::on_address_found` receives it as if authoritative, the platform wallet stages it into `per_wallet_in_sync` and commits it in `sync_finished()` (`packages/rs-platform-wallet/src/wallet/platform_addresses/provider.rs:565-573` and `:667-683`), and `sync_balances()` diffs and persists changed `AddressFunds` entries (`packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:39-64`). The TODO acknowledges the gap, but the current API still serializes unknown nonce data as a real nonce, so a post-snapshot address can overwrite persisted state with `nonce = 0` even when the chain nonce is higher.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:853-999: The three-iteration replay cap breaks the provider contract for longer gap-extension chains
`refresh_and_replay_unknown()` now re-polls `pending_addresses()` after each replay wave, which fixes the earlier one-shot bug, but the hardcoded `REPLAY_REFRESH_MAX_ITERATIONS: usize = 3` means sync no longer runs until all newly revealed pending addresses are resolved. The trait documentation still promises that `pending_addresses()` may grow after `on_address_found()` and that sync continues until all pending addresses are resolved (`packages/rs-sdk/src/platform/address_sync/provider.rs:28-35` and `:81-85`). In the current implementation, a provider that reveals one additional address per callback wave will stop making progress after three rounds, log that the rest will surface on the next sync, and then return to `sync_address_balances()`, which immediately calls `sync_finished()` and ends the pass. That is a real behavioral truncation, not just a livelock guard, and the new regression test only covers a two-wave chain, not the longer case that still fails.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…-001) The tree scan can prove an address absent (`result.absent.insert(...)` at L171 / L223) and a later chain-confirmed balance change in the catch-up phase can legitimately re-discover that same address. The catch-up path only wrote to `result.found`, leaving the stale `absent` marker in place — the same `(tag, address)` then appeared in both sets and the final `AddressSyncResult` was internally inconsistent. Drop the stale `absent` entry before inserting into `found`. Symmetric direction (`found → absent`) doesn't exist: no `result.found.remove` call is reachable from any code path, and "absent" means proven-not-in- tree (not "balance is 0"), so a found-with-balance-0 stays in `found`. Adds two regression tests: one at the `apply_address_changes` seam and one at the `apply_block_changes` level, both asserting `found` and `absent` are disjoint after the catch-up rediscovers a previously-absent address. Addresses CodeRabbit dashpay#3650 (comment) 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
…s (CMT-001/CMT-004/CMT-005/CMT-006/CMT-007)
Pre-fix `apply_block_changes` refreshed `provider.pending_addresses()`
once per block and emitted a `warn!` claiming "addresses will be
resolved on the next full sync (Found-025)" for every snapshot miss.
`GetRecentAddressBalanceChanges{,Compacted}RequestV0` carry no
per-wallet filter, so on a populated multi-wallet chain every other
wallet's address tripped that branch — refresh storm + warn-log flood
on the operator's box for legitimately-not-mine addresses.
This rewrites the catch-up flow to do exactly one refresh per call:
* `apply_block_changes` now takes a borrowed `&HashMap<...>` lookup and
appends each miss to a caller-provided `Vec<PendingUnknownChange>`.
It still applies all known-address hits synchronously so the per-block
cursor / delta semantics are preserved.
* `incremental_catch_up` threads one `pending_unknown` buffer through
both phases (compacted then recent) and calls a new
`refresh_and_replay_unknown` exactly once at end-of-pass.
* `refresh_and_replay_unknown` polls `pending_addresses()` once, keeps
only those whose key actually matches a buffered miss (CMT-006: a
precise intersection check replaces the imprecise `len() == before`
proxy), and replays only those entries. Foreign addresses fall out at
the intersection step — no allocation beyond the buffered misses
themselves.
* Foreign-address logging is demoted from `warn!` to `debug!` and the
message no longer claims "next full sync will resolve it" (CMT-007 —
no sync resolves an address the wallet never derived).
* The unconditional `address_lookup = key_to_tag.clone()` is gone
(CMT-005), as is the unconditional per-block `Vec` materialization
(CMT-004) — only buffered misses allocate, and `key_to_tag` is shared
read-only through both apply phases.
Also adds a `TODO(CMT-002)` at the two nonce=0 synthesis sites
(per-block apply + replay): the incremental RPCs carry no nonces, so
addresses first surfaced through this path persist with `nonce=0` and
clients must rely on `AddressInvalidNonceError.expected_nonce` to
recover. Resolution wants either authoritative `AddressFunds` fetch for
recovered addresses or modelling `nonce` as `Option<u32>` — deferred
out of this PR (cross-cutting change).
The pre-refactor `pub(crate) fn apply_address_changes` /
`AppliedAddressChanges` seam is removed — the only callers were two
unit tests with redundant invariants now covered by the end-to-end
`apply_block_changes` + `refresh_and_replay_unknown` regression guards.
Existing CMT-001 / Found-025 regression tests (recovers post-snapshot
address, no double-counted delta, found ∩ absent disjoint) updated to
call the new two-step API; all pass. CMT-003 regression guard (foreign
addresses ignored, no refresh storm) follows in the next commit.
Addresses thepastaclaw review at dashpay#3650 (review)
🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
…block_changes (CMT-003) Pins the CMT-001 fix from the previous commit at the test surface: four "blocks" worth of changes — one for a known wallet address, three for foreign-wallet addresses — must result in (a) zero per-block `pending_addresses()` polls (the refresh is end-of-pass only), (b) exactly one `pending_addresses()` poll from `refresh_and_replay_unknown`, (c) no foreign address in `result.found`, (d) no foreign address in `result.absent`, (e) `on_address_found` fired exactly once (for the known address), and (f) `found` and `absent` still globally disjoint. Pre-refactor this test would have failed on the poll count (per-block refresh storm) and would have stuffed each foreign address through the `unknown` channel + warn-log path. Now it pins the corrected shape on the dominant production case (multi-wallet shared chain). Addresses thepastaclaw dashpay#3650 (review) 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
3 participants
Issue being fixed or feature implemented
Fixes the rs-sdk address-sync silent-discard defect (Found-025): the SDK
silently drops platform-returned balance updates for any address absent
from the pre-RPC
pending_addresses()snapshot.Summary
sync_address_balances(packages/rs-sdk/src/platform/address_sync/mod.rs)builds a
key_to_taglookup once from a single pre-RPCprovider.pending_addresses()snapshot and passes it by immutablereference into the private
incremental_catch_up. In both the compactedapply loop and the recent apply loop the predicate
if let Some(&(tag, address)) = address_lookup.get(&addr_bytes)had noelsebranch: any balance change the platform returned for an addressderived after the snapshot was dropped with no log, metric, or error —
result.foundnever got it andon_address_foundwas never called.Unlike the tree-scan path,
incremental_catch_upnever re-polledpending_addresses()(contrastafter_branch_iteration, which refreshesduring the branch loop).
Root cause
key_to_tagsnapshot: built once from a single pre-RPCprovider.pending_addresses()call and never refreshed insideincremental_catch_up.else: theif let Some(..) = address_lookup.get(..)predicatehad no fallback, so a snapshot miss was a no-op.
post-snapshot address went to neither
result.foundnoron_address_found, and the wallet's local sync map showed the addressempty.
Reproduction
Aafter thepass's
pending_addresses()snapshot is taken (routine underconcurrent multi-identity funding).
A; let it chain-confirm.A's balance, but theSDK discards it:
result.foundnever getsA,on_address_foundisnever called, the wallet's local sync map shows
Aempty.On single-threaded runs the derive usually precedes the snapshot, hiding
the bug — hence the symptom is flaky, not always red, in live e2e.
Under concurrent multi-identity funding the derive→fund→sync interleave is
routine, which is why the rs-platform-wallet e2e funding gates
TK-001 / TK-007 / TK-013 / TK-014 / id_005 flaked here (PASS
single-threaded, flaky under 14-thread concurrency).
What was done?
pub(crate) apply_address_changesseam — noSdk, no network, noasync. It returns the applied
(tag, address, funds)updates plus theaddresses the platform reported a change for that were absent from the
snapshot (
unknown), instead of silently dropping them.apply_block_changesdrives the asyncon_address_foundcallbacksoutside the pure seam, and on an unknown-address miss re-polls
provider.pending_addresses()(mirroring the existingafter_branch_iterationtree-scan refresh) and replays only thepreviously-unknown subset — so a freshly-derived receive address is
recovered while known-address
AddToCreditsdeltas are neverdouble-counted.
warn(observable, never silently dropped) per project
tracingconventions.(verified by the 27 pre-existing
address_synctests, all still green).How Has This Been Tested?
cargo build -p dash-sdk,cargo clippy -p dash-sdk --all-targets(clean),
cargo test -p dash-sdk --lib address_sync— 30/30 pass,including the 27 pre-existing address_sync tests (no regression from the
refactor).
Three new deterministic
#[cfg(test)]regression guards on the pure seam(no proof/Sdk/network):
found_025_apply_address_changes_surfaces_unknown_address— a knownaddress still applies identically; a post-snapshot address is surfaced
in
unknowninstead of silently dropped.found_025_apply_block_changes_recovers_post_snapshot_address— aprovider that derived an address mid-pass gets the balance applied and
on_address_foundfired after the in-pass refresh (the exact Found-025scenario).
found_025_known_delta_not_double_counted_on_refresh— a knownAddToCreditsdelta applies exactly once across the refresh+replay.Red-pre / green-post proof: restoring the silent-discard behavior
(drop the
else, no refresh) makes guards 1 and 2 FAIL(
unknown == [],result.found == None); the fix makes all three PASS.Would have caught Found-025 at unit level. The extracted
pub(crate)pure seam makes the discard a pure-data assertion (no
Sdk/proof) pinnedin
rs-sdk's own test module.This is the
v3.1-devproduction PR. Applying onto #3549 for combinede2e validation is a separate sequenced step.
Test plan
cargo build -p dash-sdkcargo clippy -p dash-sdk --all-targets(clean)cargo test -p dash-sdk --lib address_sync— 30/30 passfound_025_apply_address_changes_surfaces_unknown_addressfound_025_apply_block_changes_recovers_post_snapshot_addressfound_025_known_delta_not_double_counted_on_refreshsilent-discard logic)
Breaking Changes
None. Public API unchanged; the new seam is
pub(crate). Known-addressbehavior is identical.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit