rpc: Deprecate totalfee argument in `bumpfee` #15996

instagibbs commented at 8:48 PM on May 9, 2019: member

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 commented at 8:52 PM on May 9, 2019: member

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 added the label RPC/REST/ZMQ on May 9, 2019

DrahtBot added the label Tests on May 9, 2019

DrahtBot added the label Wallet on May 9, 2019

DrahtBot commented at 9:04 PM on May 9, 2019: 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:

#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.

DrahtBot added the label Needs rebase on May 10, 2019

instagibbs force-pushed on May 10, 2019

DrahtBot removed the label Needs rebase on May 10, 2019

in src/wallet/rpcwallet.cpp:3256 in 7ccf9dc2bd outdated

3252 | @@ -3253,20 +3253,20 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3253 |                  "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3254 |                  "An opt-in RBF transaction with the given txid must be in the wallet.\n"
3255 |                  "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"
3256 | -                "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"
3257 | +                "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"

jonatack commented at 8:33 PM on May 11, 2019:

Perhaps separate totalFee and (DEPRECATED) with a space?

in src/wallet/rpcwallet.cpp:3261 in 7ccf9dc2bd outdated

3258 |                  "All inputs in the original transaction will be included in the replacement transaction.\n"
3259 |                  "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3260 |                  "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3261 |                  "The user can specify a confirmation target for estimatesmartfee.\n"
3262 | -                "Alternatively, the user can specify totalFee, or use RPC settxfee to set a higher fee rate.\n"
3263 | +                "Alternatively, the user can specify totalFee(DEPRECATED), or use RPC settxfee to set a higher fee rate.\n"

jonatack commented at 8:33 PM on May 11, 2019:

Idem.

in src/wallet/rpcwallet.cpp:3269 in 7ccf9dc2bd outdated

3267 |                      {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The txid to be bumped"},
3268 |                      {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
3269 |                          {
3270 |                              {"confTarget", RPCArg::Type::NUM, /* default */ "fallback to wallet's default", "Confirmation target (in blocks)"},
3271 | -                            {"totalFee", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis.\n"
3272 | +                            {"totalFee", RPCArg::Type::NUM, /* default */ "(DEPRECATED) fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis.\n"

jonatack commented at 8:41 PM on May 11, 2019:

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 commented at 9:09 PM on May 11, 2019: member

ACK 7ccf9dc2bd05caa0952c6e7ed438e9b265d808c1. 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 commented at 9:08 AM on May 13, 2019: member

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

instagibbs force-pushed on May 13, 2019

instagibbs commented at 1:38 PM on May 13, 2019: member

@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 commented at 1:51 PM on May 13, 2019: member

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

instagibbs commented at 1:55 PM on May 13, 2019: member

SGTM! Would appreciate the assist.

promag commented at 3:41 PM on May 13, 2019: member

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

jonatack commented at 4:13 PM on May 13, 2019: member

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 commented at 3:09 PM on June 12, 2019: member

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 commented at 6:48 PM on June 12, 2019: contributor

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 commented at 7:11 PM on June 12, 2019: member

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

etscrivner commented at 8:11 PM on June 12, 2019: contributor

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

instagibbs commented at 1:31 PM on June 13, 2019: member

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

jnewbery commented at 1:37 PM on June 13, 2019: member

@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 commented at 1:50 PM on June 13, 2019: member

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

jnewbery commented at 3:55 PM on June 13, 2019: member

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.

fanquake renamed this:
~~Deprecate totalfee argument in `bumpfee`~~
rpc: Deprecate totalfee argument in `bumpfee`
on Jun 18, 2019

fanquake added the label Needs release note on Jun 18, 2019

fridokus commented at 8:35 AM on June 18, 2019: contributor

ACK 59bc35c10386a4a58d7835d0a983fdde10eb2400 Ran functional tests locally and reviewed code.

DrahtBot added the label Needs rebase on Jul 3, 2019

instagibbs force-pushed on Jul 11, 2019

instagibbs commented at 6:12 PM on July 11, 2019: member

rebased

DrahtBot removed the label Needs rebase on Jul 11, 2019

in src/rpc/server.cpp:331 in c56947435a outdated

 327 | @@ -328,13 +328,6 @@ bool RPCIsInWarmup(std::string *outStatus)
 328 |      return fRPCInWarmup;
 329 |  }
 330 |  
 331 | -bool IsDeprecatedRPCEnabled(const std::string& method)

ryanofsky commented at 7:08 PM on July 23, 2019:

This should not actually be moved from server to util, since it's accessing global state of the RPC server. I'm assuming there were link errors trying to build without this change, but these would be fixed by calling Chain::rpcEnableDeprecated (see below).

instagibbs commented at 6:11 PM on July 26, 2019:

reverted

in src/wallet/rpcwallet.cpp:3328 in c56947435a outdated

3324 | @@ -3325,6 +3325,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3325 |          } else if (options.exists("confTarget")) { // TODO: alias this to conf_target
3326 |              coin_control.m_confirm_target = ParseConfirmTarget(options["confTarget"], pwallet->chain().estimateMaxBlocks());
3327 |          } else if (options.exists("totalFee")) {
3328 | +            if (!IsDeprecatedRPCEnabled("totalFee")) {

ryanofsky commented at 7:14 PM on July 23, 2019:

This should call rpcEnableDeprecated instead of IsDeprecatedRPCEnabled to support wallets running in different processes from the RPC server. The aren't current examples of rpcEnableDeprecated in use, but you can see the last one that was removed in b4338c151d4788c33f4b7c54daaf7f94b193a624

instagibbs commented at 6:10 PM on July 26, 2019:

fixed

ryanofsky changes_requested

ryanofsky commented at 7:18 PM on July 23, 2019: member

Conditional utACK c56947435a79da3b4f3a96dc3acc5effb5fffe68 with suggested change to avoid moving IsDeprecatedRPCEnabled

deprecate totalFee argument in bumpfee RPC call a92d9ce8cf

Add RPC bumpfee totalFee deprecation test

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

2f7eb772f6

instagibbs force-pushed on Jul 26, 2019

ryanofsky approved

ryanofsky commented at 6:18 PM on July 26, 2019: member

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

jonatack commented at 7:25 PM on July 26, 2019: member

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

meshcollider commented at 10:20 AM on July 27, 2019: contributor

Code Review ACK 2f7eb772f6250442d4a0071318047cb2deeb31fa

meshcollider merged this on Jul 27, 2019

meshcollider closed this on Jul 27, 2019

meshcollider referenced this in commit c606e6fc53 on Jul 27, 2019

MarcoFalke commented at 1:45 PM on July 29, 2019: member

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 commented at 7:27 PM on July 30, 2019: contributor

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 removed the label Needs release note on Jul 31, 2019

fanquake commented at 12:34 AM on July 31, 2019: member

Release notes being added in #16504

MarcoFalke referenced this in commit 3f288a1c05 on Jul 31, 2019

laanwj referenced this in commit 694f4cbd78 on Mar 26, 2020

MarcoFalke removed the label Tests on Jan 15, 2021

DrahtBot locked this on Aug 16, 2022