Skip to content

Commit

Permalink
Merge bitcoindevkit/bdk#672: Fix wallet sync not finding coins of add…
Browse files Browse the repository at this point in the history
…resses which are not cached

7e2ad9b Fix wallet sync not finding coins of addresses which are not cached (志宇)

Pull request description:

  Fixes #521
  Fixes #451

  ^ However, only for electrum-based `Blockchain` implementations. For RPC and Compact Block Filters, syncing works differently, and so are the bugs - I have created a separate ticket for this (#677).

  ### Description

  Previously, electrum-based blockchain implementations only synced for `scriptPubKey`s that are already cached in `Database`.

  This PR introduces a feedback mechanism, that uses `stop_gap` and the difference between "current index" and "last active index" to determine whether we need to cache more `scriptPubKeys`.

  The `WalletSync::wallet_setup` trait now may return an `Error::MissingCachedScripts` error which contains the number of extra `scriptPubKey`s to cache, in order to satisfy `stop_gap` for the next call.

  `Wallet::sync` now calls `WalletSync` in a loop, caching in-between subsequent calls (if needed).

  #### Notes to reviewers

  1. The caveat to this solution is that it is not the most efficient. Every call to `WalletSync::wallet_setup` starts polling the Electrum-based server for `scriptPubKey`s starting from index 0.

      However, I feel like this solution is the least "destructive" to the API of `Blockchain`. Also, once the `bdk_core` sync logic is integration, we can select specific ranges of `scriptPubKey`s to sync.

  2. Also note that this PR only fixes the issue for electrum-based `Blockchain` implementations (currently `blockchain::electrum` and `blockchain::esplora` only).

  3. Another thing to note is that, although `Database` assumes 1-2 keychains, the current `WalletSync` "feedback" only returns one number (which is interpreted as the larger "missing count" of the two keychains). This is done for simplicity, and because we are planning to only have one keychain per database in the future.

      https://github.com/bitcoindevkit/bdk/blob/f0c876e7bf38566c0d224cbe421ee312ffc06660/src/blockchain/mod.rs#L157-L161

  4. Please have a read of bitcoindevkit/bdk#672 (comment) for additional context.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [x] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  afilini:
    ACK 7e2ad9b

Tree-SHA512: aee917ed4821438fc0675241432a7994603a09a77d5a72e96bad863e7cdd55a9bc6fbd931ce096fef1153905cf1b786e1d8d932dc19032d549480bcda7c75d1b
  • Loading branch information
afilini committed Jul 21, 2022
2 parents 6bf7878 + 7e2ad9b commit b651bbb
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion src/blockchain/esplora/reqwest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,6 @@ impl WalletSync for EsploraBlockchain {
};

database.commit_batch(batch_update)?;

Ok(())
}
}
Expand Down

0 comments on commit b651bbb

Please sign in to comment.