rpc: Deprecate totalfee argument in `bumpfee` #15996

instagibbs · 2019-05-09T20:48:35Z

totalFee argument is of questionable use, and should be removed in favor of feerate-based features.

I first moved IsDeprecatedRPCEnabled because bitcoin-wallet doesn't link libbitcoin_server.

instagibbs · 2019-05-09T20:52:19Z

just noting that actual removal of the RPC call will likely call for some additional test-rewriting since most tests are using the totalFee argument for high-precision transaction malleation.

DrahtBot · 2019-05-09T21:04:46Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15911 (Use wallet RBF default for walletcreatefundedpsbt by Sjors)
#15888 (QA: Add wallet_implicitsegwit to test the ability to transform keys between address types by luke-jr)

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

ACK 7ccf9dc. Feel free to ignore the mentioned nits.

Tested beyond code review by compiling and verifying (1) bumpfee help output and (2) that all the wallet_bumpfee functional tests using totalFee raise a JSONRPCException with the expected deprecation message when -deprecatedrpc=totalFee is not passed:

test_small_output_fails
test_dust_to_fee
test_rebumping
test_rebumping_not_replaceable
test_change_script_match

The other subtests in wallet_bumpfee.py all still pass without -deprecatedrpc=totalFee.

jonatack · 2019-05-11T20:33:37Z

src/wallet/rpcwallet.cpp

                "An opt-in RBF transaction with the given txid must be in the wallet.\n"
                "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
-                "If `totalFee` is given, adding inputs is not supported, so there must be a single change output that is big enough or it will fail.\n"
+                "If `totalFee`(DEPRECATED) is given, adding inputs is not supported, so there must be a single change output that is big enough or it will fail.\n"


Perhaps separate totalFee and (DEPRECATED) with a space?

jonatack · 2019-05-11T20:33:56Z

src/wallet/rpcwallet.cpp

                "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 totalFee, or use RPC settxfee to set a higher fee rate.\n"
+                "Alternatively, the user can specify totalFee(DEPRECATED), or use RPC settxfee to set a higher fee rate.\n"


jonatack · 2019-05-11T20:41:39Z

