wallet: Explicit feerate for bumpfee #16727

pull instagibbs wants to merge 4 commits into bitcoin:master from instagibbs:feerate_bumpfee changing 3 files +131 −33
  1. instagibbs commented at 3:25 PM on August 26, 2019: member

    Taking over for #16492 which seems to have gone inactive.

    Only minor commit cleanups, rebase, and some help text fixes on top of previous PR. Renamed feeRate to fee_rate to reflect updated guidelines.

  2. rpc bumpfee: move feerate estimation logic into separate method 1a4c791cf4
  3. in test/functional/wallet_bumpfee.py:98 in 664f08b53f outdated 
      94 |      rbfid = spend_one_input(rbf_node, dest_address)
  95 |      rbftx = rbf_node.gettransaction(rbfid)
  96 |      self.sync_mempools((rbf_node, peer_node))
  97 |      assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool()
  98 | -    bumped_tx = rbf_node.bumpfee(rbfid)
  99 | +    if(mode == "fee_rate"):
    promag commented at 3:41 PM on August 26, 2019:

    Drop ().

    instagibbs commented at 6:43 PM on September 13, 2019:

    fixed

  4. instagibbs force-pushed on Aug 26, 2019
  5. instagibbs commented at 4:26 PM on August 26, 2019: member

    Added some additional basic argument testing, changed the logic to use BTC/kB like everywhere else.

  6. instagibbs force-pushed on Aug 26, 2019
  7. DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2019
  8. DrahtBot added the label Tests on Aug 26, 2019
  9. DrahtBot added the label Wallet on Aug 26, 2019
  10. DrahtBot commented at 7:13 PM on August 26, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

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

  11. fanquake renamed this:
    Explicit feerate for bumpfee
    wallet: Explicit feerate for bumpfee
    on Aug 27, 2019
  12. fanquake removed the label Tests on Aug 27, 2019
  13. Sjors commented at 10:01 AM on August 27, 2019: member

    Concept ACK.

    Alternatively you could rebase on #11413 and use that syntax instead, which is easier to use on the command line than a JSON options object.

  14. Relaxo143 commented at 10:41 AM on August 27, 2019: none

    Why does Bitcoin Core use btc/kB? Every other service/wallet seems to use sats/byte.

  15. Sjors commented at 11:41 AM on August 27, 2019: member

    @Relaxo143 you'll be pleased to see #11413 then...

  16. instagibbs commented at 11:45 AM on August 27, 2019: member

    @relaxo143 Unless you allow decimal points it will have less precision.

    On Tue, Aug 27, 2019, 6:44 AM Relaxo143 notifications@github.com wrote:

    Why does Bitcoin Core use btc/kB? Every other service/wallet seems to use sats/byte.

    — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16727?email_source=notifications&email_token=ABMAFUYWXNZF556SIQ4CPFTQGUAQXA5CNFSM4IPQ24D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5HJXSY#issuecomment-525245387, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMAFUZSS2H575PNQTKBVL3QGUAQXANCNFSM4IPQ24DQ .

  17. promag commented at 3:43 PM on August 27, 2019: member

    Concept ACK.

  18. luke-jr commented at 11:12 PM on September 2, 2019: member

    Renamed feeRate to fee_rate to reflect updated guidelines.

    Every other RPC uses "feeRate"...

  19. instagibbs commented at 1:28 PM on September 3, 2019: member

    Every other RPC uses "feeRate"...

    We used to use confTarget, but now have conf_target in use in various newer args. I see no reason to introduce old styles for old style sake.

  20. instagibbs force-pushed on Sep 13, 2019
  21. instagibbs commented at 6:45 PM on September 13, 2019: member

    going to leave #11413 type ideas alone for now.

    I think this is a pretty simple feature and I'd like it in.

  22. in src/wallet/rpcwallet.cpp:3354 in 33911b9d20 outdated 
    3352 |              },
