Skip to content

feat: unify full_scan and sync into a single scan API#2181

Closed
noahjoeris wants to merge 5 commits intobitcoindevkit:masterfrom
noahjoeris:feat/improved-sync
Closed

feat: unify full_scan and sync into a single scan API#2181
noahjoeris wants to merge 5 commits intobitcoindevkit:masterfrom
noahjoeris:feat/improved-sync

Conversation

@noahjoeris
Copy link
Copy Markdown

Description

Proposal: unify full_scan and sync into a single scan API. scan(...) combines explicit sync and keychain discovery in one request/call, so callers no longer need to stitch together sync(...) + full_scan(...). Open to naming/API changes. I also tried to increase DX a bit.

Fixes #2057. The old spks_from_indexer(...) used an unbounded iterator starting at index 0. A single stop-gap scan therefore covered both revealed and unrevealed scripts in one pass, which could cause it to terminate before reaching a higher revealed address. test_scan_revealed_scripts_beyond_stop_gap covers this now.

scan splits the old behavior along the right seam:

  • revealed_spks_from_indexer(...): explicit sync of the already revealed range 0..=last_revealed
  • discover_from_indexer(...): discovery starting at last_revealed + 1, with stop_gap applied only to the unrevealed part

Other changes:

  • The new builder uses discover_keychain(...) instead of spks_for_keychain(...) (renamed for clarity)
  • stop_gap moved onto the builder. Defaults to 20 (matches BIP44/Electrum/Sparrow defaults). I assume most callers use the default unless they need something custom.
  • Unified inspect/progress API via ScanItem / ScanProgress.
  • full_scan(...) and sync(...) kept in place for gradual migration.

API changes

New Old / change
scan(...) sync(...) + full_scan(...); new unified entrypoint for explicit sync and discovery
ScanRequest / ScanResponse SyncRequest/SyncResponse + FullScanRequest/FullScanResponse; new unified request/response types
ScanRequestBuilder::discover_keychain(...) + stop_gap(...) FullScanRequestBuilder::spks_for_keychain(...) + full_scan(..., stop_gap, ...); same discovery input, but stop_gap moved onto the builder
ScanRequestBuilderExt::revealed_spks_from_indexer(...) + discover_from_indexer(...) FullScanRequestBuilderExt::spks_from_indexer(...); split into explicit sync of revealed scripts plus discovery starting at last_revealed + 1

Notes to the reviewers

Waiting for concept (N)ACK before filling in more tests, example updates, or deprecations.

Other findings while working on this:

  • Esplora clamps stop_gap to 1 via .max(1) but electrum did not. I aligned electrum here.
  • Due to parallel_requests in Esplora, the scan can overshoot stop_gap. Same behavior as the previous full_scan, no harm, keeping as-is.

Limitations:

  • expected_spk_txids currently applies only to explicitly synced scripts (spks / spks_with_indexes), not to discovery scripts (discover_keychain / discover_from_indexer). This matches the current design where expected txids are associated with known scripts rather than the unrevealed discovery suffix. Extending to discovery is possible if we want it to be agnostic later.

Follow-ups (post concept-ACK):

  • Update examples to use scan
  • Mark sync / full_scan as #[deprecated]
  • Consider migration helpers to ease adoption
  • More test coverage

Changelog notice

  • Added unified scan API in bdk_core::spk_client: ScanRequest, ScanResponse, ScanRequestBuilder, ScanRequestBuilderExt, ScanItem, ScanProgress.
  • Added BdkElectrumClient::scan, EsploraExt::scan, and EsploraAsyncExt::scan.
  • Fixed stop-gap terminating before reaching higher already-revealed scripts (Unintuitive and undocumented full_scan behavior #2057).

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Concept NACK

First off, thanks for working on this. However, I think full_scan and sync should remain separate APIs. They have different inputs and use cases, and merging them adds complexity without adding capability.

The underlying issue (#2057) is valid, but the fix should be targeted: give full_scan awareness of last_revealed so the stop gap only applies beyond that point. See #2057 (comment).

@noahjoeris
Copy link
Copy Markdown
Author

Appreciate the feedback! 👍 Yep I can do that too.

My idea was to address this comment by @ValuedMammal #2057 (comment) which @luisschwab also upvoted.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Unintuitive and undocumented full_scan behavior

2 participants