fix(sdk): address-sync no longer silently discards balance changes for post-snapshot addresses (Found-025)#3650
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:
📝 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 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 ff2d04c) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3650 +/- ##
=========================================
Coverage 87.19% 87.19%
=========================================
Files 2636 2636
Lines 327822 327822
=========================================
Hits 285852 285852
Misses 41970 41970
🚀 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: "feac78ea737ee2cd2479c8d7e5c991f870bf7ae8649ed2b553163521c1832d90"
)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
|
@lklimek I pushed a focused comment-only cleanup for your inline feedback here:
What changed: shortened the long TODO/intentional/livelock comments in Validation:
No PR created, per tracker worker instructions. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (a711cc8..a793310) cleanly resolves both prior code findings: the cap-hit warn at lines 1013-1024 now only fires when still_unknown > 0, and the post-scan current_balances seed at lines 409-414 fixes the AddToCredits base-balance gap on the full-scan branch with regression test current_balances_fold_seeds_base_balance_on_full_scan. Codex's new blocking claim that incremental-only sync silently skips compacted history is contradicted by the explicit intentional test test_incremental_mode_checkpoint_zero_skips_compacted (lines 1408-1424) — known balances are seeded from current_balances, so the design assumes full_rescan_after_time_s is tuned shorter than the recent retention window; dropped as speculative. The one carried-forward remaining concern is documentation: the cumulative PR reshapes the publicly re-exported AddressProvider trait (associated types, #[async_trait], iterator returns, new watermark methods) while the PR description still claims Breaking Changes: None.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/address_sync/provider.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/provider.rs:47-198: PR description claims no breaking changes, but the publicly re-exported `AddressProvider` trait was reshaped
Carried-forward from the prior review (a711cc86) and re-validated against HEAD a793310f. The latest delta does not touch `provider.rs`, but the cumulative PR replaces the prior synchronous `AddressProvider` trait with one that is incompatible at the trait-impl level vs `v3.1-dev`:
- New associated types `type Tag: Copy + Ord + Eq + Hash + Send + Sync` and `type Address: AddressToBytes`.
- `#[async_trait]` on the trait; `on_address_found` / `on_address_absent` / `sync_finished` are now `async` with new signatures (`&Self::Address` instead of `&[u8]`).
- `pending_addresses` now returns `impl Iterator<Item = (Self::Tag, Self::Address)> + '_` instead of `Vec<(AddressIndex, AddressKey)>`.
- New required/defaulted methods: `current_balances`, `last_sync_height`, `last_known_recent_block_height`.
- `sync_address_balances` itself now takes `last_sync_timestamp` and returns a generic `AddressSyncResult<P::Tag, P::Address>`.
`AddressProvider` is re-exported from `address_sync`, so any external `impl AddressProvider for …` written against `v3.1-dev` will fail to compile. On a 4.0.0-beta line, breaking the trait is fine, but the PR description's `Breaking Changes: None — Public API unchanged; the new seam is pub(crate)` only covers the Found-025 `apply_address_changes` seam — it does not cover the trait reshape. Update the PR body's Breaking Changes section to enumerate the trait changes (and consider adding `!` to the conventional-commit title), or confirm in the description that `AddressProvider` was considered an internal surface before this series.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding: the AddressProvider trait reshape vs v3.1-dev is OUTDATED — the reshape is already merged into the current v3.1-dev base, and the PR's only provider.rs change is a 15-line doc-only Invariant block. New delta (a793310..e42f66d) is a v3.1-dev merge that pulls in unrelated drive/wallet-storage work and does not touch the address-sync seam. No new defects: the only newly-proposed finding (unbounded pending_unknown buffer) is an intentional, self-documented tradeoff with a logged threshold and a noted bounded-memory fix path. Approving.
…_balances⊆pending invariant Three residual gaps from the PR #3650 verification pass: - Cap-hit warn no longer over-claims. The refresh+replay tail is now split into wallet-owned loss (a key the provider still offers, stranded by the livelock guard) and foreign noise (a key the provider never offers). Only wallet-owned loss raises a WARN; foreign leftovers stay at DEBUG, since a full rescan re-ignores them rather than recovering them. The comment now matches the code instead of claiming a split that was never performed. New test asserts foreign-only leftovers do not warn while a stranded wallet-owned address does. - AddressProvider::pending_addresses rustdoc now points to the current_balances ⊆ pending_addresses invariant, so the mod.rs fold cross-reference resolves to a documented invariant. - sync_address_balances seeds base balances from current_balances() before the no-pending early return, so an invariant-violating provider exposing a current-balance-only address with an empty pending set no longer silently drops its known base balance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The foreign-only sub-case of cap_hit_warns_on_wallet_owned_loss_but_not_foreign_noise used a provider that offers no pending addresses, so the replay loop tripped the `extras.is_empty()` early-return on iteration 0 and returned before the wallet-owned/foreign classifier ran. The 0-WARN assertion passed via that short-circuit, leaving the foreign-counting branch with zero coverage. The provider now offers one resolvable wallet-owned address, so the loop runs past the early-return; a foreign miss it never offers survives as `foreign`, exercising the foreign branch with wallet_owned_lost == 0. A new assertion that the owned address resolved to its replayed balance proves the classifier actually executed (the early-return path would have skipped that replay). Verified non-false-green by mutation: counting foreign as wallet_owned_lost makes this test fail on the foreign sub-case; reverting restores green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the three remaining findings from claude-rust-quality against HEAD adb6b20. All three are real, all are in-scope (the unsafe Send impl, the always-cloned recent_proof debug field, and the guarded unwrap were introduced in this PR via b1bcc0b), and all are non-blocking. Prior cap-warn, base-balance-seeding, and AddressProvider trait-reshape findings remain resolved or outdated. COMMENT review with three suggestions/nitpicks.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
3 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-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:105-106: Drop the hand-written `unsafe impl Send` for `AddressSyncContext` — let auto-derive carry it
`AddressSyncContext<'a, P>` has only three fields: `&'a mut P`, `&'a mut HashMap<Vec<u8>, (P::Tag, P::Address)>`, and `&'a mut AddressSyncResult<P::Tag, P::Address>`. `AddressProvider` requires `Send`, `P::Tag: Send + Sync`, and `P::Address: AddressToBytes` (whose supertraits include `Send + Sync`), so every field is `Send` already and the auto-derive would do the right thing without `unsafe`. The cost of keeping the manual impl is that any future non-`Send` field added here (an `Rc`, a raw pointer, a `MutexGuard`, etc.) silently gets promoted to `Send` instead of failing to compile — converting a would-be compile error into a soundness bug. The SAFETY comment also doesn't enumerate the per-field reasoning. Remove the `unsafe impl` and let the compiler derive it; if a future bound makes auto-derive fail, fix the offending field rather than asserting safety by hand.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:543-544: `recent_proof: Vec<u8>` clones the raw GroveDB proof bytes into every successful result for debug-only use
Every successful sync clones the raw GroveDB proof bytes into `result.recent_proof` (types.rs documents the field as "Raw GroveDB proof bytes from the most recent query (for debugging)"). The bincode boundary allows up to 256 MiB, and real proofs are tens-to-hundreds of KB per sync, so this is a non-trivial per-call allocation paid by every caller, debugging or not. Two concerns: (1) once a debug capture ships as a public field on the canonical `AddressSyncResult`, it's hard to remove because downstream code may start to rely on it; (2) the clone is also avoidable — `recent_proof.grovedb_proof` is read again only once (at `get_last_recent_block_from_proof(&recent_proof)`), so `std::mem::take` after the boundary decode would skip the copy entirely. Either gate the field behind a feature flag/config, or take instead of clone, before this surface ships.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at HEAD 9ed0890. The new-push delta is a v3.1-dev merge that does not touch the Found-025 surface (packages/rs-sdk/src/platform/address_sync/). All three prior address_sync findings remain STILL VALID at byte-identical line locations (unsafe Send impl, always-cloned debug recent_proof, guarded unwrap). The new memo and shielded_snapshot findings from agents are technically correct but came in via the unrelated v3.1-dev merge and belong to their originating PRs — recorded as out-of-scope, not in the review.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
3 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-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:105-106: Drop the hand-written `unsafe impl Send` for `AddressSyncContext` and let auto-derive carry it
`AddressSyncContext<'a, P>` holds only `&'a mut P`, `&'a mut HashMap<Vec<u8>, (P::Tag, P::Address)>`, and `&'a mut AddressSyncResult<P::Tag, P::Address>`. The `AddressProvider` trait already requires `Send`, with `P::Tag: Send + Sync` and `P::Address: AddressToBytes` (whose supertrait chain includes `Send + Sync`), so every field is `Send` and the compiler can auto-derive `Send` for the struct without `unsafe`. The cost of keeping the manual impl is that any future non-`Send` field added here (an `Rc`, raw pointer, `MutexGuard`, `RefCell`, …) is silently promoted to `Send` instead of failing to compile — turning a would-be type error into a latent soundness bug. The SAFETY comment also does not enumerate per-field reasoning. Remove the `unsafe impl` and rely on auto-derive; if a future change makes auto-derive fail, fix the offending field rather than asserting safety by hand.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:543-544: `recent_proof: Vec<u8>` clones raw GroveDB proof bytes into every successful result for debug-only use
Every successful sync clones the raw GroveDB proof bytes into `result.recent_proof` even though `types.rs` documents the field as "Raw GroveDB proof bytes from the most recent query (for debugging)". The bincode boundary admits up to 256 MiB, and real proofs are tens-to-hundreds of KB per sync, so this is a non-trivial per-call allocation paid by every production caller whether they inspect the field or not. Two concerns: (1) once a debug capture ships as a public field on the canonical `AddressSyncResult`, downstream code can begin to rely on it and removing the field becomes a breaking change; (2) the clone is also avoidable — `recent_proof.grovedb_proof` is read only once more, at `get_last_recent_block_from_proof(&recent_proof)`, so `std::mem::take` after that boundary use would skip the copy entirely. Either gate the field behind a `cfg`/config flag, or `take` instead of `clone`, before this surface ships.
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: prior-unsafe-send-address-sync-context STILL VALID; prior-recent-proof-debug-clone STILL VALID; prior-guarded-unwrap-boundary-height STILL VALID. New findings in the latest delta: none; the merge payload does not change packages/rs-sdk/src/platform/address_sync/mod.rs. Additional cumulative PR-owned findings: the full-scan current_balances reseed can mask a proved-absent stale wallet balance, so this review requests changes.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (3)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/mod.rs:105-106: Remove the manual unsafe Send implementation —AddressSyncContextonly stores mutable references to the provider, the key lookup map, and the sync result. Under theAddressProviderbounds, those fields already deriveSendthrough Rust's auto-trait rules. Keeping a hand-written unsafe impl weakens future maintenance because a later non-Send field would still compile instead of being rejected by the type system. - [SUGGESTION]
packages/rs-sdk/src/platform/address_sync/mod.rs:543-544: Avoid cloning the raw GroveDB proof into every result — The successful incremental path clonesrecent_proof.grovedb_proofonly so the same proof can still be borrowed for boundary extraction later. That proof can be large, andrecent_proofis documented as a debugging/inspection field, so normal sync callers pay an avoidable allocation and copy. Computeneed_compactedfirst, then moverecent_proof.grovedb_proofintoresult.recent_proofonce the proof is no longer borrowed. - [NITPICK]
packages/rs-sdk/src/platform/address_sync/mod.rs:1087-1096: Express boundary parsing without unwrap — Thekey.len() == 8guard makes the unwrap safe today, but this is production SDK proof-parsing code and the local style expects unwrap-free error handling. Using the fallible conversion directly keeps the invariant local to the conversion and prevents a future edit from separating the guard from the unwrap.
🤖 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-sdk/src/platform/address_sync/mod.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/address_sync/mod.rs:418-420: Do not reinsert balances the scan just proved absent
After a full tree scan, `process_trunk_result` and `process_branch_result` insert proved-missing addresses into `result.absent` and call `on_address_absent`. The platform wallet provider stages that absence until `sync_finished`, while `current_balances()` still reads the committed `per_wallet.found` map. This reseed loop therefore adds the same stale pre-sync balance back into `result.found`. The wallet diff treats `found` as winning over `absent`, so a stale persisted balance from a reset or reorg case does not get zeroed even though the proof showed it absent. Skip entries already present in `result.absent`; a later incremental credit will still remove the absent marker and insert a real found balance through `apply_block_changes`.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:105-106: Remove the manual unsafe Send implementation
`AddressSyncContext` only stores mutable references to the provider, the key lookup map, and the sync result. Under the `AddressProvider` bounds, those fields already derive `Send` through Rust's auto-trait rules. Keeping a hand-written unsafe impl weakens future maintenance because a later non-Send field would still compile instead of being rejected by the type system.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:543-544: Avoid cloning the raw GroveDB proof into every result
The successful incremental path clones `recent_proof.grovedb_proof` only so the same proof can still be borrowed for boundary extraction later. That proof can be large, and `recent_proof` is documented as a debugging/inspection field, so normal sync callers pay an avoidable allocation and copy. Compute `need_compacted` first, then move `recent_proof.grovedb_proof` into `result.recent_proof` once the proof is no longer borrowed.
| for (tag, address, funds) in provider.current_balances() { | ||
| result.found.entry((tag, address)).or_insert(funds); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Do not reinsert balances the scan just proved absent
After a full tree scan, process_trunk_result and process_branch_result insert proved-missing addresses into result.absent and call on_address_absent. The platform wallet provider stages that absence until sync_finished, while current_balances() still reads the committed per_wallet.found map. This reseed loop therefore adds the same stale pre-sync balance back into result.found. The wallet diff treats found as winning over absent, so a stale persisted balance from a reset or reorg case does not get zeroed even though the proof showed it absent. Skip entries already present in result.absent; a later incremental credit will still remove the absent marker and insert a real found balance through apply_block_changes.
| for (tag, address, funds) in provider.current_balances() { | |
| result.found.entry((tag, address)).or_insert(funds); | |
| } | |
| for (tag, address, funds) in provider.current_balances() { | |
| let result_key = (tag, address); | |
| if !result.absent.contains(&result_key) { | |
| result.found.entry(result_key).or_insert(funds); | |
| } | |
| } |
source: ['direct-codex-general']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Targeted re-review @ fb9aebe9
The 2026-07-05 push is a v4.1-dev merge. packages/rs-sdk/src/platform/address_sync/mod.rs is byte-identical to the July 2 reviewed head (4900d6dd..fb9aebe9 has no diff for that file), so the prior address-sync findings carry forward.
🔴 Still blocking: the full-scan reseed loop still writes every provider.current_balances() entry into result.found with or_insert after process_trunk_result / process_branch_result may already have inserted the same (tag, address) into result.absent. Because wallet reconciliation treats found as winning over absent, a stale committed balance can survive a proof that showed the address absent. Please skip reseeding entries already present in result.absent; later incremental credits still remove the absent marker through the normal catch-up path.
The carried-forward non-blocking items are also still present: the manual unsafe impl Send, the debug recent_proof clone, and the guarded unwrap in boundary parsing. No new latest-delta address_sync finding from today's merge.
|
Fable Review @lklimek All verdicts are in. Here's the review. Review: [dashpay/platform#3650](#3650)Overview. This PR fixes a real silent-loss defect in the rs-sdk address sync: balance changes for addresses derived after the pre-RPC The problems are concentrated in one place: the defensive Findings (most severe first)
What was checked and came back cleanSeveral scary-sounding candidates were refuted or defused under verification: the livelock-guard/watermark loss is real but is exactly the in-code Bottom line: the core fix is good and I'd want it merged — but findings 1 and 2 are concrete correctness bugs in the new defensive seeding, both reachable through the FFI batch provider path, and are worth fixing in this PR since they touch the same lines. The rest is polish. |
…y logic Addresses the Fable review of #3650. All changes are in rs-sdk/src/platform/address_sync/mod.rs; public API unchanged. - seed_base_balances (new shared helper for the three current_balances seed sites): resolve each key through key_to_tag (pending wins) so a tag mismatch between pending_addresses and current_balances can't split one address across two result keys and double-count its delta; and skip any key the tree scan proved absent so a stale cached balance can't resurrect a proven-absent address into `found` (scan is authoritative). - apply_change (new shared helper): the forward apply pass and the end-of-pass replay now share one copy of the found/absent disjointness rule and the synthesized-nonce rule, instead of ~20 duplicated lines. - Empty-pending path returns a visibly-empty result again (no current_balances seed) so a zero-watermark no-op is not disguised as a populated, successful sync. - Correct the unworkable bounded-memory comment sketch (the RPC entries are dropped per pagination iteration, so a dropped change cannot be re-derived without re-fetching). - Soften the foreign-vs-owned tail classifier comment and both log lines: "did not offer during this pass" instead of asserting foreignness. - Add two red-pre/green-post regression guards: no resurrection of a scan-proven-absent address, and tag reconciliation on a mismatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Cumulative incremental review at ff2d04ca: the latest delta fixes the prior blocking reseed-vs-absent bug, and I found no new latest-delta findings. Three non-blocking prior findings remain still valid and are carried forward below.
Source: reviewers claude general opus (unparseable), codex general gpt-5.5, claude rust-quality opus (unparseable), codex rust-quality gpt-5.5; verifier codex gpt-5.5.
Reviewed commit: ff2d04caadfefa4891b7c18ef1b2b3bdef836260
Prior Finding Reconciliation
- FIXED - [Carried-forward prior finding] Reseed reinserts balances the scan just proved absent: Fixed in the latest delta. The raw current_balances reseed loops now call seed_base_balances, which resolves the result key through key_to_tag and skips entries already present in result.absent. Evidence: packages/rs-sdk/src/platform/address_sync/mod.rs:462-477 implements the absent-scan guard, and seed_base_balances_skips_scan_proven_absent at lines 2931-3010 covers the stale-balance resurrection case.
- STILL VALID - Manual unsafe
Sendimplementation forAddressSyncContext. - STILL VALID - Raw GroveDB proof clone on the incremental re-sync path.
- STILL VALID - Boundary parsing still uses guarded
unwrap()in proof parsing.
Carried-Forward Prior Findings
[SUGGESTION] [Carried-forward prior finding - STILL VALID] Remove the manual unsafe Send implementation
packages/rs-sdk/src/platform/address_sync/mod.rs:105-106
AddressSyncContext only contains mutable references to the provider, the key lookup, and the sync result. AddressProvider requires P: Send, AddressToBytes requires the address type to be Send + Sync, and the contained HashMap/AddressSyncResult fields are Send under those bounds, so Rust can derive Send automatically. Keeping the manual unsafe impl weakens future checks because adding a non-Send field would keep compiling instead of being rejected by the type system.
[SUGGESTION] [Carried-forward prior finding - STILL VALID] Avoid cloning the raw GroveDB proof on the re-sync path
packages/rs-sdk/src/platform/address_sync/mod.rs:578-579
The incremental success path clones recent_proof.grovedb_proof only because recent_proof is borrowed later for boundary extraction. The proof bytes can be large, and recent_proof is documented as debugging/inspection state, so every normal incremental sync pays an avoidable allocation. Compute the boundary decision first, then move recent_proof.grovedb_proof into result.recent_proof after the proof is no longer borrowed.
[NITPICK] [Carried-forward prior finding - STILL VALID] Express boundary parsing without unwrap
packages/rs-sdk/src/platform/address_sync/mod.rs:1137-1145
The key.len() == 8 guard makes the current try_into().unwrap() safe today, but this is proof-parsing code in a production SDK path and the local style is unwrap-free. Using the fallible conversion directly keeps the invariant local to the conversion and prevents a future edit from separating the guard from the unwrap.
let max_height = all_boundaries
.iter()
.filter_map(|key| {
<[u8; 8]>::try_from(key.as_slice())
.ok()
.map(u64::from_be_bytes)
})
.max();
New Findings In Latest Delta
None.
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] packages/rs-sdk/src/platform/address_sync/mod.rs:105-106: [Carried-forward prior finding - STILL VALID] Remove the manual unsafe Send implementation
AddressSyncContext only contains mutable references to the provider, the key lookup, and the sync result. AddressProvider requires P: Send, AddressToBytes requires the address type to be Send + Sync, and the contained HashMap/AddressSyncResult fields are Send under those bounds, so Rust can derive Send automatically. Keeping the manual unsafe impl weakens future checks because adding a non-Send field would keep compiling instead of being rejected by the type system.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:578-579: [Carried-forward prior finding - STILL VALID] Avoid cloning the raw GroveDB proof on the re-sync path
The incremental success path clones recent_proof.grovedb_proof only because recent_proof is borrowed later for boundary extraction. The proof bytes can be large, and recent_proof is documented as debugging/inspection state, so every normal incremental sync pays an avoidable allocation. Compute the boundary decision first, then move recent_proof.grovedb_proof into result.recent_proof after the proof is no longer borrowed.
- [NITPICK] packages/rs-sdk/src/platform/address_sync/mod.rs:1137-1145: [Carried-forward prior finding - STILL VALID] Express boundary parsing without unwrap
The key.len() == 8 guard makes the current try_into().unwrap() safe today, but this is proof-parsing code in a production SDK path and the local style is unwrap-free. Using the fallible conversion directly keeps the invariant local to the conversion and prevents a future edit from separating the guard from the unwrap.
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
Resolves the planned overlap with #3650 (Found-025): the height-pin delta gate now lives inside the extracted `apply_op` / `apply_change` seam, so the forward pass AND the end-of-pass unknown-address replay gate identically, and the gate is unit-testable without an Sdk. - `apply_op(op, current: AddressFunds, op_height)` gates per the pin: recent changes at or below it are already inside the pinned absolute; compacted operations gate per-op on their own heights. Applying advances the pin. - the call sites pass the height a change is recorded AS OF (recent: the entry's block height; compacted: the range end) instead of the pagination cursor — the cursor-based compacted filter is subsumed by per-address pins, which also fixes the fresh-address undercount the cursor filter caused on replay (a just-derived address's true base is 0 from genesis, so its older ops must apply). - `seed_base_balances` (#3650) becomes safe by construction: the seeded `current_balances()` funds now carry their pins, so the recent replay can no longer double-count a reconciled absolute routed through the defensive seed. - #3650's tests adapted from cursor to pin semantics (real block heights instead of 0-as-no-filter); new `apply_block_changes_drops_recent_delta_already_inside_pinned_absolute` covers the exact ADDR-09 shape on the pure seam. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up on an interaction with #4019 (the ADDR-09 height-pin fix), now that both are converging on The two PRs fix dual failure modes of the same apply loops:
Interaction worth knowing: Resolution (merged into #4019 at cfa400c): the pin gating now lives inside your pure
Net: the extraction in this PR made the ADDR-09 fix strictly better. Thanks for the seam. |
) + address-balance height pin Upstream #3841 rewrote the platform-wallet-ffi DashPay API and #3650 added the AddressFunds height pin; this adapts the Android port with full fidelity: - tokens.rs: reject → ignore/unignore contact-sender exports (local mute, DP-06); send/accept/payment gained the coreSignerHandle resolver param; Kotlin TokensNative/Dashpay wrappers + FriendsScreen updated (Reject button → Ignore). - persistence.rs: contacts vtable now carries ignored-sender deltas (new onPersistContactIgnored) and 6 new contact metadata fields; IdentityRestoreEntryFFI's contact/profile/ignored fields wired through the staging-vecs pipeline with matching free-trampoline reclaim; onPersistAddressBalance forwards as_of_height (stored as lastSeenHeight, the Swift handler's shape) and the transfer request path passes the documented 0 pin. - Room: DashDatabase v2 (MIGRATION_1_2) — dashpay_ignored_senders table + contact-request metadata columns; DashpayDao/entity extended; 3 new round-trip unit tests (metadata, ignore delta, restore). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 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