Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely #20250
Conversation
Was only in bumpfee's helper
…ve to conf_target
|
@jonatack What do you think of this approach? (Actually what I thought it already was!) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| if (options.exists("feeRate") && options.exists("fee_rate")) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "feeRate and fee_rate options should not both be set. Use fee_rate (feeRate is deprecated)."); | ||
| } | ||
| auto fee_rate = options.exists("feeRate") ? options["feeRate"] : options["fee_rate"]; |
There was a problem hiding this comment.
Concept ACK on moving away from feeRate
|
I agree this is more sane than currently, especially aliasing |
…imateMode separately to replace broken code duplication For compatibility, bumpfee and fundraw accept fee_rate without estimate_mode
|
Fixed review comments including adapting tests |
4af8b95 to
7bef214
Compare
|
Concept/approach ACK, also superficial code review ACK. Needs linter fix. |
| } else if (!estimate_param.isNull()) { | ||
| cc.m_confirm_target = ParseConfirmTarget(estimate_param, pwallet->chain().estimateMaxBlocks()); | ||
| } else { | ||
| if (&conf_target_param != &fee_rate_param && !fee_rate_param.isNull()) { |
There was a problem hiding this comment.
I had to look this up as this is comparing pointers to unrelated objects, but my reading of https://en.cppreference.com/w/cpp/language/operator_comparison is that this is indeed well defined.
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
| @@ -440,7 +451,7 @@ static RPCHelpMan sendtoaddress() | |||
| {"subtractfeefromamount", RPCArg::Type::BOOL, /* default */ "false", "The fee will be deducted from the amount being sent.\n" | |||
| "The recipient will receive less bitcoins than you enter in the amount field."}, | |||
| {"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"}, | |||
| {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, | |||
| {"conf_target|fee_rate", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, | |||
| {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" | |||
There was a problem hiding this comment.
should this also be aliased to something? Like fee_rate_type?
|
Closing in favour of #20305 |
Expects
fee_modefor explicit feerate modes instead of overloadingconf_target(positional args still accept either for now; that can be restricted once we finally have #17356).fundrawtransactionused to expect its option to be calledfeeRate, diverging from otherfee_rateoption names. It now accepts either, withfeeRatesoft-deprecated.fundrawtransactionandbumpfeestill acceptfee_ratewithoutestimate_modefor backward compatibility.Borrows tests from #20220 (adapted for interface changes). (NOTE: NOT including 9ea6d39 77a4f98 f97a3cd)