wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets

achow101 commented at 11:03 pm on September 4, 2022: member

Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet’s ScriptPubKeyMans. This is done in various places, such as IsMine, and helper functions for fetching a ScriptPubKeyMan and a SolvingProvider. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.

As these functions are based on doing IsMine for each ScriptPubKeyMan, we can improve this performance by caching IsMine scriptPubKeys for all descriptors and use that to determine which ScriptPubKeyMan to actually use for those things. This cache is used exclusively and we no longer iterate the SPKMs.

Also added a benchmark for IsMine.

achow101 marked this as ready for review on Sep 4, 2022

achow101 force-pushed on Sep 4, 2022

DrahtBot added the label Wallet on Sep 5, 2022

achow101 force-pushed on Sep 5, 2022

achow101 commented at 4:35 am on September 5, 2022: member

Dropped the signing changes as they were causing issues with being able to sign transactions that we have the keys for but not the descriptors. #23417 would allow us to resolve the performance issues for signing while retaining this behavior.

DrahtBot commented at 6:27 am on September 5, 2022: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, josibake, furszy
Concept ACK	aureleoules, darosior, willcl-ark

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:

#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28201 (Silent Payments: sending by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

achow101 force-pushed on Sep 6, 2022

achow101 force-pushed on Sep 20, 2022

DrahtBot added the label Needs rebase on Sep 21, 2022

in src/wallet/wallet.cpp:3566 in 020f5b8221 outdated

3472+    if (!ranged) {
3473+        for (const auto& script : spkm->GetScriptPubKeys()) {
3474+            m_cached_spks[script].insert(spkm);
3475+            m_uncached_spkms.erase(spkm);
3476+        }
3477+    }

w0xlt commented at 2:21 pm on September 21, 2022:

0    if (!ranged) {
1        for (const auto& script : spkm->GetScriptPubKeys()) {
2            m_cached_spks[script].insert(spkm);
3        }
4    }
5    else {
6        m_uncached_spkms.insert(spkm);
7    }

achow101 commented at 3:08 pm on September 21, 2022:

I prefer the way it was written as it ensures that no spkm will be accidentally missed.

in src/bench/wallet_balance.cpp:40 in fec1d94443 outdated

29@@ -31,7 +30,6 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b
30         LOCK(wallet.cs_wallet);
31         wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
32         wallet.SetupDescriptorScriptPubKeyMans();
33-        if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false);

w0xlt commented at 2:28 pm on September 21, 2022:

What would be the reason to remove this validation ?

achow101 commented at 3:11 pm on September 21, 2022:

It is incorrect, unneeded, and caused a segfault.

It is incorrect because LoadWallet is supposed to be called before SetupDescriptorScriptPubKeyMans. Doing it after will break things that SetupDescriptorScriptPubKeyMans sets, which also caused a segfault. It is also entirely unneeded as loading an empty wallet doesn’t do anything, and SetupDescriptorScriptPubKeyMans is doing all of the setup needed.

achow101 force-pushed on Sep 21, 2022

DrahtBot removed the label Needs rebase on Sep 21, 2022

in src/wallet/wallet.h:348 in f9f467b249 outdated

340@@ -341,6 +341,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
341     // ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
342     std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;
343 
344+    //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms
345+    std::unordered_map<CScript, std::unordered_set<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks;
346+    //! Set of which descriptors are not in m_cached_spks
347+    std::unordered_set<ScriptPubKeyMan*> m_uncached_spkms;

w0xlt commented at 6:00 pm on September 21, 2022:

I think the names below work better and make the purpose of the variable clearer.

0    std::unordered_map<CScript, std::unordered_set<ScriptPubKeyMan*>, SaltedSipHasher> m_ranged_spks;
1    //! Set of which descriptors are not in m_cached_spks
2    std::unordered_set<ScriptPubKeyMan*> m_non_ranged_spkms;

achow101 commented at 10:27 pm on January 23, 2023:

I prefer describing the variable with how we expect to use it, rather than what it contains. We’re using it as a cache of spkms, the type of spkm doesn’t matter.

w0xlt commented at 6:06 pm on September 21, 2022: contributor

This change looks good, but adds complexity by adding two caches instead of accessing m_spk_managers directly.

Perhaps the PR description might include benchmark improvement as a reason for the added complexity.

DrahtBot added the label Needs rebase on Oct 27, 2022

achow101 force-pushed on Nov 4, 2022

DrahtBot removed the label Needs rebase on Nov 4, 2022

in src/bench/wallet_ismine.cpp:45 in fd1b75ec32 outdated

60+    } else {
61+        options.create_flags = WALLET_FLAG_DESCRIPTORS;
62+        options.require_format = DatabaseFormat::SQLITE;
63+    }
64+    auto database = CreateMockWalletDatabase(options);
65+    auto wallet = BenchLoadWallet(std::move(database), context, options);

aureleoules commented at 5:06 pm on December 1, 2022:

This seems to be duplicated code from bench/wallet_loading.cpp, could it be re-used?

achow101 commented at 10:51 pm on January 23, 2023:

Refactored.

aureleoules commented at 5:07 pm on December 1, 2022: member

Concept ACK

achow101 force-pushed on Dec 2, 2022

achow101 force-pushed on Jan 23, 2023

achow101 force-pushed on Jan 25, 2023

achow101 force-pushed on Jan 26, 2023

DrahtBot added the label Needs rebase on Feb 1, 2023

achow101 force-pushed on Feb 1, 2023

DrahtBot removed the label Needs rebase on Feb 1, 2023

darosior commented at 12:07 pm on February 6, 2023: member

