wallet: remove BDB dependency from wallet migration benchmark

furszy commented at 4:42 pm on November 7, 2024: member

Part of the legacy wallet removal working path #20160.

Stops creating a bdb database in the wallet migration benchmark. Instead, the benchmark now creates the db in memory and re-uses it for the migration process.

DrahtBot commented at 4:42 pm on November 7, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31241.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, brunoerg, theStack

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:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#29694 (fuzz: wallet: add target for spkm migration by brunoerg)

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.

DrahtBot added the label Wallet on Nov 7, 2024

fanquake commented at 5:21 pm on November 7, 2024: member

https://cirrus-ci.com/task/4957802181951488?logs=ci#L1777:

 0[17:17:27.480]   Thread T4 'b-httpworker.1' (tid=27041, running) created by main thread at:
 1[17:17:27.480]     [#0](/bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xf7df5) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 2[17:17:27.480]     [#1](/bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create[abi:ne190103](unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-19/bin/../include/c++/v1/__thread/support/pthread.h:182:10 (bitcoind+0x6e06fb) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 3[17:17:27.480]     [#2](/bitcoin-bitcoin/2/) std::__1::thread::thread<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, 0>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__thread/thread.h:211:14 (bitcoind+0x6e06fb)
 4[17:17:27.480]     [#3](/bitcoin-bitcoin/3/) std::__1::thread* std::__1::construct_at[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, std::__1::thread*>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/construct_at.h:41:46 (bitcoind+0x6e033a) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 5[17:17:27.480]     [#4](/bitcoin-bitcoin/4/) std::__1::thread* std::__1::__construct_at[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, std::__1::thread*>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/construct_at.h:49:10 (bitcoind+0x6e033a)
 6[17:17:27.480]     [#5](/bitcoin-bitcoin/5/) void std::__1::allocator_traits<std::__1::allocator<std::__1::thread>>::construct[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void, 0>(std::__1::allocator<std::__1::thread>&, std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/allocator_traits.h:328:5 (bitcoind+0x6e033a)
 7[17:17:27.480]     [#6](/bitcoin-bitcoin/6/) std::__1::thread* std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::__emplace_back_slow_path<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/vector:1538:3 (bitcoind+0x6e033a)
 8[17:17:27.480]     [#7](/bitcoin-bitcoin/7/) std::__1::thread& std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::emplace_back<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/vector:1558:13 (bitcoind+0x6d6818) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 9[17:17:27.480]     [#8](/bitcoin-bitcoin/8/) StartHTTPServer() /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./httpserver.cpp:504:31 (bitcoind+0x6d6818)
10[17:17:27.480]     [#9](/bitcoin-bitcoin/9/) AppInitServers(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:707:5 (bitcoind+0x1a5d89) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
11[17:17:27.480]     [#10](/bitcoin-bitcoin/10/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:1368:14 (bitcoind+0x19c2da) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
12[17:17:27.480]     [#11](/bitcoin-bitcoin/11/) AppInit(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:231:43 (bitcoind+0x17e72f) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
13[17:17:27.480]     [#12](/bitcoin-bitcoin/12/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:277:10 (bitcoind+0x17e72f)
14[17:17:27.480] 
15[17:17:27.480] SUMMARY: ThreadSanitizer: heap-use-after-free /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./util/fs.h:77:57 in fs::u8path(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
16[17:17:27.480] ==================

DrahtBot added the label CI failed on Nov 7, 2024

furszy force-pushed on Nov 7, 2024

furszy commented at 6:22 pm on November 7, 2024: member

https://cirrus-ci.com/task/4957802181951488?logs=ci#L1777:

 0[17:17:27.480]   Thread T4 'b-httpworker.1' (tid=27041, running) created by main thread at:
 1[17:17:27.480]     [#0](/bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xf7df5) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 2[17:17:27.480]     [#1](/bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create[abi:ne190103](unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-19/bin/../include/c++/v1/__thread/support/pthread.h:182:10 (bitcoind+0x6e06fb) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 3[17:17:27.480]     [#2](/bitcoin-bitcoin/2/) std::__1::thread::thread<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, 0>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__thread/thread.h:211:14 (bitcoind+0x6e06fb)
 4[17:17:27.480]     [#3](/bitcoin-bitcoin/3/) std::__1::thread* std::__1::construct_at[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, std::__1::thread*>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/construct_at.h:41:46 (bitcoind+0x6e033a) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 5[17:17:27.480]     [#4](/bitcoin-bitcoin/4/) std::__1::thread* std::__1::__construct_at[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, std::__1::thread*>(std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/construct_at.h:49:10 (bitcoind+0x6e033a)
 6[17:17:27.480]     [#5](/bitcoin-bitcoin/5/) void std::__1::allocator_traits<std::__1::allocator<std::__1::thread>>::construct[abi:ne190103]<std::__1::thread, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&, void, 0>(std::__1::allocator<std::__1::thread>&, std::__1::thread*, void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/__memory/allocator_traits.h:328:5 (bitcoind+0x6e033a)
 7[17:17:27.480]     [#6](/bitcoin-bitcoin/6/) std::__1::thread* std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::__emplace_back_slow_path<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/vector:1538:3 (bitcoind+0x6e033a)
 8[17:17:27.480]     [#7](/bitcoin-bitcoin/7/) std::__1::thread& std::__1::vector<std::__1::thread, std::__1::allocator<std::__1::thread>>::emplace_back<void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int&>(void (&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&) /usr/lib/llvm-19/bin/../include/c++/v1/vector:1558:13 (bitcoind+0x6d6818) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
 9[17:17:27.480]     [#8](/bitcoin-bitcoin/8/) StartHTTPServer() /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./httpserver.cpp:504:31 (bitcoind+0x6d6818)
10[17:17:27.480]     [#9](/bitcoin-bitcoin/9/) AppInitServers(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:707:5 (bitcoind+0x1a5d89) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
11[17:17:27.480]     [#10](/bitcoin-bitcoin/10/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:1368:14 (bitcoind+0x19c2da) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
12[17:17:27.480]     [#11](/bitcoin-bitcoin/11/) AppInit(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:231:43 (bitcoind+0x17e72f) (BuildId: 759dffa438a525872edb969c48b6c5c4c6341482)
13[17:17:27.480]     [#12](/bitcoin-bitcoin/12/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:277:10 (bitcoind+0x17e72f)
14[17:17:27.480] 
15[17:17:27.480] SUMMARY: ThreadSanitizer: heap-use-after-free /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./util/fs.h:77:57 in fs::u8path(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
16[17:17:27.480] ==================

fixed

DrahtBot removed the label CI failed on Nov 7, 2024

achow101 commented at 2:26 am on November 13, 2024: member

While this does remove the BDB requirement, it doesn’t remove the reliance on legacy wallet specific code that will be removed. Would it be possible to change this to create a wallet with a LegacyDataSPKM without utilizing any of the legacy wallet functions?

in src/bench/wallet_migration.cpp:36 in f7dbb300d7 outdated

38-    DatabaseStatus status;
39-    bilingual_str error;
40-    auto database = MakeWalletDatabase(fs::PathToString(test_setup->m_path_root / "legacy"), options, status, error);
41     uint64_t create_flags = 0;
42-    auto wallet = TestLoadWallet(std::move(database), context, create_flags);
43+    auto wallet = TestLoadWallet(CreateMockableWalletDatabase(), context, create_flags);

brunoerg commented at 12:13 pm on November 19, 2024:

In f7dbb300d7a4d18c9d85f80e5fdd7e5bcd21c6f0: Do we really need TestLoadWallet here? Couldn’t we simply create the wallet directly (e.g.

std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(test_setup->m_node.chain.get()), "", CreateMockableWalletDatabase())};

)?

furszy commented at 3:37 pm on November 23, 2024:

In f7dbb30: Do we really need TestLoadWallet here? Couldn’t we simply create the wallet directly (e.g. std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(test_setup->m_node.chain.get()), "", CreateMockableWalletDatabase())};)?

Not entirely, needed to also set the best locator manually. But done too. Thanks.

furszy force-pushed on Nov 23, 2024

furszy commented at 4:31 pm on November 23, 2024: member

While this does remove the BDB requirement, it doesn’t remove the reliance on legacy wallet specific code that will be removed. Would it be possible to change this to create a wallet with a LegacyDataSPKM without utilizing any of the legacy wallet functions?

Yes. Pushed the watch-only part. The spendable ones part is not that trivial, will finish it in the coming days.

furszy force-pushed on Nov 23, 2024

DrahtBot added the label CI failed on Nov 23, 2024

DrahtBot commented at 10:53 pm on November 23, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33429169153

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

furszy commented at 10:57 pm on November 23, 2024: member

Done, updated. The benchmark is no longer depending on wallet functions to fill-up the legacy wallet. Tested that it works fine with #28710 (only need to tackle #28710 (review)).

DrahtBot removed the label CI failed on Nov 23, 2024

in src/bench/wallet_migration.cpp:60 in e272736009 outdated

68+        Assert(legacy_spkm->LoadKey(key, pubkey));
69+        // Load spendable scripts and create address book records
70+        bool is_bech32 = j % 2;
71+        CTxDestination dest = is_bech32 ? WitnessV0KeyHash(pubkey) : CTxDestination{PKHash(pubkey)};
72+        std::string label = is_bech32 ? strprintf("bech32_%d", j) : strprintf("legacy_%d", j);
73+        Assert(legacy_spkm->LoadCScript(GetScriptForDestination(dest)));

achow101 commented at 8:26 pm on December 5, 2024:

This LoadCScript is unnecessary since the spks for public keys is either added via ImplicitlyLearnRelatedKeyScripts or is not in the wallet explicitly.

furszy commented at 4:22 pm on December 6, 2024:

Done, updated. Thanks.

in src/bench/wallet_migration.cpp:64 in e272736009 outdated

72+        std::string label = is_bech32 ? strprintf("bech32_%d", j) : strprintf("legacy_%d", j);
73+        Assert(legacy_spkm->LoadCScript(GetScriptForDestination(dest)));
74+        Assert(wallet->SetAddressBook(dest, label, /*purpose=*/std::nullopt));
75+        // Extra: replicate legacy wallet 'LearnRelatedScripts' behavior during p2pkh dest generation
76+        if (!is_bech32) {
77+            Assert(legacy_spkm->LoadCScript(GetScriptForDestination(WitnessV0KeyHash(pubkey.GetID()))));

achow101 commented at 8:27 pm on December 5, 2024:

This LoadCScript is unnecessary as LoadKey calls FillableSigningProvider::AddKeyPubKey which calls ImplicitlyLearnRelatedKeyScripts which adds the the script to mapScripts.

furszy commented at 4:22 pm on December 6, 2024:

Done, updated. Thanks.

wallet: remove BDB dependency from wallet migration benchmark

Stops creating a bdb database in the wallet migration benchmark.
Instead, the benchmark now creates the db in memory and re-uses
it for the migration process.

18619b4732

furszy force-pushed on Dec 6, 2024

achow101 commented at 4:47 pm on December 6, 2024: member

ACK 18619b473255786b80f27dd33b46eb26a63fffc7

brunoerg approved

brunoerg commented at 8:44 pm on December 6, 2024: contributor

code review ACK 18619b473255786b80f27dd33b46eb26a63fffc7

achow101 referenced this in commit 17db84dbb8 on Dec 30, 2024

in src/wallet/wallet.cpp:4437 in 18619b4732

4446+    const std::string wallet_name = local_wallet->GetName();
4447+
4448     // Helper to reload as normal for some of our exit scenarios
4449     const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) {
4450-        // Reset options.require_format as wallets of any format may be reloaded.
4451-        options.require_format = std::nullopt;

theStack commented at 11:32 am on January 22, 2025:

Why is this reset not needed anymore?

furszy commented at 4:05 pm on January 22, 2025:

Why is this reset not needed anymore?

Because the new function uses a fresh DatabaseOptions, which sets this field to nullopt by default, it no longer reuses the one from the CWallet::Create call (which is now in another function).

theStack commented at 4:44 pm on January 22, 2025:

I see, thanks. Given that the reset happened within the lambda, I assumed that the options object could change between different reload_wallet calls, but that’s obviously not the case. (Historical review context for other reviewers: #26596 (review))

theStack commented at 11:32 am on January 22, 2025: contributor

Concept ACK

in src/wallet/wallet.cpp:4443 in 18619b4732

4438+    MigrationResult res;
4439+    bilingual_str error;
4440+    std::vector<bilingual_str> warnings;
4441+
4442+    DatabaseOptions options;
4443+    options.require_existing = true;

theStack commented at 4:44 pm on January 22, 2025:

nit, if you have to retouch:

0    const DatabaseOptions options {.require_existing = true};

theStack approved

theStack commented at 4:45 pm on January 22, 2025: contributor

Code-review ACK 18619b473255786b80f27dd33b46eb26a63fffc7

achow101 merged this on Jan 24, 2025

achow101 closed this on Jan 24, 2025

in src/bench/wallet_migration.cpp:37 in 18619b4732

40-    bilingual_str error;
41-    auto database = MakeWalletDatabase(fs::PathToString(test_setup->m_path_root / "legacy"), options, status, error);
42-    uint64_t create_flags = 0;
43-    auto wallet = TestLoadWallet(std::move(database), context, create_flags);
44+    std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
45+    wallet->chainStateFlushed(ChainstateRole::NORMAL, CBlockLocator{});

davidgumberg commented at 11:59 pm on January 24, 2025:

just an observation that may be helpful to others this is here to satisfy the need for a record locator in ApplyMigrationData:

https://github.com/bitcoin/bitcoin/blob/2d07384243c9552aa8e95c80d7a279e2d224a753/src/wallet/wallet.cpp#L4138-L4142

manually triggering the chainStateFlushed() handler is equivalent to:

0    WalletBatch(wallet->GetDatabase()).WriteBestBlock(CBlockLocator{});

or triggering the blockConnected signal

This used to be handled by the first “phase” of the now split up MigrateLegacyToDescriptor()

furszy deleted the branch on Jan 25, 2025

davidgumberg commented at 1:45 am on January 25, 2025: contributor

post-merge ACK https://github.com/bitcoin/bitcoin/pull/31241/commits/18619b473255786b80f27dd33b46eb26a63fffc7

Doesn’t change what the benchmark is measuring while dropping the BDB dependency, I don’t think this benchmark is ideal as a benchmark since it can only be run for one epoch, but that hasn’t changed in this PR and is out-of-scope.

wallet: remove BDB dependency from wallet migration benchmark #31241

Code Coverage & Benchmarks

Reviews

Conflicts