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 23 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:
📝 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 9ed0890) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3650 +/- ##
=========================================
Coverage 87.15% 87.15%
=========================================
Files 2641 2641
Lines 327793 327793
=========================================
Hits 285701 285701
Misses 42092 42092
🚀 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
… enum + alias Pure refactor of the Found-025 change-application seam in address_sync. The three named types carried no behavior the borrow split actually required two distinct named types for: - Rename AddressBalanceChange<'a> -> BalanceOp<'a> (the genuine 2-variant borrowed sum type; still Clone+Copy, still threads the hot apply path clone-free). - Move new_balance() off the enum into free fn apply_op(); the compacted height-filtered sum and recent flat add/set are preserved byte-for-byte. - Replace OwnedAddressBalanceChange + its into_owned()/as_borrowed() methods with a minimal OwnedBalanceOp enum (owned arity needed only to buffer a miss past its response-entry borrow), an inline clone at the cold buffer site, and a free borrow_op() for replay. - Replace struct PendingUnknownChange with `type PendingMiss = (Vec<u8>, OwnedBalanceOp, u64)` and positional access. Net: zero hand-written structs for this concept (2 enums + 1 alias + 2 free fns). Hot apply path keeps zero new clones; the only clone stays on the cold miss buffer path, exactly where into_owned() cloned before. All Found-025 + CMT-001 tests pass unchanged in logic (constructor syntax only). cargo check / clippy --all-features --tests clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for 000eeba: carried forward STILL VALID prior findings: #1:synthesized-nonce0-first-seen-addresses, #2:replay-refresh-three-iteration-cap-truncates-gaps; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
PR #3650 fixes the silent-discard defect for post-snapshot addresses by buffering changes for first-miss addresses and replaying them after an end-of-pass pending_addresses() refresh. The latest delta (000eeba) is a quality-neutral rename refactor (struct→enum/tuple-alias + free helpers); semantics unchanged. Both prior findings are STILL VALID at the head and are carried forward as suggestions.
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:789-801: Incremental write paths synthesize `nonce = 0` for first-seen addresses
In both `apply_block_changes` (line 795) and `refresh_and_replay_unknown` (line 932), `AddressFunds.nonce` is built as `result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0)`. When an address is first observed via an incremental balance delta this pass — exactly the post-snapshot/gap-extension case this PR is recovering — there is no prior `result.found` entry, so a literal `0` is inserted. That value escapes as authoritative state through `AddressSyncResult.found` and `AddressProvider::on_address_found`, and providers can persist it as canonical wallet state until a later authoritative refresh corrects it. The two in-source TODOs (lines 790–794 and 931) acknowledge the gap but do not fix it. Recommended fixes: model `nonce` as `Option<u32>` so callers cannot misuse a placeholder, fetch authoritative `AddressFunds` for newly-found addresses before publishing, or refuse to emit `on_address_found` from the incremental path until a real nonce is known. Re-validated at 000eeba8 — the rename refactor preserves the logic verbatim. Carried forward from the c25705d4 review.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:836-978: Hard-coded 3-iteration replay cap can still truncate long gap-extension chains
`refresh_and_replay_unknown` loops `pending_addresses()` refresh + replay but is hard-capped at `REPLAY_REFRESH_MAX_ITERATIONS = 3` (line 836). The `AddressProvider` trait contract (`provider.rs`) says `pending_addresses()` may grow after `on_address_found` and that sync continues until all pending addresses are resolved. A provider that reveals one additional derived address per callback wave makes forward progress for three rounds and then has remaining buffered changes deferred — re-introducing partial silent loss in the same gap-extension regime the PR identifies as the Found-025 root cause. The loop already breaks on `iteration_resolved == 0` (line 962), so the cap is redundant against true livelock when each iteration is making strict progress. The new regression tests only prove a two-step `A → A+1` chain, not the longer chains the trait contract promises. The cap-hit diagnostic (lines 974–978) is `debug!` only and is invisible at default log levels — there is no structured signal in `AddressSyncResult` to tell the caller a follow-up sync is required. Fix surface: lift the cap when each iteration makes strict progress (rely on the existing zero-progress break), require two consecutive zero-progress refreshes to terminate, or surface a structured `truncated`/`more_pending` flag on `AddressSyncResult` so the caller can immediately schedule another pass. Re-validated at 000eeba8 — the refactor did not touch the cap or loop shape. Carried forward from the c25705d4 review.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR scope is a single-file fix to rs-sdk address-sync (Found-025): post-snapshot addresses are no longer silently dropped. The address-sync change is the only in-scope code; the ~76k LOC merge delta is v3.1-dev base, not this PR. Two prior in-scope suggestions remain valid at HEAD: synthesized nonce=0 for first-seen addresses and the 3-iteration replay cap. The shielded-notes verifier and snapshot-unwrap findings flagged by Codex are pre-existing v3.1-dev code outside this PR's diff and are dropped from this review.
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:789-803: Incremental write paths synthesize nonce=0 for first-seen addresses
`apply_block_changes` (line 795) and `refresh_and_replay_unknown` (line 932) both build `AddressFunds.nonce` as `result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0)`. When an address is first observed via an incremental balance delta — exactly the post-snapshot/gap-extension case this PR is recovering — there is no prior `result.found` entry, so a literal `0` is inserted and propagated as authoritative state through `AddressSyncResult.found` and `AddressProvider::on_address_found`. The in-source TODO at lines 790-794 acknowledges the gap and points clients at `AddressInvalidNonceError.expected_nonce` for recovery, but providers that persist `funds` as canonical wallet state will hold a semantically wrong nonce until a later authoritative refresh corrects it. Consider modeling `nonce` as `Option<u32>` so callers cannot misuse the placeholder, fetching authoritative `AddressFunds` for newly-found addresses before publishing, or withholding `on_address_found` from the incremental path until a real nonce is known. Re-validated at HEAD 660e8c76 — byte-for-byte unchanged since the prior review.
- [SUGGESTION] In `packages/rs-sdk/src/platform/address_sync/mod.rs`:832-979: Hard-coded 3-iteration replay cap can truncate long gap-extension chains
`refresh_and_replay_unknown` is bounded by `REPLAY_REFRESH_MAX_ITERATIONS = 3` (line 836). The `AddressProvider` contract in `provider.rs` documents that `pending_addresses()` may grow after `on_address_found` and that sync continues until all pending addresses are resolved. A provider that legitimately reveals one additional derived address per callback wave makes forward progress for three rounds and then has remaining buffered changes deferred to the next sync pass — reintroducing partial same-pass loss for exactly the gap-extension scenario this PR is recovering. The loop already breaks on `iteration_resolved == 0` (line 962), so the numeric cap is redundant whenever each iteration makes strict progress. The cap-hit diagnostic at lines 973-979 emits only at `debug!` level (invisible at production default), and there is no structured `truncated`/`more_pending` signal on `AddressSyncResult` for the caller to schedule a follow-up. The new regression tests only cover a two-step A→A+1 chain, not the longer chains the trait contract promises. Options: drop the cap and rely on the zero-progress break, raise it to a soft livelock guard (e.g. 32) with a `warn!` on hit, require two consecutive zero-progress refreshes to terminate, or surface a structured truncation flag on `AddressSyncResult`. Re-validated at HEAD 660e8c76 — unchanged since the prior review.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative re-validation at 5136dae. The latest delta (commit #3784) is docs-only: it converts the nonce=0 TODO into an explicit cosmetic-nonce justification and lightly rewrites the unbounded-buffer comment. No behavioral change. Two prior findings on address_sync remain: the synthesized nonce=0 (now intentionally deferred with inline justification, but still surfaces through the public AddressSyncResult/AddressProvider surface) and the hard-coded 3-iteration replay cap (still structurally redundant with the existing zero-progress break and can silently truncate long gap-extension chains). No new in-scope defects in the latest delta.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:789-805: Synthesized nonce=0 still escapes through the public AddressSyncResult / provider-persistence surface
Carried-forward prior finding (the author has INTENTIONALLY DEFERRED this with a new inline justification at lines 790–800, but the justification is incomplete). The new comment correctly notes that the cached nonce is never broadcast because every SDK spend path re-fetches the authoritative nonce via `fetch_inputs_with_nonce` + `nonce_inc`. That covers the on-chain consensus concern. However, `apply_block_changes` (line 801) and the replay path (line 944) both still compute `AddressFunds.nonce` as `result.found.get(&result_key).map(|f| f.nonce).unwrap_or(0)`, and the resulting `AddressFunds { nonce, balance }` is a public type exposed through `AddressSyncResult.found` and `AddressProvider::on_address_found`. The platform-wallet provider stages that exact value into `per_wallet_in_sync.found`, commits it in `sync_finished`, and `sync_balances` persists changed `AddressFunds` entries — so a post-snapshot address whose true platform nonce is non-zero can be recorded and persisted as nonce 0 until a later authoritative refresh corrects it. Consumers of the public API cannot distinguish 'known zero' from 'unknown', so local persistence/display/change-diff code can treat the invented value as authoritative. If the author considers this acceptable for now, the inline comment should at least call out the persistence/public-surface caveat; a cleaner long-term fix is the `Option<u32>` representation the comment already gestures at.
Re-review @
|
| Thread | Verified |
|---|---|
CodeRabbit — clear result.absent on re-discovery |
absent.remove present at both sites (mod.rs:810, :950), each before found.insert. No symmetric found→absent leak (found.remove doesn't exist; absent.insert only in the pre-catch-up tree scan). |
| CMT-001 (HIGH) — multi-wallet refresh storm | Single end-of-pass refresh_and_replay_unknown (one call, :688); apply_block_changes never polls; foreign addresses excluded at the extras-intersection. Test asserts 0 per-block polls + 1 end-of-pass poll. |
| CMT-003 / 004 / 005 / 006 / 007 | Foreign-address test added; iterator streamed (no Vec); no key_to_tag.clone(); precise intersection (no len() proxy); warn!→debug! + dropped "next full sync" wording. All confirmed. |
No action needed beyond resolving these.
🟡 Standing item 1 — synthesized nonce=0 (CMT-002) → defer, but tighten the comment
The bot is factually correct that the 0 is durably persisted (the deferral comment currently undersells this): sync.rs:53-83 builds the changeset with the full funds incl. nonce and calls persister.store, the FFI write emits nonce: entry.funds.nonce, and it's round-tripped via current_balances() to seed the next pass.
However the implied harm does not materialize:
- Every spend re-fetches the on-chain nonce at build time (
fetch_inputs_with_nonce→AddressInfo::fetch_many,address_inputs.rs:28-34); the cached value is never read for a transition. - The managed-account reload pushes only
funds.balanceand drops the nonce (wallet.rs:138,withdrawal.rs:141,provider.rs:601). - The only surface that reads it is an FFI display struct that's already self-documenting ("Zero when unknown/unused").
So the residual is purely observability — a host UI could mislabel a post-snapshot spent-from address as nonce 0 until a later authoritative refresh. That matches the "handle edge cases user-side" stance and is a reasonable defer.
Recommendation: keep nonce=0; do not do the Option<u32> rework in this PR (it churns the public AddressFunds type, FFI structs, and persistence for zero correctness gain). Just extend the inline deferral comment to acknowledge the value is persisted/round-tripped but non-authoritative, with a one-line "do not display as the authoritative nonce" caveat, at both mod.rs:790-800 and the replay site :943.
🟡 Standing item 2 — hard-coded 3-iteration replay cap → fix now (nearly free)
Verified: the numeric cap is not redundant with the iteration_resolved == 0 break — they terminate on different conditions, and for a strictly-progressing gap-extension chain deeper than 3, the cap is exactly what truncates.
But severity is low: the production provider has gap-limit 20 and derives a whole gap window per on_address_found wave (provider.rs:51, 580-628), so hitting the cap needs ~60 indices of brand-new post-snapshot funding in a single ~15s window — implausible. And it's bounded deferral, not loss: a full tree rescan reads absolute balances and recovers anything dropped.
Still worth closing the cliff:
- Raise
REPLAY_REFRESH_MAX_ITERATIONSto 32 (≈640 indices of headroom at gap-limit 20) and reword it as a livelock guard. Don't drop it entirely — keep the finite guard against a buggy/adversarial provider that emits ever-new pending addresses. - Promote the cap-hit log
debug! → warn!(mod.rs:986). - Add one regression test for a chain deeper than the old cap of 3.
- A structured
truncated/more_pendingflag onAddressSyncResultis a reasonable observability add but optional — fine to defer.
Additional findings (independent, adversarially verified)
None are correctness-critical; all are bounded and self-heal on a full rescan. In priority order:
A. Undocumented invariant current_balances() ⊆ pending_addresses() (low; FFI-reachable, production-safe)
In incremental-only mode, key_to_tag and the replay extras are built only from pending_addresses() (mod.rs:330-333, 893-901), but result.found is seeded from current_balances() (mod.rs:370). A known address present in current_balances but not in pending would have its catch-up delta buffered-as-unknown and never recovered — the same Found-025 class, residual. The production platform-wallet provider satisfies the subset by construction (both constructors insert all managed/persisted addresses into pending, provider.rs:247, 318-328), so prod is safe. The FFI BatchAddressProvider takes pending and known_balances as two independent caller arrays with no enforced subset, so a caller can violate it. Suggest: document the invariant on the AddressProvider trait, or defensively fold current_balances() into key_to_tag.
B. Truncation watermark / observability cluster (low)
- On cap truncation,
new_sync_height/last_known_recent_blockadvance past the dropped block (mod.rs:690,697); the next incremental query usesRangeAfter, so the incremental path never re-fetches it — only a full rescan does. - The cap-hit comment "any further gap-extension addresses will surface on the next sync" is false for the buffered balance changes — true only for address discovery (
mod.rs:986). - Genuine wallet-owned loss at the cap is logged at
debug!with the same wording as benign foreign-address noise ("not tracked by this wallet"), conflating the two (mod.rs:998). - A transient recent-query failure returns early setting only
new_sync_height, leavinglast_known_recent_block/timestamp at 0, whichupdate_sync_statethen writes unconditionally → forces an unnecessary full rescan next pass (mod.rs:513; likely pre-existing). Suggest: these all improve together with astill_unknown > 0 → warn!that separates foreign vs wallet-owned counts — bundle with the cap fix.
C. Test-coverage gaps (the suite is thorough but Compacted-only)
- Highest value: the entire Recent op path (
apply_opRecent arms,OwnedBalanceOp::Recentbuffer/replay) — the common 15s-resync hot path — has zero tests; every test usesCompacted. - The compacted
height >= current_heightfilter (the anti-double-count mechanism) is never exercised with a discriminating height. - Also untested: multi-entry same-key replay accumulation; a chain deeper than the cap;
incremental_catch_upitself (the "decision" tests duplicate the logic instead of invoking it); found/absent disjoint on the replay path; the partial-recovery debug path. codecov's "all modified lines covered" masks these — they're branch/semantic gaps, not line gaps.
Bottom line
Sound PR, no blocker. Cheapest path to merge-clean: (1) tighten the nonce comment (one sentence), (2) raise the cap to 32 + warn! + one test, (3) document the current_balances ⊆ pending invariant, (4) add the Recent-path + height-filter tests. All small, all in address_sync/mod.rs (plus one trait doc-comment).
🤖 AI-assisted review (multi-agent verification pass); all findings independently re-verified against 5136dae1.
|
@QuantumExplorer I implemented the small follow-up set from your re-review on a fork branch:
What changed:
Validation:
No PR created, per tracker worker instructions. |
|
@lklimek I rebased the follow-up fork branch onto the new PR head Updated branch:
Validation on the rebased branch:
No PR created, per tracker worker instructions. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at head 5d375d5. The latest 5136dae..5d375d5 delta is a v3.1-dev merge that touches no PR-scope files — the address_sync module is byte-identical to the prior reviewed state. Prior-finding status: (1) synthesized nonce=0 for first-seen addresses — INTENTIONALLY DEFERRED (author added an explicit justification comment at mod.rs:790-800 documenting that the value is cosmetic and that spends re-fetch the authoritative nonce); (2) hard-coded 3-iteration replay cap — STILL VALID, carried forward as a suggestion. One additional cumulative in-scope suggestion is preserved: known balances from current_balances() are not folded into the entry-time lookup, leaving a residual silent-drop class for providers that don't already enforce the implicit subset invariant. No new latest-delta findings. Implementation is sound; review action is COMMENT.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:327-378: Known balances outside pending_addresses() can still be treated as unknown
The entry-time lookup `key_to_tag` (mod.rs:330-333) is built only from `provider.pending_addresses()`, while incremental-only mode separately seeds `result.found` from `provider.current_balances()` (mod.rs:376-378). Both `apply_block_changes` and `refresh_and_replay_unknown` route deltas via `key_to_tag` only, so if a provider returns a previously-known balance from `current_balances()` but does not also include that same address in `pending_addresses()`, an incremental delta for that address is buffered as unknown — and the refresh path's intersection probe (mod.rs:895-901) cannot recover it unless the next `pending_addresses()` poll yields it. The platform-wallet provider appears to satisfy the subset invariant by rebuilding pending from its persisted address map, but the `AddressProvider` trait does not document this invariant, and the FFI `BatchAddressProvider` accepts pending addresses and known balances as independent caller arrays. Either fold `current_balances()` into `key_to_tag` before catch-up, or document/enforce `current_balances() ⊆ pending_addresses()` so this PR closes the silent-drop class for custom providers rather than just for the in-tree wallet provider.
…iant docs Triaged review fixes for the Found-025 address-sync replay path: - Replay-iteration cap raised 3 -> 32 and reframed as a livelock guard, not a functional gap-extension depth limit. Cap-hit log promoted debug! -> warn! with corrected wording (dropped buffered deltas need a full rescan, NOT just the next sync). Split the still-unresolved tail log: warn! for cap-truncated wallet-owned loss, debug! for genuinely foreign noise. - Test coverage: added the Recent-op buffer+replay path (the ~15s resync hot path, previously zero coverage), the >= current_height anti-double-count height filter with a discriminating cursor, a gap-extension chain deeper than the old cap of 3 (depth 5, one pass), and a current_balances-fold regression test. - Documented the AddressProvider::current_balances invariant (keys MUST be a subset of pending_addresses) and added a defensive fold of current_balances keys into the entry-time lookup so a bridge provider's two-array shape cannot silently drop deltas. - Added TODOs at the watermark-past-cap-dropped-block and transient-recent-failure early-return sites (behavioral; left unchanged to avoid risking sync-state logic). - Documented the accepted nonce=0 synthesis risk (published + persisted via changeset PlatformAddressBalanceEntry but non-authoritative; every spend re-fetches the on-chain nonce) at both synthesis sites; the Option<u32> rework is deliberately not done. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @QuantumExplorer — actioned in a711cc8. We first ran a full adversarial re-trace of every finding (yours and the bots') against the code: every substantive claim held, but 3 evidence citations were inaccurate (1 hallucinated — the bot's
🤖 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
Cumulative incremental review at head a711cc8. Both prior blocking findings are FIXED: the replay loop's cap is raised from 3 to 32 with cap-hit warn + a new depth-5 chain test (mod.rs:885-894, 1045-1050, 2284-2378), and current_balances() is folded into key_to_tag with a documented subset-of-pending invariant on the trait + regression test (mod.rs:334-345, provider.rs:150-163, mod.rs:2540-2632). The prior synthesized-nonce-0 finding remains INTENTIONALLY DEFERRED with an expanded justification at mod.rs:822-842 and mod.rs:989-995. One new latest-delta defect is convergent across reviewers: the cap-hit warn! fires even when all misses resolved on the final allowed iteration (false data-loss alarm). One carried-forward concern from claude-rust-quality has merit: the defensive current_balances fold seeds key_to_tag but not result.found, so on the full-scan path an AddToCredits for an invariant-violating address would apply from a zero base. Codex-general flagged an API-break/PR-description mismatch on the AddressProvider trait; the trait was introduced for this Found-025 series so the practical risk is low, kept as a suggestion to update the PR description. The remaining flagged items were noisy nits or pre-existing concerns the author already classified via explicit TODO(address-sync) markers and are out of scope.
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/address_sync/mod.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:1032-1051: Cap-hit warn! fires even when refresh+replay resolved every miss on the final iteration
`hit_iteration_cap` is set whenever the last allowed iteration applied at least one change (line 1032-1034), before the code checks whether anything is actually unresolved. The unconditional `warn!` at lines 1045-1050 then asserts that 'buffered balance change(s) ... were dropped and will only be recovered by a full rescan' — but a legitimate gap-extension chain that happens to finish exactly on iteration 32 leaves `still_unknown == 0`, so nothing was actually dropped. The result is an operator-visible false-positive data-loss warning that wasn't operationally noisy when the cap was a `debug!`, but is now since the delta promoted it to `warn!`. Compute `still_unknown` first and only emit the cap-loss warning when unresolved entries actually remain — collapse the two log sites into the existing `if still_unknown > 0` block.
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/mod.rs:334-345: Defensive current_balances fold lands the delta on the direct path but the AddToCredits base balance is wrong on the full-scan branch
The fold at lines 341-345 adds invariant-violating `current_balances` keys into `key_to_tag` so their incremental deltas stop being silently buffered — good. But only the incremental-only branch (line 388-390) seeds the *balance* into `result.found`; the full-scan branch (line 392+) populates `result.found` exclusively from the tree scan, which iterates `pending_addresses` and will never encounter an address listed only in `current_balances`. When an `AddToCredits(X)` delta then arrives in `apply_block_changes` for such an address, `current_balance = result.found.get(..).unwrap_or(0)` returns 0 instead of the actual current balance, so the published `funds.balance` becomes `0 + X` rather than `existing + X`. `SetCredits` is unaffected. The new regression test `current_balances_fold_keeps_unpending_address_off_the_drop_path` papers over this by pre-seeding `result.found` manually before invoking `apply_block_changes`, so it doesn't exercise the production seeding gap. The provider-side invariant docstring puts the burden on the implementer, so this is a defense-completeness suggestion — but if the fold is meant to be a real shield (not just a symptom mask), mirror the seam by folding the funds too.
In `packages/rs-sdk/src/platform/address_sync/provider.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/address_sync/provider.rs:46-166: AddressProvider trait shape changes — reconcile with the PR description's no-API-break claim
Cumulative (not introduced by the latest 5d375d5f..a711cc8 delta). The `AddressProvider` trait is publicly re-exported from `address_sync` and this PR series gives it the new shape: associated `Tag`/`Address` types with `AddressToBytes`, async callbacks, iterator-returning `pending_addresses()`/`current_balances()`, and an `AddressSyncResult` generic over the provider key types. Anyone implementing the previously-public trait shape will no longer compile, which contradicts the PR description's 'Public API unchanged' / 'Breaking Changes: None' wording. The practical risk is low — this trait was introduced as part of the Found-025 series and all in-tree implementers are updated — but the PR description should call out the API surface explicitly so downstream rs-sdk consumers aren't surprised, or add a compat-adapter note if you intend to support the old shape.
… tighten comments (review)
Re-review fixes for the address-sync hardening commit:
- Cap-hit warning: removed the standalone `if hit_iteration_cap { warn! }`
block that fired even when every buffered change resolved
(still_unknown == 0) — a false "data dropped" warning. The remaining
gated block already warns only on genuine cap-truncated loss
(still_unknown > 0 && hit_iteration_cap) and debugs on foreign noise.
- current_balances defensive fold: the key-only fold left result.found
unseeded, so on the full-scan branch an AddToCredits(X) on a
current-balances-only address computed 0 + X instead of existing + X.
Added a base-balance gap-seed (result.found.entry().or_insert(funds))
AFTER the authoritative tree scan, so the scan wins on overlap
(it uses .insert()) and the seed only fills gaps. Reworked the
regression test to drop the manual result.found pre-seed, mirror both
full-scan seams, and assert existing + X (5000 + 1500), not 0 + X.
- Tightened five verbose comments (fold, both TODO(address-sync), both
INTENTIONAL nonce notes) to <=3 lines, preserving the TODO tags and the
INTENTIONAL accepted-risk / non-authoritative / "do not treat as
authoritative" essentials.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@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.
4 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