Fix wallet sync not finding coins of addresses which are not cached

[evanlinjin]

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

   bdk/src/blockchain/mod.rs

   Lines 157 to 161 in f0c876e

   	fn wallet_sync<D: BatchDatabase>(
   	&self,
   	database: &mut D,
   	progress_update: Box<dyn Progress>,
   	) -> Result<usize, Error> {

4. Please have a read of Fix wallet sync not finding coins of addresses which are not cached #672 (comment) for additional context.

Checklists

All Submissions:

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

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]

After talking with @LLFourn in a private conversation, it seems that the solution provided in this PR does not fully solve #521.

The problem that still exists

The problem that still exists is as follows. Given:

• A wallet established with a keychain, which contains a transaction in which at least one input spends from an owned scriptPubKey, and at least one output sends to a different owned scriptPubKey.
• The Database only contains the cache of one of these scriptPubKeys.

If the wallet performs a sync, and attempts to compile data (such as owned outputs, UTXOs and balances) from the transaction, it will miss including data from an owned scriptPubKey.

The fix?

Currently, we define stop_gap as the largest expected (child index) gap between two "used" scriptPubKeys (where "used" means, contained within atleast one transaction output).

I propose adding an additional constraint to the definition of stop_gap. Given a transaction, any two scriptPubKeys referenced by that transaction (either being spent, or received to) should not have a difference in index greater than stop_gap.

If we can enforce the new definition of stop_gap in code, users of BDK can have a more predictable sync/update experience, hence fixing #521.

How does everything work currently?

Analyzing the syncing logic (of electrum-based Blockchain implementations), we see that syncing is defined in 4 steps (as per defined in blockchain::Request.

bdk/src/blockchain/script_sync.rs

Lines 15 to 25 in 9165fae

	/// A request for on-chain information
	pub enum Request<'a, D: BatchDatabase> {
	/// A request for transactions related to script pubkeys.
	Script(ScriptReq<'a, D>),
	/// A request for confirmation times for some transactions.
	Conftime(ConftimeReq<'a, D>),
	/// A request for full transaction details of some transactions.
	Tx(TxReq<'a, D>),
	/// Requests are finished here's a batch database update to reflect data gathered.
	Finish(D::Batch),
	}

The first two steps are as follows:

1. Request::Script: First we send the electrum-based server a list of scriptPubKeys, obtaining a bunch of txids (and their confirmation times).
2. Request::TxReq: We obtain the full raw transactions of txids (if not already exist in Database) and compile metadata of the transaction (such as sent/received amounts).

Then the final step is where a batch update is prepared for the Database, in which one of these steps is to "set every UTXO we observed" (in the above steps).

As you can see, compiling sent/received balances from transactions, as well as finding owned UTXOs of new transactions require checking against the cache of owned scriptPubKeys. If we "under-cache" before these steps, we will "miss out" on finding crucial data.

We cannot fully fix this problem (unless we check with every possible scriptPubKey - but that is ridiculously expensive in computation), so we use our new definition of stop_gap to ensure sync behaves as expected for the user.

How do we enforce the new stop_gap definition?

So we need to ensure the following are fulfilled before compiling metadata (such as new outputs, utxos and balances):

1. We have obtained all transactions of owned scriptPubKeys (where scriptPubKeys are searched for in chronological order starting from child index 0), in which the last stop_gap+1 scriptPubKeys are not associated with any transactions.
2. We have cached a stop_gap number of scriptPubKeys above the last scriptPubKey index we queried for during Request::Script.

What changes do we need to make?

This should now all be implemented in this commit: 12afbe3

In conclusion

Wallet sync is a difficult problem since we are either sacrificing performance for reliability, or sacrificing reliability for performance. The best thing we can do, is to make the syncing logic as predictable as we can for the developer/user.

Additionally, let's design a good API for sync logic.

[evanlinjin]

I just had a thought. I feel like to improve the efficiently, the WalletSync implementation can internally self-cache txids before returning Error::MissingCachedScripts. This way, subsequent calls can use the cache to avoid re-polling the electrum-based server (for the scriptPubKeys that are already requested for).

[afilini]

Concept ACK, a few comments.

[afilini]

I think you can avoid duplicating this piece of code by moving this inside the loop, and then breaking if the descriptor is not derivable.

Something like:

for _ in 0..MAX_ROUNDS {
    let sync_res = // .... ;

    if !is_derivable {
        break;
    }

    // do everything else to get the database ready for the next iteration
}

[afilini]

I'm not sure what to do with the progress, I feel like we should have our own "internal" progress (that can be cloned and reused) that wraps the real progress and maps the first iteration to a maximum of like 70%, then if there are more iterations each has a lower "weight", like the second iteration going to 0 to 100 only causes the overall progress to go from 70 to 80 maybe and then at the end we just set it to 100.

It's not great but I can't think of any other way to predict how many iterations there's going to be. We should adjust those weights based on probability, most of the times it's just one (so that gets a larger weight).

If you have better ideas let me know.

[evanlinjin]

I did something else to avoid duplicating code, let me if you are happy with my solution :)

Regarding the SyncProgress object... Because right now electrum-based blockchains don't actually report progress at all, and only electrum-based blockchains will return Error::MissingCachedScripts (for now), I feel like we shouldn't bother changing behavior for it in this PR?

Maybe we should do another ticket to fix progress-reporting behavior. I realized that not all sync attempts will report progress (for example, rpc only reports sync progress on it's initial sync).

[evanlinjin]

@afilini @LLFourn would it make sense to try to fix the same bugs in the other Blockchain implementations in this PR?

[afilini]

I think it would be nice to have everything in a single PR, if you can fix those as well we can merge everything together.

[evanlinjin]

I think it would be nice to have everything in a single PR, if you can fix those as well we can merge everything together.

Fair, I've created a separate ticket (#677) since it seems the "nature" of the bugs are a little different, but I'll try tackle them in this PR as well.

[evanlinjin]

@afilini I started working on fixing sync for RPC... But because sync works very different, so the nature of the bug is also really different. I reckon it should be tackled in a separate PR because it feels like a completely different set of problems.

I've created a separate ticket here (#677), would it be okay to work on this in a different PR? What do you think?

[afilini]

Yes, that makes sense, I'll go ahead and merge this one then

[afilini]

ACK 5c940c3