Concept ACK. This was hit in practice by a user in #27002. From #27002 (comment):

v24.0.1:

0$ time ./bitcoin-cli -rpcwallet=desc listunspent|grep txid|wc -l
1224
2
3real	1m15.596s
4user	0m0.008s
5sys	0m0.000s

This PR:

0$ time ./bitcoin-cli -rpcwallet=desc listunspent|grep txid|wc -l
1314
2
3real	0m0.464s
4user	0m0.009s
5sys	0m0.000s

willcl-ark commented at 8:13 pm on February 6, 2023: contributor

Concept ACK.

I wonder if the performance boost here might also be enough to close out #19942 even though it’s not implementing the functionality OP there thought would solve their issue?

achow101 force-pushed on Feb 6, 2023

achow101 commented at 9:01 pm on February 6, 2023: member

I’ve ended up rewriting a significant chunk of this PR. Instead of caching the scriptPubKeys of non-ranged descriptors, I’ve changed this to cache the scriptPubKeys of all descriptors. I think this makes it easier to understand and is also less fragile.

Originally I didn’t cache the spks of ranged descriptors because I thought it would be difficult to update the cached spks every time a TopUp was called as that is used in several places internally. My solution for that issue is to have a callback in the SPKM that is called by TopUp to inform the containing CWallet of the newly added spks. This allows the spk cache to always match the descriptor’s spk set.

One concern of this approach is that it maintains duplicate copies of the scriptPubKeys, thus significantly increasing the memory footprint. However, this may not really be a concern given that scriptPubKeys are relatively small and so it really just ends up being a couple of kB.

This also managed to improve the performance of IsMine even without a ton of non-ranged descriptors. Here are the benchmark results for the original, and the new:

Original:

ns/op	op/s	err%	ins/op	bra/op	miss%	total	benchmark
11,445.75	87,368.70	0.2%	103,638.12	18,444.02	0.1%	0.01	`WalletIsMineDescriptors`
8,256.94	121,110.23	0.1%	74,299.09	10,755.01	0.1%	0.01	`WalletIsMineLegacy`
12,289.10	81,372.91	0.3%	114,209.13	20,204.02	0.1%	0.01	`WalletIsMineMigratedDescriptors`

New:

ns/op	op/s	err%	ins/op	bra/op	miss%	total	benchmark
1,734.82	576,429.48	0.2%	15,191.02	2,579.00	0.0%	0.01	`WalletIsMineDescriptors`
8,372.88	119,433.26	0.2%	77,389.09	11,220.01	0.1%	0.01	`WalletIsMineLegacy`
1,753.04	570,438.89	0.4%	15,359.02	2,606.00	0.0%	0.01	`WalletIsMineMigratedDescriptors`

achow101 renamed this:
~~wallet: cache IsMine scriptPubKeys to improve performance of wallets with a lot of non-ranged descriptors~~
wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets
on Feb 6, 2023

achow101 force-pushed on Feb 13, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

achow101 force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

achow101 force-pushed on Feb 18, 2023

DrahtBot added the label Needs rebase on Feb 27, 2023

achow101 force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 1, 2023

darosior commented at 4:00 pm on April 19, 2023: member

I really like the performance gains here, however i feel like this is making the spks management bleed into the wallet from the spkman. But there is no other way for the wallet to efficiently locate the spkman(s)..

I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys, and thought we might as well share a single mapping owned by CWallet across all spkmans. This would also have the nice property that we don’t need to make sure moving forward we don’t introduce any inconsistency between the CWallet cache and what the spkmans track (as we’d have to with the approach here). However i can’t say i like this approach better.. Thought i’d share it here anyway: https://github.com/darosior/bitcoin/tree/pr_26008 (quick and dirty, there is a bug [0], but should be readable enough to have an idea of what this approach could look like). Performances according to the benchmarks introduced here are similar.

[0] Interestingly all tests pass, this bug only shows up in one of the benchmark, but not one of those introduced here.

achow101 commented at 7:29 pm on April 20, 2023: member

I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys

I added the callback to also allow for other cases where we need to know that the scriptPubKey set has changed, such as with fast rescans with block filters.

DrahtBot added the label Needs rebase on May 15, 2023

achow101 force-pushed on May 15, 2023

DrahtBot removed the label Needs rebase on May 15, 2023

achow101 commented at 8:42 pm on May 15, 2023: member

First 2 commits split into #27666. Marking as draft for now.

achow101 marked this as a draft on May 15, 2023

achow101 force-pushed on May 15, 2023

DrahtBot added the label CI failed on May 15, 2023

DrahtBot removed the label CI failed on May 16, 2023

achow101 force-pushed on May 27, 2023

achow101 force-pushed on May 30, 2023

achow101 marked this as ready for review on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

achow101 force-pushed on Sep 27, 2023

DrahtBot removed the label Needs rebase on Sep 27, 2023

DrahtBot added the label CI failed on Sep 27, 2023

DrahtBot removed the label CI failed on Sep 28, 2023

aureleoules commented at 0:21 am on December 3, 2023: member

