rpc: Add function walletestimatefee #22673

1. 
   pranabp-bit commented at 6:37 AM on August 10, 2021: contributor

   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

2. fanquake requested review from instagibbs on Aug 10, 2021
3. pranabp-bit renamed this:
   Add function walletestimatefee
   rpc: Add function walletestimatefee
   on Aug 10, 2021
4. 
   darosior commented at 6:52 AM on August 10, 2021: member

   Why a new command while we could just fix estimatesmartfee?

5. DrahtBot added the label RPC/REST/ZMQ on Aug 10, 2021
6. DrahtBot added the label Wallet on Aug 10, 2021
7. 
   instagibbs commented at 7:02 AM on August 10, 2021: member

   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 https://github.com/bitcoin/bitcoin/pull/22673#pullrequestreview-726066519, 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 .

8. 
   DrahtBot commented at 7:48 AM on August 10, 2021: 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:

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

9. 
   meshcollider commented at 10:05 AM on August 10, 2021: contributor

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

10. 
    pranabp-bit commented at 10:56 AM on August 10, 2021: contributor

    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.

11. pranabp-bit force-pushed on Aug 10, 2021
12. Add function walletestimatefee

    Add test for walletestimatefee in feature_fee_estimation.py

    3d71755b79
13. pranabp-bit force-pushed on Aug 10, 2021
14. pranabp-bit requested review from darosior on Aug 10, 2021
15. in src/wallet/rpcwallet.cpp:562 in 3d71755b79

     557 | +            "\"" + FeeModes("\"\n\"") + "\""},
     558 | +                },
     559 | +                RPCResult{
     560 | +                    RPCResult::Type::OBJ, "", "",
     561 | +                    {
     562 | +                        {RPCResult::Type::NUM, "feerate", /* optional */ true, "estimate fee rate in " + CURRENCY_UNIT + "/kvB (only present if no errors were encountered)"},
    

    ---

    ---

    

    jonatack commented at 2:33 PM on August 10, 2021:

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

    ---

    

    jonatack commented at 2:34 PM on August 10, 2021:

    See #20484 (comment) for more info.

16. 
    fanquake commented at 2:39 AM on August 11, 2021: member

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

    Fixing / adapting estimatesmartfee, not adding a new RPC.

17. 
    achow101 commented at 2:52 AM on August 11, 2021: member

    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.

18. 
    instagibbs commented at 1:30 AM on August 14, 2021: member

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

    1. 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
    2. minrelay fee is reasonable change in behavior by default to me

    thoughts?

19. pranabp-bit closed this on Aug 17, 2021

    ---

20. 
    meshcollider commented at 6:42 AM on August 17, 2021: contributor

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

21. 
    pranabp-bit commented at 10:38 AM on August 28, 2021: contributor

    @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

22. meshcollider referenced this in commit b55232a337 on Sep 28, 2021
23. sidhujag referenced this in commit efce452c82 on Sep 28, 2021
24. DrahtBot locked this on Aug 28, 2022

Contributors

pranabp-bitdarosiorinstagibbsDrahtBotmeshcolliderjonatackfanquakeachow101

Review Requested
instagibbsdarosior