Repository: bitcoin/bitcoin (language: cpp)

Issue:
Can't bump fee after abandoning child transaction
Steps:
1. Send transaction (A) to a different wallet
2. Using the change, create another transaction (B) to a different wallet
3. `abandontransaction` B
4. `bumpfee` A fails with `Transaction has descendants in the wallet`


Discussion / hints:
This is particularly annoying now that `-zapwallettxes` has been removed.

<img width="426" alt="Scherm­afbeelding 2022-12-08 om 14 55 29" src="https://user-images.githubusercontent.com/10217/206554825-fa31ba91-73d3-4c13-94dc-b726243eef7b.png">

Having trouble replicating. When I try to `abandontransaction` on B, I get:

`Transaction not eligible for abandonment (code -5)`

I'm using 24.0.
@john-moffett that error happens because the transaction is in your mempool. One way around that is to stop the node, delete `mempool.dat` and start again with `-connect=0 -walletbroadcast=0`.

The first argument makes it so your node doesn't connect to other peers that would echo the transaction back. The second ensures your wallet doesn't put the transaction in the mempool.
@Sjors Thank you! 

It appears that `HasWalletSpend` doesn't make an exception for abandoned transactions:

https://github.com/bitcoin/bitcoin/blob/7d515600034b84ebf23346a3642cf0a714880e25/src/wallet/wallet.cpp#L646-L657

However, `IsSpent` does make an exception:

https://github.com/bitcoin/bitcoin/blob/7d515600034b84ebf23346a3642cf0a714880e25/src/wallet/wallet.cpp#L712-L727

`HasWalletSpend` appears to to be used only by `feebumper` (and tests). I'll take a look tomorrow to see if that will fix the issue.

On a broader level, it's unclear to me why `abandontransaction` won't remove transactions from the mempool and mark them as abandoned. Perhaps there ought to be a `force` option? 
> On a broader level, it's unclear to me why `abandontransaction` won't remove transactions from the mempool and mark them as abandoned. Perhaps there ought to be a `force` option?

Explicitly removing a transaction from our mempool has no effect on the network and could mislead users into thinking that they have "canceled" their transaction. Doing so may surprise users when the "abandoned" transaction ends up confirming, or a subsequent spend ends up not propagating because it conflicts with the abandoned transaction (that all of the other nodes still have in their mempool). However, by allowing abandoning when our own mempool has dropped the transaction, we can more safely assume that other nodes have dropped the transaction as well. Abandoning in this case has a higher likelihood of the transaction actually having been forgotten by a significant portion of the network.

Fix patch:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 2c0ce89929c5d..604fe00cb5025 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -648,8 +648,7 @@ bool CWallet::HasWalletSpend(const CTransactionRef& tx) const
     AssertLockHeld(cs_wallet);
     const uint256& txid = tx->GetHash();
     for (unsigned int i = 0; i < tx->vout.size(); ++i) {
-        auto iter = mapTxSpends.find(COutPoint(txid, i));
-        if (iter != mapTxSpends.end()) {
+        if (IsSpent(COutPoint(txid, i))) {
             return true;
         }
     }


Test patch:
diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
index 07ec55f8f3ceb..0d8b5f3d987e4 100755
--- a/test/functional/wallet_bumpfee.py
+++ b/test/functional/wallet_bumpfee.py
@@ -88,6 +88,7 @@ def run_test(self):
         test_nonrbf_bumpfee_fails(self, peer_node, dest_address)
         test_notmine_bumpfee(self, rbf_node, peer_node, dest_address)
         test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_address)
+        test_bumpfee_with_abandoned_descendant_succeeds(self, rbf_node, rbf_node_address, dest_address)
         test_dust_to_fee(self, rbf_node, dest_address)
         test_watchonly_psbt(self, peer_node, rbf_node, dest_address)
         test_rebumping(self, rbf_node, dest_address)
@@ -286,6 +287,35 @@ def test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_ad
     self.clear_mempool()
 
 
+def test_bumpfee_with_abandoned_descendant_succeeds(self, rbf_node, rbf_node_address, dest_address):
+    self.log.info('Test that fee can be bumped when it has abandoned descendant')
+    # parent is send-to-self, so we don't have to check which output is change when creating the child tx
+    parent_id = spend_one_input(rbf_node, rbf_node_address)
+    # Submit child transaction with low fee
+    child_id = rbf_node.send(outputs={dest_address: 0.00020000},
+                             options={"inputs": [{"txid": parent_id, "vout": 0}], "fee_rate": 2})["txid"]
+    assert child_id in rbf_node.getrawmempool()
+
+    # Restart the node with higher min relay fee so the descendant tx is no longer in mempool so that we can abandon it
+    self.restart_node(1, ['-minrelaytxfee=0.00005'] + self.extra_args[1])
+    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
+    self.connect_nodes(1, 0)
+    assert parent_id in rbf_node.getrawmempool()
+    assert child_id not in rbf_node.getrawmempool()
+    # Should still raise an error even if not in mempool
+    assert_raises_rpc_error(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
+    # Now abandon the child transaction and bump the original
+    rbf_node.abandontransaction(child_id)
+    bumped_result = rbf_node.bumpfee(parent_id, {"fee_rate": HIGH})
+    assert bumped_result['txid'] in rbf_node.getrawmempool()
+    assert parent_id not in rbf_node.getrawmempool()
+    # Cleanup
+    self.restart_node(1, self.extra_args[1])
+    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
+    self.connect_nodes(1, 0)
+    self.clear_mempool()
+
+
 def test_small_output_with_feerate_succeeds(self, rbf_node, dest_address):
     self.log.info('Testing small output with feerate bump succeeds')