wallet: remove deprecated fee bumping by totalFee #18312

jonatack commented at 8:01 PM on March 10, 2020: member

Since 0.19, fee-bumping using totalFee was deprecated in #15996 and replaced by fee_rate in #16727. This changeset removes it.

DrahtBot added the label GUI on Mar 10, 2020

DrahtBot added the label RPC/REST/ZMQ on Mar 10, 2020

DrahtBot added the label Tests on Mar 10, 2020

DrahtBot added the label Wallet on Mar 10, 2020

in test/functional/wallet_bumpfee.py:378 in 066c6a6174 outdated

 345 | @@ -375,21 +346,6 @@ def test_watchonly_psbt(self, peer_node, rbf_node, dest_address):
 346 |      rbf_node.unloadwallet("watcher")
 347 |      rbf_node.unloadwallet("signer")
 348 |  
 349 | -def test_rebumping(self, rbf_node, dest_address):

instagibbs commented at 8:27 PM on March 10, 2020:

this seems easily updateable to feerate and worthwhile to check?

jonatack commented at 11:37 PM on March 13, 2020:

thanks -- done

in test/functional/wallet_bumpfee.py:386 in 066c6a6174 outdated

 352 | -    bumped = rbf_node.bumpfee(rbfid, {"totalFee": 2000})
 353 | -    assert_raises_rpc_error(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 3000})
 354 | -    rbf_node.bumpfee(bumped["txid"], {"totalFee": 3000})
 355 | -
 356 | -
 357 | -def test_rebumping_not_replaceable(self, rbf_node, dest_address):

instagibbs commented at 8:27 PM on March 10, 2020:

this seems easily updateable to feerate and worthwhile to check?

jonatack commented at 11:37 PM on March 13, 2020:

thanks -- done

in test/functional/wallet_bumpfee.py:256 in 066c6a6174 outdated

 235 | @@ -253,18 +236,6 @@ def test_small_output_with_feerate_succeeds(self, rbf_node, dest_address):
 236 |      rbf_node.generatetoaddress(1, rbf_node.getnewaddress())
 237 |      assert_equal(rbf_node.gettransaction(rbfid)["confirmations"], 1)
 238 |  
 239 | -def test_dust_to_fee(self, rbf_node, dest_address):

instagibbs commented at 8:28 PM on March 10, 2020:

might want to think about how to convert this test to feerate. It seems like a worthwhile testing gap currently.

jonatack commented at 11:37 PM on March 13, 2020:

thanks! -- done

in test/functional/wallet_bumpfee.py:207 in 066c6a6174 outdated

 194 | @@ -204,14 +195,6 @@ def test_bumpfee_with_descendant_fails(self, rbf_node, rbf_node_address, dest_ad
 195 |      rbf_node.sendrawtransaction(tx["hex"])
 196 |      assert_raises_rpc_error(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id)
 197 |  
 198 | -def test_small_output_fails(self, rbf_node, dest_address):

instagibbs commented at 8:28 PM on March 10, 2020:

this is a correct deletion, feerate arg succeeds where totalFee fails :+1:

DrahtBot commented at 8:37 PM on March 10, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18354 (Protect wallet by using shared pointers by bvbfan)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonatack force-pushed on Mar 10, 2020

jonatack commented at 9:01 PM on March 10, 2020: member

Thanks @instagibbs for the quick feedback -- looking into it.

fanquake requested review from achow101 on Mar 10, 2020

fanquake requested review from meshcollider on Mar 10, 2020

fanquake removed the label GUI on Mar 10, 2020

in test/functional/wallet_bumpfee.py:128 in 59c1e0a428 outdated

 119 | @@ -125,10 +120,6 @@ def test_feerate_args(self, rbf_node, peer_node, dest_address):
 120 |      self.sync_mempools((rbf_node, peer_node))
 121 |      assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
 122 |  
 123 | -    assert_raises_rpc_error(-8, "confTarget can't be set with totalFee or fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.", rbf_node.bumpfee, rbfid, {"fee_rate":0.00001, "confTarget":1})

kallewoof commented at 5:33 AM on March 12, 2020:

This test is still valid, so shouldn't be removed (the message changed though).

jonatack commented at 11:36 PM on March 13, 2020:

Thanks! -- done.

kallewoof commented at 5:37 AM on March 12, 2020: member

Concept ACK and code review with comments 59c1e0a428651ed4c7ea23ad6cfca0429069e232

Verified that the total fee feature is not used anywhere in the code right now (createBumpTransaction's total_fee is set to the constant 0 in the one call to the method, in walletmodel.cpp, in master).

Agree with @instagibbs points about converting existing tests rather than deleting them.

jonatack force-pushed on Mar 13, 2020

jonatack commented at 11:44 PM on March 13, 2020: member

Thanks @instagibbs and @kallewoof for reviewing. I've updated the existing tests that used totalFee and tried to convert test_dust_to_fee too. Could you have a look? Thanks again.

jonatack renamed this:
~~wallet: remove deprecated totalFee argument from fee bumping~~
wallet: remove deprecated fee bumping by totalFee
on Mar 13, 2020

jnewbery commented at 12:51 AM on March 19, 2020: member

Concept ACK

in test/functional/wallet_bumpfee.py:35 in d7da1fa2fd outdated

  29 | @@ -30,14 +30,20 @@
  30 |  WALLET_PASSPHRASE = "test"
  31 |  WALLET_PASSPHRASE_TIMEOUT = 3600
  32 |  
  33 | +# Fee rates
  34 | +INSUFFICIENT = 0.00001000
  35 | +ECONOMY      = 0.00050000

kallewoof commented at 3:22 AM on March 22, 2020:

μNit (ignore unless you rebase or do further updates): ECONOMICAL?

jonatack commented at 10:13 AM on March 22, 2020:

Good catch! git grep -ni econom shows the codebase and rpc help use that. Done.

kallewoof commented at 3:27 AM on March 22, 2020: member

Re-ACK 9dcee5fcd8ba9aba9fadfd4de008d846f022dfc6

test: update bumpfee testing from totalFee to fee_rate a6d1ab8caa

test: delete wallet_bumpfee_totalfee_deprecation.py bd05f96d79

jonatack force-pushed on Mar 22, 2020

jonatack commented at 10:10 AM on March 22, 2020: member

Thanks for reviewing @kallewoof. Updated constant name as per git diff 9dcee5f..abf2ab15. The Travis CI failure is unrelated.

kallewoof commented at 10:18 AM on March 22, 2020: member

Re-ACK abf2ab1

diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
index 5aa6a1b6f..38c980775 100755
--- a/test/functional/wallet_bumpfee.py
+++ b/test/functional/wallet_bumpfee.py
@@ -30,9 +30,9 @@ from test_framework.util import (
 WALLET_PASSPHRASE = "test"
 WALLET_PASSPHRASE_TIMEOUT = 3600

-# Fee rates
+# Fee rates (in BTC per 1000 vbytes)
 INSUFFICIENT = 0.00001000
-ECONOMY      = 0.00050000
+ECONOMICAL   = 0.00050000
 NORMAL       = 0.00100000
 HIGH         = 0.00500000
 TOO_HIGH     = 1.00000000
@@ -379,7 +379,7 @@ def test_watchonly_psbt(self, peer_node, rbf_node, dest_address):
 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, {"fee_rate": ECONOMY})
+    bumped = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL})
     assert_raises_rpc_error(-4, "already bumped", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL})
     rbf_node.bumpfee(bumped["txid"], {"fee_rate": NORMAL})

