wallet: improve IBD sync time by skipping block scanning prior birth time #27469

furszy · 2023-04-15T19:32:14Z

During initial block download, the node's wallet(s) scans every arriving block looking for data that it owns.
This process can be resource-intensive, as it involves sequentially scanning all transactions within each
arriving block.

To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time,
since these blocks are guaranteed not to contain any relevant wallet data.

This has direct implications (an speed improvement) on the underlying blockchain synchronization process
as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no
more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain
(activating the best chain to be more precise).
Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are
processed by the wallet(s).

So, by skipping not relevant blocks in the wallet's IBD scanning process, we will also improve the chain
synchronization time.

DrahtBot · 2023-04-15T19:32:17Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, ishaanam, achow101
Concept ACK	pablomartin4btc, MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26836 (wallet: finish addressbook encapsulation and simplify addressbook migration by furszy)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pinheadmz · 2023-04-16T03:05:07Z

concept ACK, seems like a great idea.

maflcko · 2023-04-17T14:45:20Z

src/interfaces/chain.h

@@ -83,6 +83,7 @@ struct BlockInfo {
    const uint256& hash;
    const uint256* prev_hash = nullptr;
    int height = -1;
+    int64_t time = 0;


Any reason to use int64_t, when the block time is denoted in a type-safe NodeSeconds?

Edit: I guess all you need to do is switch from ...::GetBlockTime() to ...::Time()?

Mainly to be consistent with the chain interface, wallet and spkm sources. Timestamps are represented as int64_t there. E.g. the FoundBlock class, findFirstBlockWithTimeAndHeight method, the CWallet::m_best_block_time member, first key time, descriptor creation time etc.

I think that is clear that block times are always in seconds, so the use of an arithmetic type shouldn't be much of a worry but we could switch all of them to NodeSeconds too (probably in another PR that does it all at once?).

e07dd5f test: fix bumpfee 'spend_one_input' occasional failure (furszy) Pull request description: CI test failure, in master: https://cirrus-ci.com/task/5975232842825728. In #27469 https://cirrus-ci.com/task/6452468402356224 Most of the subtests in `wallet_bumpfee.py` expect to find spendable UTXOs of 0.001 btc in the rbf wallet. They use the `spend_one_input()` method which fails if none of them exist. The sporadic failure comes from the recently added `test_feerate_checks_replaced_outputs` subtest that can spend all them. Leaving the next subtests with no 0.001 UTXOs to spend. To solve it, this PR moves the recently added case into a "context independent subtests" section. Which is placed at the end to not affect other cases. ACKs for top commit: achow101: ACK e07dd5f pablomartin4btc: ACK e07dd5f. Tree-SHA512: c150ed6fcfbb6f1502eaf6370a57aae0da991c0fc48e8bb3d446805f4336abba5d80ff0de26094914da95432dd0255030fe527001af4510dfdcefbc7230f14d6

…ailure e07dd5f test: fix bumpfee 'spend_one_input' occasional failure (furszy) Pull request description: CI test failure, in master: https://cirrus-ci.com/task/5975232842825728. In bitcoin#27469 https://cirrus-ci.com/task/6452468402356224 Most of the subtests in `wallet_bumpfee.py` expect to find spendable UTXOs of 0.001 btc in the rbf wallet. They use the `spend_one_input()` method which fails if none of them exist. The sporadic failure comes from the recently added `test_feerate_checks_replaced_outputs` subtest that can spend all them. Leaving the next subtests with no 0.001 UTXOs to spend. To solve it, this PR moves the recently added case into a "context independent subtests" section. Which is placed at the end to not affect other cases. ACKs for top commit: achow101: ACK e07dd5f pablomartin4btc: ACK bitcoin@e07dd5f. Tree-SHA512: c150ed6fcfbb6f1502eaf6370a57aae0da991c0fc48e8bb3d446805f4336abba5d80ff0de26094914da95432dd0255030fe527001af4510dfdcefbc7230f14d6

pinheadmz

Did some general code review. I see we already have nCreateTime for descriptors and part of this PR seems to be just bringing that up to the wallet level, so that's not a redundancy.

Also I don't know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet should miss the first transaction.

src/wallet/wallet.cpp

furszy · 2023-04-18T14:37:23Z

Thanks for the review pinheadmz.

Also I don't know if this is a problematic edge case or it could just be a good way to test but you could create a key and send BTC to it in a block, then a few blocks later import into a new descriptor and rescan. The wallet should miss the first transaction.

This PR is not modifying the rescan process. Only IBD.
Guess that you provided a timestamp that is +2 hours ahead of the block time (network time variability window) or the "now" arg to skip rescan?

pablomartin4btc

Concept ACK. Light code review, I'd to spend more time with it, which I'll do soon. Good stuff!

src/interfaces/chain.h

src/wallet/wallet.h

furszy · 2023-04-20T22:11:47Z

Updated per feedback, thanks achow.

Changes:

Replaced BLANK_BIRTH_TIME constant by setting the m_birth_time to its max numeric value.
Tackled a block time variability concern by using the maximum time in the chain (a moving forward only timestamp) instead of the raw block time.
Extended the grace period to be twice as big as the one used in the rescan process. Even when this is not strictly needed, the rationale is to be a bit more conservative during IBD scanning because it's an automatic process with no user interaction (unlike the rescan process that is usually triggered by an user action such an import).

ishaanam

Approach ACK, this looks good so far but I haven't reviewed the first commit yet.

src/wallet/wallet.h

src/wallet/scriptpubkeyman.cpp

maflcko

Concept ACK. Haven't reviewed the code.

pinheadmz · 2023-05-25T15:17:03Z

This PR is not modifying the rescan process. Only IBD.

sanity check: because we already skip old blocks during rescan?

#26679

furszy · 2023-05-25T19:09:13Z

sanity check: because we already skip old blocks during rescan?

#26679

Yes. Different entry points.

#26679 addressed the scanning of the existing chain during load time.
And here, the emphasis shifts to the synchronization process when the
wallet is active.
When the node activates the best chain on each processed block and
signal the "block connected" event.

pinheadmz · 2023-05-25T23:14:00Z

To review this PR I wrote a simple functional test: pinheadmz@1f0f945 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a combo(...) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn't actually skip any blocks. But if that's the case, does that also mean #26679 doesn't work either?

test output:

2023-05-25T23:10:29.244000Z TestFramework (INFO): PRNG seed is: 8288012825723617302
2023-05-25T23:10:29.245000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__5pngcq0
2023-05-25T23:10:29.886000Z TestFramework (INFO): Generate 10 blocks with 10-hour timestamp gaps
2023-05-25T23:10:29.927000Z TestFramework (INFO): Initial blockchain sync the wallet's node

not(500.00000000 == 250)

Why does this descriptor have a timestamp of 1?
{'desc': 'combo(022d715e6f0d7c037dbccd3327f0b3bc3c76258a7e017e1a64902f76406c87dfda)#pesd53nq', 'timestamp': 1, 'active': False}

2023-05-25T23:10:31.083000Z TestFramework (INFO): Stopping nodes
2023-05-25T23:10:31.306000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test__5pngcq0
2023-05-25T23:10:31.306000Z TestFramework (INFO): Tests successful

and then in bitcoin debug.log, this output from CWallet::blockConnected(): m_birth_time.load(): 1. Every time we run that function, the log says wallet birthday is 1

furszy · 2023-05-26T00:27:27Z

To review this PR I wrote a simple functional test: pinheadmz@1f0f945 which surprised me by failing. I added logging and comments to the test to share it because I don't entirely understand what's happening but it looks to me like, the wallet generates a combo(...) descriptor with a timestamp of 1 and therefore the wallet creation time is always 1 and so the new logic doesn't actually skip any blocks. But if that's the case, does that also mean #26679 doesn't work either?

The issue is on your test setup. The combo descriptor comes from the test framework default wallet creation process, not the inner wallet.

The framework, after creating the default wallet, imports one of the hardcoded private keys (link) used to receive the coinbases coins (link).

This means calling the test framework importprivkey, which is a proxy function with two possible outcomes:

For legacy wallet, call the RPC command importprivkey: which doesn't receive a timestamp, so creation time is initialized to the minimum one.
For descriptors wallet, call the RPC command importdescriptors with timestamp=0. (which then is changed by the node internally to 1, because of the minimum supported timestamp)

The simplest solution for the test is to create the wallet by yourself, by calling createwallet, instead of using the default one initialized by the framework.
And, remember to set the mock time before creating the wallet, so it uses the time established by you and not the clock time.

Thanks for testing :).