The benchmark WalletCreateTxUsePresetInputsAndCoinSelection seems to segfault occasionally on this pull (increasing min-time makes it crash consistently)

 0$ valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
 1==131665== Memcheck, a memory error detector
 2==131665== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
 3==131665== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
 4==131665== Command: ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
 5==131665==
 6==131665== Invalid read of size 8
 7==131665==    at 0x607B83: wallet::CWallet::AddToWalletIfInvolvingMe(std::shared_ptr<CTransaction const> const&, std::variant<wallet::TxStateConfirmed, wallet::TxStateInMempool, wallet::TxStateInactive> const&, bool, bool) (wallet.cpp:1252)==131665==    by 0x60879A: SyncTransaction (wallet.cpp:1409)
 8==131665==    by 0x60879A: wallet::CWallet::blockConnected(interfaces::BlockInfo const&) (wallet.cpp:1479)
 9==131665==    by 0x28B59C: generateFakeBlock(CChainParams const&, node::NodeContext const&, wallet::CWallet&, CScript const&) (wallet_create_tx.cpp:73)
10==131665==    by 0x28C982: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:103)
11==131665==    by 0x215E9A: operator() (std_function.h:590)
12==131665==    by 0x215E9A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
13==131665==    by 0x1F3CB6: main (bench_bitcoin.cpp:132)
14==131665==  Address 0x974ad60 is 0 bytes inside a block of size 632 free'd
15==131665==    at 0x484BB6F: operator delete(void*, unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
16==131665==    by 0x5FE039: operator() (unique_ptr.h:85)
17==131665==    by 0x5FE039: reset (unique_ptr.h:182)
18==131665==    by 0x5FE039: operator= (unique_ptr.h:167)
19==131665==    by 0x5FE039: operator= (unique_ptr.h:212)
20==131665==    by 0x5FE039: operator= (unique_ptr.h:371)
21==131665==    by 0x5FE039: wallet::CWallet::AddScriptPubKeyMan(uint256 const&, std::unique_ptr<wallet::ScriptPubKeyMan, std::default_delete<wallet::ScriptPubKeyMan> >) (wallet.cpp:3509)
22==131665==    by 0x615D95: wallet::CWallet::LoadDescriptorScriptPubKeyMan(uint256, wallet::WalletDescriptor&) (wallet.cpp:3556)
23==131665==    by 0x63D847: wallet::LoadDescriptorWalletRecords(wallet::CWallet*, wallet::DatabaseBatch&, int)::{lambda(wallet::CWallet*, DataStream&, CDataStream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)#1}::operator()(wallet::CWallet*, DataStream&, CDataStream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) const (walletdb.cpp:807)
24==131665==    by 0x631799: operator() (std_function.h:590)
25==131665==    by 0x631799: wallet::LoadRecords(wallet::CWallet*, wallet::DatabaseBatch&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, DataStream&, std::function<wallet::DBErrors (wallet::CWallet*, DataStream&, CDataStream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)>) (walletdb.cpp:509)
26==131665==    by 0x631F16: wallet::LoadRecords(wallet::CWallet*, wallet::DatabaseBatch&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::function<wallet::DBErrors (wallet::CWallet*, DataStream&, CDataStream&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)>) (walletdb.cpp:523)
27==131665==    by 0x63E81D: LoadDescriptorWalletRecords (walletdb.cpp:789)
28==131665==    by 0x63E81D: wallet::WalletBatch::LoadWallet(wallet::CWallet*) (walletdb.cpp:1177)
29==131665==    by 0x5F85DD: wallet::CWallet::LoadWallet() (wallet.cpp:2311)
30==131665==    by 0x28C913: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:93)
31==131665==    by 0x215E9A: operator() (std_function.h:590)
32==131665==    by 0x215E9A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
33==131665==    by 0x1F3CB6: main (bench_bitcoin.cpp:132)
34==131665==  Block was alloc'd at
35==131665==    at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
36==131665==    by 0x6011D7: wallet::CWallet::SetupDescriptorScriptPubKeyMans(CExtKey const&) (wallet.cpp:3572)
37==131665==    by 0x60B3E9: wallet::CWallet::SetupDescriptorScriptPubKeyMans() (wallet.cpp:3604)
38==131665==    by 0x28C90B: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:92)
39==131665==    by 0x215E9A: operator() (std_function.h:590)
40==131665==    by 0x215E9A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
41==131665==    by 0x1F3CB6: main (bench_bitcoin.cpp:132)
42==131665==
43==131665== Invalid read of size 8
44==131665==    at 0x607B86: wallet::CWallet::AddToWalletIfInvolvingMe(std::shared_ptr<CTransaction const> const&, std::variant<wallet::TxStateConfirmed, wallet::TxStateInMempool, wallet::TxStateInactive> const&, bool, bool) (wallet.cpp:1252)==131665==    by 0x60879A: SyncTransaction (wallet.cpp:1409)
45==131665==    by 0x60879A: wallet::CWallet::blockConnected(interfaces::BlockInfo const&) (wallet.cpp:1479)
46==131665==    by 0x28B59C: generateFakeBlock(CChainParams const&, node::NodeContext const&, wallet::CWallet&, CScript const&) (wallet_create_tx.cpp:73)
47==131665==    by 0x28C982: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:103)
48==131665==    by 0x215E9A: operator() (std_function.h:590)
49==131665==    by 0x215E9A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
50==131665==    by 0x1F3CB6: main (bench_bitcoin.cpp:132)
51==131665==  Address 0x75707428686b70c7 is not stack'd, malloc'd or (recently) free'd
52==131665==
53==131665==
54==131665== Process terminating with default action of signal 11 (SIGSEGV)
55==131665==  General Protection Fault
56==131665==    at 0x607B86: wallet::CWallet::AddToWalletIfInvolvingMe(std::shared_ptr<CTransaction const> const&, std::variant<wallet::TxStateConfirmed, wallet::TxStateInMempool, wallet::TxStateInactive> const&, bool, bool) (wallet.cpp:1252)==131665==    by 0x60879A: SyncTransaction (wallet.cpp:1409)
57==131665==    by 0x60879A: wallet::CWallet::blockConnected(interfaces::BlockInfo const&) (wallet.cpp:1479)
58==131665==    by 0x28B59C: generateFakeBlock(CChainParams const&, node::NodeContext const&, wallet::CWallet&, CScript const&) (wallet_create_tx.cpp:73)
59==131665==    by 0x28C982: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:103)
60==131665==    by 0x215E9A: operator() (std_function.h:590)
61==131665==    by 0x215E9A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
62==131665==    by 0x1F3CB6: main (bench_bitcoin.cpp:132)
63==131665==
64==131665== HEAP SUMMARY:
65==131665==     in use at exit: 43,227,660 bytes in 59,138 blocks
66==131665==   total heap usage: 342,235 allocs, 283,097 frees, 77,605,937 bytes allocated
67==131665==
68==131665== LEAK SUMMARY:
69==131665==    definitely lost: 0 bytes in 0 blocks
70==131665==    indirectly lost: 0 bytes in 0 blocks
71==131665==      possibly lost: 912 bytes in 3 blocks
72==131665==    still reachable: 43,226,748 bytes in 59,135 blocks
73==131665==         suppressed: 0 bytes in 0 blocks
74==131665== Rerun with --leak-check=full to see details of leaked memory
75==131665==
76==131665== For lists of detected and suppressed errors, rerun with: -s
77==131665== ERROR SUMMARY: 555 errors from 2 contexts (suppressed: 0 from 0)
78Segmentation fault

