Skip to content

docs: clarify address-sync catch-up nonce and buffer comments#3784

Open
QuantumExplorer wants to merge 1 commit into
fix/rs-sdk-address-sync-found-025from
docs/address-sync-review-fixes
Open

docs: clarify address-sync catch-up nonce and buffer comments#3784
QuantumExplorer wants to merge 1 commit into
fix/rs-sdk-address-sync-found-025from
docs/address-sync-review-fixes

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Review follow-ups to #3650 (stacked on top of its branch). Comment-only — no behavior change, no logic touched. Addresses three reviewer notes:

  1. The synthesized-nonce comment was misleading. It claimed clients recover via AddressInvalidNonceError.expected_nonce, but there is no such handler anywhere in rs-sdk / rs-platform-wallet / swift-sdk. The actual reason nonce = 0 is harmless is that every address-spend path re-fetches the authoritative nonce at build time (fetch_inputs_with_nonce + nonce_inc in address_inputs.rs), so the synced value is cosmetic and never lands in a broadcast transition. A future reader following the old comment would go hunting for a recovery path that doesn't exist.
  2. result.absent.remove(...) was undocumented. It's a deliberate fix that keeps found and absent disjoint when a post-checkpoint funding lands on an address the branch scan proved absent (previously the address could appear in both sets). Now commented as such at both call sites.
  3. In-source reviewer handle / PR number removed. The unbounded-buffer NOTE referenced @thepastaclaw and PR #3650, which go stale once merged into history. Kept the substance (bounded-memory mitigation: buffer keys only, re-derive on replay).

What was done?

  • Rewrote the nonce comment in apply_block_changes to state the real reason the synthesized 0 is safe (build-time authoritative fetch), and downgraded the replay-site comment to a back-reference.
  • Added comments documenting the found/absent disjointness fix at both the forward-pass and replay sites.
  • Removed the reviewer handle and PR number from the buffer NOTE while preserving the mitigation guidance.

How Has This Been Tested?

No code paths changed — the diff is exclusively // line comments inside function bodies, so it cannot affect compilation or runtime behavior. The existing address_sync unit tests on the original PR remain the source of truth.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Review follow-ups to PR #3650 (comment-only, no behavior change):

1. Correct the synthesized-nonce comment. The old TODO claimed clients
   recover via `AddressInvalidNonceError.expected_nonce`, but no such
   handler exists anywhere in the SDK/wallet. The real reason nonce=0 is
   safe is that every spend path re-fetches the authoritative nonce at
   build time (`fetch_inputs_with_nonce` + `nonce_inc`), so the synced
   value is cosmetic and is never broadcast.

2. Document `result.absent.remove(...)` as a deliberate fix that keeps
   `found` and `absent` disjoint when a post-checkpoint funding lands on
   an address the branch scan proved absent (previously it could appear
   in both sets).

3. Drop the in-source reviewer handle and PR number from the
   unbounded-buffer NOTE; keep the bounded-memory mitigation guidance.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@QuantumExplorer QuantumExplorer requested a review from shumkov as a code owner June 2, 2026 18:47
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 2, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2ce933b7-c99d-4db4-aed9-740e9b51fafa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/address-sync-review-fixes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Jun 2, 2026

✅ Review complete (commit 9b2f988)

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Comment-only PR clarifying three review notes in packages/rs-sdk/src/platform/address_sync/mod.rs from PR #3650. Both reviewers verified the new wording is factually accurate (e.g., fetch_inputs_with_nonce + nonce_inc flow exists, no AddressInvalidNonceError.expected_nonce handler), and there is no behavior change. Nothing to flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants