Repository: bitcoin/bitcoin (language: cpp)

Issue:
abandontransaction needed after spending orphaned block reward
When a block reward is spent together with other outputs in the wallet and that block reward is later orphaned, the wallet does not show the other funds as available anymore.  Even a `-rescan` is not enough - only `-zapwallettxes` or an explicit `abandontransaction` "recovers" the funds.  I prepared a [test case](https://github.com/domob1812/bitcoin/blob/wallet-bug-invalidated-block-rewards/test/functional/wallet_bug.py) that shows this behaviour.

Is this a known issue or the expected behaviour?  While this situation is something that will presumably not occur in practice (as it requires a long reorg), I wonder if that transaction should be automatically abandoned as it seems clear it won't be able to be confirmed anymore.


Discussion / hints:
Why should it be automatically abandoned only when spending rewards? What if there is another reorg bringing back the reward?
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?
@MarcoFalke Yes, this still applies to current `master`.  I've re-created a testcase showing this behaviour:  [test.zip](https://github.com/bitcoin/bitcoin/files/4539131/test.zip)

Of course, @promag's point is valid.  Perhaps that's the expected behaviour.
I think this is still a bug. Maybe submit the test case as a pull request, so that this can be fixed easier?
@MarcoFalke Sure, created #18795.

Fix patch:
diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index 27983e356d7cd..8da5f5532dc74 100644
--- a/src/wallet/transaction.h
+++ b/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(); }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 431e970edc11a..22d8fcb67a429 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1067,6 +1067,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" : ""));
 
@@ -1276,7 +1303,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 patch:
diff --git a/test/functional/wallet_orphanedreward.py b/test/functional/wallet_orphanedreward.py
index 7295db46536f1..a617abae8327e 100755
--- a/test/functional/wallet_orphanedreward.py
+++ b/test/functional/wallet_orphanedreward.py
@@ -31,29 +31,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()