Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely#20250
Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely#20250luke-jr wants to merge 11 commits intobitcoin:masterfrom
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. |
src/wallet/rpcwallet.cpp
Outdated
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 |
luke-jr
force-pushed
the
rpcwallet_explicit_fixups
branch
from
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". |
| {"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 |
6 participants
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)