DrahtBot added the label Needs rebase on Dec 8, 2023

achow101 force-pushed on Dec 8, 2023

DrahtBot removed the label Needs rebase on Dec 8, 2023

achow101 force-pushed on Dec 8, 2023

achow101 commented at 10:53 pm on December 8, 2023: member

Benchmark crash should be fixed.

achow101 force-pushed on Dec 11, 2023

DrahtBot added the label CI failed on Dec 12, 2023

achow101 force-pushed on Dec 12, 2023

DrahtBot removed the label CI failed on Dec 12, 2023

DrahtBot added the label CI failed on Jan 14, 2024

josibake commented at 6:01 pm on January 26, 2024: member

Concept ACK

maflcko commented at 5:27 pm on January 31, 2024: member

Rebase for fresh CI?

achow101 force-pushed on Jan 31, 2024

achow101 commented at 7:02 pm on January 31, 2024: member

Rebased.

DrahtBot removed the label CI failed on Jan 31, 2024

in src/wallet/scriptpubkeyman.h:380 in eddf9499ef outdated

376@@ -374,7 +377,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
377 
378     bool TopUpChain(WalletBatch& batch, CHDChain& chain, unsigned int size);
379 public:
380-    LegacyScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {}
381+    LegacyScriptPubKeyMan(WalletStorage& storage, std::function<void(const std::set<CScript>&, ScriptPubKeyMan*)> topup_callback, int64_t keypool_size) : ScriptPubKeyMan(storage, topup_callback), m_keypool_size(keypool_size) {}

josibake commented at 9:58 am on February 2, 2024:

in “wallet: Introduce a callback called after TopUp completes” (https://github.com/bitcoin/bitcoin/pull/26008/commits/eddf9499eff037a94cf28869de6ef9954f64c361):

Seems a bit weird to pass the topup_callback when in the same commit we comment that the LegacyScriptPubKey can’t use the SPK cache due to us not knowing what scripts its watching for.

What about removing it from the LegacyScriptPubKeyMan and passing a nullptr to ScriptPubKeyMan for the call back function, and we can move the comment here? Seems safer this way because if someone tried to use the callback with the LSPKM, they’d get a bad_function_call exception (I think?).

achow101 commented at 8:23 pm on February 2, 2024:

It was being used for migration as LegacyScriptPubKeyMan::MigrateToDescriptor() creates DescriptorScriptPubKeyMan which needs it. However, these can also just take a no-op function, and I’ve done that as well as your suggestion.

in src/wallet/wallet.cpp:3907 in 3279e0e807 outdated

3903@@ -3897,11 +3904,16 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
3904         if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest);
3905     }
3906 
3907+    m_cached_spks.clear();

josibake commented at 10:09 am on February 2, 2024:

