Merge bitcoin/bitcoin#26499: wallet: Abandon descendants of orphaned coinbases

b0fa598 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd7 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)

Pull request description:

  When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.

  Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.

  Fixes #14148

ACKs for top commit:
  furszy:
    ACK b0fa598 with a not-blocking nit.
  aureleoules:
    reACK b0fa598
  ishaanam:
    ACK b0fa598

Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac

Author: glozow

src/wallet/transaction.h

@@ -293,6 +293,7 @@ class CWalletTx
 
     bool isAbandoned() const { return state<TxStateInactive>() && state<TxStateInactive>()->abandoned; }
     bool isConflicted() const { return state<TxStateConflicted>(); }
+    bool isInactive() const { return state<TxStateInactive>(); }
     bool isUnconfirmed() const { return !isAbandoned() && !isConflicted() && !isConfirmed(); }
     bool isConfirmed() const { return state<TxStateConfirmed>(); }
     const uint256& GetHash() const { return tx->GetHash(); }

src/wallet/wallet.cpp

@@ -1065,6 +1065,33 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
         }
     }
 
+    // Mark inactive coinbase transactions and their descendants as abandoned
+    if (wtx.IsCoinBase() && wtx.isInactive()) {
+        std::vector<CWalletTx*> txs{&wtx};
+
+        TxStateInactive inactive_state = TxStateInactive{/*abandoned=*/true};
+
+        while (!txs.empty()) {
+            CWalletTx* desc_tx = txs.back();
+            txs.pop_back();
+            desc_tx->m_state = inactive_state;
+            // Break caches since we have changed the state
+            desc_tx->MarkDirty();
+            batch.WriteTx(*desc_tx);
+            MarkInputsDirty(desc_tx->tx);
+            for (unsigned int i = 0; i < desc_tx->tx->vout.size(); ++i) {
+                COutPoint outpoint(desc_tx->GetHash(), i);
+                std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(outpoint);
+                for (TxSpends::const_iterator it = range.first; it != range.second; ++it) {
+                    const auto wit = mapWallet.find(it->second);
+                    if (wit != mapWallet.end()) {
+                        txs.push_back(&wit->second);
+                    }
+                }
+            }
+        }
+    }
+
     //// debug print
     WalletLogPrintf("AddToWallet %s  %s%s\n", hash.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""));
 
@@ -1274,7 +1301,11 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
             wtx.MarkDirty();
             batch.WriteTx(wtx);
             NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);
-            // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too
+            // Iterate over all its outputs, and mark transactions in the wallet that spend them abandoned too.
+            // States are not permanent, so these transactions can become unabandoned if they are re-added to the
+            // mempool, or confirmed in a block, or conflicted.
+            // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their
+            // states change will remain abandoned and will require manual broadcast if the user wants them.
             for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) {
                 std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(COutPoint(now, i));
                 for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) {

test/functional/wallet_orphanedreward.py

@@ -34,29 +34,40 @@ def run_test(self):
         # the existing balance and the block reward.
         self.generate(self.nodes[0], 150)
         assert_equal(self.nodes[1].getbalance(), 10 + 25)
+        pre_reorg_conf_bals = self.nodes[1].getbalances()
         txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 30)
+        orig_chain_tip = self.nodes[0].getbestblockhash()
+        self.sync_mempools()
 
         # Orphan the block reward and make sure that the original coins
         # from the wallet can still be spent.
         self.nodes[0].invalidateblock(blk)
-        self.generate(self.nodes[0], 152)
-        # Without the following abandontransaction call, the coins are
-        # not considered available yet.
-        assert_equal(self.nodes[1].getbalances()["mine"], {
-          "trusted": 0,
-          "untrusted_pending": 0,
-          "immature": 0,
-        })
-        # The following abandontransaction is necessary to make the later
-        # lines succeed, and probably should not be needed; see
-        # https://github.com/bitcoin/bitcoin/issues/14148.
-        self.nodes[1].abandontransaction(txid)
+        blocks = self.generate(self.nodes[0], 152)
+        conflict_block = blocks[0]
+        # We expect the descendants of orphaned rewards to no longer be considered
         assert_equal(self.nodes[1].getbalances()["mine"], {
           "trusted": 10,
           "untrusted_pending": 0,
           "immature": 0,
         })
-        self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 9)
+        # And the unconfirmed tx to be abandoned
+        assert_equal(self.nodes[1].gettransaction(txid)["details"][0]["abandoned"], True)
+
+        # The abandoning should persist through reloading
+        self.nodes[1].unloadwallet(self.default_wallet_name)
+        self.nodes[1].loadwallet(self.default_wallet_name)
+        assert_equal(self.nodes[1].gettransaction(txid)["details"][0]["abandoned"], True)
+
+        # If the orphaned reward is reorged back into the main chain, any unconfirmed
+        # descendant txs at the time of the original reorg remain abandoned.
+        self.nodes[0].invalidateblock(conflict_block)
+        self.nodes[0].reconsiderblock(blk)
+        assert_equal(self.nodes[0].getbestblockhash(), orig_chain_tip)
+        self.generate(self.nodes[0], 3)
+
+        assert_equal(self.nodes[1].getbalances(), pre_reorg_conf_bals)
+        assert_equal(self.nodes[1].gettransaction(txid)["details"][0]["abandoned"], True)
+
 
 if __name__ == '__main__':
     OrphanedBlockRewardTest().main()