doc, rpc : `#30275` followups #30525

ismaelsadeeq · 2024-07-25T14:51:14Z

This PR:

Adds release notes for Fee Estimation: change estimatesmartfee default mode to economical #30275
Describe fee estimation modes in RPC help texts

DrahtBot · 2024-07-25T14:51:17Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, willcl-ark, tdb3, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

willcl-ark

Release note looks good to me.

Left a few thoughts on the RPC help strings for consideration.

src/common/messages.cpp

src/rpc/fees.cpp

DrahtBot · 2024-07-25T17:14:41Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/27925476433}

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

tdb3

Approach ACK
Nice follow-up. Left a couple of small comments.
Also manually ran "help " for each change and didn't notice any rendering issues.

src/rpc/fees.cpp

src/common/messages.cpp

tdb3

ACK f38aaefbbfbc3be3751eff61ea3995ce30d303ba

tdb3

ACK 4f5ded6905ff7a425aabe53c1a4124f2cb7b5365
Nice cleanup
Re-inspected "help" for each changed RPC and didn't notice any rendering issues. Seems ok to remove Available modes: text.

src/common/messages.cpp

doc/release-notes-30275.md

src/rpc/fees.cpp

- Add description that indicates the fee estimation modes behaviour. - This description will be returned in the RPC's help texts.

glozow

ACK fa2f269

willcl-ark

ACK fa2f269

This changes the help text from

2. estimate_mode    (string, optional, default="conservative") The fee estimate mode.
                    Whether to return a more conservative estimate which also satisfies
                    a longer history. A conservative estimate potentially returns a
                    higher feerate and is more likely to be sufficient for the desired
                    target, but is not as responsive to short term drops in the
                    prevailing fee market. Must be one of (case insensitive):
                    "unset"
                    "economical"
                    "conservative"

to

2. estimate_mode    (string, optional, default="economical") The fee estimate mode.
                    unset, economical, conservative
                    unset means no mode set (default mode will be used).
                    economical estimates use a shorter time horizon, making them more
                    responsive to short-term drops in the prevailing fee market. This mode
                    potentially returns a lower fee rate estimate.
                    conservative estimates use a longer time horizon, making them
                    less responsive to short-term drops in the prevailing fee market. This mode
                    potentially returns a higher fee rate estimate.

Not a blocker for me but IMO a patch to fa2f269 could make this slightly clearer by wrapping modes in quotes and helping to break up the large block of text:

Patch

diff --git a/src/common/messages.cpp b/src/common/messages.cpp
index da464106777..3666d4953b3 100644
--- a/src/common/messages.cpp
+++ b/src/common/messages.cpp
@@ -46,9 +46,9 @@ std::string StringForFeeReason(FeeReason reason)
 const std::vector<std::pair<std::string, FeeEstimateMode>>& FeeModeMap()
 {
     static const std::vector<std::pair<std::string, FeeEstimateMode>> FEE_MODES = {
-        {"unset", FeeEstimateMode::UNSET},
-        {"economical", FeeEstimateMode::ECONOMICAL},
-        {"conservative", FeeEstimateMode::CONSERVATIVE},
+        {"'unset'", FeeEstimateMode::UNSET},
+        {"'economical'", FeeEstimateMode::ECONOMICAL},
+        {"'conservative'", FeeEstimateMode::CONSERVATIVE},
     };
     return FEE_MODES;
 }
@@ -83,7 +83,9 @@ std::string FeeModesDetail(std::string default_info)
 
 std::string FeeModes(const std::string& delimiter)
 {
-    return Join(FeeModeMap(), delimiter, [&](const std::pair<std::string, FeeEstimateMode>& i) { return i.first; });
+    std::string modes = "Available modes: ";
+    modes += Join(FeeModeMap(), delimiter, [&](const std::pair<std::string, FeeEstimateMode>& i) { return i.first; });
+    return modes;
 }
 
 std::string InvalidEstimateModeErrorMessage()

which would render as:

2. estimate_mode    (string, optional, default="economical") The fee estimate mode.
                    Available modes: 'unset', 'economical', 'conservative'
                    'unset' means no mode set (default mode will be used).
                    'economical' estimates use a shorter time horizon, making them more
                    responsive to short-term drops in the prevailing fee market. This mode
                    potentially returns a lower fee rate estimate.
                    'conservative' estimates use a longer time horizon, making them
                    less responsive to short-term drops in the prevailing fee market. This mode
                    potentially returns a higher fee rate estimate.