in “wallet: Use scriptPubKey cache in IsMine” (https://github.com/bitcoin/bitcoin/pull/26008/commits/3279e0e8078b92bc590d1ab65ba35f8d197edc12):

Why are we clearing the cache? Wouldn’t this invalidate the cache for any existing descriptors I have? If I’m not supposed to do wallet migration in a wallet that already has descriptors, maybe it would be better to check that the cache is empty and throw an error here if not?

achow101 commented at 8:24 pm on February 2, 2024:

It was necessary since the cache was being refilled by LegacyScriptPubKeyMan::MigrateToDescriptor when it creates the DescriptorScriptPubKeyMan replacements. But since those are now made with a no-op callback, this is unnecessary, so I’ve removed this and replaced it with an Assume(m_cached_spks.empty()).

in src/wallet/wallet.cpp:1579 in 3279e0e807 outdated

1577+
1578+    // Search the cache
1579+    const auto& it = m_cached_spks.find(script);
1580+    if (it != m_cached_spks.end()) {
1581+        // The cache is only used for descriptor wallets. If a script is in the cache, then it is automatically ISMINE_SPENDABLE.
1582+        return ISMINE_SPENDABLE;

josibake commented at 10:34 am on February 2, 2024:

in “wallet: Use scriptPubKey cache in IsMine” (https://github.com/bitcoin/bitcoin/pull/26008/commits/3279e0e8078b92bc590d1ab65ba35f8d197edc12):

While this assumption is true today (and probably always will be), it feels like we are having CWallet make a decision that should be left up to the SPKMan. What if we searched the cache the same way as before by returning the max result from the SPKMan(s)?

0        isminetype result = ISMINE_NO;
1        for (const auto& spk : it->second) {
2            result = std::max(result, spk->IsMine(script));
3        }
4        return result;
5    }

I didn’t compile this and test the benchmark, but I doubt it hurts the performance gains much (if any).

achow101 commented at 8:24 pm on February 2, 2024:

Done

It has a very minor impact on the benchmark. However this would have a much more noticeable impact if the same scriptPubKey showed up in multiple ScriptPubKeyMans which the benchmark does not check.

in src/wallet/wallet.cpp:3440 in a78ad3f406 outdated

3436-    for (const auto& spk_man_pair : m_spk_managers) {
3437-        if (spk_man_pair.second->CanProvide(script, sigdata)) {
3438-            spk_mans.insert(spk_man_pair.second.get());
3439-        }
3440+
3441+    // Search the cache first

josibake commented at 10:48 am on February 2, 2024:

in “wallet: Use scriptPubKey cache in GetScriptPubKeyMans” (https://github.com/bitcoin/bitcoin/pull/26008/commits/a78ad3f40678d553cbe6a28ed9daaddd3097d196):

nit:

0    // Search the cache for descriptor scriptPubKeyMans

achow101 commented at 8:25 pm on February 2, 2024:

The cache is intended to be somewhat generic, so if we had another type of ScriptPubKeyMan in the future, it would still be used, so leaving this comment as is.

josibake commented at 1:39 pm on February 2, 2024: member

Dropped the signing changes as they were causing issues with being able to sign transactions that we have the keys for but not the descriptors. #23417 would allow us to resolve the performance issues for signing while retaining this behavior.

Might be worth revisiting this now that the cache is much simpler. The signing stuff could be a follow-up PR, but if it’s simple enough it would be really nice to include it here. Otherwise, someone using migrate wallet could still hit a wall even with the cache when trying to sign a transaction.

I started playing with the idea here: https://github.com/josibake/bitcoin/commit/5c3a2dad385ee5ce971ed9f3cfc35f9dfbadd0b8 but didn’t spend very much time thinking about it, so feel free to ignore if its not helpful or if I’m missing something obvious.

achow101 commented at 4:49 pm on February 2, 2024: member

Might be worth revisiting this now that the cache is much simpler. The signing stuff could be a follow-up PR, but if it’s simple enough it would be really nice to include it here. Otherwise, someone using migrate wallet could still hit a wall even with the cache when trying to sign a transaction.

I don’t think it’s possible to do caching for signing in a sane way that doesn’t involve rewriting how we manage keys. The crux of the issue is that we will sign for things even if the wallet has only the keys, but not the scripts.

achow101 force-pushed on Feb 2, 2024

josibake commented at 9:18 pm on February 2, 2024: member

ACK https://github.com/bitcoin/bitcoin/commit/46b3c569cfd01a45e5d9cf9428a07b06652e3df9

Looks good, thanks for taking the suggestions! Having the callbacks seems like a really nice feature: based on one of your review comments, I took a stab at using the callbacks with fast wallet rescans and using them simplifies that code quite a bit.

DrahtBot requested review from willcl-ark on Feb 2, 2024

DrahtBot requested review from darosior on Feb 2, 2024

DrahtBot requested review from aureleoules on Feb 2, 2024

in src/wallet/wallet.cpp:3566 in 0b387232da outdated

3541@@ -3542,6 +3542,12 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
3542         auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, std::bind(&CWallet::CacheNewScriptPubKeys, this, std::placeholders::_1, std::placeholders::_2), desc, m_keypool_size));
3543         AddScriptPubKeyMan(id, std::move(spk_manager));
3544     }
3545+
3546+    // Cache scriptPubKeys
3547+    ScriptPubKeyMan* spkm = m_spk_managers.at(id).get();
3548+    for (const auto& script : spkm->GetScriptPubKeys()) {
3549+        m_cached_spks[script].insert(spkm);
3550+    }

furszy commented at 2:02 pm on February 12, 2024:

This does not seems to be necessary. At this point (LoadDescriptorScriptPubKeyMan), the spkm should’t contain any script.

achow101 commented at 6:42 pm on February 12, 2024:

Good catch, removed.

furszy commented at 2:35 pm on February 12, 2024: member

Concept ACK

I have been considering the idea of using a probabilistic data structure instead of the deterministic one for the membership-test over the past few days. Mainly, because I’m a bit concerned about the unbounded cache size. I was going to share it today, but it requires more work and the local benchmarks show a substantial speed difference in favor of this approach.

So.. as feature freeze is close, will finish review in few hours. I believe this can be implemented simpler, and shorter, using a signal instead of providing the topup callback to all spkm constructors.

in src/wallet/wallet.cpp:3765 in 46b3c569cf outdated

3768@@ -3740,7 +3769,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3769         WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
3770         spk_man->UpdateWalletDescriptor(desc);

furszy commented at 4:58 pm on February 12, 2024:

Shouldn’t be a problem but, just to keep this process consistent; as UpdateWalletDescriptor clears the SPKM internal scripts cache, shouldn’t do the same for the general wallet cache too? E.g. remove the scripts belonging to this spkm here, so the TopUp call that is executed few lines below (re)populates the scripts derived from the updated SPKM.

achow101 commented at 6:31 pm on February 12, 2024:

Removing the spks from the wallet’s cache for a particular spkm requires iterating all of the spks and looking for ones that point to this spkm. I think that would be quite slow, and also redundant, since AddWalletDescriptor can only ever add scripts to the wallet. Arguably, UpdateWalletDescriptor shouldn’t be clearing m_map_script_pub_keys either.

achow101 force-pushed on Feb 12, 2024

josibake commented at 7:04 pm on February 12, 2024: member

reACK https://github.com/bitcoin/bitcoin/pull/26008/commits/a96647308ea40028806e2db6d35fb5dd14bea15f

DrahtBot requested review from furszy on Feb 12, 2024

in src/wallet/wallet.cpp:3923 in c88c07e133 outdated

3909             error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.");
3910             return false;
3911         }
3912+        for (const auto& script : desc_spkm->GetScriptPubKeys()) {
3913+            m_cached_spks[script].insert(desc_spkm.get());
3914+        }