@@ -387,7 +387,7 @@ def test_rebumping(self, rbf_node, dest_address):
 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, {"fee_rate": ECONOMY, "replaceable": False})
+    bumped = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL, "replaceable": False})
     assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"],
                             {"fee_rate": NORMAL})

@@ -464,7 +464,7 @@ def test_change_script_match(self, rbf_node, dest_address):
     assert_equal(len(change_addresses), 1)

     # Now find that address in each subsequent tx, and no other change
-    bumped_total_tx = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMY})
+    bumped_total_tx = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL})
     assert_equal(change_addresses, get_change_address(bumped_total_tx['txid']))
     bumped_rate_tx = rbf_node.bumpfee(bumped_total_tx["txid"])
     assert_equal(change_addresses, get_change_address(bumped_rate_tx['txid']))

</details>

laanwj added this to the milestone 0.20.0 on Mar 26, 2020

rpc: remove deprecated totalFee arg from RPC bumpfee e347cfa9a7

wallet: remove totalfee from createBumpTransaction() 4a0b27bb01

wallet: remove CreateTotalBumpTransaction() c3857c5fcb

in src/wallet/rpcwallet.cpp:3349 in abf2ab1588 outdated

3346 |                  "All inputs in the original transaction will be included in the replacement transaction.\n"
3347 |                  "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3348 |                  "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3349 |                  "The user can specify a confirmation target for estimatesmartfee.\n"
3350 | -                "Alternatively, the user can specify totalFee (DEPRECATED), or fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction .\n"
3351 | +                "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction .\n"

laanwj commented at 4:43 PM on March 26, 2020:

nit: If you're changing this line anyway, please remove the space before the ..

jonatack commented at 4:58 PM on March 26, 2020:

thanks -- good eye!

jonatack force-pushed on Mar 26, 2020

jonatack commented at 5:02 PM on March 26, 2020: member

Fixup to remove an extra space as per git diff abf2ab1..c3857c5.

laanwj commented at 5:33 PM on March 26, 2020: member

ACK c3857c5fcb21836ddc1b79a6b19cffe562cade10

Only difference before is the space doc change:

 $ git diff abf2ab1 c3857c5
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 7df846e0fc57053d9c082ff232804d7a83e29bbd..05342472bbeeb6484b947dff649810a55723f563 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3346,7 +3346,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
                 "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
                 "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
                 "The user can specify a confirmation target for estimatesmartfee.\n"
-                "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction .\n"
+                "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction.\n"
                 "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
                 "returned by getnetworkinfo) to enter the node's mempool.\n",
                 {

laanwj merged this on Mar 26, 2020

laanwj closed this on Mar 26, 2020

jonatack deleted the branch on Mar 26, 2020

jonatack commented at 5:43 PM on March 26, 2020: member

Thanks -- will add a release note on the dev wiki.

jnewbery commented at 5:54 PM on March 26, 2020: member

utACK c3857c5fcb21836ddc1b79a6b19cffe562cade10

MarcoFalke removed the label Tests on Mar 26, 2020

DrahtBot locked this on Feb 15, 2022