Merge #18311: Bumpfee test fix

f1b4503 bumpfee test: exit loop at proper time with new fee value being compared (Gregory Sanders)
2e4edc6 Add some test logging to wallet_bumpfee.py (Gregory Sanders)

Pull request description:

  In the loop we accidentally used `origfee` which is not the value to check, and also allowed the loop to exit too early since the new fee must be strictly greater than `0.0005`.

  Also converted/added a bunch of logging from comments.

  Resolves bitcoin/bitcoin#17716

ACKs for top commit:
  MarcoFalke:
    ACK f1b4503 🏈

Tree-SHA512: eb73297fc82b09b9ec08d85ba3f0bec662119d0ff63ccf5d978a7bad6a674b5915f5ed021ec42f72a732c9ee7af43212d1de87361f50a970df7755caec96f6d8

Author: MarcoFalke

test/functional/wallet_bumpfee.py

@@ -71,28 +71,29 @@ def run_test(self):
         test_simple_bumpfee_succeeds(self, "default", rbf_node, peer_node, dest_address)
         test_simple_bumpfee_succeeds(self, "fee_rate", rbf_node, peer_node, dest_address)
         test_feerate_args(self, rbf_node, peer_node, dest_address)
-        test_segwit_bumpfee_succeeds(rbf_node, dest_address)
-        test_nonrbf_bumpfee_fails(peer_node, dest_address)
-        test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address)
-        test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address)
-        test_small_output_fails(rbf_node, dest_address)
-        test_dust_to_fee(rbf_node, dest_address)
-        test_settxfee(rbf_node, dest_address)
+        test_segwit_bumpfee_succeeds(self, rbf_node, dest_address)
+        test_nonrbf_bumpfee_fails(self, peer_node, dest_address)
+        test_notmine_bumpfee_fails(self, rbf_node, peer_node, dest_address)
+        test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_address)
+        test_small_output_fails(self, rbf_node, dest_address)
+        test_dust_to_fee(self, rbf_node, dest_address)
+        test_settxfee(self, rbf_node, dest_address)
         test_watchonly_psbt(self, peer_node, rbf_node, dest_address)
-        test_rebumping(rbf_node, dest_address)
-        test_rebumping_not_replaceable(rbf_node, dest_address)
-        test_unconfirmed_not_spendable(rbf_node, rbf_node_address)
-        test_bumpfee_metadata(rbf_node, dest_address)
-        test_locked_wallet_fails(rbf_node, dest_address)
-        test_change_script_match(rbf_node, dest_address)
+        test_rebumping(self, rbf_node, dest_address)
+        test_rebumping_not_replaceable(self, rbf_node, dest_address)
+        test_unconfirmed_not_spendable(self, rbf_node, rbf_node_address)
+        test_bumpfee_metadata(self, rbf_node, dest_address)
+        test_locked_wallet_fails(self, rbf_node, dest_address)
+        test_change_script_match(self, rbf_node, dest_address)
         test_maxtxfee_fails(self, rbf_node, dest_address)
         # These tests wipe out a number of utxos that are expected in other tests
-        test_small_output_with_feerate_succeeds(rbf_node, dest_address)
-        test_no_more_inputs_fails(rbf_node, dest_address)
+        test_small_output_with_feerate_succeeds(self, rbf_node, dest_address)
+        test_no_more_inputs_fails(self, rbf_node, dest_address)
         self.log.info("Success")
 
 
 def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
+    self.log.info('Test simple bumpfee')
     rbfid = spend_one_input(rbf_node, dest_address)
     rbftx = rbf_node.gettransaction(rbfid)
     self.sync_mempools((rbf_node, peer_node))
@@ -119,6 +120,7 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
     assert_equal(bumpedwtx["replaces_txid"], rbfid)
 
 def test_feerate_args(self, rbf_node, peer_node, dest_address):
+    self.log.info('Test feerate args')
     rbfid = spend_one_input(rbf_node, dest_address)
     self.sync_mempools((rbf_node, peer_node))
     assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
@@ -135,7 +137,8 @@ def test_feerate_args(self, rbf_node, peer_node, dest_address):
     assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate":1})
 
 
-def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
+def test_segwit_bumpfee_succeeds(self, rbf_node, dest_address):
+    self.log.info('Test that segwit-sourcing bumpfee works')
     # Create a transaction with segwit output, then create an RBF transaction
     # which spends it, and make sure bumpfee can be called on it.
 
@@ -165,14 +168,14 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
     assert rbfid not in rbf_node.getrawmempool()
 
 
-def test_nonrbf_bumpfee_fails(peer_node, dest_address):
-    # cannot replace a non RBF transaction (from node which did not enable RBF)
+def test_nonrbf_bumpfee_fails(self, peer_node, dest_address):
+    self.log.info('Test that we cannot replace a non RBF transaction')
     not_rbfid = peer_node.sendtoaddress(dest_address, Decimal("0.00090000"))
     assert_raises_rpc_error(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
 
 
-def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
-    # cannot bump fee unless the tx has only inputs that we own.
+def test_notmine_bumpfee_fails(self, rbf_node, peer_node, dest_address):
+    self.log.info('Test that it cannot bump fee if non-owned inputs are included')
     # here, the rbftx has a peer_node coin and then adds a rbf_node input
     # Note that this test depends upon the RPC code checking input ownership prior to change outputs
     # (since it can't use fundrawtransaction, it lacks a proper change output)
@@ -192,34 +195,36 @@ def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address):
                             rbf_node.bumpfee, rbfid)
 
 
