Use a universal lookahead value for KeychainTxOutIndex and have a reasonable default

[darosior]

Description

The bdk::Wallet is currently created without setting any lookahead value for the keychain. This implicitly makes it a lookahead of 0. As this is a high-level interface we should avoid footguns and aim for a reasonable default.

To fix this, we have also decided to change KeychainTxOutIndex to have a default lookahead. Additionally, we have simplified the KeychainTxOutIndex API to have a single lookahead that is ONLY set at construction KeychainTxOutIndex::new(lookahead: u32) -> Self. This avoids the footguns of having methods which allows the caller to decrease the lookahead (which will panic).

Notes to the reviewers

A way to set this value externally is introduced in #1172. This PR only aims to use a saner default than 0. 1_000 is the value used by the Bitcoin Core wallet, and that seems reasonable to me.

Edit: we should NOT allow setting the lookahead value after-the-fact. Instead, the lookahead should be provided to the wallet's constructor.

@evanlinjin: I don't think additional tests are necessary as no additional features are added, and the surface area of the API is decreased. The original tests already thoroughly test the lookahead concept.

Checklists

All Submissions:

(This section was updated as this PR changed from being a simple setting to introducing a new method.)

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
  * [ ] I've added tests
• I've added docs

[darosior]

Not sure how i would implement a test which:

1. Creates a fresh wallet
2. Sends a transaction on the second address from the keychain
3. Asserts the wallet notices it (this would fail on master)

[LLFourn]

See the tests in /tests for where the wallet tests are. There's a bunch of them that test little things like this.

Historical note that I actually pushed against having a lookahead for a little while. I felt like it encouraged a design where people had multiple devices giving out addresses for the same keychain which I feel is bad practice. The wallet giving out addresses for the xpub knows the last one that has been produced so it's technically not possible for it exist on the blockchain and application developers should be using this invariant.

My complaints were put aside when @evanlinjin pointed out the impossibility of doing block-by-block updating without a lookahead. The default was probably set to 0 to placate me because I still wanted the developer to consciously engage this feature for that scenario. But I think this is fine for Wallet -- I'd leave 0 as the default for KeychainTxOutIndex changed my mind just have a nice default in both places.

[benthecarman]

Does this mean it is watching the next 1000 addresses? That seems pretty expensive for syncing

[LLFourn]

Does this mean it is watching the next 1000 addresses? That seems pretty expensive for syncing

No what you actually go out and fetch from the chain is under your control and this lookahead doesn't effect it. This is about what the txout index will find when you feed it transaction. It's only relevant when you are doing block style syncing -- it shouldn't effect at all electrum/esplora.

[notmandatory]

Please rebase now that #1183 has been merged.

[evanlinjin]

@darosior If it is alright with you, could I take over this PR? I think this is important for syncing from block-by-block chain sources.

[evanlinjin]

After discussion with @LLFourn, we have come to agreement to use a universal lookahead value which is specified when creating the KeychainTxOutIndex.

pub fn new(lookahead: u32) -> Self { todo!() }

I will move this PR forward in this manner.

[notmandatory]

I fixed the CI issues with #1247, it just needs to be ACKd and merged and then this PR rebased.

[notmandatory]

The main changes looks good, but when I tried to do some manual regtest testing with the example_bitcoind_rpc_polling example code I ran into an issue where the genesis block isn't set. I was able to get past that by adding the below to the example main.rs, but pretty sure it's the right way to fix it.

if init_changeset.0.is_empty() {
        let genesis_hash = genesis_block(Regtest).block_hash();
        dbg!(genesis_hash);
        let  (_chain, changeset) = LocalChain::from_genesis_hash(genesis_hash);
        init_changeset.0 = changeset;
        db.lock().unwrap().stage(init_changeset.clone());
        db.lock().unwrap().commit()?;
    }

[notmandatory]

tACK bc796f4

I manually tested using example_bitcoind_rpc_polling cli with the steps in gist: https://gist.github.com/notmandatory/6fa85dd942089833629bdb40d2b54858

[LLFourn]

I pushed a commit to improve the lookahead doc slightly.

self-ACK: c9467dc

[LLFourn]

ACK 1def76f