3353 |              true, true);
3354 | -
3355 | -        if (options.exists("confTarget") && options.exists("totalFee")) {
3356 | -            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction.");
3357 | +        // EZTODO: If FeeRate is passed in BUT its 0, give JSONRPCError.
    MarcoFalke commented at 6:37 PM on September 17, 2019:

    :eyes:

    instagibbs commented at 6:42 PM on September 17, 2019:

    lol, will change

    instagibbs commented at 1:46 PM on September 23, 2019:

    fixed

  23. instagibbs force-pushed on Sep 23, 2019
  24. MarcoFalke added this to the milestone 0.19.0 on Sep 23, 2019
  25. in src/wallet/rpcwallet.cpp:3293 in ad4eb894b6 outdated 
    3289 | @@ -3290,7 +3290,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3290 |                  "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3291 |                  "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3292 |                  "The user can specify a confirmation target for estimatesmartfee.\n"
3293 | -                "Alternatively, the user can specify totalFee (DEPRECATED), or use RPC settxfee to set a higher fee rate.\n"
3294 | +                "Alternatively, the user can specify totalFee (DEPRECATED), or fee_rate (in satoshis per K) for the new transaction .\n"
    Sjors commented at 5:15 PM on September 27, 2019:

    "BTC per kB"

  26. in src/wallet/rpcwallet.cpp:3305 in ad4eb894b6 outdated 
    3301 | @@ -3302,6 +3302,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3302 |              "                         In rare cases, the actual fee paid might be slightly higher than the specified\n"
3303 |              "                         totalFee if the tx change output has to be removed because it is too close to\n"
3304 |              "                         the dust threshold."},
3305 | +                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "FeeRate (NOT total fee) to pay, in " + CURRENCY_UNIT + "\n"
    Sjors commented at 5:22 PM on September 27, 2019:

    CURRENCY_UNIT + "per kB\n"

  27. in src/wallet/rpcwallet.cpp:3306 in ad4eb894b6 outdated 
    3301 | @@ -3302,6 +3302,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3302 |              "                         In rare cases, the actual fee paid might be slightly higher than the specified\n"
3303 |              "                         totalFee if the tx change output has to be removed because it is too close to\n"
3304 |              "                         the dust threshold."},
3305 | +                            {"fee_rate", RPCArg::Type::NUM, /* default */ "fallback to 'confTarget'", "FeeRate (NOT total fee) to pay, in " + CURRENCY_UNIT + "\n"
3306 | +            "                         The fee_rate argument allows the users to use their own fee estimator. The resulting bump transaction will "
    Sjors commented at 5:25 PM on September 27, 2019:

    \n missing

    Sjors doesn't like being addressed in third person. Also there's no need to explain that a fee rate determines the total fee; we don't do that in other places either. More importantly, this doesn't cover the actual RBF rules.

    What about: "Specify a specific fee rate instead of relying on the built-in fee estimator. Must be at least 0.0001 BTC per kB higher than the current rate."

    Fun fact: there's no convenient way to find the current fee rate.

  28. in src/wallet/rpcwallet.cpp:3355 in ad4eb894b6 outdated 
    3353 |              true, true);
