feat(core): add non-consuming SyncRequest iterators by alok844937-design · Pull Request #2236 · bitcoindevkit/bdk

alok844937-design · 2026-06-28T07:26:30Z

Description

This PR introduces dedicated consuming and non-consuming iteration APIs for SyncRequest while preserving backwards compatibility, as proposed in #2220.

Previously, SyncRequest exposed iter_txids(&mut self) and iter_outpoints(&mut self) as consuming iterators(draining the request via SyncIter), despite their iter_* naming suggesting read-only iteration. At the same time, there was no API for inspecting pending spks, txids, or outpoints without consuming the request.

This PR addresses these issues while preserving backwards compatibility.

API Changes

Adds new read-only accessors:

SyncRequest::spks(&self)
SyncRequest::txids(&self)
SyncRequest::outpoints(&self)

Renames the consuming API to:

SyncRequest::drain_txids(&mut self)
SyncRequest::drain_outpoints(&mut self)

Reintroduces:

SyncRequest::iter_txids(&mut self)
SyncRequest::iter_outpoints(&mut self)

as deprecated compatibilty wrappers that delegate to the corresponding drain_* methods. This preserves existing downstream code while providing a migration path to the clearer API.

Backend updates

Updated all in-tree call-sites to use the new consuming APIs:

bdk_esplora (blocking and async)
bdk_electrum

Testing

Added tests verifying that:

spks(), txids(), and outpoints() are non-consuming.
Repeated calls to the read-only accessors return consistent results.
drain_txids() continues to consume all items as before.

Design rationale

This follows the Rust API Guidelines:

drain_* clearly indicates consuming iteration over the request contents.
spks(), txids(), and outpoints() provide borrowed, read-only access.
Existing iter_* APIs remain available as deprecated compatibility wrappers, avoiding an immediate breaking change for downstream users.

Fixes #2220

Notes to the reviewers

This revision follows the API direction suggested during review:

consuming methods renamed to drain_*
read-only methods exposed as spks(), txids(), and outpoints()
iter_* methods retained as deprecated compatibility wrappers

The read-only accessors return impl ExactSizeIterator + '_, matching the capabilities of the underlying collections.
I observed an intermittent failure of bdk_electrum's test_sync during local testing (trusted_pending vs confirmed balance assertion). Re-running the same test on the same commit passed without any code changes, suggesting pre-existing test flakiness rather than a regression introduced by this PR.

Verified locally with:

cargo fmt
cargo build
cargo test -p bdk_core --test test_spk_client
multiple runs of cargo test -p bdk_electrum --test test_electrum

Changelog notice

Added

Added SyncRequest::spks, SyncRequest::txids, and SyncRequest::outpoints for non-consuming inspection of pending sync data.

Changed

Added SyncRequest::drain_txids and SyncRequest::drain_outpoints as the consuming iterator APIs.
Deprecated SyncRequest::iter_txids and SyncRequest::iter_outpoints in favor of drain_* for consuming iteration or the new read-only accessors.

Checklists

All Submissions

I followed the contribution guidelines.

New Features

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

Bugfixes

I'm linking the issue being fixed by this PR.

alok844937-design · 2026-06-28T07:27:30Z

@evanlinjin PTAL

evanlinjin

Thanks for picking this up.

This PR silently changes iter_... methods from consuming methods to non-consuming. This will be a problem for downstream callers.

Additionally, into_... is the wrong naming convention. As per Rust API Guidelines, the into prefix should be reserved for methods that consume all of self.

To fix these naming problems, I propose the following:

Rename consuming methods to use the drain_ prefix.
Reintroduce iter_ methods that call the drain_ methods, but mark them as deprecated. This way these changes are backwards compatible.
Use no prefix (spks(), txids(), etc.) for read-only methods.

codecov · 2026-06-28T08:30:01Z

Codecov Report

❌ Patch coverage is 47.61905% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.46%. Comparing base (6d03fc3) to head (e1d4c6f).

Files with missing lines	Patch %	Lines
crates/core/src/spk_client.rs	35.29%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2236      +/-   ##
==========================================
- Coverage   78.65%   78.46%   -0.20%     
==========================================
  Files          30       30              
  Lines        5909     5924      +15     
  Branches      279      279              
==========================================
  Hits         4648     4648              
- Misses       1185     1200      +15     
  Partials       76       76