tdb3

re ACK fa2f269

tdb3 · 2024-08-05T21:55:32Z

src/common/messages.cpp

+    for (const auto& fee_mode : FeeModeMap()) {
+        info += FeeModeInfo(fee_mode, default_info);
+    }
+    return strprintf("%s \n%s", FeeModes(", "), info);


nitty nit: If this file ends up being changed again, could remove the extra space between the first %s and the newline.

Thank you.
I will address this and @willcl-ark nit if there is need to retouch.

achow101 · 2024-08-07T03:00:16Z

ACK fa2f269

fanquake mentioned this pull request Jul 25, 2024

Fee Estimation: change estimatesmartfee default mode to economical #30275

Merged

fanquake added this to the 28.0 milestone Jul 25, 2024

ismaelsadeeq mentioned this pull request Jul 25, 2024

Improve documentation of estimatesmartfee #18217

Closed

willcl-ark reviewed Jul 25, 2024

View reviewed changes

src/common/messages.cpp Outdated Show resolved Hide resolved

src/rpc/fees.cpp Outdated Show resolved Hide resolved

ismaelsadeeq force-pushed the 07-2024-estimatesmartfee-doc-update branch 2 times, most recently from eaa4337 to c4671e9 Compare July 25, 2024 17:14

DrahtBot added the CI failed label Jul 25, 2024

DrahtBot removed the CI failed label Jul 25, 2024

tdb3 reviewed Jul 26, 2024

View reviewed changes

src/rpc/fees.cpp Outdated Show resolved Hide resolved

src/common/messages.cpp Outdated Show resolved Hide resolved

ismaelsadeeq force-pushed the 07-2024-estimatesmartfee-doc-update branch from c4671e9 to f38aaef Compare July 26, 2024 09:22

kevkevinpal reviewed Jul 26, 2024

View reviewed changes

src/common/messages.cpp Outdated Show resolved Hide resolved

tdb3 approved these changes Jul 27, 2024

View reviewed changes

ismaelsadeeq force-pushed the 07-2024-estimatesmartfee-doc-update branch from f38aaef to 4f5ded6 Compare July 31, 2024 15:11

tdb3 approved these changes Aug 1, 2024

View reviewed changes

src/common/messages.cpp Outdated Show resolved Hide resolved

glozow reviewed Aug 2, 2024

View reviewed changes

doc/release-notes-30275.md Outdated Show resolved Hide resolved

src/rpc/fees.cpp Outdated Show resolved Hide resolved

ismaelsadeeq added 2 commits August 2, 2024 15:40

[doc]: add 30275 release notes

6e7e620

[rpc, fees]: add more detail on the fee estimation modes

fa2f269

- Add description that indicates the fee estimation modes behaviour. - This description will be returned in the RPC's help texts.

ismaelsadeeq force-pushed the 07-2024-estimatesmartfee-doc-update branch 3 times, most recently from e387642 to fa2f269 Compare August 6, 2024 13:43

glozow reviewed Aug 6, 2024

View reviewed changes

DrahtBot requested a review from tdb3 August 6, 2024 13:54

willcl-ark approved these changes Aug 6, 2024

View reviewed changes

tdb3 approved these changes Aug 7, 2024

View reviewed changes

achow101 merged commit 2987ba6 into bitcoin:master Aug 7, 2024

ismaelsadeeq deleted the 07-2024-estimatesmartfee-doc-update branch August 7, 2024 07:40

ismaelsadeeq mentioned this pull request Aug 12, 2024

Fee Estimation via Fee rate Forecasters tracking issue #30392

Closed

7 tasks

bitcoin locked and limited conversation to collaborators Aug 7, 2025

doc, rpc : #30275 followups #30525

doc, rpc : #30275 followups #30525

Uh oh!

Conversation

ismaelsadeeq commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jul 25, 2024

Uh oh!

tdb3 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

tdb3 left a comment • 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!

tdb3 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

doc, rpc : `#30275` followups #30525

doc, rpc : `#30275` followups #30525

ismaelsadeeq commented Jul 25, 2024 •

edited

Loading

DrahtBot commented Jul 25, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

tdb3 left a comment •

edited

Loading

tdb3 left a comment •

edited

Loading