3354 | -
3355 | -        if (options.exists("confTarget") && options.exists("totalFee")) {
3356 | -            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction.");
3357 | +        if (options.exists("confTarget") && (options.exists("totalFee") || options.exists("fee_rate"))) {
3358 | +            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget can't be set with totalFee or fee_rate. Please provide either a conftarget for fee estimation, or an explicit total fee or fee rate for the transaction.");
    Sjors commented at 5:37 PM on September 27, 2019:

    "either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate (or an absolute fee amount - deprecated)."

    instagibbs commented at 11:34 AM on September 28, 2019:

    not going to tell users about a deprecated arg here, otherwise taking suggestion

  29. in src/wallet/rpcwallet.cpp:3368 in ad4eb894b6 outdated 
    3364 | @@ -3360,6 +3365,12 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3365 |              if (totalFee <= 0) {
3366 |                  throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee %s (must be greater than 0)", FormatMoney(totalFee)));
3367 |              }
3368 | +        } else if (options.exists("fee_rate")) {
    Sjors commented at 5:38 PM on September 27, 2019:

    May want to check that it's not null.

  30. Sjors changes_requested
  31. Sjors commented at 5:46 PM on September 27, 2019: member

    Quite a bit of code duplication in 74b271616016a5efd03d96c3077bdac79c38b134 rpc bumpfee check fee_rate argument. Can you add a helper?

    Also left some documentation nits as of d086d25.

  32. rpc bumpfee: add fee_rate argument 88e5f997df
  33. rpc bumpfee check fee_rate argument 9f25de3d9e
  34. instagibbs force-pushed on Sep 28, 2019
  35. instagibbs commented at 11:39 AM on September 28, 2019: member

    Fixed most nits, @Sjors if you're referring to code duplication with respect to TotalBump, I don't really feel like optimizing a piece of code we're going to rip out in 0.20.

  36. laanwj commented at 9:23 AM on September 30, 2019: member

    The bumpfee test is failing in the CIs.

  37. laanwj added the label Waiting for author on Sep 30, 2019
  38. test bumpfee fee_rate argument c812aba394
  39. instagibbs force-pushed on Sep 30, 2019
  40. instagibbs commented at 1:40 PM on September 30, 2019: member

    pushed fixed test

  41. Sjors commented at 3:31 PM on September 30, 2019: member

    Code review ACK c812aba

  42. MarcoFalke added the label Needs release note on Oct 1, 2019
  43. MarcoFalke removed the label Waiting for author on Oct 1, 2019
  44. in src/wallet/feebumper.cpp:126 in c812aba394 
     121 | +
 122 | +    // The node has a configurable incremental relay fee. Increment the fee by
 123 | +    // the minimum of that and the wallet's conservative
 124 | +    // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to
 125 | +    // network wide policy for incremental relay fee that our node may not be
 126 | +    // aware of. This ensures we're over the over the required relay fee rate
    laanwj commented at 1:26 PM on October 2, 2019:

    "over the over the" Though I see this typo exists in the old code as well so there's no need to fix that in this PR.

  45. in src/wallet/feebumper.cpp:317 in c812aba394 
     337 | -    new_coin_control.m_feerate = std::max(feerate, min_feerate);
 338 | +    if (coin_control.m_feerate) {
 339 | +        // The user provided a feeRate argument.
 340 | +        // We calculate this here to avoid compiler warning on the cs_wallet lock
 341 | +        const int64_t maxTxSize = CalculateMaximumSignedTxSize(*wtx.tx, wallet);
 342 | +        Result res = CheckFeeRate(wallet, wtx, *(new_coin_control.m_feerate), maxTxSize, errors);
    laanwj commented at 1:31 PM on October 2, 2019:

    Why the set of parentheses around new_coin_control.m_feerate?

  46. in src/wallet/feebumper.cpp:71 in c812aba394 
      66 | +    // moment earlier. In this case, we report an error to the user, who may adjust the fee.
  67 | +    CFeeRate minMempoolFeeRate = wallet->chain().mempoolMinFee();
  68 | +
  69 | +    if (newFeerate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
  70 | +        errors.push_back(strprintf(
  71 | +            "New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool -- ",
    laanwj commented at 1:32 PM on October 2, 2019:

    Does this error message need to specify units for the numbers?

  47. laanwj approved
  48. laanwj commented at 1:41 PM on October 2, 2019: member

    ACK c812aba3949b6ab81030dc708cda7c8821be2f70 Some small remarks but nothing that needs to hold up the merge.

  49. laanwj referenced this in commit 8afa602f30 on Oct 2, 2019
  50. laanwj merged this on Oct 2, 2019
  51. laanwj closed this on Oct 2, 2019
  52. in src/wallet/feebumper.cpp:112 in c812aba394 
     107 | +    }
 108 | +
 109 | +    return feebumper::Result::OK;
 110 | +}
 111 | +
 112 | +static CFeeRate EstimateFeeRate(CWallet* wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee)
    MarcoFalke commented at 8:11 PM on October 9, 2019:

    This can be const CWallet& wallet, as this shouldn't be mutable, nor nullable.

  53. MarcoFalke commented at 8:13 PM on October 9, 2019: member

    post merge ACK c812aba3949b6ab81030dc708cda7c8821be2f70

  54. MarcoFalke removed the label Needs release note on Oct 9, 2019
  56. MarcoFalke referenced this in commit e180be49d7 on Oct 15, 2019
  57. sidhujag referenced this in commit 5b52c182ea on Oct 16, 2019
  58. laanwj referenced this in commit 694f4cbd78 on Mar 26, 2020
  59. DrahtBot locked this on Dec 16, 2021
Contributors
instagibbspromagDrahtBotSjorsRelaxo143luke-jrMarcoFalkelaanwj
Labels
WalletRPC/REST/ZMQ

Milestone
0.19.0

Linked (view graph)
#16492 rpc: Add feeRate argument to bumpFee RPC#17098 refactor: Feebumper EstimateFeeRate follow-up#18312 wallet: remove deprecated fee bumping by totalFee
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me