pinheadmz · 2023-05-26T14:40:10Z

@furszy awwwwwesome thank you for catching that. Ok, passing test is now at pinheadmz@f123870

pinheadmz

ACK 82bb783

Reviewed code and tested locally, wrote a test. I am running this branch side-by-side with master on a VPS and will report who wins the race in a day or so ;-)

Show Signature

pinheadmz's public key is on keybase

src/wallet/wallet.cpp

ishaanam · 2023-05-26T22:09:13Z

re-ACK 82bb783

achow101 · 2023-05-27T01:18:22Z

re-ACK 82bb783

…scanning prior birth time 82bb783 wallet: skip block scan if block was created before wallet birthday (furszy) a082434 refactor: single method to append new spkm to the wallet (furszy) Pull request description: During initial block download, the node's wallet(s) scans every arriving block looking for data that it owns. This process can be resource-intensive, as it involves sequentially scanning all transactions within each arriving block. To avoid wasting processing power, we can skip blocks that occurred before the wallet's creation time, since these blocks are guaranteed not to contain any relevant wallet data. This has direct implications (an speed improvement) on the underlying blockchain synchronization process as well. The reason is that the validation interface queue is limited to 10 tasks per time. This means that no more than 10 blocks can be waiting for the wallet(s) to be processed while we are synchronizing the chain (activating the best chain to be more precise). Which can be a bottleneck if blocks arrive and are processed faster from the network than what they are processed by the wallet(s). So, by skipping not relevant blocks in the wallet's IBD scanning process, we will also improve the chain synchronization time. ACKs for top commit: ishaanam: re-ACK 82bb783 achow101: re-ACK 82bb783 pinheadmz: ACK 82bb783 Tree-SHA512: 70158c9657f1fcc396badad2c4410b7b7f439466142640b31a9b1a8cea4555e45ea254e48043c9b27f783d5e4d24d91855f0d79d42f0484b8aa83cdbf3d6c50b

