wallet: Keep track of the wallet's own transaction outputs in memory#27286
wallet: Keep track of the wallet's own transaction outputs in memory#27286glozow merged 13 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/27286. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
Updated the description with recent benchmarks. |
src/wallet/receive.cpp
Outdated
There was a problem hiding this comment.
In 72378b2 (wallet: Exit IsTrustedTx early if wtx is already in trusted_parents):
nanonit: commit message IsTrustedTx -> TxIsTrusted
davidgumberg
left a comment
davidgumberg
left a comment
There was a problem hiding this comment.
ACK 3973425
Left a lot of non-blocking nits/questions/observations, many of which are out-of-scope, and all of which (except for commit hygiene nits) can be addressed in a follow up.
Benchmarked this branch with the hard-coded nEpochIterations removed as suggested by @rkrux (#27286 (review)) against master with this commit cherry-picked on top, with nEpochIterations removed, and got the following results:
$ ./build/bin/bench_bitcoin -min-time=10000 '-filter=Wallet(Available|CreateTxUse).*'PR branch
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 47,042,915.76 | 21.26 | 0.1% | 792,728,393.18 | 162,978,291.67 | 4.864 | 13,610,250.59 | 0.1% | 11.00 | WalletAvailableCoins |
| 910,290.93 | 1,098.55 | 0.1% | 16,006,199.89 | 3,166,046.15 | 5.056 | 259,763.75 | 0.5% | 11.02 | WalletCreateTxUseOnlyPresetInputs |
| 33,100,784.93 | 30.21 | 0.4% | 375,640,418.09 | 114,901,658.67 | 3.269 | 67,268,574.55 | 0.4% | 10.78 | WalletCreateTxUsePresetInputsAndCoinSelection |
master
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 46,834,069.48 | 21.35 | 0.0% | 792,798,330.50 | 162,224,895.00 | 4.887 | 15,794,362.95 | 0.1% | 10.96 | WalletAvailableCoins |
| 929,561.19 | 1,075.78 | 0.3% | 16,460,517.63 | 3,232,134.94 | 5.093 | 293,191.79 | 0.4% | 11.04 | WalletCreateTxUseOnlyPresetInputs |
| 41,569,035.73 | 24.06 | 0.2% | 517,355,083.92 | 144,349,136.46 | 3.584 | 91,003,243.17 | 0.2% | 11.00 | WalletCreateTxUsePresetInputsAndCoinSelection |
WalletCreateTxUsePresetInputsAndCoinSelection is substantially faster, but AvailableCoins() looks similar.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
In 41c82af (wallet: Keep track of transaction outputs owned by the wallet)
Feel-free-to-disregard-big-picture-out-of-PR-scope-question-comment:
I feel like one of the challenges in reading CWallet is how much state exists, and how many places that state is modified/read/accessed from, maybe most of this is unavoidable, but I wonder if it would be a benefit to have some of these wallet state members like m_txos, manage themselves more.
E.g. it seems ugly that the wallet is directly modifying the m_txos map here, where in AddWallet() and LoadToWallet() at least behavior is encapsulated in RefreshTXOsFromTx(), but I can see why another CWallet() member function was not added since this erasing only happens in one place. But, when reading the code, I think this does have the effect of scattering the places where m_txos is modified, and makes it harder to reason about where and in what way m_txos can change, when there aren't a few narrowly defined functions for modifying it.
I wonder what you think of e.g. making a bespoke class that houses m_txos and mapWallet where RefreshTXOsFromTx, RemoveTx etc. would live, and one could always be sure that the wallet's tracked transactions and tracked outputs always stay in sync. I admit that this scatters the code still, just in another way, but I wonder if at this size, we have to start more aggressively encapsulating functionality.
There was a problem hiding this comment.
I wonder what you think of e.g. making a bespoke class that houses m_txos and mapWallet where RefreshTXOsFromTx, RemoveTx etc. would live, and one could always be sure that the wallet's tracked transactions and tracked outputs always stay in sync. I admit that this scatters the code still, just in another way, but I wonder if at this size, we have to start more aggressively encapsulating functionality.
I don't think that makes sense to do as it would end up being just another wrapper around a std::map.
In any case, definitely out of scope for this PR.
src/wallet/rpc/backup.cpp
Outdated
There was a problem hiding this comment.
8d9c7c3 (wallet: Recalculate the wallet's txos after any imports)
Just an observation:
It's interesting to me that it's expected behavior that even without a rescan, previously untracked outputs of tracked transactions that become spendable are known to the wallet. I suppose this is a side effect of the logic before this PR, and this PR preserves that behavior. This is tested for in the earlier b3182f3 (test: Test for balance update due to untracked output becoming spendable)
Initially when reviewing this I wanted to be very sure that RefreshAllTXOs took place anywhere the result of IsMine() on a transaction could happen (as far as I can tell this is the case), but the worst case scenario of this not happening is that a previously untracked output of a tracked transaction doesn't become available on import without a rescan, and even that, only until the next time you load the wallet. So, really not something to drip with sweat over.
|
reACK ec25de3 Reviewed range-diff ( The primary changes are:
I benchmarked again (w/ different hardware1) and it looks like the performance gap in the
|
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 39,666,039.18 | 25.21 | 0.2% | 743,701,163.19 | 184,433,998.97 | 4.032 | 13,191,744.00 | 0.1% | 32.98 | WalletAvailableCoins |
| 818,711.43 | 1,221.43 | 0.1% | 16,376,793.42 | 3,821,152.74 | 4.286 | 264,358.23 | 0.2% | 33.07 | WalletCreateTxUseOnlyPresetInputs |
| 28,955,730.17 | 34.54 | 0.7% | 370,601,527.15 | 135,128,956.19 | 2.743 | 66,153,487.66 | 0.4% | 33.26 | WalletCreateTxUsePresetInputsAndCoinSelection |
master (e217437 + a39074c52afd073abd9101dc6)
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 44,211,982.81 | 22.62 | 0.2% | 803,845,700.31 | 205,309,518.10 | 3.915 | 18,081,465.08 | 0.0% | 33.04 | WalletAvailableCoins |
| 855,419.56 | 1,169.02 | 0.1% | 16,581,860.02 | 3,991,552.91 | 4.154 | 355,371.44 | 0.2% | 33.02 | WalletCreateTxUseOnlyPresetInputs |
| 35,074,882.27 | 28.51 | 0.2% | 505,591,550.24 | 163,578,562.91 | 3.091 | 91,987,283.54 | 0.2% | 33.16 | WalletCreateTxUsePresetInputsAndCoinSelection |
old pr branch (3973425) with epochIterations removed
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 42,297,947.25 | 23.64 | 0.0% | 774,588,933.86 | 196,770,180.57 | 3.937 | 15,813,962.88 | 0.1% | 32.95 | WalletAvailableCoins |
| 852,312.97 | 1,173.28 | 0.2% | 16,650,566.31 | 3,978,823.89 | 4.185 | 374,875.56 | 0.2% | 33.04 | WalletCreateTxUseOnlyPresetInputs |
| 28,841,852.53 | 34.67 | 0.4% | 373,458,753.83 | 134,591,208.06 | 2.775 | 68,667,702.77 | 0.4% | 33.26 | WalletCreateTxUsePresetInputsAndCoinSelection |
Footnotes
-
AMD Ryzen 9 7900X 12-Core Processor used in these benchmarks. ↩
There was a problem hiding this comment.
Perf Review ec25de3
Unfortunately, I have not been able to do a code review of this PR yet but I wanted to ensure that I finish the performance review of this change; I will able to do a code review in a week or two.
While benchmarks are reliable enough to give an idea about the performance, I wanted to contextualise the findings of those benchmarks by calling the related RPCs, and also wanted to address the concern I had about additional memory usage.
Motivated by this issue comment from achow, I created a perf test in my regtest using this script by generating a wallet with 7,117 UTXOs among 35,744 Bech32 addresses spanning across 362,621 wallet transactions generated by 7370 blocks. After generating the initial coinbase transactions, I created passthrough transactions that let the unspents flow through them - every such transaction spent 1 unspent and sent 99.5% of the amount to a new wallet address and the remaining amount equally divided among 10 non-wallet addresses. I did this to simulate the scenario where the user is sending funds to different addresses and the rest is sent back to the wallet as change.
Results:
System - MacOS Sequoia, 2.6 GHz 6-Core Intel Core i7, 16GB Memory
Observations:
- As conveyed by the benchmarks as well, there is remarkable improvement in the
getbalancesRPC latency. listunspentRPC also has a reduced latency. ThesendRPC, which relates to theWalletCreateTxUsePresetInputsAndCoinSelectionbenchmark also improved considerably in the second run, though it took slightly more time in the first run.- I expect the last 3 RPCs listed to further improve after #27865 when unspendable TXOs are separated from the spendable ones.
- I didn't expect a lot of memory consumption increase because the
WalletTXOobject contains only references toCWalletTx&CTxOut, and, moreover, them_isminemember will also be removed after #32523. This expectation was met in this perf test as there is not a lot of increase in themaximum resident set sizeparameter of thetimecommand. - The consistent tracking of the
top -pid $(pgrep bitcoind)command after the RPCs execution showed me that roughly a ~20MB increment was there in this perf test compared to master in this wallet having more than 7k unspents in ~360k transactions.
Conclusion:
While the results show data from 2 runs, more runs might form consistency in the metrics delta of loadwallet & listtransactions RPCs. The listunspent & getbalances results already show consistent improvement to me; the memory consumption increase is also not significant, which is good imo. I suppose the changes of this PR furthered by #27865 and #32523 should bring in more improvement.
So, Concept ACK ec25de3.
Edit: Apparently, my test has ~37k transactions and not ~360k. I used the output of listtransactions RPC but realised the difference later when I checked the txcount output of getwalletinfo RPC.
One of the main issues with AvailableCoins is its performance when txs have unrelated outputs, so update the benchmark to check the performance of that.
After adding a wallet descriptor (typically by import), mark all balance caches dirty. This allows transactions that the wallet already knows about that have outputs that are now ISMINE_SPENDABLE after the import to actually be shown in balance calculations. Legacy wallet imports would do this, but importdescriptors did not.
Need to load txs last so that IsMine works.
When loading transactions to the wallet, check the outputs, and keep track of the ones that are IsMine.
Since we track the outputs owned by the wallet with m_txos, we can now calculate the balance of the wallet by iterating m_txos and summing up the amounts of the unspent txos. As ISMINE_USED is not an actual isminetype that we attach to outputs and was just passed into `CachedTxGetAvailableCredit` for convenience, we pull the same determining logic from that function into `GetBalances` in order to preserve existing behavior.
Instead of iterating every transaction and every output stored in wallet when trying to figure out what outputs can be spent, iterate the TXO set which should be a lot smaller.
Instead of searching mapWallet for the preselected inputs, search m_txos. wallet_fundrawtransaction.py spends external inputs and needs the change output to also belong to the test wallet for the oversized tx test.
When a legacy wallet has been migrated to contain descriptors, but before the transactions have been updated to match, we need to recompute the wallet TXOs so that the transaction update will work correctly.
Instead of looking up the previous tx in mapWallet, and then calling IsMine on the specified output, use the TXO set and its cached IsMine value.
These two functions are no longer used as GetBalances now uses the TXO set rather than per-tx cached balances
|
untested reACK 215e599 9eb2c82 removed the unused
$ git range-diff 4b1d48a..ec25de3 f27898c..215e599 1: a39074c52a = 1: e02f2d331c bench: Have AvailableCoins benchmark include a lot of unrelated utxos
2: 8ff188806b = 2: 8222341d4f wallet: MarkDirty after AddWalletDescriptor
3: 0b35048307 = 3: 5cc32ee2a7 test: Test for balance update due to untracked output becoming spendable
4: b379ec2467 ! 4: 0f269bc48c walletdb: Load Txs last
@@ src/wallet/walletdb.cpp: DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
result = std::max(LoadAddressBookRecords(pwallet, *m_batch), result);
- // Load tx records
-- result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result);
+- result = std::max(LoadTxRecords(pwallet, *m_batch, any_unordered), result);
-
// Load SPKMs
result = std::max(LoadActiveSPKMs(pwallet, *m_batch), result);
@@ src/wallet/walletdb.cpp: DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
result = std::max(LoadDecryptionKeys(pwallet, *m_batch), result);
+
+ // Load tx records
-+ result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result);
++ result = std::max(LoadTxRecords(pwallet, *m_batch, any_unordered), result);
} catch (std::runtime_error& e) {
// Exceptions that can be ignored or treated as non-critical are handled by the individual loading functions.
// Any uncaught exceptions will be caught here and treated as critical.
5: 220f2582db = 5: ae0876ec42 wallet: Keep track of transaction outputs owned by the wallet
6: cda977bc72 = 6: ae888c38d0 wallet: Exit IsTrustedTx early if wtx is already in trusted_parents
7: c7a85cc417 = 7: 96e7a89c5e wallet: Recalculate the wallet's txos after any imports
8: 96864062c6 = 8: dde7cbe105 wallet: Change balance calculation to use m_txos
9: 2703d84b7e = 9: c1801b78f1 wallet: Use wallet's TXO set in AvailableCoins
10: 5ab95ac68e = 10: 764016eb22 wallet: Retrieve TXO directly in FetchSelectedInputs
11: 5082dea76f = 11: 17d453cb3a wallet: Recompute wallet TXOs after descriptor migration
12: 56f55f412c = 12: 49675de035 wallet: Have GetDebit use the wallet's TXO set
13: ec25de35b2 = 13: 215e5999e2 wallet: Remove unused CachedTxGet{Available,Immature}Credit
</details> |
| if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) { | ||
| ret.m_mine_immature += credit_mine; | ||
| ret.m_watchonly_immature += credit_watchonly; | ||
| } else if (is_trusted && tx_depth >= min_depth) { | ||
| ret.m_mine_trusted += credit_mine; | ||
| ret.m_watchonly_trusted += credit_watchonly; | ||
| } else if (!is_trusted && wtx.InMempool()) { | ||
| ret.m_mine_untrusted_pending += credit_mine; | ||
| ret.m_watchonly_untrusted_pending += credit_watchonly; |
There was a problem hiding this comment.
I was just rereading the commit "wallet: Change balance calculation to use m_txos" (dde7cbe) and I’m aware that this is not a behavior change, but I am wondering about how the balances are calculated.
As seen, this distinguishes three groups of TXOs:
- "immature"
if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) { - "trusted"
} else if (is_trusted && tx_depth >= min_depth) { - "untrusted_pending"
} else if (!is_trusted && wtx.InMempool()) {
and it made me wonder, whether a change output on an unconfirmed transaction our wallet created would just not count toward any of these balances. We trust our own transactions, so our change output should be is_trusted—and min_depth is 0 by default. Nevermind.
| # Mock the time forward by 1 day so that "now" will exclude the block we just mined | ||
| self.nodes[0].setmocktime(int(time.time()) + 86400) | ||
| # Mine 11 blocks to move the MTP past the block we just mined | ||
| self.generate(self.nodes[0], 11, sync_fun=self.no_op) |
There was a problem hiding this comment.
In 5cc32ee "test: Test for balance update due to untracked output becoming spendable"
very minor nit: it is not clear to me as to why the time needs to be moved forward for this test to be effective, so perhaps a sentence could be added to explain that?
There was a problem hiding this comment.
I agree that this part is pretty confusing, it's because a rescan timestamp of now means rescan all blocks from the MTP of the most recent block:
bitcoin/src/wallet/rpc/backup.cpp
Line 378 in 6e5b67a
bitcoin/src/wallet/rpc/backup.cpp
Line 383 in 6e5b67a
bitcoin/src/wallet/rpc/backup.cpp
Lines 399 to 401 in 6e5b67a
In the test, in order to avoid rescanning the block that contains the transaction of interest when the key is imported below, the MTP is advanced by moving the time forward and mining 11 blocks.
13 participants
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in
mapWallet, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is especially problematic for wallets that have a lot of transactions, or transactions that have a lot of unrelated outputs (as may occur with coinjoins or batched payments).This PR helps to resolve this issue by making the wallet track all of the outputs that belong to it in a new member
m_txos. Note that this includes outputs that may have already been spent. Both balance calculation (GetBalance) and coin selection (AvailableCoins) are updated to iteratem_txos. This is generally faster since it ignores all of the unrelated outputs, and it is not slower as in the worst case of wallets containing only single output transactions, it's exactly the same number of outputs iterated.m_txosis memory only, and it is populated during wallet loading. When each transaction is loaded, all of its outputs are checked to see if it isIsMine, and if so, an entry added tom_txos. When new transactions are received, the same procedure is done.Since imports can change the
IsMinestatus of a transaction (although they can only be "promoted" from watchonly to spendable), all of the import RPCs will be a bit slower as they re-iterate all transactions and all outputs to updatem_txos.Each output in
m_txosis stored in a newWalletTXOclass. This class contains references to the parentCWalletTxand theCTxOutitself. It also caches theIsMinevalue of the txout. This should be safe asIsMineshould not change unless there are imports. This allows us to have additional performance improvements in places that use theseWalletTXOs as they can use the cachedIsMinerather than repeatedly callingIsMinewhich can be expensive.The existing
WalletBalancebenchmark demonstrates the performance improvement that this PR makes. The existingWalletAvailableCoinsbenchmark doesn't as all of the outputs used in that benchmark belong to the test wallet. I've updated that benchmark to have a bunch of unrelated outputs in each transaction so that the difference is demonstrated.This is part of a larger project to have the wallet actually track and store a set of its UTXOs.
Built on #24914 as it requires loading the txs last in order for
m_txosto be built correctly.Benchmarks:
Master:
WalletAvailableCoinsWalletBalanceCleanWalletBalanceDirtyWalletBalanceMineWalletBalanceWatchWalletCreateEncryptedWalletCreatePlainWalletCreateTxUseOnlyPresetInputsWalletCreateTxUsePresetInputsAndCoinSelectionWalletIsMineDescriptorsWalletIsMineMigratedDescriptorsWalletLoadingDescriptorsWalletMigrationPR:
WalletAvailableCoinsWalletBalanceCleanWalletBalanceDirtyWalletBalanceMineWalletBalanceWatchWalletCreateEncryptedWalletCreatePlainWalletCreateTxUseOnlyPresetInputsWalletCreateTxUsePresetInputsAndCoinSelectionWalletIsMineDescriptorsWalletIsMineMigratedDescriptorsWalletLoadingDescriptorsWalletMigration