src/wallet/rpcwallet.cpp

                        {
                            {"confTarget", RPCArg::Type::NUM, /* default */ "fallback to wallet's default", "Confirmation target (in blocks)"},
-                            {"totalFee", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis.\n"
+                            {"totalFee", RPCArg::Type::NUM, /* default */ "(DEPRECATED) fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis.\n"


Perhaps display the deprecation similarly to EntryDescriptionString in src/rpc/blockchain.cpp:L387, e.g.:

{"totalFee", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis (DEPRECATED).\n"

jonatack · 2019-05-13T09:08:11Z

Wrote a test for the deprecation itself. Feel free to steal/use/adapt.

instagibbs · 2019-05-13T13:38:56Z

@jonatack I took the deprecation test itself, comment changes seem somewhat unrelated so left those out of this PR in favor of your follow-up PR. Thanks!

jonatack · 2019-05-13T13:51:00Z

@instagibbs All good. Might update the follow-up to removing totalFee from the bumpfee tests if this gets in.

instagibbs · 2019-05-13T13:55:35Z

SGTM! Would appreciate the assist.

promag

Instead of adding a new test why not update test/functional/wallet_bumpfee.py?

jonatack · 2019-05-13T16:13:38Z

Instead of adding a new test why not update test/functional/wallet_bumpfee.py?

I began by doing that and found that this is cleaner, because for now the bumpfee tests need to use totalFee. Testing the deprecation separately makes sense. I also considered doing it in rpc_deprecated.py but this change only concerns an argument, not the rpc itself. When this is removed entirely it also seems easier to just have a separate test file to remove.

The spend_one_input function in both bumpfee testfiles could be extracted, but since one of the two testfiles is destined be removed by 0.2.0 I'm not sure it's worthwhile to do unless other test files could use that function.

jnewbery · 2019-06-12T15:09:15Z

Instead of adding a new test why not update test/functional/wallet_bumpfee.py?

When this is removed entirely it also seems easier to just have a separate test file to remove.

I agree. Adding a separate test for this deprecated option makes it very easy to remove when the deprecated option is removed.

ACK 59bc35c10386a4a58d7835d0a983fdde10eb2400
(reviewed code and run functional tests locally. No manual testing)

etscrivner · 2019-06-12T18:48:52Z

ACK reviewed code and ran functional tests locally. No manual tests.

One thing I do worry about with deprecating this is closing off the possibility of clients using alternative fee-rate estimation algorithms or techniques. Are there more details on why we think the value of this argument is questionable?

instagibbs · 2019-06-12T19:11:35Z

@etscrivner I think it'd be more worthwhile to allow feerate as an argument instead, since only confTarget is allowed right now.

etscrivner · 2019-06-12T20:11:23Z

@instagibbs Agree, that sounds better than totalFee to me.

instagibbs · 2019-06-13T13:31:24Z

@etscrivner #16203 made an issue, if you feel like tackling this as a feature

jnewbery · 2019-06-13T13:37:37Z

@instagibbs I think a valid concern could be that a user might want to bump the fee on a descendant transaction of unconfirmed transactions. The aim would be to bring the feerate of the package up to some desired value. Without being able to specify the totalfee on the child, it would be very difficult to target a feerate for the entire package.

instagibbs · 2019-06-13T13:50:14Z

@jnewbery something to think about sure, but that's an incredibly advances use-case that someone could manually RBF anyways.

jnewbery · 2019-06-13T15:55:04Z

that's an incredibly advances use-case

Yeah, you're right. For a consumer-facing wallet (which is what Bitcoin Core wallet primarily is), fee-bumping child transactions is quite advanced.

fridokus · 2019-06-18T08:35:06Z

ACK 59bc35c10386a4a58d7835d0a983fdde10eb2400
Ran functional tests locally and reviewed code.

instagibbs · 2019-07-11T18:12:30Z

rebased

ryanofsky

Conditional utACK c56947435a79da3b4f3a96dc3acc5effb5fffe68 with suggested change to avoid moving IsDeprecatedRPCEnabled

src/rpc/server.cpp

src/wallet/rpcwallet.cpp

Next steps: remove `totalFee` from the wallet_bumpfee functional tests.

ryanofsky

utACK 2f7eb77. Only change since last review is leaving IsDeprecatedRPCEnabled in its happy home, and switching to rpcEnableDeprecated instead. (Thanks!)

jonatack

ACK 2f7eb77. Built locally, manually tested rpc bumpfee, help output (gist), all tests pass. Travis failures appears to be unrelated, the bitcoin builds are green.

meshcollider

Code Review ACK 2f7eb77

2f7eb77 Add RPC bumpfee totalFee deprecation test (Jon Atack) a92d9ce deprecate totalFee argument in bumpfee RPC call (Gregory Sanders) Pull request description: totalFee argument is of questionable use, and should be removed in favor of feerate-based features. I first moved IsDeprecatedRPCEnabled because `bitcoin-wallet` doesn't link `libbitcoin_server`. ACKs for top commit: ryanofsky: utACK 2f7eb77. Only change since last review is leaving IsDeprecatedRPCEnabled in its happy home, and switching to rpcEnableDeprecated instead. (Thanks!) jonatack: ACK 2f7eb77. Built locally, manually tested rpc bumpfee, help output ([gist](https://gist.github.com/jonatack/863673eacc02f9da39ff6d6712f9d837)), all tests pass. Travis failures appears to be unrelated, the [bitcoin builds are green](https://bitcoinbuilds.org/index.php?build=121). meshcollider: Code Review ACK 2f7eb77 Tree-SHA512: c97465205ee59575df37894bcbb6c4ecf8858dd8fe9d89503f9342b226768c1dcb553153bc9eb3055f7bf5eb41573e48b8efa57e083cd255793cbe5280f0026a

maflcko · 2019-07-29T13:45:06Z

totalFee argument is of questionable use, and should be removed in favor of feerate-based features.

What are those feerate-based features? I couldn't find any in bumpfee. It seems weird to remove a feature that is likely used, without replacement and without mention in the release notes.

ezegom · 2019-07-30T19:27:57Z

totalFee argument is of questionable use, and should be removed in favor of feerate-based features.

What are those feerate-based features? I couldn't find any in bumpfee. It seems weird to remove a feature that is likely used, without replacement and without mention in the release notes.

Currently working on this, see : #16492

fanquake · 2019-07-31T00:34:19Z

Release notes being added in #16504

…e option of bumpfee 73b692b doc: Add release note for the deprecated totalFee option of bumpfee (João Barbosa) Pull request description: Adds release notes for bitcoin#15996. Top commit has no ACKs. Tree-SHA512: 2d75c2fbdd122aa02e808013dd3424843495038bac289b64acc6cc9889bb8ee30d6a91ec2a1b61e6949b1b6e4437331388588d804c81a83757d35d07bf579bc3

c3857c5 wallet: remove CreateTotalBumpTransaction() (Jon Atack) 4a0b27b wallet: remove totalfee from createBumpTransaction() (Jon Atack) e347cfa rpc: remove deprecated totalFee arg from RPC bumpfee (Jon Atack) bd05f96 test: delete wallet_bumpfee_totalfee_deprecation.py (Jon Atack) a6d1ab8 test: update bumpfee testing from totalFee to fee_rate (Jon Atack) Pull request description: Since 0.19, fee-bumping using `totalFee` was deprecated in #15996 and replaced by `fee_rate` in #16727. This changeset removes it. ACKs for top commit: laanwj: ACK c3857c5 Tree-SHA512: c1bb15d664baf4d2dea06981f36384af02057d125c51fcbc8640b9d5563532187c7b84aa952f7b575255a88ce383ed4d7495bec920a47b05b6fc0d432dce1f00

DrahtBot added RPC/REST/ZMQ Tests Wallet labels May 9, 2019

DrahtBot added the Needs rebase label May 10, 2019

instagibbs force-pushed the deprecate_totalfee branch from 7c04b8e to 7ccf9dc Compare May 10, 2019 13:49

DrahtBot removed the Needs rebase label May 10, 2019

jonatack reviewed May 11, 2019

View reviewed changes

jonatack mentioned this pull request May 13, 2019

tests: RPC bumpfee totalFee deprecation #16016

Closed

instagibbs force-pushed the deprecate_totalfee branch from 7ccf9dc to 59bc35c Compare May 13, 2019 13:38

promag reviewed May 13, 2019

View reviewed changes

benthecarman mentioned this pull request May 16, 2019

rpc: Bumpfee units change, satoshis to BTC #15157

Closed

fanquake changed the title ~~Deprecate totalfee argument in bumpfee~~ rpc: Deprecate totalfee argument in bumpfee Jun 18, 2019

fanquake added the Needs release note label Jun 18, 2019

DrahtBot added the Needs rebase label Jul 3, 2019

instagibbs force-pushed the deprecate_totalfee branch from 59bc35c to c569474 Compare July 11, 2019 18:12

DrahtBot removed the Needs rebase label Jul 11, 2019

ryanofsky requested changes Jul 23, 2019

View reviewed changes

src/rpc/server.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

instagibbs and others added 2 commits July 26, 2019 14:09

deprecate totalFee argument in bumpfee RPC call

a92d9ce

Add RPC bumpfee totalFee deprecation test

2f7eb77

Next steps: remove `totalFee` from the wallet_bumpfee functional tests.

instagibbs force-pushed the deprecate_totalfee branch from c569474 to 2f7eb77 Compare July 26, 2019 18:10

ryanofsky approved these changes Jul 26, 2019

View reviewed changes

jonatack reviewed Jul 26, 2019

View reviewed changes

meshcollider reviewed Jul 27, 2019

View reviewed changes

meshcollider merged commit 2f7eb77 into bitcoin:master Jul 27, 2019

maflcko mentioned this pull request Jul 29, 2019

bumpfee.TotalFee removed without replacement #16485

Closed

ezegom mentioned this pull request Jul 30, 2019

rpc: Add feeRate argument to bumpFee RPC #16492

Closed

promag mentioned this pull request Jul 31, 2019

doc: Add release note for the deprecated totalFee option of bumpfee #16504

Merged

fanquake removed the Needs release note label Jul 31, 2019

jonatack mentioned this pull request Mar 10, 2020

wallet: remove deprecated fee bumping by totalFee #18312

Merged

adamjonas mentioned this pull request Jan 14, 2021

bumpfee rpc uses satoshis as amount, not BTC #14815

Closed

maflcko removed the Tests label Jan 15, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

rpc: Deprecate totalfee argument in bumpfee #15996

rpc: Deprecate totalfee argument in bumpfee #15996

Uh oh!

Conversation

instagibbs commented May 9, 2019

Uh oh!

instagibbs commented May 9, 2019

Uh oh!

DrahtBot commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack May 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonatack May 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonatack May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 13, 2019

Uh oh!

instagibbs commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented May 13, 2019

Uh oh!

instagibbs commented May 13, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented May 13, 2019

Uh oh!

jnewbery commented Jun 12, 2019

Uh oh!

etscrivner commented Jun 12, 2019

Uh oh!

instagibbs commented Jun 12, 2019

Uh oh!

etscrivner commented Jun 12, 2019

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

jnewbery commented Jun 13, 2019

Uh oh!

instagibbs commented Jun 13, 2019

Uh oh!

jnewbery commented Jun 13, 2019

Uh oh!

fridokus commented Jun 18, 2019

Uh oh!

instagibbs commented Jul 11, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 29, 2019

Uh oh!

ezegom commented Jul 30, 2019

Uh oh!

fanquake commented Jul 31, 2019

Uh oh!

rpc: Deprecate totalfee argument in `bumpfee` #15996

rpc: Deprecate totalfee argument in `bumpfee` #15996

DrahtBot commented May 9, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack May 11, 2019 •

edited

Loading

instagibbs commented May 13, 2019 •

edited

Loading