maflcko · 2023-05-30T17:34:09Z

Looks like the fuzz test needs an update?

82bb7831fa6052620998c7eef47e48ed594248a8 is the first bad commit

echo 'AQAAAAEAAAA='|base64  --decode  > /tmp/a
FUZZ=wallet_notifications ./src/test/fuzz/fuzz  /tmp/a

fuzz: wallet/test/fuzz/notifications.cpp:175: void wallet::(anonymous namespace)::wallet_notifications_fuzz_target(FuzzBufferType): Assertion `total_amount == bal_a + bal_b' failed.
==301525== ERROR: libFuzzer: deadly signal

furszy · 2023-05-30T20:53:02Z

Thanks Marco. Fixed in #27786.
Would be nice if the CI alert us about this kind of stuff prior merging the PR.
The base fuzzing corpus should have detected it right?.

maflcko · 2023-05-31T05:58:26Z

The base fuzzing corpus should have detected it right?.

Yes, but the fuzz target is too slow right now, so the fuzz input directory is empty.

a10f032 fuzz: fix wallet notifications.cpp (furszy) Pull request description: Fixing bitcoin/bitcoin#27469 (comment). As the fuzzing test requires all blocks to be scanned by the wallet (because it is asserting the wallet balance at the end), we need to ensure that no blocks are skipped by the recently added wallet birth time functionality. This just means setting the chain accumulated time to the maximum value, so the wallet birth time is always below it, and the block is always processed by the wallet. ACKs for top commit: MarcoFalke: lgtm ACK a10f032, thanks Tree-SHA512: c9b38c52917cc36674415470752625b8161fc6b878b0b87d6926b462ba9666be3c225d396604c7e944a4c268fc35fc624807777aa0ed94bddbe18d8f8436de3c

a10f032 fuzz: fix wallet notifications.cpp (furszy) Pull request description: Fixing bitcoin#27469 (comment). As the fuzzing test requires all blocks to be scanned by the wallet (because it is asserting the wallet balance at the end), we need to ensure that no blocks are skipped by the recently added wallet birth time functionality. This just means setting the chain accumulated time to the maximum value, so the wallet birth time is always below it, and the block is always processed by the wallet. ACKs for top commit: MarcoFalke: lgtm ACK a10f032, thanks Tree-SHA512: c9b38c52917cc36674415470752625b8161fc6b878b0b87d6926b462ba9666be3c225d396604c7e944a4c268fc35fc624807777aa0ed94bddbe18d8f8436de3c

…gging 8a553c9 wallet: Add TxStateString function for debugging and logging (Ryan Ofsky) Pull request description: I found this useful while debugging silent conflict between #10102 and #27469 recently ACKs for top commit: ishaanam: utACK 8a553c9 achow101: ACK 8a553c9 furszy: Code ACK 8a553c9 Tree-SHA512: 87965c66bcb59a21e7639878bb567e583a0e624735721ff7ad1104eed6bb9fba60607d0e3de7be3304232b3a55f48bab7039ea9c26b0e81963e59f9acd94f666

… and logging 8a553c9 wallet: Add TxStateString function for debugging and logging (Ryan Ofsky) Pull request description: I found this useful while debugging silent conflict between bitcoin#10102 and bitcoin#27469 recently ACKs for top commit: ishaanam: utACK 8a553c9 achow101: ACK 8a553c9 furszy: Code ACK 8a553c9 Tree-SHA512: 87965c66bcb59a21e7639878bb567e583a0e624735721ff7ad1104eed6bb9fba60607d0e3de7be3304232b3a55f48bab7039ea9c26b0e81963e59f9acd94f666

DrahtBot added the Wallet label Apr 15, 2023

furszy mentioned this pull request Apr 16, 2023

test: fix bumpfee 'spend_one_input' occasional failure #27471

Merged

This was referenced Apr 16, 2023

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

wallet: rpc to add automatically generated descriptors #25907

Closed

DrahtBot mentioned this pull request Apr 16, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

maflcko reviewed Apr 17, 2023

View reviewed changes

pinheadmz reviewed Apr 17, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_birthtime branch from 4698614 to 9785754 Compare April 18, 2023 14:35

furszy force-pushed the 2023_wallet_birthtime branch from 9785754 to a2d3902 Compare April 18, 2023 14:42

pablomartin4btc reviewed Apr 18, 2023

View reviewed changes

furszy force-pushed the 2023_wallet_birthtime branch 2 times, most recently from 3914f4f to 1a9a349 Compare April 19, 2023 00:29

achow101 reviewed Apr 19, 2023

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

src/wallet/wallet.h Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_birthtime branch 2 times, most recently from 7bfe7b4 to 7be71a2 Compare April 20, 2023 21:53

This was referenced Apr 21, 2023

wallet: group independent db writes on single batched db transaction #25297

Closed

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

ishaanam reviewed May 17, 2023

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

src/wallet/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_birthtime branch from 7be71a2 to 76396ca Compare May 23, 2023 21:25

furszy force-pushed the 2023_wallet_birthtime branch from 7cf50fb to 82bb783 Compare May 25, 2023 13:47

maflcko approved these changes May 25, 2023

View reviewed changes

pinheadmz approved these changes May 26, 2023

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

DrahtBot requested review from achow101 and ishaanam May 26, 2023 14:41

DrahtBot removed the request for review from ishaanam May 26, 2023 22:09

DrahtBot removed the request for review from achow101 May 27, 2023 01:18

achow101 merged commit 10c4a46 into bitcoin:master May 27, 2023

furszy deleted the 2023_wallet_birthtime branch May 27, 2023 01:42

furszy mentioned this pull request May 30, 2023

fuzz: fix wallet notifications.cpp #27786

Merged

furszy mentioned this pull request Jun 22, 2023

Allow for rescan using block filters with pruned node #21267

Open

ryanofsky mentioned this pull request Sep 27, 2023

wallet: Add TxStateString function for debugging and logging #28544

Merged

achow101 mentioned this pull request Nov 16, 2023

Wallets should update key/descriptor birthdates when txs older than current birthdates are found #28897

Closed

furszy mentioned this pull request Jan 4, 2024

Prune Node Rescan Project Tracking #29183

Open

18 tasks

bitcoin locked and limited conversation to collaborators May 30, 2024

wallet: improve IBD sync time by skipping block scanning prior birth time #27469

wallet: improve IBD sync time by skipping block scanning prior birth time #27469

Uh oh!

Conversation

furszy commented Apr 15, 2023

Uh oh!

DrahtBot commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

pinheadmz commented Apr 16, 2023

Uh oh!

maflcko Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furszy commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy commented Apr 20, 2023

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented May 25, 2023

Uh oh!

furszy commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented May 25, 2023

Uh oh!

furszy commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pinheadmz commented May 26, 2023

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ishaanam commented May 26, 2023

Uh oh!

achow101 commented May 27, 2023

Uh oh!

maflcko commented May 30, 2023

Uh oh!

furszy commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 31, 2023

Uh oh!

Uh oh!

DrahtBot commented Apr 15, 2023 •

edited

Loading

maflcko Apr 17, 2023 •

edited

Loading

furszy commented Apr 18, 2023 •

edited

Loading

furszy commented May 25, 2023 •

edited

Loading

furszy commented May 26, 2023 •

edited

Loading

furszy commented May 30, 2023 •

edited

Loading