Flag	Coverage Δ
rust	`78.46% <47.61%> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alok844937-design · 2026-06-28T08:35:12Z

Thanks for the feedback!

I've updated the PR based on your suggestions:

Renamed the consuming methods to drain_*.
Added read-only accessors as spks(), txids(), and outpoints().
Reintroduced iter_txids and iter_outpoints as deprecated compatibility wrappers to preserve backwards compatibility.
Updated the in-tree call sites, tests, and the PR description to reflect these changes.

PTAL when you have a chance. Thanks!

- Rename consuming iterators to drain_txids/drain_outpoints - Add read-only accessors: spks(), txids(), outpoints() - Deprecate iter_txids/iter_outpoints as compatibility wrappers delegating to drain_txids/drain_outpoints - Update call-sites in esplora and electrum backends - Update/rename tests accordingly Fixes bitcoindevkit#2220

evanlinjin · 2026-06-28T21:00:01Z

+fn test_spks_does_not_consume() {
+    let spk1 = bitcoin::ScriptBuf::new();
+    let spk2 = bitcoin::ScriptBuf::new();
+
+    let request: SyncRequest<u32, BlockHash> = SyncRequest::builder()
+        .spks_with_indexes(vec![(0u32, spk1), (1u32, spk2)])
+        .build();
+
+    assert_eq!(request.spks().count(), 2);
+    assert_eq!(request.spks().count(), 2, "must not consume");
+}
+
+#[test]
+fn test_txids_does_not_consume() {
+    let txid1 = Txid::from_byte_array([0x01; 32]);
+
+    let request: SyncRequest<(), BlockHash> = SyncRequest::builder().txids(vec![txid1]).build();
+
+    assert_eq!(request.txids().count(), 1);
+    assert_eq!(request.txids().count(), 1, "must not consume");
+}
+
+#[test]
+fn test_outpoints_does_not_consume() {
+    let outpoint1 = OutPoint::null();
+
+    let request: SyncRequest<(), BlockHash> =
+        SyncRequest::builder().outpoints(vec![outpoint1]).build();
+
+    assert_eq!(request.outpoints().count(), 1);
+    assert_eq!(request.outpoints().count(), 1, "must not consume");
+}
+
+#[test]
+fn test_drain_txids_still_consumes_all_items() {
+    let txid1 = Txid::from_byte_array([0x01; 32]);
+    let txid2 = Txid::from_byte_array([0x02; 32]);
+
+    let mut request: SyncRequest<(), BlockHash> =
+        SyncRequest::builder().txids(vec![txid1, txid2]).build();
+
+    assert_eq!(request.txids().count(), 2);
+
+    let consumed: Vec<_> = request.drain_txids().collect();
+    assert_eq!(
+        consumed.len(),
+        2,
+        "drain_txids must still consume all items"
+    );
+}


These tests don't earn their keep imo. It's apparent from the method's signature.

Thanks for the feedback! I removed those tests since the non-consuming behavior is already apparent from the method signatures. I've updated the PR accordingly. PTAL when you have a chance.

alok844937-design · 2026-06-29T02:07:44Z

Hi @evanlinjin, After removing the tests per your suggestion, the Codecov patch coverage dropped because these new accessors are no longer exercised. Would you prefer adding a small focused test for the new APIs, or is the coverage drop acceptable in this case?

alok844937-design requested review from evanlinjin, luisschwab and oleonardolima as code owners June 28, 2026 07:26

github-project-automation Bot added this to BDK Chain Jun 28, 2026

evanlinjin requested changes Jun 28, 2026

View reviewed changes

alok844937-design force-pushed the feature/sync-request-iterators-2220 branch from 67933be to 7ba14fe Compare June 28, 2026 10:14

evanlinjin requested changes Jun 28, 2026

View reviewed changes

test(core): remove redundant SyncRequest iterator tests

e1d4c6f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(core): add non-consuming SyncRequest iterators#2236

feat(core): add non-consuming SyncRequest iterators#2236
alok844937-design wants to merge 2 commits into
bitcoindevkit:masterfrom
alok844937-design:feature/sync-request-iterators-2220

alok844937-design commented Jun 28, 2026 •

edited

Loading

Uh oh!

alok844937-design commented Jun 28, 2026

Uh oh!

evanlinjin left a comment •

edited

Loading

Uh oh!

codecov Bot commented Jun 28, 2026 •

edited

Loading

Uh oh!

alok844937-design commented Jun 28, 2026 •

edited

Loading

Uh oh!

evanlinjin Jun 28, 2026

Uh oh!

alok844937-design Jun 28, 2026

Uh oh!

alok844937-design commented Jun 29, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

alok844937-design commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

API Changes

Backend updates

Testing

Design rationale

Notes to the reviewers

Changelog notice

Added

Changed

Checklists

Uh oh!

alok844937-design commented Jun 28, 2026

Uh oh!

evanlinjin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alok844937-design commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

alok844937-design Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

alok844937-design commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alok844937-design commented Jun 28, 2026 •

edited

Loading

evanlinjin left a comment •

edited

Loading

codecov Bot commented Jun 28, 2026 •

edited

Loading

alok844937-design commented Jun 28, 2026 •

edited

Loading

alok844937-design commented Jun 29, 2026 •

edited

Loading