Skip to content
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

Jump to bottom

WalletSync issues with RpcBlockchain and CompactFiltersBlockchain #677

Closed
evanlinjin opened this issue Jul 21, 2022 · 2 comments · Fixed by #683
Closed

WalletSync issues with RpcBlockchain and CompactFiltersBlockchain #677

evanlinjin opened this issue Jul 21, 2022 · 2 comments · Fixed by #683
Assignees
@evanlinjin
Labels
bug Something isn't working
Milestone
Release 0.21.0 Fe...

Comments

@evanlinjin
Copy link
Member

evanlinjin commented Jul 21, 2022 •
edited
Loading

The bug

After looking through the codebase for RpcBlockchain, it looks as if importdescriptors would only be called on the first sync, or not at all if there were already more than 100 scriptPubKeys cached before the first sync attempt. In other words, the Bitcoin Core wallet would only be aware of 100 scriptPubKeys or none at all.

The CompactFiltersBlockchain implementation may have similar issues to the electrum-based Blockchain implementations (fixed in #672), where owned outputs can be missed when compiling data from raw transactions (when scriptPubKeys are under-cached), and subsequent syncs will not rediscover these missed outputs.

Note that these deductions are not tested, but mere assumptions from scanning over the codebase.

Tests to write

I recommend writing the following tests (to confirm the existence of these bugs):

  1. After caching greater-than-100 scriptPubKeys before doing initial Wallet::sync, we should be able to detect correct balance and owned UTXO set (assuming that all txs interact with owned scriptPubKeys with derivation index less-than-100).

  2. Given a positive integer that is greater-than-0 (x), after doing the initial Wallet::sync, create a self-send transaction, sending from scriptPubKey at derivation index 99 to derivation index 99+x. Cache at-least x more scriptPubKeys and re-sync. Wallet should have the correct balance and output-set.

Additional context

Feel free to look at PR #672 (which fixes #521 and #451 for electrum-based blockchains). However, note that the sync logic differs from RPC and Compact Block Filters, so the fix will be different.

If I have understood the codebase correctly, and these bugs exist, it will be good to have a fix ASAP since these are critical problems.
@evanlinjin evanlinjin added the bug Something isn't working label Jul 21, 2022
@evanlinjin evanlinjin changed the title Potential WalletSync issues with RpcBlockchain and CompactFiltersBlockchain WalletSync issues with RpcBlockchain and CompactFiltersBlockchain Jul 21, 2022
@evanlinjin evanlinjin mentioned this issue Jul 21, 2022
Merged
6 tasks
afilini added a commit that referenced this issue Jul 21, 2022
@afilini
Merge #672: Fix wallet sync not finding coins of addresses which are …
3644a45 
…not cached

5c940c3 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 #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 5c940c3

Tree-SHA512: aee917ed4821438fc0675241432a7994603a09a77d5a72e96bad863e7cdd55a9bc6fbd931ce096fef1153905cf1b786e1d8d932dc19032d549480bcda7c75d1b
@evanlinjin

This comment was marked as resolved.

Sign in to view
@notmandatory notmandatory moved this to Todo in BDK Maintenance Jul 22, 2022
@evanlinjin evanlinjin mentioned this issue Jul 25, 2022
Merged
8 tasks
@evanlinjin
Copy link
Member Author

Looking at how importdescriptors is called...

bdk/src/blockchain/rpc.rs

Lines 201 to 204 in 3644a45

json!({
"timestamp": "now",
"desc": format!("{}#{}", desc, get_checksum(&desc).unwrap()),
})

And the documentation for importdescriptors...

image

We see that old wallets cannot recover funds using BDK which syncs with RpcBlockchain.

I was wrong. Transactions are fetched later via multiple calls with rescanblockchain.

@afilini afilini closed this as completed in dc7adb7 Aug 4, 2022
Repository owner moved this from Todo to Done in BDK Maintenance Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evanlinjin evanlinjin

Labels
bug Something isn't working
Projects
BDK Maintenance
Archived in project
Milestone
Release 0.21.0 Feature Freeze
Development

Successfully merging a pull request may close this issue.

Fix wallet sync for RpcBlockchain evanlinjin/bdk
2 participants
@notmandatory @evanlinjin