fix: define and document stop_gap

[storopoli]

Description

• changes the code implementation to "the maximum number of consecutive unused addresses" in esplora async and blocking extensions.
• for all purposes treats stop_gap = 0 as stop_gap = 1.
• renames past_gap_limit to gap_limit_reached to indicate we want to break once the gap limit is reached and not go further in full_scan, as suggested in Clearly define and document stop_gap #1227 (comment)
• change the tests according to the new implementation.
• add notes on what stop_gap means and links to convergent definition in other Bitcoin-related software.

Closes #1227.

Notes to the reviewers

We can iterate over the wording and presentation of the stop_gap definition
and details.

Changelog notice

• BREAKING: change stop_gap definition and effects in full_scan
  to reflect the common definitions in most Bitcoin-related software.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

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

[evanlinjin]

Thanks for this work.

I'm actually in favor of renaming the variable instead of changing the definition.

I think if stop_gap is renamed to allowed_empty_history_count it can avoid the confusion and we can avoid an API that panics on it being 0.

[storopoli]

I think if stop_gap is renamed to allowed_empty_history_count it can avoid the confusion and we can avoid an API that panics on it being 0.

I think that stop_gap is more widely used and will be a better fit for users without having to "reinvent the wheel".
There's no need to add more complexity for something that is de-facto a stop_gap like all major wallets and Bitcoin-related software defines.

We can discuss on how to handle 0.

[ValuedMammal]

I think the code in bdk_electrum suffers from the same stop gap bug. If so, we should try to fix that in this PR also, and probably have a test for it.

In terms of Changelog I consider this a bugfix but I wouldn't call it a breaking change, per se ?

[ValuedMammal]

Suggested change

	last_index > i.saturating_add(stop_gap as u32)
	last_index >= i.saturating_add(stop_gap as u32)

Imagine 10 addresses and we send a tx to the 4th address and the 10th address (like in the test). We need to scan 6 times in order to find the tx at the 10th address (5, 6, 7, 8, 9, 10). So here, when i is 3 and the last_index scanned is 9, we have

9 >= 3 + 6

and we break.

This means the second half of the test test_update_tx_graph_gap_limit should read

// A scan with gap limit 5 won't find the second transaction, but a scan with gap limit 6 will.

[danielabrozzoni]

Hey, this was marked as resolved, but the code didn't really change. Unresolving.

[danielabrozzoni]

I agree with José here, although allowed_empty_history_count would let us avoid the problem with stop_gap = 0, I don't think it's worth it to re-invent the wheel and introduce a new concept. We should stick with what users already know.

I'm fine with both panicking or returning an error if the stop_gap is 0.

[evanlinjin]

I'll argue that allowed_empty_count is not a difficult concept to understand, so if it helps us avoid the stop_gap=0 situation then it's very much worth it.

Returning an error when stop_gap=0 is overdoing it imo. If we go with stop_gap, I much prefer a panic when it is 0.

[danielabrozzoni]

I'll argue that allowed_empty_count is not a difficult concept to understand

You're right that it's not difficult to understand, but it's yet another thing to find out and learn about bdk. My intuition is that for a developer that wants to use bdk, there's already a lot to be learned, should we really add yet another concept, even if small?

Returning an error when stop_gap=0 is overdoing it imo.

I would agree if we didn't already return a result, but since we do, it's probably not so bad to add a new error variant. But again, both error and panic are ok for me.

[storopoli]

We can discuss Error/Panic on Tue's call

[danielabrozzoni]

I would agree if we didn't already return a result, but since we do, it's probably not so bad to add a new error variant.

Gosh, I just realized we return a esplora error, and so we can't create a new error variant easily 😅

[storopoli]

I would agree if we didn't already return a result, but since we do, it's probably not so bad to add a new error variant.

Gosh, I just realized we return a esplora error, and so we can't create a new error variant easily 😅

I have a stash that wraps the errors:

/// Custom error type for handling errors related to `esplora_client` operations
/// and additional conditions specific to our application logic, such as invalid parameters.
#[derive(Debug)]
enum EsploraError {
    /// Represents errors originating from the `esplora_client` crate.
    EsploraClientError(esplora_client::Error),
    /// Error to indicate an invalid `stop_gap` value, specifically when `stop_gap` is 0.
    InvalidStopGap,
}

impl From<esplora_client::Error> for EsploraError {
    fn from(err: esplora_client::Error) -> EsploraError {
        EsploraError::EsploraClientError(err)
    }
}

impl std::fmt::Display for EsploraError {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        match *self {
            EsploraError::EsploraClientError(ref err) => write!(f, "Esplora client error: {}", err),
            EsploraError::InvalidStopGap => write!(f, "Invalid stop_gap: stop_gap cannot be 0"),
        }
    }
}

impl std::error::Error for EsploraError {}

Not beautiful but works 😂

[evanlinjin]

Can we not create an error variant for this? It's more baggage which callers will have to deal with.

[storopoli]

Can we not create an error variant for this? It's more baggage which callers will have to deal with.

We cannot extend enums variants from other crates in Rust.
That's why I propose the wrapper above.

[storopoli]

Discussed in 2024-02-27 BDK Lib Meeting:

• Not introduce an Error Variant
• Not panic
• Threat stop_gap = 0 as 1 and document this thoroughly.

[danielabrozzoni]

There are still some issues with your code, that were pointed out by other contributors but weren't resolved.

I don't want to be harsh, and I know it's not easy, but please try to put attention to details. Making sure to incorporate all the feedback that is given is important for speeding up review time.

[danielabrozzoni]

Hey, this was marked as resolved, but the code didn't really change. Unresolving.

[storopoli]

@danielabrozzoni sorry about the oversight and thank you for pointing that out!
I have corrected the logic and math on stop_gap in a new commit

[danielabrozzoni]

Sweet, can you remove the FIXME and NOTE in the tests and squash the two commits together? Then it's ready to merge for me

[danielabrozzoni]

ACK 0c5220d

[ValuedMammal]

Just to note: the fix for electrum may be as simple as changing a > to >= but right now you kind of have to test manually to convince yourself that's the case

[storopoli]

Rebased to master.
Cc @danielabrozzoni and @notmandatory.

[notmandatory]

ACK 7c1861a