wallet: Keep track of the wallet’s own transaction outputs in memory #27286

pull achow101 wants to merge 13 commits into bitcoin:master from achow101:wallet-txos changing 12 files +297 −306
  1. achow101 commented at 9:02 pm on March 20, 2023: member

    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 iterate m_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_txos is memory only, and it is populated during wallet loading. When each transaction is loaded, all of its outputs are checked to see if it is IsMine, and if so, an entry added to m_txos. When new transactions are received, the same procedure is done.

    Since imports can change the IsMine status 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 update m_txos.

    Each output in m_txos is stored in a new WalletTXO class. This class contains references to the parent CWalletTx and the CTxOut itself. It also caches the IsMine value of the txout. This should be safe as IsMine should not change unless there are imports. This allows us to have additional performance improvements in places that use these WalletTXOs as they can use the cached IsMine rather than repeatedly calling IsMine which can be expensive.

    The existing WalletBalance benchmark demonstrates the performance improvement that this PR makes. The existing WalletAvailableCoins benchmark 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_txos to be built correctly.


    Benchmarks:

    Master:

    ns/op op/s err% ins/op cyc/op IPC bra/op miss% total benchmark
    34,590,013.00 28.91 0.0% 812,669,269.00 148,360,642.50 5.478 18,356,853.00 0.2% 0.76 WalletAvailableCoins
    3,193.46 313,139.91 0.4% 96,868.06 13,731.82 7.054 26,238.01 0.1% 0.01 WalletBalanceClean
    26,871.18 37,214.59 3.3% 768,179.50 115,544.39 6.648 154,171.09 0.1% 0.01 WalletBalanceDirty
    3,177.30 314,732.47 0.2% 96,868.06 13,646.20 7.099 26,238.01 0.1% 0.01 WalletBalanceMine
    10.73 93,186,952.53 0.1% 157.00 46.14 3.403 36.00 0.0% 0.01 WalletBalanceWatch
    590,497,920.00 1.69 0.1% 12,761,692,005.00 2,536,899,595.00 5.030 129,124,399.00 0.7% 6.50 WalletCreateEncrypted
    182,929,529.00 5.47 0.0% 4,199,271,397.00 785,477,302.00 5.346 75,363,377.00 1.1% 2.01 WalletCreatePlain
    699,337.20 1,429.93 0.7% 18,054,294.00 3,005,072.20 6.008 387,756.60 0.3% 0.04 WalletCreateTxUseOnlyPresetInputs
    32,068,583.80 31.18 0.5% 562,026,110.00 137,457,635.60 4.089 90,667,459.40 0.3% 1.78 WalletCreateTxUsePresetInputsAndCoinSelection
    36.62 27,306,578.40 0.5% 951.00 157.05 6.056 133.00 0.0% 0.01 WalletIsMineDescriptors
    35.00 28,569,989.42 0.7% 937.00 150.33 6.233 129.00 0.0% 0.01 WalletIsMineMigratedDescriptors
    203,284,889.00 4.92 0.0% 4,622,691,895.00 872,875,275.00 5.296 90,345,002.00 1.2% 1.02 WalletLoadingDescriptors
    1,165,766,084.00 0.86 0.0% 24,139,316,211.00 5,005,218,705.00 4.823 2,664,455,775.00 0.1% 1.17 WalletMigration

    PR:

    ns/op op/s err% ins/op cyc/op IPC bra/op miss% total benchmark
    33,975,750.50 29.43 0.1% 794,719,150.50 145,763,550.00 5.452 16,036,630.50 0.2% 0.75 WalletAvailableCoins
    2,442.01 409,498.46 0.2% 60,782.04 10,500.60 5.788 9,492.01 0.3% 0.01 WalletBalanceClean
    2,763.12 361,909.21 0.2% 61,493.05 11,859.48 5.185 9,625.01 0.2% 0.01 WalletBalanceDirty
    2,347.98 425,898.72 0.3% 60,782.04 10,082.73 6.028 9,492.01 0.2% 0.01 WalletBalanceMine
    11.67 85,654,630.36 0.2% 176.00 50.18 3.508 40.00 0.0% 0.01 WalletBalanceWatch
    590,119,519.00 1.69 0.1% 12,754,398,258.00 2,534,998,522.00 5.031 129,078,027.00 0.7% 6.50 WalletCreateEncrypted
    183,124,790.00 5.46 0.1% 4,199,212,926.00 786,323,886.00 5.340 75,354,437.00 1.1% 2.02 WalletCreatePlain
    669,643.00 1,493.33 0.1% 17,213,904.20 2,877,336.40 5.983 394,292.80 0.3% 0.04 WalletCreateTxUseOnlyPresetInputs
    26,205,987.80 38.16 0.8% 365,551,340.80 112,376,905.20 3.253 65,684,276.20 0.4% 1.44 WalletCreateTxUsePresetInputsAndCoinSelection
    34.75 28,778,846.38 0.1% 937.00 149.41 6.271 129.00 0.0% 0.01 WalletIsMineDescriptors
    29.91 33,428,072.85 0.2% 920.00 128.63 7.152 126.00 0.0% 0.01 WalletIsMineMigratedDescriptors
    202,437,985.00 4.94 0.1% 4,626,686,256.00 869,439,274.00 5.321 90,961,305.00 1.1% 1.02 WalletLoadingDescriptors
    1,158,394,152.00 0.86 0.0% 24,143,589,972.00 4,971,946,380.00 4.856 2,665,355,654.00 0.1% 1.16 WalletMigration
  2. DrahtBot commented at 9:02 pm on March 20, 2023: 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/27286.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK davidgumberg
    Concept ACK remyers, jonatack
    Stale ACK ishaanam, murchandamus, w0xlt

    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:

    • #bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
    • #32618 (wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs by achow101)
    • #32523 (wallet: Remove watchonly behavior and isminetypes by achow101)
    • #32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
    • #29062 (Wallet: (Refactor) GetBalance to calculate used balance by BrandonOdiwuor)
    • #27865 (wallet: Track no-longer-spendable TXOs separately 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.

  3. DrahtBot added the label Wallet on Mar 20, 2023
  4. DrahtBot added the label Needs rebase on Apr 3, 2023
  5. achow101 force-pushed on Apr 10, 2023
  6. DrahtBot removed the label Needs rebase on Apr 10, 2023
  7. achow101 force-pushed on Apr 10, 2023
  8. DrahtBot added the label Needs rebase on Apr 11, 2023
  9. achow101 force-pushed on Apr 11, 2023
  10. DrahtBot removed the label Needs rebase on Apr 11, 2023
  11. DrahtBot added the label Needs rebase on Apr 12, 2023
  12. achow101 force-pushed on Apr 12, 2023
  13. DrahtBot removed the label Needs rebase on Apr 12, 2023
  14. DrahtBot added the label Needs rebase on May 1, 2023
  15. achow101 force-pushed on May 1, 2023
  16. DrahtBot removed the label Needs rebase on May 1, 2023
  17. DrahtBot added the label CI failed on May 9, 2023
  18. DrahtBot added the label Needs rebase on May 15, 2023
  19. achow101 force-pushed on May 15, 2023
  20. DrahtBot removed the label Needs rebase on May 15, 2023
  21. DrahtBot removed the label CI failed on May 16, 2023
  22. DrahtBot added the label Needs rebase on May 16, 2023
  23. achow101 force-pushed on May 16, 2023
  24. DrahtBot removed the label Needs rebase on May 16, 2023
  25. DrahtBot added the label Needs rebase on May 18, 2023
  26. achow101 force-pushed on May 19, 2023
  27. DrahtBot removed the label Needs rebase on May 19, 2023
  28. achow101 force-pushed on May 19, 2023
  29. achow101 force-pushed on May 27, 2023
  30. achow101 force-pushed on May 27, 2023
  31. DrahtBot added the label Needs rebase on Jun 2, 2023
  32. achow101 force-pushed on Jun 2, 2023
  33. DrahtBot removed the label Needs rebase on Jun 2, 2023
  34. DrahtBot added the label Needs rebase on Jun 12, 2023
  35. achow101 force-pushed on Jun 12, 2023
  36. DrahtBot removed the label Needs rebase on Jun 12, 2023
  37. DrahtBot added the label Needs rebase on Jun 28, 2023
  38. achow101 force-pushed on Jun 28, 2023
  39. DrahtBot removed the label Needs rebase on Jun 28, 2023
  40. murchandamus commented at 8:13 pm on July 6, 2023: contributor
    Concept ACK
  41. achow101 marked this as ready for review on Jul 10, 2023
  42. in src/wallet/receive.cpp:262 in 68bb1463dc outdated
    318-                ret.m_mine_untrusted_pending += tx_credit_mine;
    319-                ret.m_watchonly_untrusted_pending += tx_credit_watchonly;
    320+        for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
    321+            Assert(MoneyRange(txo.GetTxOut().nValue));
    322+
    323+            const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx(), trusted_parents)};
    


    ishaanam commented at 6:51 pm on July 12, 2023:

    In 68bb1463dc1f995fbfdb75d3acef625bde104275 “wallet: Change balance calculation to use m_txos "

    nit:

    0            const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx())};
    

    & also remove the unused trusted_parents from above. (for GetAddressBalances as well)


    achow101 commented at 6:35 pm on September 15, 2023:
    Done
  43. in src/wallet/receive.cpp:285 in 68bb1463dc outdated
    341+                    ret.m_mine_immature += credit_mine;
    342+                    ret.m_watchonly_immature += credit_watchonly;
    343+                } else if (is_trusted && tx_depth >= min_depth) {
    344+                    ret.m_mine_trusted += credit_mine;
    345+                    ret.m_watchonly_trusted += credit_watchonly;
    346+                } if (!is_trusted && tx_depth == 0 && txo.GetWalletTx().InMempool()) {
    


    ishaanam commented at 5:10 pm on July 13, 2023:

    In 68bb1463dc1f995fbfdb75d3acef625bde104275 " wallet: Change balance calculation to use m_txos "

    nit: checking the tx depth is redundant here because if the transaction is TxStateMempool then GetTxDepthInMainChain will always return 0.

    0                } if (!is_trusted && txo.GetWalletTx().InMempool()) {
    

    achow101 commented at 6:35 pm on September 15, 2023:
    Done
  44. in src/wallet/wallet.h:508 in 3f3246efe2 outdated
    479@@ -480,6 +480,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    480     const std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher>& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; };
    481 
    482     void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    483+    void RefreshWalletTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    


    ishaanam commented at 5:27 pm on July 13, 2023:

    In 3f3246efe299552e450362a03c61c602a168f7de “wallet: Recalculate the wallet’s txos after any imports "

    Does this also need to be called during sethdseed and keypoolrefill.


    achow101 commented at 6:38 pm on September 15, 2023:
    Hmm, good point. The expected usage of these is that only new keys will be generated/added, so it shouldn’t have any effect on the existing transactions in the wallet. However, I suppose they could be used in a way that does result in existing txs having their IsMine changed. I’ve also added this to addmultisigaddress and newkeypool since they could be used in the same way.
  45. in src/wallet/rpc/backup.cpp:634 in 3f3246efe2 outdated
    630@@ -627,6 +631,7 @@ RPCHelpMan importwallet()
    631 
    632             progress++;
    633         }
    634+        pwallet->RefreshWalletTXOs();
    


    ishaanam commented at 5:32 pm on July 13, 2023:

    In https://github.com/bitcoin/bitcoin/commit/3f3246efe299552e450362a03c61c602a168f7de “wallet: Recalculate the wallet’s txos after any imports "

    Is this needed if the wallet will rescan anyways?


    achow101 commented at 6:38 pm on September 15, 2023:
    The wallet may not rescan every transaction in the wallet since it does look at the given key birthdays.
  46. in src/wallet/spend.cpp:337 in 881650ed31 outdated
    297-        for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
    298-            const CTxOut& output = wtx.tx->vout[i];
    299-            const COutPoint outpoint(wtxid, i);
    300+        if (mine == ISMINE_NO) {
    301+            continue;
    302+        }
    


    ishaanam commented at 5:47 pm on July 13, 2023:

    In 881650ed319c6ed74e66ae56c34cdee93125231b " wallet: Use wallet’s TXO set in AvailableCoins "

    Shouldn’t txos in m_txos never be ISMINE_NO?

    0        assert(mine != ISMINE_NO);
    

    achow101 commented at 6:39 pm on September 15, 2023:
    Done
  47. in src/wallet/spend.cpp:410 in 881650ed31 outdated
    390-                is_from_p2sh = true;
    391-            }
    392+            tx_safe_cache[outpoint.hash] = {true, safeTx};
    393+        }
    394+        const auto& [tx_ok, tx_safe] = tx_safe_cache.at(outpoint.hash);
    395+        Assert(tx_ok);
    


    ishaanam commented at 7:09 pm on July 13, 2023:

    In https://github.com/bitcoin/bitcoin/commit/881650ed319c6ed74e66ae56c34cdee93125231b " wallet: Use wallet’s TXO set in AvailableCoins "

    Can’t tx_ok still be false at this point if a transaction that has a depth of 0 and is not in the mempool has two txos that belong to the wallet? Since then when the first txo is evaluated, a {false, false} entry will be created for that transaction hash, and then when the second txo is evaluated, that entry will be used because the mempool check will be skipped?


    achow101 commented at 6:39 pm on September 15, 2023:
    Good point, changed to an if.

    ishaanam commented at 1:38 am on October 16, 2023:
    Looking at this again, I think I mislead you here. In the scenario I described above, tx_ok won’t be false at this point, because if a {false, false} entry has already been created for a txid, then it would not reach this point because it would already get removed here (https://github.com/bitcoin/bitcoin/pull/27286/commits/9315bb7872210dc767dfd7ea01febb13fca5525b#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R328-R329)

    achow101 commented at 10:15 pm on October 16, 2023:
    Indeed. I’ve left the if there anyways as a belt-and-suspenders, and changed it to also Assume.
  48. ishaanam commented at 7:54 pm on July 13, 2023: contributor

    Concept ACK

    Still working on reviewing this, but here are some initial things. Thanks for breaking it up into twelve commits, it makes it easy to review.

  49. ishaanam commented at 10:57 pm on July 20, 2023: contributor
    The commit message for 68bb1463dc1f995fbfdb75d3acef625bde104275 should be updated because now that #27145 has gotten merged, the described issue has already been fixed.
  50. DrahtBot added the label Needs rebase on Sep 6, 2023
  51. achow101 force-pushed on Sep 7, 2023
  52. DrahtBot removed the label Needs rebase on Sep 7, 2023
  53. DrahtBot added the label Needs rebase on Sep 14, 2023
  54. achow101 force-pushed on Sep 14, 2023
  55. DrahtBot removed the label Needs rebase on Sep 14, 2023
  56. DrahtBot added the label CI failed on Sep 15, 2023
  57. remyers commented at 10:01 am on September 15, 2023: contributor

    Concept ACK

    My preliminary testing confirmed that this PR will significantly speed up common operations like fundrawtransaction on wallets with many years of transaction history. A single fundrawtransaction RPC call that took 5 sec on a simulated large wallet now takes 50ms. Services providers with large wallets that need to make many RPC calls per minute will benefit from reduced lock contention on cs_wallet; for example, LSPs using bitcoin core wallets like Eclair.

    Caching the wallet’s tx outputs in memory seems like a straight forward solution as long as the cache of tx outs in m_txos are always updated (or regenerated) if the underlying set of wallet txs in mapWallet or the m_spk_managers change.

  58. remyers commented at 10:50 am on September 15, 2023: contributor

    Exposing RefreshWalletTXOs() as a public method puts the responsibility of knowing when it should be called outside of the CWallet scope. What do you think about making it private and clearing m_txos (or adding a new flag) to trigger RefreshWalletTXOs() at query time?

    This alternate approach would require GetTXO(const COutPoint& outpoint) and GetTXOs() to first call RefreshWalletTXOs() if m_txos is empty. There are only a few methods that change m_spk_managers that would need to clear m_txos to trigger a one time RefreshWalletTXOs():

    • ImportPrivKeys
    • ImportPublicKeys
    • ImportScriptPubKeys
    • AddScriptPubKeyMan

    It’s also only a small set of places where keys are imported in backup.cpp, so the question of leaving RefreshWalletTXOs() public might just be a matter of which approach would be less foot-gunny in the future and if there are disadvantages to delaying the regeneration of txos.

    I think you can safely make RefreshWalletTxTXOs(const CWalletTx& wtx) a private method now though because it is only called by wallet methods that add a wtx to mapWallet like LoadToWallet and AddToWallet.

  59. achow101 commented at 4:40 pm on September 15, 2023: member

    Exposing RefreshWalletTXOs() as a public method puts the responsibility of knowing when it should be called outside of the CWallet scope. What do you think about making it private and clearing m_txos (or adding a new flag) to trigger RefreshWalletTXOs() at query time?

    I’m not sure that’s meaningfully different. The caller still needs to know when to clear m_txos in order to trigger a future RefreshWalletTXOs(). The way it is now also provides a better UX as the slow part of refreshing the TXOs occurs during slow import operations rather than the first time those TXOs are needed after the import.

  60. achow101 force-pushed on Sep 15, 2023
  61. DrahtBot removed the label CI failed on Sep 15, 2023
  62. remyers commented at 8:38 am on September 16, 2023: contributor

    I’m not sure that’s meaningfully different. The caller still needs to know when to clear m_txos in order to trigger a future RefreshWalletTXOs(). The way it is now also provides a better UX as the slow part of refreshing the TXOs occurs during slow import operations rather than the first time those TXOs are needed after the import.

    Just to clarify, I mean that a reviewer, but not method caller, would need to make sure every method in CWallet that invalidates m_txos also clears it. I agree with your point though that forcing the refresh at import time makes sense. I also notice that there are a number of places where a ScriptPubKeyMan gets added or updated outside of CWallet, so I understand now why it’s not really possible to make RefreshWalletTXOs() something internal to CWallet.

  63. in test/functional/wallet_abandonconflict.py:235 in fdf3ab0579 outdated
    230@@ -231,7 +231,8 @@ def run_test(self):
    231         assert_equal(newbalance, balance + Decimal("20"))
    232         balance = newbalance
    233 
    234-        # Invalidate the block with the double spend. B & C's 10 BTC outputs should no longer be available
    235+        # Invalidate the block with the double spend and both B and C's 10 BTC output should no longer be available
    236+        # AB1 and ABC2's outputs are also not available since neither of those txs are in the mempool
    


    ishaanam commented at 0:44 am on October 16, 2023:

    In fdf3ab0579be6d6f2be31907e3451a830e25291e “wallet: Change balance calculation to use m_txos “:

    nit: This no longer needs to be changed in this commit.


    achow101 commented at 10:14 pm on October 16, 2023:
    Done
  64. in src/wallet/wallet.h:508 in d7b3603822 outdated
    503@@ -504,6 +504,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    504     const std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher>& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; };
    505 
    506     void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    507+    void RefreshWalletTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    


    ishaanam commented at 0:54 am on October 16, 2023:

    In d7b3603822663fbc713970bf37dc010904aaa583 “wallet: Recalculate the wallet’s txos after any imports “:

    RefreshWalletTXOs looks similar to RefreshWalletTxTXOs and it is not immediately clear what the distinction is without actually reading the functions. I would suggest s/RefreshWalletTXOs/RefreshAllTXOs (putting “Wallet” seems redundant here because the method belongs to wallet anyways, and GetTXOs doesn’t mention “Wallet” in the function name).


    achow101 commented at 10:14 pm on October 16, 2023:
    Renamed as suggested.
  65. ishaanam commented at 12:06 pm on October 16, 2023: contributor

    ACK 9199832d4440a875a150aa3ba3bc1bc201d3fd72

    I’ve reviewed everything but the tests and benchmarks.

  66. DrahtBot requested review from murchandamus on Oct 16, 2023
  67. DrahtBot added the label Needs rebase on Oct 16, 2023
  68. achow101 force-pushed on Oct 16, 2023
  69. achow101 force-pushed on Oct 16, 2023
  70. DrahtBot added the label CI failed on Oct 16, 2023
  71. DrahtBot removed the label Needs rebase on Oct 16, 2023
  72. achow101 force-pushed on Oct 23, 2023
  73. in src/bench/wallet_create_tx.cpp:58 in 9292951f0c outdated
    54     coinbase_tx.vout[1].nValue = 1 * COIN;
    55+
    56+    // Fill the coinbase with outputs that don't belong to the wallet in order to benchmark
    57+    // AvailableCoins' behavior with unnecessary TXOs
    58+    for (int i = 0; i < 50; ++i) {
    59+        coinbase_tx.vout.push_back(CTxOut(1 * COIN / 50, CScript(OP_TRUE)));
    


    maflcko commented at 7:51 am on October 24, 2023:
    0bench/wallet_create_tx.cpp:58:26: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
    1   58 |         coinbase_tx.vout.push_back(CTxOut(1 * COIN / 50, CScript(OP_TRUE)));
    2      |                          ^~~~~~~~~~~~~~~~~                               ~
    3      |                          emplace_back(
    

    achow101 commented at 3:41 pm on October 24, 2023:
    Done
  74. achow101 force-pushed on Oct 24, 2023
  75. DrahtBot removed the label CI failed on Oct 25, 2023
  76. DrahtBot added the label Needs rebase on Oct 29, 2023
  77. in src/wallet/receive.cpp:216 in b2790645e9 outdated
    255@@ -256,6 +256,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
    256 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)
    257 {
    258     AssertLockHeld(wallet.cs_wallet);
    259+
    260+    // This wtx is already trusted
    261+    if (trusted_parents.count(wtx.GetHash()) > 0) return true;
    


    murchandamus commented at 8:39 pm on October 30, 2023:
    How would that ever happen? Wouldn’t we only have seen a trusted_parent before if we received two outputs from the same transaction?

    achow101 commented at 11:21 pm on October 30, 2023:
    Yes, and that does happen, e.g. send to self transactions. The tests also do this a lot.

    murchandamus commented at 7:09 pm on March 11, 2024:

    In 0283525d64a28c79f4588ef0a71bd280b2fc7768 (wallet: Exit IsTrustedTx early if wtx is already in trusted_parents):

    0    if (trusted_parents.contains(wtx.GetHash())) return true;
    

    achow101 commented at 2:53 pm on March 12, 2024:
    Done
  78. in src/wallet/receive.cpp:313 in 73bff03d1f outdated
    358 
    359-            if (wallet.IsTxImmatureCoinBase(wtx))
    360-                continue;
    361+            CTxDestination addr;
    362+            Assume(wallet.IsMine(txo.GetTxOut()));
    363+            if(!ExtractDestination(txo.GetTxOut().scriptPubKey, addr)) continue;
    


    murchandamus commented at 8:49 pm on October 30, 2023:
    Wouldn’t this mean that we don’t count P2PK outputs to our balance in combination with the changes to “Destinations” for Silent Payments?

    achow101 commented at 9:33 pm on October 30, 2023:
    Yes, P2PK is not an address type.
  79. in src/wallet/wallet.cpp:1572 in c91b1de218 outdated
    1554@@ -1555,16 +1555,10 @@ void CWallet::BlockUntilSyncedToCurrentChain() const {
    1555 // and a not-"is mine" (according to the filter) input.
    1556 CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const
    


    murchandamus commented at 8:58 pm on October 30, 2023:

    What does GetDebit do? The explanation in wallet.h is also self-referential:

    0 769     /**
    1 770      * Returns amount of debit if the input matches the
    2 771      * filter, otherwise returns 0
    3 772      */
    

    Does this just get the amount for an input that matches the filter criteria?


    achow101 commented at 9:36 pm on October 30, 2023:
    “Debit” refers to the amount leaving the wallet, assuming the UTXO spent by the input matches the filter. “Debit” operates on a transaction’s inputs, while “credit” operates on the outputs.

    yancyribbens commented at 6:30 pm on March 10, 2025:
    Is this documented anywhere? This is a good question and I would imagine others will have the same in the future.

    murchandamus commented at 5:12 pm on May 23, 2025:
    It would probably be best documented by updating the description of the GetDebit function whenever someone changes code nearby.
  80. achow101 force-pushed on Nov 13, 2023
  81. DrahtBot removed the label Needs rebase on Nov 13, 2023
  82. DrahtBot added the label Needs rebase on Nov 24, 2023
  83. achow101 force-pushed on Nov 28, 2023
  84. DrahtBot removed the label Needs rebase on Nov 28, 2023
  85. DrahtBot added the label Needs rebase on Dec 8, 2023
  86. achow101 force-pushed on Dec 8, 2023
  87. DrahtBot removed the label Needs rebase on Dec 8, 2023
  88. DrahtBot added the label Needs rebase on Dec 11, 2023
  89. achow101 force-pushed on Dec 11, 2023
  90. DrahtBot removed the label Needs rebase on Dec 11, 2023
  91. DrahtBot added the label Needs rebase on Dec 12, 2023
  92. achow101 force-pushed on Dec 19, 2023
  93. DrahtBot removed the label Needs rebase on Dec 19, 2023
  94. in src/wallet/spend.cpp:347 in 91e4f19f66 outdated
    366-        // accepted.
    367-        if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) {
    368-            safeTx = false;
    369-        }
    370+        if (wallet.IsSpent(outpoint))
    371+            continue;
    


    murchandamus commented at 7:32 pm on December 28, 2023:

    I have the impression that m_txos is also used to look up information on old transactions and we therefore want to keep all transaction outputs the wallet ever had.

    Would it perhaps make sense to also have a reduced set that only keeps the potentially unspent TXOs? I could see a wallet that participates in a few dozen transactions every day have magnitudes more TXOs than UTXOs. So perhaps the work the wallet has to perform every time it builds a transaction could be shortcut by storing e.g. m_is_mature_spent on WalletTXO which could be set to true when the transaction spending an output has 100 confirmations, and then either separately keeping a list of all TXOs that are not m_is_mature_spent or at least iterate over them more quickly directly on the basis of that member, without needing to have complex reorg handling to keep this collection up to date.


    murchandamus commented at 7:39 pm on December 28, 2023:
  95. murchandamus commented at 7:38 pm on December 28, 2023: contributor

    ACK 0c0022311a0cffc4c03b3763c9b60f93ede13687

    While I am pretty sure that the reorg logic should handle any transactions getting added or removed from the wallet, it might be useful to add a test that the m_txos set would be correct after a reorg, if that’s not obvious.

  96. DrahtBot requested review from ishaanam on Dec 28, 2023
  97. in src/wallet/receive.cpp:284 in 0c0022311a outdated
    291+                    ret.m_mine_immature += credit_mine;
    292+                    ret.m_watchonly_immature += credit_watchonly;
    293+                } else if (is_trusted && tx_depth >= min_depth) {
    294+                    ret.m_mine_trusted += credit_mine;
    295+                    ret.m_watchonly_trusted += credit_watchonly;
    296+                } if (!is_trusted && txo.GetWalletTx().InMempool()) {
    


    aureleoules commented at 5:02 am on January 3, 2024:

    Seems like a hazard

    0                } else if (!is_trusted && txo.GetWalletTx().InMempool()) {
    

    achow101 commented at 9:05 pm on January 4, 2024:
    Good catch, fixed.
  98. in src/wallet/spend.cpp:357 in 0c0022311a outdated
    405+            continue;
    406 
    407-            // Skip manually selected coins (the caller can fetch them directly)
    408-            if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
    409-                continue;
    410+        if (tx_safe_cache.count(outpoint.hash) == 0 ) {
    


    aureleoules commented at 5:25 am on January 3, 2024:

    nit: Could use contains (and other places too)

    0        if (!tx_safe_cache.contains(outpoint.hash)) {
    

    achow101 commented at 9:05 pm on January 4, 2024:
    Done
  99. achow101 force-pushed on Jan 4, 2024
  100. DrahtBot added the label CI failed on Jan 13, 2024
  101. murchandamus commented at 3:26 pm on January 24, 2024: contributor
    ReACK https://github.com/bitcoin/bitcoin/commit/39f0ab3e126704b30f7ed2b86690f60b631d2ec4 compared to my prior review, there are only the changes mentioned in response to @aureleoules.
  102. DrahtBot added the label Needs rebase on Feb 3, 2024
  103. achow101 force-pushed on Feb 8, 2024
  104. DrahtBot removed the label Needs rebase on Feb 8, 2024
  105. DrahtBot removed the label CI failed on Feb 9, 2024
  106. DrahtBot added the label Needs rebase on Feb 20, 2024
  107. achow101 force-pushed on Feb 20, 2024
  108. DrahtBot removed the label Needs rebase on Feb 20, 2024
  109. in src/wallet/wallet.cpp:1146 in 20cec0c84b outdated
    1142@@ -1143,6 +1143,9 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
    1143     // Break debit/credit balance caches:
    1144     wtx.MarkDirty();
    1145 
    1146+    // Get the outputs that belong to the wallet
    


    murchandamus commented at 7:00 pm on March 11, 2024:

    In 20cec0c84bbe783ddfc0c9b6c3ce1cb45f93385e (wallet: Keep track of transaction outputs owned by the wallet):

    0    // Cache the outputs that belong to the wallet on the wallet’s transactions
    

    achow101 commented at 2:52 pm on March 12, 2024:

    Changed to “cache”.

    “on the wallet’s transactions” does not make the comment a valid English sentence, so I’m not sure what meaning you were trying to get across.


    murchandamus commented at 5:33 pm on March 20, 2024:
    I think I wanted to express that this function operates only on the transactions that the wallet tracks already.
  110. in src/wallet/wallet.cpp:4489 in 20cec0c84b outdated
    4484+        isminetype ismine = IsMine(txout);
    4485+        if (ismine == ISMINE_NO) {
    4486+            continue;
    4487+        }
    4488+        COutPoint outpoint(wtx.GetHash(), i);
    4489+        if (m_txos.count(outpoint) > 0) {
    


    murchandamus commented at 7:03 pm on March 11, 2024:

    In 20cec0c84bbe783ddfc0c9b6c3ce1cb45f93385e (wallet: Keep track of transaction outputs owned by the wallet):

    0        if (m_txos.contains(outpoint)) {
    

    achow101 commented at 2:52 pm on March 12, 2024:
    Done
  111. in src/wallet/wallet.h:428 in 20cec0c84b outdated
    424@@ -425,6 +425,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    425     //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms
    426     std::unordered_map<CScript, std::vector<ScriptPubKeyMan*>, SaltedSipHasher> m_cached_spks;
    427 
    428+    //! Set of transaction outputs owned by this wallet
    


    murchandamus commented at 7:05 pm on March 11, 2024:

    In 20cec0c84bbe783ddfc0c9b6c3ce1cb45f93385e (wallet: Keep track of transaction outputs owned by the wallet):

    0    //! Set of spent and unspent transaction outputs owned by this wallet
    

    achow101 commented at 2:53 pm on March 12, 2024:
    Done
  112. in src/wallet/receive.cpp:255 in b217f576a5 outdated
    297@@ -297,27 +298,40 @@ bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx)
    298 Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse)
    299 {
    300     Balance ret;
    301-    isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
    302+    bool allow_used_addresses = !avoid_reuse || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
    


    murchandamus commented at 7:37 pm on March 11, 2024:

    In b217f576a5f284ac7261986ee6ec9334bbbdbbc6 (wallet: Change balance calculation to use m_txos):

    I dug into this line, because I was wondering whether this line changed the behavior, but found that in src/wallet/receive.cpp in CachedTxGetAvailableCredit(…) the reuse_filter was used to derive the same resulting allow_used_addresses on basis of the filter and WALLET_FLAG_AVOID_REUSE. So, I’m thinking now that this change is not a change in behavior. It would perhaps help future reviewers if the commit message mentioned why this change is valid and doesn’t change the behavior.


    achow101 commented at 2:53 pm on March 12, 2024:
    Mentioned in the commit message.
  113. in src/wallet/wallet.cpp:4480 in 20cec0c84b outdated
    4488+        COutPoint outpoint(wtx.GetHash(), i);
    4489+        if (m_txos.count(outpoint) > 0) {
    4490+            m_txos.at(outpoint).SetIsMine(ismine);
    4491+        } else {
    4492+            m_txos.emplace(outpoint, WalletTXO{wtx, txout, ismine});
    4493+        }
    


    murchandamus commented at 7:47 pm on March 11, 2024:

    In 20cec0c84bbe783ddfc0c9b6c3ce1cb45f93385e (wallet: Keep track of transaction outputs owned by the wallet):

    A few commits later, you use wallet.IsSpent(outpoint) to check whether a TXO is still available for spending. Why can’t we just do store the outcome of wallet.IsSpent(outpoint) here as well?


    achow101 commented at 2:27 pm on March 12, 2024:

    Outputs don’t always stay spent. Replacements, reorgs, and eviction could all make a transaction no longer be “IsSpent”, so checking that here would require a lot of other structural changes to ensure that it is correct.

    Storing the value of IsSpent could be an alternative to #27865, however I would still leave that for a followup. Any change that tries to actually have a proper UTXO set for the wallet can have unexpected side effects and/or things we didn’t notice so I want to keep that separate for now. This PR is to setup for that by making the wallet operate on a TXO set.

  114. in src/wallet/receive.cpp:265 in b217f576a5 outdated
    321+            Assert(MoneyRange(txo.GetTxOut().nValue));
    322+
    323+            const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx())};
    324+            const int tx_depth{wallet.GetTxDepthInMainChain(txo.GetWalletTx())};
    325+
    326+            if (!wallet.IsSpent(outpoint) && (allow_used_addresses || !wallet.IsSpentKey(txo.GetTxOut().scriptPubKey))) {
    


    murchandamus commented at 7:51 pm on March 11, 2024:

    In b217f576a5f284ac7261986ee6ec9334bbbdbbc6 (wallet: Change balance calculation to use m_txos):

    Instead of wallet.IsSpentKey(…) couldn’t you just check here if txo.GetIsMine() has the ISMINE_USED bit set?


    achow101 commented at 2:37 pm on March 12, 2024:
    ISMINE_USED is not actually an isminetype that is returned by IsMine, so it will not be set in the txo.GetIsMine()’s result. It’s really just a hack for it to be an isminetype so it can be passed as part of various ismine filters, but these always check that separately from the actual IsMine result.
  115. in src/wallet/receive.cpp:275 in b217f576a5 outdated
    331+                    credit_mine = txo.GetTxOut().nValue;
    332+                } else if (txo.GetIsMine() == ISMINE_WATCH_ONLY) {
    333+                    credit_watchonly = txo.GetTxOut().nValue;
    334+                } else {
    335+                    // We shouldn't see any other isminetypes
    336+                    Assume(false);
    


    murchandamus commented at 7:54 pm on March 11, 2024:

    In b217f576a5f284ac7261986ee6ec9334bbbdbbc6 (wallet: Change balance calculation to use m_txos):

    I was wondering if we could see ISMINE_SPENDABLE | ISMINE_USED or ISMINE_WATCH_ONLY | ISMINE_USED here.


    achow101 commented at 2:37 pm on March 12, 2024:
    No. ISMINE_USED is never actually returned from IsMine.
  116. in src/wallet/receive.cpp:278 in b217f576a5 outdated
    334+                } else {
    335+                    // We shouldn't see any other isminetypes
    336+                    Assume(false);
    337+                }
    338+
    339+                // Set the amounts in the return object
    


    murchandamus commented at 8:13 pm on March 11, 2024:

    In b217f576a5f284ac7261986ee6ec9334bbbdbbc6 (wallet: Change balance calculation to use m_txos):

    It seems to me that the below should be matching replacements here, but previously every TXO in this loop got added to …_untrusted_pending, and now only TXOs of unconfirmed untrusted transactions, and immature coinbase transactions get added to …_untrusted_pending. Doesn’t this commit change the balances we would be returning?

     0- if (is_trusted && tx_depth >= min_depth) {
     1-     ret.m_mine_trusted += tx_credit_mine;
     2-     ret.m_watchonly_trusted += tx_credit_watchonly;
     3- if (!is_trusted && tx_depth == 0 && wtx.InMempool()) {
     4-     ret.m_mine_untrusted_pending += tx_credit_mine;
     5-     ret.m_watchonly_untrusted_pending += tx_credit_watchonly;
     6- }
     7- ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE);
     8- ret.m_watchonly_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_WATCH_ONLY);
     9+ // Set the amounts in the return object
    10+ if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && wallet.IsTxInMainChain(txo.GetWalletTx())) {
    11+     ret.m_mine_immature += credit_mine;
    12+     ret.m_watchonly_immature += credit_watchonly;
    13+ } else if (is_trusted && tx_depth >= min_depth) {
    14+     ret.m_mine_trusted += credit_mine;
    15+     ret.m_watchonly_trusted += credit_watchonly;
    16+ } else if (!is_trusted && txo.GetWalletTx().InMempool()) {
    17+     ret.m_mine_untrusted_pending += credit_mine;
    18+     ret.m_watchonly_untrusted_pending += credit_watchonly;
    19+ }
    

    achow101 commented at 2:44 pm on March 12, 2024:
    Can you be more specific as to what kind of transaction you think would result in a different balance? Nothing in this PR should change behavior.

    murchandamus commented at 5:11 pm on May 23, 2025:
    I think I just didn’t fully understand what was happening here then.
  117. in test/functional/wallet_balance.py:314 in 900f95cd56 outdated
    396+        assert_equal(balances["mine"]["trusted"], amount)
    397+
    398+        # Don't rescan to make sure that the import updates the wallet txos
    399+        wallet.importprivkey(privkey=import_key2.privkey, rescan=False)
    400+        balances = wallet.getbalances()
    401+        assert_equal(balances["mine"]["trusted"], amount * 2)
    


    murchandamus commented at 8:20 pm on March 11, 2024:

    In 900f95cd56951f5e0f0d0f3a93c09d1fb0bc5787 (test: Test for balance update due to watchonly becoming spendable):

    Do I understand right that discovering this TXO without a rescan only works because the transaction was already in our mapWallet?


    achow101 commented at 2:45 pm on March 12, 2024:
    That is correct.
  118. in src/wallet/spend.cpp:323 in c88832eca5 outdated
    319@@ -320,137 +320,145 @@ CoinsResult AvailableCoins(const CWallet& wallet,
    320     std::vector<COutPoint> outpoints;
    321 
    322     std::set<uint256> trusted_parents;
    323-    for (const auto& entry : wallet.mapWallet)
    324-    {
    325-        const uint256& txid = entry.first;
    326-        const CWalletTx& wtx = entry.second;
    327+    // Cache for whether each tx passes the tx checks (first bool), and whether the transaction is "safe" (second bool)
    


    murchandamus commented at 8:29 pm on March 11, 2024:

    In c88832eca5df0a391a4a68c73b6fc123f9af2f4b (wallet: Use wallet’s TXO set in AvailableCoins):

    What are the “tx checks” that are being passed? It seems to me me that it refers to TXs either having been sent by ourselves that are not involved in an RBF process or TXs with sufficient confirmations that we rely on them. Either way, it would be nice if this comment were a bit more elaborate than just mentioning “tx checks”. Also, the relationship to safe_tx could be made a bit clearer in this code block.

    Perhaps something along the lines of:

    0    // Cache for each transaction whether we consider its outputs acceptable for spending (first bool), and whether the transaction is considered "safe" (second bool).
    1    // A transaction’s outputs are considered acceptable for spending when <elaborate>
    2    // A transaction is considered safe when <elaborate>
    

    Alternatively or additionally if the CachedTxIsTrusted(…) functions had a bit more documentation in either src/wallet/receive.h or src/wallet/receive.cpp.


    achow101 commented at 2:51 pm on March 12, 2024:

    The checks are already explained in detail below where they are actually checked. “tx checks” just refers to the checks that occur per tx rather than per output. I don’t think it is useful to duplicate the explanations here as well.

    I’ve added a comment that should make it easier to find where the tx checks are performed so you can read the comments that already exist.

  119. achow101 force-pushed on Mar 12, 2024
  120. DrahtBot added the label CI failed on Mar 14, 2024
  121. DrahtBot removed the label CI failed on Mar 19, 2024
  122. DrahtBot added the label Needs rebase on Mar 27, 2024
  123. achow101 force-pushed on Mar 29, 2024
  124. achow101 force-pushed on Mar 29, 2024
  125. DrahtBot removed the label Needs rebase on Mar 29, 2024
  126. achow101 force-pushed on Jun 6, 2024
  127. DrahtBot added the label CI failed on Jul 5, 2024
  128. achow101 force-pushed on Jul 9, 2024
  129. achow101 commented at 11:37 pm on July 9, 2024: member
    Rebased for silent merge conflict.
  130. DrahtBot removed the label CI failed on Jul 10, 2024
  131. DrahtBot added the label Needs rebase on Jul 11, 2024
  132. achow101 force-pushed on Jul 22, 2024
  133. DrahtBot removed the label Needs rebase on Jul 22, 2024
  134. in src/wallet/wallet.h:513 in d6f67e97b7 outdated
    509@@ -507,6 +510,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    510 
    511     std::set<uint256> GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    512 
    513+    void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    


    murchandamus commented at 2:16 pm on August 7, 2024:

    In d6f67e97b7ec0b5fdc39e5ad52e0c03dd0e03fd1 wallet: Keep track of transaction outputs owned by the wallet:

    It would have helped me if this function had some documentation. Also, “Wallet transaction transaction outputs” feels a bit redundant, and the name RefreshWalletTxTXOs(…) is extremely similar to RefreshAllTXOs() that gets introduced in the later commit “wallet: Recalculate the wallet’s txos after any imports”. If you have to touch this again, perhaps consider calling this function CacheWalletTXOs?

    0    /** Processes one wallet transaction and caches outputs that belong to our wallet. */
    1    void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    

    achow101 commented at 12:14 pm on October 16, 2024:
    Done
  135. in src/wallet/wallet.h:522 in 9d3762913b outdated
    512@@ -513,6 +513,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    513     const std::unordered_map<COutPoint, WalletTXO, SaltedOutpointHasher>& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; };
    514 
    515     void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    516+    void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    


    murchandamus commented at 5:30 pm on August 7, 2024:

    In 9d3762913ba02d1f6cd3b42889c2c3b31f03e90c “wallet: Recalculate the wallet’s txos after any imports”:

    Perhaps add a documentation comment here along the lines of:

    0    /** Inspects every wallet transaction and caches all outputs that belong to our wallet. */
    1    void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    

    achow101 commented at 12:14 pm on October 16, 2024:
    Done
  136. murchandamus commented at 6:06 pm on August 8, 2024: contributor
    ReACK 3b6ac646ffea930ae7d8c5d93acaba3587cc9dcd
  137. DrahtBot added the label Needs rebase on Aug 27, 2024
  138. achow101 force-pushed on Aug 29, 2024
  139. DrahtBot removed the label Needs rebase on Aug 29, 2024
  140. achow101 requested review from murchandamus on Oct 15, 2024
  141. achow101 force-pushed on Oct 16, 2024
  142. DrahtBot added the label Needs rebase on Oct 24, 2024
  143. achow101 force-pushed on Oct 24, 2024
  144. DrahtBot removed the label Needs rebase on Oct 24, 2024
  145. DrahtBot added the label CI failed on Oct 25, 2024
  146. DrahtBot removed the label CI failed on Oct 25, 2024
  147. jonatack commented at 12:33 pm on November 26, 2024: member
    Concept ACK, if this patchset can meaningfully improve performance and reliably refresh wallet utxos to keep the in-memory cache in sync.
  148. DrahtBot requested review from jonatack on Jan 24, 2025
  149. murchandamus commented at 7:48 pm on January 24, 2025: contributor
    ReACK 48545bdb3b9f59a8b78640dbf20afff6e2663ad7
  150. bitcoin deleted a comment on Feb 23, 2025
  151. bitcoin deleted a comment on Feb 23, 2025
  152. jonatack commented at 10:16 pm on March 7, 2025: member
    I plan to review this soon.
  153. in src/wallet/wallet.cpp:4464 in 48545bdb3b outdated
    4626@@ -4621,4 +4627,40 @@ std::optional<CKey> CWallet::GetKey(const CKeyID& keyid) const
    4627     }
    4628     return std::nullopt;
    4629 }
    4630+
    4631+void CWallet::RefreshSingleTxTXOs(const CWalletTx& wtx)
    


    yancyribbens commented at 6:32 pm on March 10, 2025:
    This method name looks funky. SingleTxTXO is short for single transaction transaction output?

    achow101 commented at 1:04 am on April 10, 2025:
    Yes. It is a single transaction’s transaction outputs.

    murchandamus commented at 7:41 pm on May 9, 2025:
    In wallet: Keep track of transaction outputs owned by the wallet (e82763903b5e0723abf09cb46874953c7b523d24): I’m also not excited about “TxTXOs”. RefreshTXOsFromSingleTx(…) would be just slightly longer and easier to parse. Alternatively CacheOwnTXOsFromTx(…)?

    davidgumberg commented at 10:37 pm on May 14, 2025:

    millinit: Maybe the function signature is sufficient to communicate that it’s just for a single TX?

    0void CWallet::RefreshTXOs(const CWalletTx& wtx)
    

    achow101 commented at 8:43 pm on May 15, 2025:
    Renamed to RefreshTXOsFromTx.
  154. w0xlt commented at 5:06 am on April 2, 2025: contributor

    This PR also simplifies the GetBalance and AvailableCoins methods. The introduction of the GetTXO() method also makes the wallet interface clearer. As far as I could verify, the m_txo in-memory cache is reliably kept in sync.

    ACK https://github.com/bitcoin/bitcoin/pull/27286/commits/48545bdb3b9f59a8b78640dbf20afff6e2663ad7

  155. rkrux commented at 5:26 am on April 2, 2025: contributor

    I plan to review this soon.

    Same, I intend to review it this month.

  156. DrahtBot added the label CI failed on Apr 3, 2025
  157. DrahtBot removed the label CI failed on Apr 4, 2025
  158. DrahtBot added the label Needs rebase on Apr 25, 2025
  159. achow101 force-pushed on Apr 25, 2025
  160. DrahtBot removed the label Needs rebase on Apr 25, 2025
  161. DrahtBot requested review from murchandamus on Apr 30, 2025
  162. in src/wallet/wallet.h:518 in 93563ecef5 outdated
    513@@ -514,6 +514,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
    514 
    515     /** Cache outputs that belong to the wallet from a single transaction */
    516     void RefreshSingleTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    517+    /** Cache outputs that belong to the wallt for all tranasctions in the wallet */
    


    maflcko commented at 5:23 pm on May 4, 2025:
    nit in 93563ecef5e1be1522d4784b0092737487156bab: wallt → wallet

    maflcko commented at 5:24 pm on May 4, 2025:
    tranasctions → transactions

    achow101 commented at 7:01 pm on May 7, 2025:
    Fixed
  163. DrahtBot added the label Needs rebase on May 7, 2025
  164. achow101 force-pushed on May 7, 2025
  165. DrahtBot removed the label Needs rebase on May 7, 2025
  166. achow101 force-pushed on May 7, 2025
  167. in src/wallet/receive.cpp:217 in fe545f997d outdated
    256@@ -257,6 +257,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
    257 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)
    258 {
    259     AssertLockHeld(wallet.cs_wallet);
    260+
    261+    // This wtx is already trusted
    262+    if (trusted_parents.contains(wtx.GetHash())) return true;
    


    w0xlt commented at 6:30 am on May 8, 2025:
    Is this optimization directly related to this PR, or did you take the opportunity to include it here?

    achow101 commented at 6:02 pm on May 8, 2025:
    I believe it’s necessary so that repeated calls to CachedTxIsTrusted that would now occur once for each TXO can be skipped early.
  168. in test/functional/wallet_balance.py:293 in fb8644ca5f outdated
    286@@ -285,5 +287,30 @@ def test_balances(*, fee_node_1=0):
    287         assert_equal(tx_info['lastprocessedblock']['height'], prev_height)
    288         assert_equal(tx_info['lastprocessedblock']['hash'], prev_hash)
    289 
    290+        self.log.info("Test that the balance is updated by an import that makes an untracked output in an existing tx \"mine\"")
    291+        default = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
    292+        self.nodes[0].createwallet("importupdate")
    


    w0xlt commented at 5:00 pm on May 8, 2025:
    Is this wallet watch-only (per commit description) ?

    achow101 commented at 6:05 pm on May 8, 2025:
    It is not. That part of the test no longer exists after #28710. I’ve updated the commit message.
  169. achow101 force-pushed on May 8, 2025
  170. in src/wallet/transaction.h:385 in e82763903b outdated
    380+public:
    381+    WalletTXO(const CWalletTx& wtx, const CTxOut& output, const isminetype ismine)
    382+    : m_wtx(wtx),
    383+    m_output(output),
    384+    m_ismine(ismine)
    385+    {}
    


    davidgumberg commented at 8:18 pm on May 14, 2025:

    Maybe belt and suspenders check that the output belongs to the tx:

    0    {
    1        Assume(std::ranges::find(wtx.tx->vout, output) != wtx.tx->vout.end());
    2    }
    

    achow101 commented at 9:14 pm on May 15, 2025:
    Done as suggested
  171. in src/wallet/wallet.cpp:4480 in e82763903b outdated
    4475+        COutPoint outpoint(wtx.GetHash(), i);
    4476+        if (m_txos.contains(outpoint)) {
    4477+            m_txos.at(outpoint).SetIsMine(ismine);
    4478+        } else {
    4479+            m_txos.emplace(outpoint, WalletTXO{wtx, txout, ismine});
    4480+        }
    


    davidgumberg commented at 11:03 pm on May 14, 2025:

    style nit:

    Could be made a bit shorter with:

    0        auto [it, inserted] = m_txos.emplace(COutPoint{wtx.GetHash(), i}, WalletTXO{wtx, txout, ismine});
    1        if (!inserted) {
    2            it->second.SetIsMine(ismine);
    3        }
    

    But maybe as-is is more legible.


    achow101 commented at 9:15 pm on May 15, 2025:
    As-is is more legible, and this area of code changes significantly in #27865 where having it like it is now makes those changes easier to read.

    davidgumberg commented at 10:03 pm on May 15, 2025:
    Agreed, makes sense.
  172. in src/wallet/rpc/backup.cpp:402 in 850de8343e outdated
    400@@ -401,6 +401,7 @@ RPCHelpMan importdescriptors()
    401             }
    402         }
    403         pwallet->ConnectScriptPubKeyManNotifiers();
    404+        pwallet->RefreshAllTXOs();
    


    murchandamus commented at 0:03 am on May 15, 2025:
    In “wallet: Recalculate the wallet’s txos after any imports” (850de8343e36a17a5c875b1be974153bf06efaca): I’m surprised that the prior commit did not have failing tests if the addition of RefreshAllTXOs() is necessary here. Is there maybe test coverage missing here?

    achow101 commented at 8:40 pm on May 15, 2025:
    The test coverage is added in the next commit “test: Test for balance update due to untracked output becoming spendable” which I have now moved to be earlier in the PR.
  173. in src/wallet/receive.cpp:259 in ff73562fb5 outdated
    315-                ret.m_watchonly_trusted += tx_credit_watchonly;
    316-            }
    317-            if (!is_trusted && tx_depth == 0 && wtx.InMempool()) {
    318-                ret.m_mine_untrusted_pending += tx_credit_mine;
    319-                ret.m_watchonly_untrusted_pending += tx_credit_watchonly;
    320+        for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
    


    murchandamus commented at 0:08 am on May 15, 2025:

    I think it would make more sense to me if “wallet: Change balance calculation to use m_txos” (ff73562fb5df406da95d6a6327e3d22b8d3e12f2) came after “test: Test for balance update due to untracked output becoming spendable” (f062a438620625bfa0d20022e76cad1705587737).

    First the fix, then the test, then the refactor passes the test.


    achow101 commented at 8:41 pm on May 15, 2025:

    Done.

    This actually revealed a bug in importdescriptors, I’ve added a commit to fix that, although it should actually be moot after the changes in this PR.

  174. murchandamus commented at 0:58 am on May 15, 2025: contributor
    ACK 51d4846f648e79e3e098f1f1bea7d5e823ebdea8
  175. achow101 force-pushed on May 15, 2025
  176. achow101 force-pushed on May 15, 2025
  177. DrahtBot added the label CI failed on May 15, 2025
  178. DrahtBot commented at 8:44 pm on May 15, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/42318018033 LLM reason (✨ experimental): The CI failure is due to a build error caused by an undeclared identifier “Assume”.

    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.

  179. DrahtBot removed the label CI failed on May 15, 2025
  180. DrahtBot added the label Needs rebase on May 16, 2025
  181. achow101 force-pushed on May 16, 2025
  182. DrahtBot removed the label Needs rebase on May 16, 2025
  183. murchandamus commented at 10:40 pm on May 16, 2025: contributor

    reACK https://github.com/bitcoin/bitcoin/pull/27286/commits/29cf059e6e592f7f82b982493a043219cdfa5b40

    via git range-diff efac285a0d709f48856c37b15bfa09af94c1d75b..51d4846f648e79e3e098f1f1bea7d5e823ebdea8 b15c386933eb7bc5ed8644dd37f8169d81ea5a22..29cf059e6e592f7f82b982493a043219cdfa5b40

    Hooray for Txid class!

  184. DrahtBot requested review from w0xlt on May 16, 2025
  185. rkrux commented at 7:01 pm on May 17, 2025: contributor

    I have just started reviewing this PR - have gone through the PR description & commit messages, going through the diff at the moment.

    The intent of the PR is quite cool! As I understand, the motivation comes from this issue #27002. Due to the nature of the change, I decided to run benchmarks on my system and have shared below the best results of the lot I had (the results seem to be consistent across multiple runs).

    Observations

    1. I can clearly see noticeable performance improvements in the WalletBalance benchmarks.
    2. However, both WalletAvailableCoins & WalletCreateTxUsePresetInputsAndCoinSelection show a marginal decline instead with decent fluctuation (err %).
    3. Interestingly, the WalletIsMine* benchmarks showed improvement when I ran the bench with default value of 10ms, but declined when I ran for 5s.
    4. The WalletLoading one doesn’t show any big decline, which is good.
    5. On average, the result fluctuations (err %) seemed to increase when the benchmarks ran for 5s compared to 10ms.

    I believe that both the master & PR benchmarks shared in the PR description should be updated as they are more than 2 years old now, and a latest one would paint a more accurate picture. Also, the WalletLoadingLegacy benchmark can be removed from PR description as it no longer exists.

    Since imports can change the IsMine status 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 update m_txos.

    After the removal of the legacy wallets, there is just 1 import* RPC now - importdescriptors. Even though the overall results of the benchmarks are promising, I’m wondering if this PR should add importdescriptors benchmark for us to know what’s the hit on this RPC.

    Slightly unrelated but now again that the legacy wallet is no more, the *descriptor benchmarks names can be updated to remove the descriptor suffix to ease it on the eyes - WalletLoadingDescriptors to WalletLoading - if I’m not missing anything.

    Benchmarks (MacOS Sequoia)

    0➜  bitcoin git:(master) ✗ ./build/bin/bench_bitcoin -filter="Wallet.*" -min-time=5000
    
    ns/op op/s err% total benchmark
    60,048,154.50 16.65 0.9% 1.33 WalletAvailableCoins
    8,084.01 123,701.05 1.1% 5.51 WalletBalanceClean
    64,005.21 15,623.73 0.5% 5.47 WalletBalanceDirty
    8,029.22 124,545.13 0.7% 5.47 WalletBalanceMine
    33.16 30,153,409.91 0.8% 5.31 WalletBalanceWatch
    745,284,217.00 1.34 1.6% 8.17 WalletCreateEncrypted
    344,072,227.00 2.91 1.7% 4.51 WalletCreatePlain
    1,265,623.20 790.12 1.5% 0.07 WalletCreateTxUseOnlyPresetInputs
    97,241,712.60 10.28 1.7% 5.37 WalletCreateTxUsePresetInputsAndCoinSelection
    94.82 10,546,060.33 1.1% 5.51 WalletIsMineDescriptors
    107.74 9,281,219.40 0.6% 5.48 WalletIsMineMigratedDescriptors
    347,539,943.00 2.88 1.2% 1.76 WalletLoadingDescriptors
    4,515,248,518.00 0.22 0.0% 4.52 WalletMigration
    0➜  bitcoin git:(wallet-txos) ✗ ./build/bin/bench_bitcoin -filter="Wallet.*" -min-time=5000
    
    ns/op op/s err% total benchmark
    60,103,807.50 16.64 1.5% 1.33 WalletAvailableCoins
    5,277.12 189,497.31 4.5% 5.52 WalletBalanceClean
    5,669.83 176,372.17 1.4% 5.69 WalletBalanceDirty
    5,161.86 193,728.76 1.6% 5.57 WalletBalanceMine
    35.88 27,867,699.08 2.1% 5.27 WalletBalanceWatch
    778,745,354.00 1.28 3.7% 8.69 WalletCreateEncrypted
    350,162,310.00 2.86 1.7% 3.95 WalletCreatePlain
    1,544,025.80 647.66 2.2% 0.09 WalletCreateTxUseOnlyPresetInputs
    103,045,837.80 9.70 1.3% 5.74 WalletCreateTxUsePresetInputsAndCoinSelection
    96.31 10,382,776.81 2.0% 5.48 WalletIsMineDescriptors
    116.97 8,549,245.41 0.9% 5.51 WalletIsMineMigratedDescriptors
    352,261,692.00 2.84 0.6% 1.76 WalletLoadingDescriptors
    4,671,199,177.00 0.21 0.0% 4.67 WalletMigration
  186. rkrux commented at 11:17 am on May 19, 2025: contributor

    Benchmarks at 29cf059e6e592f7f82b982493a043219cdfa5b40

    An update to my previous comment: #27286#pullrequestreview-2848375525

    I dug a bit more and realised that in order to have a fair assessment between master and the PR, the AvailableCoins benchmark changes of the last commit 29cf059e6e592f7f82b982493a043219cdfa5b40 must be present in master as well so that both the results come from the same benchmark cases. I gcp’ed the commit onto master, removed the hardcoded epochIterations from AvailableCoins & WalletCreateTx functions like below, and let the benchmarks run for 5s each.

     0--- a/src/bench/wallet_create_tx.cpp
     1+++ b/src/bench/wallet_create_tx.cpp
     2@@ -159,7 +159,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type
     3     if (coin_control.m_allow_other_inputs) target += 50 * COIN;
     4     std::vector<wallet::CRecipient> recipients = {{dest, target, true}};
     5 
     6-    bench.epochIterations(5).run([&] {
     7+    bench.run([&] {
     8         LOCK(wallet.cs_wallet);
     9         const auto& tx_res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
    10         assert(tx_res);
    11@@ -198,7 +198,7 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType
    12     auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
    13     assert(bal == 49 * COIN * (chain_size - COINBASE_MATURITY));
    14 
    15-    bench.epochIterations(2).run([&] {
    16+    bench.run([&] {
    17         LOCK(wallet.cs_wallet);
    18         const auto& res = wallet::AvailableCoins(wallet);
    19         assert(res.All().size() == (chain_size - COINBASE_MATURITY) * 2);
    

    Benchmark Results (best of the lot)

    0➜  bitcoin git:(master) ✗ ./build/bin/bench_bitcoin -filter="Wallet.*" -min-time=5000
    
    ns/op op/s err% total benchmark
    62,913,524.29 15.89 1.1% 5.59 WalletAvailableCoins
    8,347.78 119,792.35 2.4% 5.54 WalletBalanceClean
    64,450.99 15,515.66 0.5% 5.53 WalletBalanceDirty
    8,474.18 118,005.46 3.0% 5.18 WalletBalanceMine
    36.07 27,723,968.90 2.1% 5.49 WalletBalanceWatch
    764,481,945.00 1.31 0.9% 8.48 WalletCreateEncrypted
    360,601,555.00 2.77 0.9% 4.49 WalletCreatePlain
    1,358,501.41 736.11 0.8% 5.52 WalletCreateTxUseOnlyPresetInputs
    120,641,695.25 8.29 0.9% 5.53 WalletCreateTxUsePresetInputsAndCoinSelection
    114.60 8,725,874.44 0.8% 5.58 WalletIsMineDescriptors
    113.78 8,789,224.41 0.2% 5.30 WalletIsMineMigratedDescriptors
    366,820,995.00 2.73 0.5% 1.83 WalletLoadingDescriptors
    5,000,027,548.00 0.20 0.0% 5.00 WalletMigration
    0➜  bitcoin git:(wallet-txos) ✗ ./build/bin/bench_bitcoin -filter="Wallet.*" -min-time=5000
    
    ns/op op/s err% total benchmark
    60,758,414.00 16.46 0.8% 5.55 WalletAvailableCoins
    5,369.55 186,235.52 0.5% 5.51 WalletBalanceClean
    5,709.33 175,151.91 0.7% 5.50 WalletBalanceDirty
    5,289.61 189,049.70 0.4% 5.50 WalletBalanceMine
    38.12 26,233,493.24 2.5% 5.33 WalletBalanceWatch
    753,308,252.00 1.33 0.5% 8.28 WalletCreateEncrypted
    352,858,982.00 2.83 0.7% 4.26 WalletCreatePlain
    1,392,278.42 718.25 2.2% 5.21 WalletCreateTxUseOnlyPresetInputs
    103,969,867.00 9.62 0.7% 5.53 WalletCreateTxUsePresetInputsAndCoinSelection
    97.30 10,278,004.67 0.7% 5.56 WalletIsMineDescriptors
    96.68 10,343,863.28 0.6% 5.49 WalletIsMineMigratedDescriptors
    361,421,296.00 2.77 1.4% 1.81 WalletLoadingDescriptors
    4,682,511,111.00 0.21 0.0% 4.68 WalletMigration

    Observations

    1. The WalletBalance* benchmarks show remarkable improvements like in the previous comment - except WalletBalanceWatch that sees a decline.
    2. Unlike the previous case, both WalletAvailableCoins & WalletCreateTxUsePresetInputsAndCoinSelection have an improved perf now.
    3. The WalletIsMine* benchmarks have also improved, though I have noticed fluctuations in results across reruns.
    4. The WalletLoading one doesn’t show a decline anymore, it seems stable across master & PR. Improved in some test runs like the one shared above.
    Benchmarks percentage changes calculated based on op/s
    Benchmark % change
    WalletAvailableCoins 3.59
    WalletBalanceClean 55.47
    WalletBalanceDirty 1,028.87
    WalletBalanceMine 60.20
    WalletBalanceWatch -5.38
    WalletCreateEncrypted 1.53
    WalletCreatePlain 2.17
    WalletCreateTxUseOnlyPresetInputs -2.43
    WalletCreateTxUsePresetInputsAndCoinSelection 16.04
    WalletIsMineDescriptors 17.79
    WalletIsMineMigratedDescriptors 17.69
    WalletLoadingDescriptors 1.47
    WalletMigration 5.00
  187. DrahtBot added the label Needs rebase on May 19, 2025
  188. achow101 force-pushed on May 20, 2025
  189. DrahtBot removed the label Needs rebase on May 20, 2025
  190. DrahtBot added the label CI failed on May 20, 2025
  191. DrahtBot removed the label CI failed on May 21, 2025
  192. DrahtBot added the label Needs rebase on May 21, 2025
  193. achow101 force-pushed on May 21, 2025
  194. DrahtBot removed the label Needs rebase on May 21, 2025
  195. murchandamus commented at 5:17 pm on May 23, 2025: contributor
    ReACK d73e4f878d143fe86d84e0e6f18c2a88c0cb8609
  196. DrahtBot requested review from w0xlt on May 23, 2025
  197. achow101 commented at 7:09 pm on May 27, 2025: member
    Updated the description with recent benchmarks.
  198. DrahtBot added the label Needs rebase on May 30, 2025
  199. achow101 force-pushed on May 30, 2025
  200. DrahtBot removed the label Needs rebase on May 30, 2025
  201. in src/wallet/transaction.h:369 in 41c82afdfc outdated
    362@@ -362,6 +363,30 @@ struct WalletTxOrderComparator {
    363         return a->nOrderPos < b->nOrderPos;
    364     }
    365 };
    366+
    367+class WalletTXO
    368+{
    369+private:
    370+    const CWalletTx& m_wtx;
    371+    const CTxOut& m_output;
    


    davidgumberg commented at 9:56 pm on June 3, 2025:

    non-blocking suggestion:

    What if instead of a reference to a CTxOut WalletTXO just kept the vout index of the output. Not even really for performance reasons, but just because it more tightly represents / enforces the meaning/usage of WalletTXO.

    This is an implementation detail of WalletTXO, and can be handled neatly by GetTxOut(), so if the suggestion makes sense, it can be done in a follow-up.


    achow101 commented at 8:08 pm on June 5, 2025:
    The whole point is to not have CWalletTxs permanently in memory eventually, so just an index pretty much defeats that.
  202. in src/wallet/wallet.cpp:4472 in 41c82afdfc outdated
    4469+{
    4470+    AssertLockHeld(cs_wallet);
    4471+    for (uint32_t i = 0; i < wtx.tx->vout.size(); ++i) {
    4472+        const CTxOut& txout = wtx.tx->vout.at(i);
    4473+        isminetype ismine = IsMine(txout);
    4474+        if (ismine == ISMINE_NO) {
    


    davidgumberg commented at 10:24 pm on June 3, 2025:
    Is it possible for an outpoint to ever change from something else to ISMINE_NO? I’m wondering if there should be a m_txos.contains(outpoint) check and SetIsMine() update like happens below for the ISMINE_NO case.

    achow101 commented at 8:10 pm on June 5, 2025:

    Is it possible for an outpoint to ever change from something else to ISMINE_NO?

    No, we don’t allow deleting things from the wallet so this cannot happen.

  203. in src/wallet/receive.cpp:212 in 72378b2bf4 outdated
    256@@ -257,6 +257,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
    257 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<Txid>& trusted_parents)
    


    davidgumberg commented at 5:30 pm on June 4, 2025:

    In https://github.com/bitcoin/bitcoin/pull/27286/commits/72378b2bf42714b67fb3f0272b5fbb7c37a08898 (wallet: Exit IsTrustedTx early if wtx is already in trusted_parents):

    nanonit: commit message IsTrustedTx -> TxIsTrusted


    achow101 commented at 8:18 pm on June 5, 2025:
    If I need to retouch.
  204. in src/wallet/receive.cpp:217 in 72378b2bf4 outdated
    256@@ -257,6 +257,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
    257 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<Txid>& trusted_parents)
    258 {
    259     AssertLockHeld(wallet.cs_wallet);
    260+
    261+    // This wtx is already trusted
    262+    if (trusted_parents.contains(wtx.GetHash())) return true;
    


    davidgumberg commented at 6:54 pm on June 4, 2025:

    Feel free to disregard:

    I’m having some trouble building confidence about whether or not this optimization is correct.

    My understanding is that in the loop below, a transaction will only be inserted into trusted_parents if it, and all of its parents are trusted:

    https://github.com/bitcoin/bitcoin/blob/72378b2bf42714b67fb3f0272b5fbb7c37a08898/src/wallet/receive.cpp#L283-L285

    But the “cached” checks, so to speak, are all of the checks above the trusted_parents.count() check in the loop:

    https://github.com/bitcoin/bitcoin/blob/72378b2bf42714b67fb3f0272b5fbb7c37a08898/src/wallet/receive.cpp#L273-L287

    I’m not sure it makes sense to assume a transaction is trusted if we havent checked e.g. wtx.isBlockConflicted(), since this can easily change, and cause us to misrepresent a transaction that is the child of a previously confirmed parent that is now conflicted as trusted, no?

    I tried to write a test that would exhibit this misbehavior, but I couldn’t, so I think I may have got lost in the plot here, can you help me understand why we can be sure that if a wtx was trusted once, it will always be trusted?

     0    def test_descendants_with_block_conflicts(self):
     1        alice_node = self.nodes[0]
     2        alice_node.createwallet("alice_4")
     3        alice = alice_node.get_wallet_rpc("alice_4")
     4
     5        bob_node = self.nodes[1]
     6        bob_node.createwallet("bob_2")
     7        bob = bob_node.get_wallet_rpc("bob_2")
     8
     9        # Give Alice some initial funds
    10        self.nodes[2].send(outputs=[{alice.getnewaddress(): 25} for _ in range(2)])
    11        self.generate(self.nodes[2], 1)
    12        self.sync_blocks()
    13
    14        self.log.info("Test a scenario where block conflicts affect both parents and their children")
    15
    16        unspents = alice.listunspent()
    17        assert_equal(len(unspents), 2)
    18        assert_equal(Decimal(alice.getbalances()['mine']['trusted']), Decimal("50.000"))
    19
    20        # Create alice_self_send_tx: Alice sends to Bob using first unspent
    21        alice_self_send_tx = alice.createrawtransaction(inputs=[unspents[0]], outputs=[{bob.getnewaddress(): 24.999}])
    22        parent_tx_1 = alice.signrawtransactionwithwallet(alice_self_send_tx)['hex']
    23        alice_node.sendrawtransaction(parent_tx_1)
    24        assert_equal(Decimal(alice.getbalances()['mine']['trusted']), Decimal("25.000"))
    25
    26        # Create parent_tx_2: Alice sends to herself using both unspents (conflicts with parent_tx_1)
    27        raw_tx_2 = alice.createrawtransaction(inputs=[unspents[0], unspents[1]], outputs=[{alice.getnewaddress(): 49.990}])
    28        parent_tx_2 = alice.signrawtransactionwithwallet(raw_tx_2)['hex']
    29        print(alice.getbalances())
    30
    31        # Disconnect nodes to create conflicts on separate chains
    32        self.disconnect_nodes(0, 1)
    33        self.disconnect_nodes(1, 2)
    34
    35        # Broadcast parent_tx_1 on bob's node and mine it.
    36        bob_node.sendrawtransaction(parent_tx_1)
    37        self.generate(bob_node, 1, sync_fun=self.no_op)
    38        bobs_unspents = bob.listunspent()
    39        assert_equal(len(bobs_unspents), 1)
    40        assert_equal(Decimal(bob.getbalances()['mine']['trusted']), Decimal("24.999"))
    41        
    42        # Create a child of the conflict transaction that pays bob
    43        bobs_tx_child_raw = bob.createrawtransaction(inputs=[bobs_unspents[0]], outputs=[{bob.getnewaddress(): 24.998}])
    44        bobs_tx_child = bob.signrawtransactionwithwallet(bobs_tx_child_raw)['hex']
    45        bob_node.sendrawtransaction(bobs_tx_child)
    46        assert_equal(Decimal(bob.getbalances()['mine']['trusted']), Decimal("24.998"))
    47        
    48        # Broadcast parent_tx_2 on alice's node and mine it.
    49        alice_node.sendrawtransaction(parent_tx_2)
    50        self.generate(alice_node, 1, sync_fun=self.no_op) 
    51        assert_equal(Decimal(alice.getbalances()['mine']['trusted']), Decimal("49.99"))
    52
    53        self.connect_nodes(0, 1, wait_for_connect=True)
    54        self.connect_nodes(0, 2, wait_for_connect=True)
    55        self.connect_nodes(1, 2, wait_for_connect=True)
    56
    57        self.generate(alice_node, 1)  # Create longer chain
    58        self.sync_blocks()
    59
    60        assert_equal(Decimal(alice.getbalances()['mine']['trusted']), Decimal("49.99"))
    61        # print(alice.getbalances())
    62        assert_equal(Decimal(bob.getbalances()['mine']['trusted']), Decimal("24.998"))
    63        # print(bob.getbalances())
    64
    65        # Clean up
    66        alice.unloadwallet()
    67        bob.unloadwallet()
    

    achow101 commented at 8:18 pm on June 5, 2025:
    trusted_parents is not a global cache, it is a per-call cache, and is generally unique per-call to CachedTxIsTrusted. Since each call to CachedTxIsTrusted is guarded by cs_wallet, there can be no state changes made to any of the transactions. While that lock is held by the caller of CachedTxIsTrusted, the result of checking whether a specific transaction will always be the same, regardless of any state changes that may have occurred.

    davidgumberg commented at 11:29 pm on June 5, 2025:

    Oh! Thanks, I missed that trusted_parents is not global, it’s initialized in GetAddressBalances() in the same scope as a LOCK on cs_wallet.

    https://github.com/bitcoin/bitcoin/blob/ae50dcb3a49eaf40fb64fd227c447ba90c45db34/src/wallet/receive.cpp#L346-L347

  205. in src/wallet/wallet.cpp:2352 in 41c82afdfc outdated
    2352@@ -2348,6 +2353,9 @@ util::Result<void> CWallet::RemoveTxs(WalletBatch& batch, std::vector<Txid>& txs
    2353             wtxOrdered.erase(it->second.m_it_wtxOrdered);
    2354             for (const auto& txin : it->second.tx->vin)
    2355                 mapTxSpends.erase(txin.prevout);
    2356+            for (unsigned int i = 0; i < it->second.tx->vout.size(); ++i) {
    2357+                m_txos.erase(COutPoint(Txid::FromUint256(hash), i));
    2358+            }
    


    davidgumberg commented at 6:42 pm on June 5, 2025:

    In https://github.com/bitcoin/bitcoin/pull/27286/commits/41c82afdfc94d7e0e1daa5ac4f4e5814e113c398 (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.


    achow101 commented at 7:23 pm on June 6, 2025:

    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.

  206. in src/wallet/wallet.cpp:3931 in ad76a44c3d outdated
    3931@@ -3932,6 +3932,10 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
    3932         return util::Error{_("Error: Unable to read wallet's best block locator record")};
    3933     }
    3934 
    3935+    // Update m_txos to match the descriptors remaining in this wallet
    3936+    m_txos.clear();
    3937+    RefreshAllTXOs();
    


    davidgumberg commented at 6:57 pm on June 5, 2025:

    In commit https://github.com/bitcoin/bitcoin/pull/27286/commits/ad76a44c3d9f300d5d230af442d36f90b349bc12 (wallet: Recompute wallet TXOs after descriptor migration)

    nit: This commit should be moved to before m_txos is used in GetBalance()


    davidgumberg commented at 0:26 am on June 6, 2025:

    Nevermind, not needed for GetBalance and AvailableCoins() because wallets are reloaded after migration, but needed because GetDebit() is changed in the next commit to use m_txos, and part of the migration process uses IsFromMe():

    https://github.com/bitcoin/bitcoin/blob/ae024137bda9fe189f4e7ccf26dbaffd44cbbeb6/src/wallet/wallet.cpp#L1635-L1639

    https://github.com/bitcoin/bitcoin/blob/854241298059f8b56563ca7ca64f5b61b59f119b/src/wallet/wallet.cpp#L3963

    IsMine() tells you whether or not a transaction contains outputs spendable by the wallet, IsFromMe() tells you whether or not a transaction contains outputs created by the wallet.

  207. in src/wallet/rpc/backup.cpp:402 in 8d9c7c3461 outdated
    398@@ -399,6 +399,7 @@ RPCHelpMan importdescriptors()
    399             }
    400         }
    401         pwallet->ConnectScriptPubKeyManNotifiers();
    402+        pwallet->RefreshAllTXOs();
    


    davidgumberg commented at 7:18 pm on June 5, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/8d9c7c3461d088211dde0551909898b8018059bf (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 https://github.com/bitcoin/bitcoin/pull/27286/commits/b3182f311f7e2f1608463da62ed85efba4f78dec (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.

  208. in src/wallet/receive.cpp:306 in ae50dcb3a4 outdated
    316-            }
    317-            if (!is_trusted && tx_depth == 0 && wtx.InMempool()) {
    318-                ret.m_mine_untrusted_pending += tx_credit_mine;
    319-                ret.m_watchonly_untrusted_pending += tx_credit_watchonly;
    320+        for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
    321+            Assert(MoneyRange(txo.GetTxOut().nValue));
    


    davidgumberg commented at 9:40 pm on June 5, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (wallet: Change balance calculation to use m_txos)


    Why is this check added?


    achow101 commented at 7:30 pm on June 6, 2025:
    Don’t remember. Can remove if I need to retouch.

    achow101 commented at 9:03 pm on June 6, 2025:
    Done
  209. in src/wallet/receive.cpp:318 in ae50dcb3a4 outdated
    379-                balances[addr] += n;
    380-            }
    381+            CAmount n = wallet.IsSpent(outpoint) ? 0 : txo.GetTxOut().nValue;
    382+            balances[addr] += n;
    383         }
    384     }
    


    davidgumberg commented at 10:05 pm on June 5, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (wallet: Change balance calculation to use m_txos)


    nit: here and above, I think GetTxOut() and GetWalletTx() are used enough to justify a variable, also minimizes diff if names are reused, but I’m ~0 on changing them:

     0        for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
     1            const CWalletTx& wtx{txo.GetWalletTx()};
     2            const CTxOut& output{txo.GetTxOut()};
     3
     4            if (!CachedTxIsTrusted(wallet, wtx, trusted_parents)) continue;
     5            if (wallet.IsTxImmatureCoinBase(wtx)) continue;
     6
     7            int nDepth = wallet.GetTxDepthInMainChain(wtx);
     8            if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1)) continue;
     9
    10            CTxDestination addr;
    11            Assume(wallet.IsMine(output));
    12            if(!ExtractDestination(output.scriptPubKey, addr)) continue;
    13
    14            CAmount n = wallet.IsSpent(outpoint) ? 0 : output.nValue;
    15            balances[addr] += n;
    16        }
    17    }
    

    achow101 commented at 9:03 pm on June 6, 2025:
    Done
  210. in src/wallet/receive.cpp:296 in ae50dcb3a4 outdated
    297@@ -297,27 +298,41 @@ bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx)
    298 Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse)
    299 {
    300     Balance ret;
    301-    isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
    


    davidgumberg commented at 10:13 pm on June 5, 2025:

    ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (wallet: Change balance calculation to use m_txos)

    Nit that can be addressed in a follow up, this commit leaves ISMINE_USED and related logic as dead code.


    davidgumberg commented at 10:51 pm on June 5, 2025:
    would be better addressed here: https://github.com/bitcoin/bitcoin/pull/32523
  211. in src/wallet/receive.cpp:284 in ae50dcb3a4 outdated
    340+                if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
    341+                    ret.m_mine_immature += credit_mine;
    342+                    ret.m_watchonly_immature += credit_watchonly;
    343+                } else if (is_trusted && tx_depth >= min_depth) {
    344+                    ret.m_mine_trusted += credit_mine;
    345+                    ret.m_watchonly_trusted += credit_watchonly;
    


    davidgumberg commented at 10:19 pm on June 5, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/ae50dcb3a49eaf40fb64fd227c447ba90c45db34 (wallet: Change balance calculation to use m_txos)

    nit: moving this check up would make the diff smaller


    achow101 commented at 8:42 pm on June 6, 2025:
    Since we are no longer using CachedTxGetAvailableCredit and CachedTxGetImmautreCredit, we have to first check whether the output is an immature coinbase. Otherwise immature coinbases will be included in the trusted balances which is incorrect.

    davidgumberg commented at 10:34 pm on June 6, 2025:
    My mistake, that makes sense.
  212. in src/wallet/receive.cpp:282 in ae50dcb3a4 outdated
    338+
    339+                // Set the amounts in the return object
    340+                if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
    341+                    ret.m_mine_immature += credit_mine;
    342+                    ret.m_watchonly_immature += credit_watchonly;
    343+                } else if (is_trusted && tx_depth >= min_depth) {
    


    davidgumberg commented at 10:23 pm on June 5, 2025:

    Out of scope observation:

    It seems strange that there isn’t a balance type for outputs that are trusted and confirmed in a block, but below min_depth confirmations, you see some balance when the tx is in the mempool, and then when it’s got enough confirmations, but in between it disappears from your balances seems odd to me.


    achow101 commented at 7:34 pm on June 6, 2025:
    It would make more sense to drop unconfirmed balances when min_depth > 0 rather than include less than min_depth as a separate balance. Regardless, this PR is not trying to change user facing behavior, so this is out of scope.
  213. in src/wallet/receive.cpp:325 in ae50dcb3a4 outdated
    335+                    // We shouldn't see any other isminetypes
    336+                    Assume(false);
    337+                }
    338+
    339+                // Set the amounts in the return object
    340+                if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
    


    davidgumberg commented at 10:31 pm on June 5, 2025:
    0                if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed() && tx_depth >= min_depth) {
    

    or alternately, nesting the immature and trusted checks together, maybe like:

    0             if (is_trusted && tx_depth >= min_depth) {
    1                    if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) {
    2                        ret.m_mine_immature += credit_mine;
    3                        ret.m_watchonly_immature += credit_watchonly;
    4                    } else {
    5                        ret.m_mine_trusted += credit_mine;
    6                        ret.m_watchonly_trusted += credit_watchonly;
    7                    }
    

    davidgumberg commented at 11:24 pm on June 5, 2025:
    from offline discussion with @achow101 this would be a change in behavior so out of scope for this PR, and also incorrect because min_depth should only change the meaning of m_mine_trusted, and can only be set from the getbalance() rpc anyways, which only shows m_mine_trusted.
  214. in src/wallet/spend.cpp:360 in 8542412980 outdated
    398+        if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {
    399             continue;
    400         }
    401 
    402-        if (nDepth < min_depth || nDepth > max_depth) {
    403+        if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase)
    


    davidgumberg commented at 0:01 am on June 6, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/854241298059f8b56563ca7ca64f5b61b59f119b (wallet: Use wallet’s TXO set in AvailableCoins)


    nit: this is a tx-level check and should move down.


    achow101 commented at 9:04 pm on June 6, 2025:
    Done
  215. in src/wallet/spend.cpp:332 in 8542412980 outdated
    335-        const Txid& txid = entry.first;
    336-        const CWalletTx& wtx = entry.second;
    337+    // Cache for whether each tx passes the tx level checks (first bool), and whether the transaction is "safe" (second bool)
    338+    std::unordered_map<uint256, std::pair<bool, bool>, SaltedTxidHasher> tx_safe_cache;
    339+    for (const auto& [outpoint, txo] : wallet.GetTXOs()) {
    340+        const CWalletTx& wtx = txo.GetWalletTx();
    


    davidgumberg commented at 0:03 am on June 6, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/854241298059f8b56563ca7ca64f5b61b59f119b (wallet: Use wallet’s TXO set in AvailableCoins)

    nit: I think this is easier to review if the transaction level checks stay at the top, smaller diff, and easier to see that all the transaction-level and output-level checks have been kept the same.


    achow101 commented at 9:04 pm on June 6, 2025:
    Done
  216. in src/wallet/spend.cpp:368 in 8542412980 outdated
    413+        assert(mine != ISMINE_NO);
    414 
    415-            if (output.nValue < params.min_amount || output.nValue > params.max_amount)
    416-                continue;
    417+        int nDepth = wallet.GetTxDepthInMainChain(wtx);
    418+        if (nDepth < 0)
    


    davidgumberg commented at 0:07 am on June 6, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/854241298059f8b56563ca7ca64f5b61b59f119b (wallet: Use wallet’s TXO set in AvailableCoins)


    nit: this is also a tx-level check.


    achow101 commented at 9:04 pm on June 6, 2025:
    Done
  217. in src/wallet/spend.cpp:365 in 8542412980 outdated
    408+        isminetype mine = wallet.IsMine(output);
    409 
    410-        for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
    411-            const CTxOut& output = wtx.tx->vout[i];
    412-            const COutPoint outpoint(txid, i);
    413+        assert(mine != ISMINE_NO);
    


    davidgumberg commented at 0:19 am on June 6, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/854241298059f8b56563ca7ca64f5b61b59f119b (wallet: Use wallet’s TXO set in AvailableCoins)


    nit:

    The output-level checks are shuffled in this commit, easier to reason about this commit, and smaller diff if the order is preserved.

     0        if (output.nValue < params.min_amount || output.nValue > params.max_amount)
     1            continue;
     2
     3        // Skip manually selected coins (the caller can fetch them directly)
     4        if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
     5            continue;
     6
     7        if (wallet.IsLockedCoin(outpoint) && params.skip_locked)
     8            continue;
     9
    10        if (wallet.IsSpent(outpoint))
    11            continue;
    12
    13        if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase)
    14            continue;
    15
    16        isminetype mine = wallet.IsMine(output);
    17
    18        assert(mine != ISMINE_NO);
    19
    20        int nDepth = wallet.GetTxDepthInMainChain(wtx);
    21        if (nDepth < 0)
    22            continue;
    23
    24        if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) {
    25            continue;
    26        }
    

    achow101 commented at 9:06 pm on June 6, 2025:
    Done
  218. in src/bench/wallet_create_tx.cpp:202 in 39734259fc outdated
    195@@ -189,7 +196,7 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector<OutputType
    196 
    197     // Check available balance
    198     auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache
    199-    assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
    200+    assert(bal == 49 * COIN * (chain_size - COINBASE_MATURITY));
    201 
    202     bench.epochIterations(2).run([&] {
    203         LOCK(wallet.cs_wallet);
    


    davidgumberg commented at 0:39 am on June 6, 2025:

    https://github.com/bitcoin/bitcoin/pull/27286/commits/39734259fc10b1339db2ae80c5fab12916cf0aff (bench: Have AvailableCoins benchmark include a lot of unrelated utxos)

    nit: This should be the first commit of the branch in my opinion and remove the unnecessary hardcoded nEpochIterations values as suggested by @rkrux (https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076)


    achow101 commented at 9:06 pm on June 6, 2025:
    Done
  219. davidgumberg commented at 1:22 am on June 6, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/27286/commits/39734259fc10b1339db2ae80c5fab12916cf0aff

    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 (https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2850393076) against master with this commit cherry-picked on top, with nEpochIterations removed, and got the following results:

    0$ ./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.

  220. DrahtBot requested review from murchandamus on Jun 6, 2025
  221. DrahtBot requested review from w0xlt on Jun 6, 2025
  222. bench: Have AvailableCoins benchmark include a lot of unrelated utxos
    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.
    a39074c52a
  223. wallet: MarkDirty after AddWalletDescriptor
    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.
    8ff188806b
  224. test: Test for balance update due to untracked output becoming spendable 0b35048307
  225. walletdb: Load Txs last
    Need to load txs last so that IsMine works.
    b379ec2467
  226. wallet: Keep track of transaction outputs owned by the wallet
    When loading transactions to the wallet, check the outputs, and keep
    track of the ones that are IsMine.
    220f2582db
  227. wallet: Exit IsTrustedTx early if wtx is already in trusted_parents cda977bc72
  228. wallet: Recalculate the wallet's txos after any imports c7a85cc417
  229. wallet: Change balance calculation to use m_txos
    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.
    96864062c6
  230. achow101 force-pushed on Jun 6, 2025
  231. wallet: Use wallet's TXO set in AvailableCoins
    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.
    2703d84b7e
  232. wallet: Retrieve TXO directly in FetchSelectedInputs
    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.
    5ab95ac68e
  233. wallet: Recompute wallet TXOs after descriptor migration
    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.
    5082dea76f
  234. wallet: Have GetDebit use the wallet's TXO set
    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.
    56f55f412c
  235. wallet: Remove unused CachedTxGet{Available,Immature}Credit
    These two functions are no longer used as GetBalances now uses the TXO
    set rather than per-tx cached balances
    ec25de35b2
  236. achow101 force-pushed on Jun 6, 2025
  237. davidgumberg commented at 11:17 pm on June 6, 2025: contributor

    reACK https://github.com/bitcoin/bitcoin/commit/ec25de35b2a51b493d36452f75096f790230a4e9

    Reviewed range-diff (git range-diff 3973425...ec25de35), addresses all the nits I left in my previous review minus the ones where I was incorrect.

    The primary changes are:

    1. The benchmark iterations are no longer unnecessarily hardcoded and the benchmark commit was moved to be the first commit.
    2. Two transaction level checks that were being performed per-output in AvailableCoins() are now part of the cached tx-level checks.
    3. For commit hygiene, code was moved to more closely resemble the order of checks in master, making the (whitespace-hidden) diff much smaller, and the changes in this PR easier to review in my opinion: e.g. (https://github.com/bitcoin/bitcoin/commit/854241298059f8b56563ca7ca64f5b61b59f119b?w=1 vs https://github.com/bitcoin/bitcoin/pull/27286/commits/2703d84b7efafcce05387a5e018363c3c59c4b58?w=1)

    I benchmarked again (w/ different hardware1) and it looks like the performance gap in the AvailableCoins() benchmark is improved over the earlier branch (10% faster than master vs 5% faster than master)

    Benchmark results

    0$ ./build/bin/bench_bitcoin -min-time=30000 '-filter=Wallet(Available|CreateTxUse).*'
    

    pr branch (https://github.com/bitcoin/bitcoin/commit/ec25de35b2a51b493d36452f75096f790230a4e9)

    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 (e2174378aa8a3 + 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 (https://github.com/bitcoin/bitcoin/commit/39734259fc10b1339db2ae80c5fab12916cf0aff) 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

    1. AMD Ryzen 9 7900X 12-Core Processor used in these benchmarks. ↩︎


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-06-08 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me