-def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address):
-    # cannot bump fee if the transaction has a descendant
+def test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_address):
+    self.log.info('Test that fee cannot be bumped when it has 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)
     tx = rbf_node.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000})
     tx = rbf_node.signrawtransactionwithwallet(tx)
     rbf_node.sendrawtransaction(tx["hex"])
     assert_raises_rpc_error(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
 
-def test_small_output_fails(rbf_node, dest_address):
+def test_small_output_fails(self, rbf_node, dest_address):
+    self.log.info('Test totalFee bump with small output fails')
     # cannot bump fee with a too-small output
     rbfid = spend_one_input(rbf_node, dest_address)
     rbf_node.bumpfee(rbfid, {"totalFee": 50000})
 
     rbfid = spend_one_input(rbf_node, dest_address)
     assert_raises_rpc_error(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 50001})
 
-def test_small_output_with_feerate_succeeds(rbf_node, dest_address):
+def test_small_output_with_feerate_succeeds(self, rbf_node, dest_address):
+    self.log.info('Testing small output with feerate bump succeeds')
 
     # Make sure additional inputs exist
     rbf_node.generatetoaddress(101, rbf_node.getnewaddress())
     rbfid = spend_one_input(rbf_node, dest_address)
     input_list = rbf_node.getrawtransaction(rbfid, 1)["vin"]
     assert_equal(len(input_list), 1)
     original_txin = input_list[0]
-    # Keep bumping until we out-spend change output
+    self.log.info('Keep bumping until transaction fee out-spends non-destination value')
     tx_fee = 0
-    while tx_fee < Decimal("0.0005"):
+    while True:
         input_list = rbf_node.getrawtransaction(rbfid, 1)["vin"]
         new_item = list(input_list)[0]
         assert_equal(len(input_list), 1)
@@ -231,7 +236,11 @@ def test_small_output_with_feerate_succeeds(rbf_node, dest_address):
         assert rbfid not in raw_pool
         assert rbfid_new in raw_pool
         rbfid = rbfid_new
-        tx_fee = rbfid_new_details["origfee"]
+        tx_fee = rbfid_new_details["fee"]
+
+        # Total value from input not going to destination
+        if tx_fee > Decimal('0.00050000'):
+            break
 
     # input(s) have been added
     final_input_list = rbf_node.getrawtransaction(rbfid, 1)["vin"]
@@ -244,8 +253,8 @@ def test_small_output_with_feerate_succeeds(rbf_node, dest_address):
     rbf_node.generatetoaddress(1, rbf_node.getnewaddress())
     assert_equal(rbf_node.gettransaction(rbfid)["confirmations"], 1)
 
-def test_dust_to_fee(rbf_node, dest_address):
-    # check that if output is reduced to dust, it will be converted to fee
+def test_dust_to_fee(self, rbf_node, dest_address):
+    self.log.info('Test that bumped output that is dust is dropped to fee')
     # the bumped tx sets fee=49,900, but it converts to 50,000
     rbfid = spend_one_input(rbf_node, dest_address)
     fulltx = rbf_node.getrawtransaction(rbfid, 1)
@@ -257,7 +266,8 @@ def test_dust_to_fee(rbf_node, dest_address):
     assert_equal(len(full_bumped_tx["vout"]), 1)  # change output is eliminated
 
 
-def test_settxfee(rbf_node, dest_address):
+def test_settxfee(self, rbf_node, dest_address):
+    self.log.info('Test settxfee')
     assert_raises_rpc_error(-8, "txfee cannot be less than min relay tx fee", rbf_node.settxfee, Decimal('0.000005'))
     assert_raises_rpc_error(-8, "txfee cannot be less than wallet min fee", rbf_node.settxfee, Decimal('0.000015'))
     # check that bumpfee reacts correctly to the use of settxfee (paytxfee)
@@ -272,17 +282,19 @@ def test_settxfee(rbf_node, dest_address):
     rbf_node.settxfee(Decimal("0.00000000"))  # unset paytxfee
 
 
-def test_maxtxfee_fails(test, rbf_node, dest_address):
+def test_maxtxfee_fails(self, rbf_node, dest_address):
+    self.log.info('Test that bumpfee fails when it hits -matxfee')
     # size of bumped transaction (p2wpkh, 1 input, 2 outputs): 141 vbytes
     # expected bumping feerate of 20 sats/vbyte => 141*20 sats = 0.00002820 btc
