Merge #18083: [0.19] wallet: Reset reused transactions cache

f11872cbf4 wallet: Reset reused transactions cache (Fabian Jahr)

Pull request description:

  Backport of #17843

  Required porting to pre-`WalletBatch`

ACKs for top commit:
  kallewoof:
    Code review ACK f11872cbf4
  laanwj:
    code review ACK f11872cbf4
  meshcollider:
    utACK f11872cbf4

Tree-SHA512: 5cf5f136d1eafb0783c2e6799e3675ebc50997ebb56b379d8a198ac35eb3b32f6b98656760a8b1c821eeac665eb80efb1723dd4e9eb58d2b4d45c4674499bedf

src/wallet/wallet.cpp

@@ -1055,7 +1055,7 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
     return success;
 }
 
-void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool used)
+void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations)
 {
     const CWalletTx* srctx = GetWalletTx(hash);
     if (!srctx) return;
@@ -1065,7 +1065,9 @@ void CWallet::SetUsedDestinationState(const uint256& hash, unsigned int n, bool
         if (::IsMine(*this, dst)) {
             LOCK(cs_wallet);
             if (used && !GetDestData(dst, "used", nullptr)) {
-                AddDestData(dst, "used", "p"); // p for "present", opposite of absent (null)
+                if (AddDestData(dst, "used", "p")) { // p for "present", opposite of absent (null)
+                    tx_destinations.insert(dst);
+                }
             } else if (!used && GetDestData(dst, "used", nullptr)) {
                 EraseDestData(dst, "used");
             }
@@ -1110,10 +1112,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 
     if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
         // Mark used destinations
+        std::set<CTxDestination> tx_destinations;
+
         for (const CTxIn& txin : wtxIn.tx->vin) {
             const COutPoint& op = txin.prevout;
-            SetUsedDestinationState(op.hash, op.n, true);
+            SetUsedDestinationState(op.hash, op.n, true, tx_destinations);
         }
+
+        MarkDestinationsDirty(tx_destinations);
     }
 
     // Inserts only if not already there, returns tx inserted or tx found
@@ -3793,6 +3799,21 @@ int64_t CWallet::GetOldestKeyPoolTime()
     return oldestKey;
 }
 
+void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations) {
+    for (auto& entry : mapWallet) {
+        CWalletTx& wtx = entry.second;
+
+        for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
+            CTxDestination dst;
+
+            if (ExtractDestination(wtx.tx->vout[i].scriptPubKey, dst) && destinations.count(dst)) {
+                wtx.MarkDirty();
+                break;
+            }
+        }
+    }
+}
+
 std::map<CTxDestination, CAmount> CWallet::GetAddressBalances(interfaces::Chain::Lock& locked_chain)
 {
     std::map<CTxDestination, CAmount> balances;

src/wallet/wallet.h

@@ -1006,7 +1006,7 @@ public:
 
     // Whether this or any known UTXO with the same single key has been spent.
     bool IsUsedDestination(const uint256& hash, unsigned int n) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used);
+    void SetUsedDestinationState(const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations);
 
     std::vector<OutputGroup> GroupOutputs(const std::vector<COutput>& outputs, bool single_coin) const;
 
@@ -1216,6 +1216,12 @@ public:
 
     std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;
 
+    /**
+     * Marks all outputs in each one of the destinations dirty, so their cache is
+     * reset and does not return outdated information.
+     */
+    void MarkDestinationsDirty(const std::set<CTxDestination>& destinations) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+
     bool GetNewDestination(const OutputType type, const std::string label, CTxDestination& dest, std::string& error);
     bool GetNewChangeDestination(const OutputType type, CTxDestination& dest, std::string& error);
 

test/functional/wallet_avoidreuse.py

@@ -88,7 +88,8 @@ class AvoidReuseTest(BitcoinTestFramework):
         self.test_fund_send_fund_send("p2sh-segwit")
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
         self.test_fund_send_fund_send("bech32")
-
+        reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
+        self.test_getbalances_used()
 
     def test_persistence(self):
         '''Test that wallet files persist the avoid_reuse flag.'''
@@ -248,5 +249,35 @@ class AvoidReuseTest(BitcoinTestFramework):
         assert_approx(self.nodes[1].getbalance(), 1, 0.001)
         assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 11, 0.001)
 
+    def test_getbalances_used(self):
+        '''
+        getbalances and listunspent should pick up on reused addresses
+        immediately, even for address reusing outputs created before the first
+        transaction was spending from that address
+        '''
+        self.log.info("Test getbalances used category")
+
+        # node under test should be completely empty
+        assert_equal(self.nodes[1].getbalance(avoid_reuse=False), 0)
+
+        new_addr = self.nodes[1].getnewaddress()
+        ret_addr = self.nodes[0].getnewaddress()
+
+        # send multiple transactions, reusing one address
+        for _ in range(11):
+            self.nodes[0].sendtoaddress(new_addr, 1)
+
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        # send transaction that should not use all the available outputs
+        # per the current coin selection algorithm
+        self.nodes[1].sendtoaddress(ret_addr, 5)
+
+        # getbalances and listunspent should show the remaining outputs
+        # in the reused address as used/reused
+        assert_unspent(self.nodes[1], total_count=2, total_sum=6, reused_count=1, reused_sum=1)
+        assert_balances(self.nodes[1], mine={"used": 1, "trusted": 5})
+
 if __name__ == '__main__':
     AvoidReuseTest().main()