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.

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.

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.

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.

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.

We can discuss Error/Panic on Tue's call

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 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 😂

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

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.

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 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

Rebased to master.
Cc @danielabrozzoni and @notmandatory.