rpc: Add function walletestimatefee #22673

pranabp-bit · 2021-08-10T06:37:37Z

This PR is in response to the issue #19699.

walletestimatefee uses the function GetMinimumFeeRate which takes into account fallbackfee, estimateSmartFee, mempoolMinFee, relayMinFee . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as #16072.

It takes Confirmation Target (in blocks) and fee estimate mode as input and returns the expected fee rate, reason for estimation, and the block number at which estimate was found (only when the reason is estimatesmartfee) as output.

Test for this function has been added in test/functional/feature_fee_estimation.py

darosior

Why a new command while we could just fix estimatesmartfee?

instagibbs · 2021-08-10T07:02:59Z

I don't think it's obviously broken, I think breaking previous behavior isn't the right thing to do. Arguably we could expose the new behavior under a flag?

…

On Tue, Aug 10, 2021, 2:52 PM darosior ***@***.***> wrote: ***@***.**** commented on this pull request. Why a new command while we could just fix estimatesmartfee? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#22673 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU4KMHRMWPCVASBXE3LT4DECVANCNFSM5B3PJ7OA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

DrahtBot · 2021-08-10T07:48:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22513 (rpc: Allow walletprocesspsbt to sign without finalizing by achow101)

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.

meshcollider · 2021-08-10T10:05:10Z

I definitely agree that this overlaps too much with estimatesmartfee to warrant a separate RPC command. Integrating the functionality would be better 👍

pranabp-bit · 2021-08-10T10:56:34Z

The problem with estimatesmartfee was that it isn't a wallet RPC, which is required when the reason is say, fallbackfee .
I felt that if a user is able to call a command from the wallet itself and it uses the same functions which are used by commands like sendtoaddress and sendmany , it would minimize the possibility of a strange high feerate like #16072
Do suggest what the best way to move forward would be.

Add test for walletestimatefee in feature_fee_estimation.py

jonatack · 2021-08-10T14:33:35Z

src/wallet/rpcwallet.cpp

