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

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)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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

wallet: remove BDB dependency from wallet migration benchmark #31241

Code Coverage & Benchmarks

Reviews

Conflicts