-    test.restart_node(1, ['-maxtxfee=0.000025'] + test.extra_args[1])
+    self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1])
     rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
     rbfid = spend_one_input(rbf_node, dest_address)
     assert_raises_rpc_error(-4, "Unable to create transaction: Fee exceeds maximum configured by -maxtxfee", rbf_node.bumpfee, rbfid)
-    test.restart_node(1, test.extra_args[1])
+    self.restart_node(1, self.extra_args[1])
     rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
 
-def test_watchonly_psbt(test, peer_node, rbf_node, dest_address):
+def test_watchonly_psbt(self, peer_node, rbf_node, dest_address):
+    self.log.info('Test that PSBT is returned for bumpfee in watchonly wallets')
     priv_rec_desc = "wpkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#rweraev0"
     pub_rec_desc = rbf_node.getdescriptorinfo(priv_rec_desc)["descriptor"]
     priv_change_desc = "wpkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#j6uzqvuh"
@@ -334,7 +346,7 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address):
     funding_address2 = watcher.getnewaddress(address_type='bech32')
     peer_node.sendmany("", {funding_address1: 0.001, funding_address2: 0.001})
     peer_node.generate(1)
-    test.sync_all()
+    self.sync_all()
 
     # Create single-input PSBT for transaction to be bumped
     psbt = watcher.walletcreatefundedpsbt([], {dest_address:0.0005}, 0, {"feeRate": 0.00001}, True)['psbt']
@@ -363,24 +375,24 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address):
     rbf_node.unloadwallet("watcher")
     rbf_node.unloadwallet("signer")
 
-def test_rebumping(rbf_node, dest_address):
-    # check that re-bumping the original tx fails, but bumping the bumper succeeds
+def test_rebumping(self, rbf_node, dest_address):
+    self.log.info('Test that re-bumping the original tx fails, but bumping successor works')
     rbfid = spend_one_input(rbf_node, dest_address)
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 2000})
     assert_raises_rpc_error(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 3000})
     rbf_node.bumpfee(bumped["txid"], {"totalFee": 3000})
 
 
-def test_rebumping_not_replaceable(rbf_node, dest_address):
-    # check that re-bumping a non-replaceable bump tx fails
+def test_rebumping_not_replaceable(self, rbf_node, dest_address):
+    self.log.info('Test that re-bumping non-replaceable fails')
     rbfid = spend_one_input(rbf_node, dest_address)
     bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False})
     assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
                             {"totalFee": 20000})
 
 
-def test_unconfirmed_not_spendable(rbf_node, rbf_node_address):
-    # check that unconfirmed outputs from bumped transactions are not spendable
+def test_unconfirmed_not_spendable(self, rbf_node, rbf_node_address):
+    self.log.info('Test that unconfirmed outputs from bumped txns are not spendable')
     rbfid = spend_one_input(rbf_node, rbf_node_address)
     rbftx = rbf_node.gettransaction(rbfid)["hex"]
     assert rbfid in rbf_node.getrawmempool()
@@ -418,7 +430,8 @@ def test_unconfirmed_not_spendable(rbf_node, rbf_node_address):
             if t["txid"] == rbfid and t["address"] == rbf_node_address and t["spendable"]), 1)
 
 
-def test_bumpfee_metadata(rbf_node, dest_address):
+def test_bumpfee_metadata(self, rbf_node, dest_address):
+    self.log.info('Test that bumped txn metadata persists to new txn record')
     assert(rbf_node.getbalance() < 49)
     rbf_node.generatetoaddress(101, rbf_node.getnewaddress())
     rbfid = rbf_node.sendtoaddress(dest_address, 49, "comment value", "to value")
@@ -428,15 +441,17 @@ def test_bumpfee_metadata(rbf_node, dest_address):
     assert_equal(bumped_wtx["to"], "to value")
 
 
-def test_locked_wallet_fails(rbf_node, dest_address):
+def test_locked_wallet_fails(self, rbf_node, dest_address):
+    self.log.info('Test that locked wallet cannot bump txn')
     rbfid = spend_one_input(rbf_node, dest_address)
     rbf_node.walletlock()
     assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first.",
                             rbf_node.bumpfee, rbfid)
     rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
 
-def test_change_script_match(rbf_node, dest_address):
-    """Test that the same change addresses is used for the replacement transaction when possible."""
+def test_change_script_match(self, rbf_node, dest_address):
+    self.log.info('Test that the same change addresses is used for the replacement transaction when possible.')
+
     def get_change_address(tx):
         tx_details = rbf_node.getrawtransaction(tx, 1)
         txout_addresses = [txout['scriptPubKey']['addresses'][0] for txout in tx_details["vout"]]
@@ -480,7 +495,8 @@ def submit_block_with_tx(node, tx):
     node.submitblock(block.serialize().hex())
     return block
 
-def test_no_more_inputs_fails(rbf_node, dest_address):
+def test_no_more_inputs_fails(self, rbf_node, dest_address):
+    self.log.info('Test that bumpfee fails when there are no available confirmed outputs')
     # feerate rbf requires confirmed outputs when change output doesn't exist or is insufficient
     rbf_node.generatetoaddress(1, dest_address)
     # spend all funds, no change output