furszy commented at 7:17 pm on February 12, 2024:

tiny nit: sounds like these two changes should be in the previous commit (cde66ceae).

achow101 commented at 1:27 am on February 15, 2024:

Done

in src/wallet/wallet.cpp:3472 in a96647308e outdated

3470-            return spk_man_pair.second->GetSolvingProvider(script);
3471+    // Search the cache first
3472+    const auto& it = m_cached_spks.find(script);
3473+    if (it != m_cached_spks.end()) {
3474+        for (const auto& spkm : it->second) {
3475+            if (spkm->CanProvide(script, sigdata)) {

furszy commented at 7:28 pm on February 12, 2024:

What is the difference between this (a966473) and the previous commit (f0c844bda) in terms of CanProvide usage?

In the previous commit, CanProvide usage was removed, while here you kept it. I understand that you removed the CanProvide call from the previous commit because it is redundant (it calls IsMine internally which is exactly what m_cached_spks does) but I also agree that we might want to keep CanProvide because its internal implementation could vary over time.

achow101 commented at 11:01 pm on February 12, 2024:

I think this may have just been a holdover from when I was investigating using the same cache for signing. There should be no difference as CanProvide is redundant in both contexts.

achow101 commented at 1:29 am on February 15, 2024:

I’ve dropped this CanProvide.

furszy commented at 7:44 pm on February 12, 2024: member

Left two more comments. They aren’t really blocking but I’m still struggling with the memory consumption topic. Isn’t std::unordered_map going to store all keys (entire scripts) in memory for duplicated so it can solve hash collisions?

Apart from that, just as an extra note for the future: It seems that we will need to rearrange code so that these functions aren’t called so many times within the same process.

DrahtBot requested review from furszy on Feb 12, 2024

maflcko commented at 10:24 am on February 14, 2024: member

Tested time loadwallet with my normal regtest wallet (https://github.com/bitcoin/bitcoin/issues/28510#issuecomment-1727561717), didn’t review or check for correctness.

Before:

0real	0m35.182s

After:

0real	0m3.622s

achow101 force-pushed on Feb 15, 2024

achow101 commented at 1:27 am on February 15, 2024: member

They aren’t really blocking but I’m still struggling with the memory consumption topic. Isn’t std::unordered_map going to store all keys (entire scripts) in memory for duplicated so it can solve hash collisions?

Yes.

I’ve done some memory usage profiling with a large wallet, and various combinations of ordered and unordered maps and sets.

The baseline memory usage of m_map_script_pub_keys, which is the same for all of the following, is 1.2 MiB

std::unordered_map<CScript, std::unorderd_set<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 3.4 MiB
std::unordered_map<CScript, std::set<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 2.6 MiB
std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 1.2 MiB
std::map<CScript, std::unorderd_set<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 3.7 MiB
std::map<CScript, std::set<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 2.9 MiB
std::map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks: 1.5 MiB

The benchmark indicates that there isn’t really a meaningful difference between all of these approaches. It seems that not using unordered_* may be slightly faster as hashing is not needed.

In the latest push, I’ve changed to using std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks so the memory consumption should be about double the size of all scriptPubKeys.

achow101 force-pushed on Feb 15, 2024

DrahtBot commented at 1:54 am on February 15, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin/bitcoin/runs/21591404299

DrahtBot added the label CI failed on Feb 15, 2024

DrahtBot removed the label CI failed on Feb 15, 2024

maflcko commented at 11:11 am on February 15, 2024: member

I re-checked b20d882fd53c21098eb7858b2dbca84eafd2b344 and re-confirmed time loadwallet (https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180)

in src/wallet/external_signer_scriptpubkeyman.h:16 in e06705140d outdated

12@@ -13,11 +13,11 @@ namespace wallet {
13 class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
14 {
15   public:
16-  ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size)
17-      :   DescriptorScriptPubKeyMan(storage, descriptor, keypool_size)
18+  ExternalSignerScriptPubKeyMan(WalletStorage& storage, std::function<void(const std::set<CScript>&, ScriptPubKeyMan*)> topup_callback, WalletDescriptor& descriptor, int64_t keypool_size)

ryanofsky commented at 1:33 pm on February 15, 2024:

In commit “wallet: Introduce a callback called after TopUp completes” (e06705140d09adb9baf83e24babc575ecdfc0f38)

Passing both WalletStorage callbacks and a separate topup_callback to DescriptorScriptPubKeyMan objects seems awkward. Is there a reason topup_callback is not just a WalletStorage method?

achow101 commented at 5:28 pm on February 15, 2024:

Good point, I’ve changed this to use WalletStorage instead. This greatly reduces the diff.

in src/wallet/wallet.cpp:3901 in 75f0fcf2ef outdated

3896     for (auto& desc_spkm : data.desc_spkms) {
3897         if (m_spk_managers.count(desc_spkm->GetID()) > 0) {
3898             error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.");
3899             return false;
3900         }
3901+        for (const auto& script : desc_spkm->GetScriptPubKeys()) {

ryanofsky commented at 1:57 pm on February 15, 2024:

In commit “wallet: Cache scriptPubKeys for all DescriptorSPKMs” (75f0fcf2ef17e3844dac6445913f31fecc4f76b1)

It would be helpful to have a comment here explaining why this is necessary. I assume this is related to the “Use no-op topup_callback, cache is filled later in migration” comments in the earlier commit, but I don’t understand why migration code needs this special flow instead of using topup_callback / CacheNewScriptPubKeys like other wallet code. Tthis would be a good place for a code comment to explain how migration code is handling this cache.

achow101 commented at 5:29 pm on February 15, 2024:

This was because of using no-op callback, but the change to a WalletStorage method obsoletes this.

achow101 added this to the milestone 27.0 on Feb 15, 2024

in src/wallet/wallet.cpp:1575 in 523010b7bd outdated

1570@@ -1571,11 +1571,22 @@ isminetype CWallet::IsMine(const CTxDestination& dest) const
1571 isminetype CWallet::IsMine(const CScript& script) const
1572 {
1573     AssertLockHeld(cs_wallet);
1574-    isminetype result = ISMINE_NO;
1575-    for (const auto& spk_man_pair : m_spk_managers) {
1576-        result = std::max(result, spk_man_pair.second->IsMine(script));
1577+
1578+    // Search the cache

ryanofsky commented at 2:12 pm on February 15, 2024:

In commit “wallet: Use scriptPubKey cache in IsMine” (523010b7bded819fdc11396b307d9772ec8ffe33)

Would be helpful to say what cache lookups are an alternative to in this context. Maybe add comment “Search m_cached_spks cache to only call IsMine on relevant SPKMs instead of calling it on everything in m_spk_managers.” Could add similar comments to GetScriptPubKeyMan and GetSolvingProvider in following commits too.

achow101 commented at 5:29 pm on February 15, 2024:

Done

in src/wallet/wallet.cpp:3430 in 38cfcf3435 outdated

3435@@ -3436,12 +3436,16 @@ ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool intern
3436 std::set<ScriptPubKeyMan*> CWallet::GetScriptPubKeyMans(const CScript& script) const
3437 {
3438     std::set<ScriptPubKeyMan*> spk_mans;
3439-    SignatureData sigdata;
3440-    for (const auto& spk_man_pair : m_spk_managers) {
3441-        if (spk_man_pair.second->CanProvide(script, sigdata)) {

ryanofsky commented at 2:17 pm on February 15, 2024:

In commit “wallet: Use scriptPubKey cache in GetScriptPubKeyMans” (38cfcf3435a5113606c8066833d49a4ab560aea9)

New code is no longer calling CanProvide. Would be good to use Assert or Assume to document that it should be true and verify integrity of the cache. (Same comment applies to GetSolvingProvider in next commit too)

achow101 commented at 5:29 pm on February 15, 2024:

Added Assumes.

ryanofsky approved

ryanofsky commented at 2:29 pm on February 15, 2024: contributor

Code review ACK b20d882fd53c21098eb7858b2dbca84eafd2b344. I don’t have much insight into performance impact of this change, but it seems like it should make migrated legacy wallets much faster (10 times faster loading in one case: #26008 (comment)) even if it does increase memory usage, and it seems to be implemented correctly.

re: #26008#pullrequestreview-1876002633

Apart from that, just as an extra note for the future: It seems that we will need to rearrange code so that these functions aren’t called so many times within the same process.

I’m not sure what this is referring to, so would be curious

DrahtBot requested review from josibake on Feb 15, 2024

achow101 force-pushed on Feb 15, 2024

achow101 commented at 5:30 pm on February 15, 2024: member

I’ve also added one more commit that speeds up loading and migration as I noticed one spot where m_spk_managers was being iterated again during loading.

josibake commented at 12:49 pm on February 16, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/26008/commits/ac246f68299d0dc208ae513dfad1a8fc91b5e6d4

DrahtBot removed review request from josibake on Feb 16, 2024

DrahtBot requested review from ryanofsky on Feb 16, 2024

in src/wallet/wallet.cpp:3449 in b9dd0fecda outdated

3449     }
3450+    SignatureData sigdata;
3451+    Assume(std::all_of(spk_mans.begin(), spk_mans.end(), [&script, &sigdata](ScriptPubKeyMan* spkm) { return spkm->CanProvide(script, sigdata); }));
3452+
3453+    // Legacy wallet
3454+    if (IsLegacy()) spk_mans.insert(GetLegacyScriptPubKeyMan());

ryanofsky commented at 2:30 pm on February 16, 2024:

In commit “wallet: Use scriptPubKey cache in GetScriptPubKeyMans” (b9dd0fecda98bd14d8d305a69e6ce65f1e0a73fa)

This also needs to check GetLegacyScriptPubKeyMan()->CanProvide() to match previous behavior, I think

achow101 commented at 6:21 pm on February 16, 2024:

I think it’s fine as LegacyScriptPubKeyMan is designed to be able to handle unknown scripts gracefully. But I’ve added a CanProvide check just for consistency.

in src/wallet/wallet.cpp:3568 in ac246f6829 outdated

3568     } else {
3569-        auto spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size));
3570-        AddScriptPubKeyMan(id, std::move(spk_manager));
3571+        spk_manager = std::unique_ptr<ScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size));
3572     }
3573+    return dynamic_cast<DescriptorScriptPubKeyMan&>(AddScriptPubKeyMan(id, std::move(spk_manager)));

ryanofsky commented at 2:40 pm on February 16, 2024:

In commit “wallet: Retrieve ID from loaded DescSPKM directly” (ac246f68299d0dc208ae513dfad1a8fc91b5e6d4)

Not important, but you could avoid the dynamic_cast and remove some boilerplate with:

 0DescriptorScriptPubKeyMan& CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
 1{
 2    DescriptorScriptPubKeyMan* spkm;
 3    if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
 4        spkm = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size);
 5    } else {
 6        spkm = new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size);
 7    }
 8    AddScriptPubKeyMan(id, std::unique_ptr<ScriptPubKeyMan>{spkm});
 9    return *spkm;
10}

achow101 commented at 6:21 pm on February 16, 2024:

Done as suggested

ryanofsky commented at 2:43 pm on February 16, 2024: contributor

Code review ac246f68299d0dc208ae513dfad1a8fc91b5e6d4

Looks good, but wondering about a possible bug in the GetScriptPubKeyMans legacy case (see below)

DrahtBot requested review from ryanofsky on Feb 16, 2024

achow101 force-pushed on Feb 16, 2024

in src/bench/wallet_ismine.cpp:22 in 65c7997221 outdated

17+
18+namespace wallet {
19+static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_combo = 0)
20+{
21+    const auto test_setup = MakeNoLogFileContext<TestingSetup>();
22+    test_setup->m_args.ForceSetArg("-unsafesqlitesync", "1");

furszy commented at 7:16 pm on February 16, 2024:

In 65c7997221:

This is not needed. The benchmark uses a db mock.

achow101 commented at 7:36 pm on February 16, 2024:

Removed

in src/wallet/wallet.h:997 in bf898af019 outdated

993@@ -994,7 +994,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
994     void ConnectScriptPubKeyManNotifiers();
995 
996     //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it
997-    void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc);
998+    DescriptorScriptPubKeyMan& LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc);

furszy commented at 7:24 pm on February 16, 2024:

In bf898af0190d:

It was removed on the last push: LoadDescriptorScriptPubKeyMan now returns the spkm but no function utilizes it.

achow101 commented at 7:36 pm on February 16, 2024:

Oops, somehow that usage got dropped. Fixed.

furszy commented at 7:24 pm on February 16, 2024: member

Reviewed. Need to update the second commit description.

bench: Add a benchmark for ismine b276825932

wallet: Introduce a callback called after TopUp completes

After TopUp completes, the wallet containing each SPKM will want to know
what new scriptPubKeys were generated. In order for all TopUp calls
(including ones internal the the SPKM), we use a callback function in
the WalletStorage interface.

99a0cddbc0

wallet: Cache scriptPubKeys for all DescriptorSPKMs

Have CWallet maintain a cache of all known scriptPubKeys for its
DescriptorSPKMs in order to improve performance of the functions that
require searching for scriptPubKeys.

37232332bd

wallet: Use scriptPubKey cache in IsMine edf4e73a16

wallet: Use scriptPubKey cache in GetScriptPubKeyMans b410f68791

wallet: Use scriptPubKeyCache in GetSolvingProvider 39640dd34e

wallet: Retrieve ID from loaded DescSPKM directly

Instead of iterating m_spk_managers a DescriptorSPKM has been loaded in
order to get it's ID to compare, have LoadDescriptorSPKM return a
reference to the loaded DescriptorSPKM so it can be queried directly.

e041ed9b75

achow101 commented at 7:36 pm on February 16, 2024: member

Need to update the second commit description.

Done

achow101 force-pushed on Feb 16, 2024

furszy commented at 7:41 pm on February 16, 2024: member

Apart from that, just as an extra note for the future: It seems that we will need to rearrange code so that these functions aren’t called so many times within the same process.

I’m not sure what this is referring to, so would be curious @ryanofsky. An easy example is CWallet::AvailableCoins, which calls CWallet::IsMine first, few lines later calls CWallet::GetSolvingProvider and then calls CWallet::CalculateMaximumSignedInputSize which internally calls CWallet::GetSolvingProvider again. So, only this function, traverses the spkm map at least 3 times per output.

ryanofsky approved

ryanofsky commented at 8:25 pm on February 16, 2024: contributor

Code review ACK e041ed9b755468d205ed48b29f6c4b9e9df8bc9f. Just suggested changes since last review

DrahtBot requested review from josibake on Feb 16, 2024

DrahtBot requested review from furszy on Feb 16, 2024

josibake commented at 4:41 pm on February 19, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/26008/commits/e041ed9b755468d205ed48b29f6c4b9e9df8bc9f

Reviewed diff, looks good!

furszy commented at 7:59 pm on February 19, 2024: member

Code review ACK e041ed9b

It isn’t blocking, but I have to admit that I’m still not really happy with the doubled script storage. I think we can do better. Will be experimenting with possible solutions. A first measure to decrease it and remain fast, without changing the structure, could be to use the cache only for the inactive SPKMs. Since the active ones are limited in number (max 8), they can be checked quickly. However, this approach does not address the issue of handling really large scripts, which could probably be resolved by changing the structure for a probabilistic one.

fanquake merged this on Feb 20, 2024

fanquake closed this on Feb 20, 2024

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

Code Coverage

Reviews

Conflicts