Skip to content

Remove panic from stop gap scan loop #2053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
reez wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove panic from stop gap scan loop #2053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions crates/esplora/src/async_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,8 +314,11 @@ where
type TxsOfSpkIndex = (u32, Vec<esplora_client::Tx>, HashSet<Txid>);

let mut update = TxUpdate::<ConfirmationBlockTime>::default();
let mut last_index = Option::<u32>::None;
let mut last_active_index = Option::<u32>::None;
// Use consecutive_unused so unused count drives stop gap.
let mut consecutive_unused = 0usize;
// Treat stop_gap = 0 as 1 while preserving original semantics for other values.
let gap_limit = stop_gap.max(1);

loop {
let handles = keychain_spks
Expand Down Expand Up @@ -352,8 +355,10 @@ where
}

for (index, txs, evicted) in handles.try_collect::<Vec<TxsOfSpkIndex>>().await? {
last_index = Some(index);
if !txs.is_empty() {
if txs.is_empty() {
consecutive_unused = consecutive_unused.saturating_add(1);
} else {
consecutive_unused = 0;
last_active_index = Some(index);
}
for tx in txs {
Expand All @@ -368,13 +373,7 @@ where
.extend(evicted.into_iter().map(|txid| (txid, start_time)));
}

let last_index = last_index.expect("Must be set since handles wasn't empty.");
let gap_limit_reached = if let Some(i) = last_active_index {
last_index >= i.saturating_add(stop_gap as u32)
} else {
last_index + 1 >= stop_gap as u32
};
if gap_limit_reached {
if consecutive_unused >= gap_limit {
Copy link
Member

@evanlinjin evanlinjin Jan 16, 2026

Choose a reason for hiding this comment

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

@reez this is the only place that gap_limit is used. Can you explain what is functionally different when the gap_limit is minimally bound to parallel_requests vs when it's not?

I may be missing something, but I'm failing to see a clear rationale for the bound.

Copy link
Member

@evanlinjin evanlinjin Jan 16, 2026

Choose a reason for hiding this comment

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

I took a look at this original issue: bitcoindevkit/bdk_wallet#63

I think bounding to 1 makes more sense - since a stop_gap of 0 is undefined and the behaviour we want is to "treat 0 as 1".

Bounding it to parallel requests does the same thing, but the intention is obscured imo. It seems like the intention is to have consecutive_used be a multiple of gap_limit if the gap_limit is originally set lower than parallel_requests? However we are using >=, and this breaks down when gap_limit > parallel_requests anyway.

If the rationale is not obvious, the reader might assume they are missing something and spend time trying to understand it and then find out there is nothing to be understood.

Copy link
Member

@evanlinjin evanlinjin Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Consider parallel_requests=10, stop_gap=5, first batch processes indices 0-9 where indices 0-4 have transactions and 5-9 are empty:

Original code:

  • last_active_index=4, last_index=9
  • Check: 9 >= 4 + 5? → true, break

If we do .max(1):

  • consecutive_unused=5, gap_limit=5
  • 5 >= 5? → true, break ✓ (matches original)

If we keep this PR as is with .max(parallel_requests):

  • consecutive_unused=5, gap_limit=10
  • 5 >= 10? → false, continue ✗ (different from original)

So .max(parallel_requests) changes the effective stop_gap when the user provides a value less than parallel_requests, which is a behavioral change from the original code. .max(1) preserves the original semantics while handling the stop_gap=0 edge case.

Suggestion

Change to .max(1).

Copy link
Author

@reez reez Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Updated to .max(1) thanks for the deep dive review of that. Also tweaked the rationale comments. Updated PR description to mention .max(1) too now. Happy to adjust further if needed.

break;
}
}
Expand Down Expand Up @@ -596,7 +595,10 @@ mod test {
let res = chain_update(&client, &latest_blocks, &cp, &anchors).await;
use esplora_client::Error;
assert!(
matches!(*res.unwrap_err(), Error::HeaderHashNotFound(hash) if hash == genesis_hash),
matches!(
*res.unwrap_err(),
Error::HeaderHashNotFound(hash) if hash == genesis_hash
),
"`chain_update` should error if it can't connect to the local CP",
);

Expand Down
19 changes: 9 additions & 10 deletions crates/esplora/src/blocking_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,11 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
type TxsOfSpkIndex = (u32, Vec<esplora_client::Tx>, HashSet<Txid>);

let mut update = TxUpdate::<ConfirmationBlockTime>::default();
let mut last_index = Option::<u32>::None;
let mut last_active_index = Option::<u32>::None;
// Use consecutive_unused so unused count drives stop gap.
let mut consecutive_unused = 0usize;
// Treat stop_gap = 0 as 1 while preserving original semantics for other values.
let gap_limit = stop_gap.max(1);

loop {
let handles = keychain_spks
Expand Down Expand Up @@ -321,8 +324,10 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>

for handle in handles {
let (index, txs, evicted) = handle.join().expect("thread must not panic")?;
last_index = Some(index);
if !txs.is_empty() {
if txs.is_empty() {
consecutive_unused = consecutive_unused.saturating_add(1);
} else {
consecutive_unused = 0;
last_active_index = Some(index);
}
for tx in txs {
Expand All @@ -337,13 +342,7 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
.extend(evicted.into_iter().map(|txid| (txid, start_time)));
}

let last_index = last_index.expect("Must be set since handles wasn't empty.");
let gap_limit_reached = if let Some(i) = last_active_index {
last_index >= i.saturating_add(stop_gap as u32)
} else {
last_index + 1 >= stop_gap as u32
};
if gap_limit_reached {
if consecutive_unused >= gap_limit {
break;
}
}
Expand Down