+                RPCResult{
+                    RPCResult::Type::OBJ, "", "",
+                    {
+                        {RPCResult::Type::NUM, "feerate", /* optional */ true, "estimate fee rate in " + CURRENCY_UNIT + "/kvB (only present if no errors were encountered)"},


We're informally in the process of transitioning from BTC/kvB to sat/vB for fee rates, and of no longer referring to fee rates as fees. For example, it was discussed in November 2020 to soft-replace the settxfee and estimatesmartfee calls with sat/vB equivalents named setfeerate (#20391) and estimatefeerate; the BTC/kvB ones would become hidden RPCs to not break user space.

So, if we do add another fee rate RPC, consider making it denominated in sat/vB units and referring to "fee rates", rather than "fees".

See #20484 (comment) for more info.

fanquake · 2021-08-11T02:39:25Z

Do suggest what the best way to move forward would be.

Fixing / adapting estimatesmartfee, not adding a new RPC.

achow101 · 2021-08-11T02:52:25Z

I also agree with updating estimatesmartfee rather than a whole new rpc.

While fallbackfee is something that only matters to wallets, it isn't tied to specific wallets (all wallets loaded will have the same fallbackfee set) so it isn't necessary to have a wallet rpc just to account for it. Instead, you could add it to WalletClient (have WalletClient also parse the -fallbackfee option) and retrieve it from there via NodeContext. When wallets are disabled, NodeContext.wallet_client will just be nullptr, and in that situation, -fallbackfee couldn't have been set and so doesn't matter for the fee estimation.

instagibbs · 2021-08-14T01:30:55Z

Should this new behavior be hidden behind a flag or not under estimatesmartfee?

fallbackfee is opt-in only now, so as long as a user doesn't put it into their conf file, it won't be hit
minrelay fee is reasonable change in behavior by default to me

thoughts?

meshcollider · 2021-08-17T06:42:45Z

@pranabp-bit are you planning on reopening this when it is ready, or opening a new PR? Thanks 😊

pranabp-bit · 2021-08-28T10:38:52Z

@meshcollider @achow101 @fanquake I went ahead with updating the estimatesmartfee instead of adding a new rpc. Do suggest changes, if any. New PR-rpc: update estimatesmartfee

…ax of CBlockPolicyEstimator::estimateSmartFee, mempoolMinFee and minRelayTxFee ea31caf update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (pranabp-bit) Pull request description: This PR is in response to the issue [#19699](bitcoin/bitcoin#19699). Based on the discussion in the comments of PR [#22673](bitcoin/bitcoin#22673) changes have been made in the `estimatesmartfee` itself such that it takes into account `mempoolMinFee` and `relayMinFee` . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as [#16072](bitcoin/bitcoin#16072). The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality. ACKs for top commit: meshcollider: re-utACK ea31caf Tree-SHA512: 8f36153a07cbd552c5c13d11d9c6e987a7a555ea4cc83f2573c0c92dd97c706d90c30a7248671437c2f3a836d3272f8fad53d15a5fa6efaa0409ae8009b0a18d

…lockPolicyEstimator::estimateSmartFee, mempoolMinFee and minRelayTxFee ea31caf update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (pranabp-bit) Pull request description: This PR is in response to the issue [bitcoin#19699](bitcoin#19699). Based on the discussion in the comments of PR [bitcoin#22673](bitcoin#22673) changes have been made in the `estimatesmartfee` itself such that it takes into account `mempoolMinFee` and `relayMinFee` . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as [bitcoin#16072](bitcoin#16072). The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality. ACKs for top commit: meshcollider: re-utACK ea31caf Tree-SHA512: 8f36153a07cbd552c5c13d11d9c6e987a7a555ea4cc83f2573c0c92dd97c706d90c30a7248671437c2f3a836d3272f8fad53d15a5fa6efaa0409ae8009b0a18d

fanquake requested a review from instagibbs August 10, 2021 06:39

pranabp-bit changed the title ~~Add function walletestimatefee~~ rpc: Add function walletestimatefee Aug 10, 2021

darosior reviewed Aug 10, 2021

View reviewed changes

DrahtBot added RPC/REST/ZMQ Wallet labels Aug 10, 2021

DrahtBot mentioned this pull request Aug 10, 2021

rpc: Allow walletprocesspsbt to sign without finalizing #22513

Merged

pranabp-bit force-pushed the walletestimatefee branch from ff1dad5 to 39c3b1a Compare August 10, 2021 13:17

Add function walletestimatefee

3d71755

Add test for walletestimatefee in feature_fee_estimation.py

pranabp-bit force-pushed the walletestimatefee branch from 39c3b1a to 3d71755 Compare August 10, 2021 13:34

pranabp-bit requested a review from darosior August 10, 2021 13:46

jonatack reviewed Aug 10, 2021

View reviewed changes

pranabp-bit mentioned this pull request Aug 14, 2021

rpc: update estimatesmartfee pranabp-bit/bitcoin#3

Closed

pranabp-bit closed this Aug 17, 2021

pranabp-bit mentioned this pull request Aug 17, 2021

rpc: update estimatesmartfee to return max of CBlockPolicyEstimator::estimateSmartFee, mempoolMinFee and minRelayTxFee #22722

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: Add function walletestimatefee #22673

rpc: Add function walletestimatefee #22673

Uh oh!

pranabp-bit commented Aug 10, 2021 •

edited

Loading

Uh oh!

darosior left a comment

Uh oh!

instagibbs commented Aug 10, 2021 via email

Uh oh!

DrahtBot commented Aug 10, 2021

Uh oh!

meshcollider commented Aug 10, 2021

Uh oh!

pranabp-bit commented Aug 10, 2021 •

edited

Loading

Uh oh!

jonatack Aug 10, 2021 •

edited

Loading

Uh oh!

jonatack Aug 10, 2021

Uh oh!

fanquake commented Aug 11, 2021

Uh oh!

achow101 commented Aug 11, 2021

Uh oh!

instagibbs commented Aug 14, 2021

Uh oh!

meshcollider commented Aug 17, 2021

Uh oh!

pranabp-bit commented Aug 28, 2021

Uh oh!

Uh oh!

rpc: Add function walletestimatefee #22673

rpc: Add function walletestimatefee #22673

Uh oh!

Conversation

pranabp-bit commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 10, 2021 via email

Uh oh!

DrahtBot commented Aug 10, 2021

Conflicts

Uh oh!

meshcollider commented Aug 10, 2021

Uh oh!

pranabp-bit commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack Aug 10, 2021 • 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 Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 11, 2021

Uh oh!

achow101 commented Aug 11, 2021

Uh oh!

instagibbs commented Aug 14, 2021

Uh oh!

meshcollider commented Aug 17, 2021

Uh oh!

pranabp-bit commented Aug 28, 2021

Uh oh!

Uh oh!

pranabp-bit commented Aug 10, 2021 •

edited

Loading

pranabp-bit commented Aug 10, 2021 •

edited

Loading

jonatack Aug 10, 2021 •

edited

Loading