doc, rpc : `#30275` followups

ismaelsadeeq commented at 2:51 pm on July 25, 2024: member

This PR:

Adds release notes for #30275
Describe fee estimation modes in RPC help texts

DrahtBot commented at 2:51 pm on July 25, 2024: contributor

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.

fanquake added this to the milestone 28.0 on Jul 25, 2024

in src/common/messages.cpp:75 in 35c6e75dec outdated

75+            assert(false);
76+    }
77+    return strprintf("\"%s\" \n%s", mode.first, info);
78+}
79+
80+std::string FeeModesDetail()

willcl-ark commented at 3:39 pm on July 25, 2024:

I think this new format is a little difficult to read with the modes interleaved with their descriptions:

 02. estimate_mode    (string, optional, default="economical") The fee estimate mode.
 1                    "unset"
 2                    no mode set
 3                    "economical"
 4                    Economical estimates use a shorter time horizon, making them more
 5                    responsive to short-term drops in the prevailing fee market. This mode
 6                    potentially returns a lower fee rate estimate.
 7                    "conservative"
 8                    Conservative estimates use a longer time horizon, making them
 9                    less responsive to short-term drops in the prevailing fee market. This mode
10                    potentially returns a higher fee rate estimate.

IMO it may be clearer to switch to something more like this:

02. estimate_mode    (string, optional, default="economical") The fee estimate mode.
1                    Available modes: "economical", "conservative", "unset"
2
3                    Economical estimates use a shorter time horizon, making them more
4                    responsive to short-term drops in the prevailing fee market. This mode
5                    potentially returns a lower fee rate estimate.
6
7                    Conservative estimates use a longer time horizon, making them
8                    less responsive to short-term drops in the prevailing fee market. This mode
9                    potentially returns a higher fee rate estimate.

ismaelsadeeq commented at 5:09 pm on July 25, 2024:

Taken

in src/rpc/fees.cpp:54 in 7a1d3762d5 outdated

51-                "The request target will be clamped between 2 and the highest target\n"
52+                {RPCResult::Type::NUM, "blocks", "The returned fee rate estimate will likely make transaction begin\n"
53+                "confirming confirm within this value. If an estimate for the exact\n"
54+                "conf_target cannot be provided, the closest possible target\n"
55+                "for which an estimate can be provided will be returned.\n"
56+                "The requested conf_target will be clamped between 2 and the highest target\n"

willcl-ark commented at 3:51 pm on July 25, 2024:

Might it make more sense to change this to something simpler:

0The number of blocks from the current tip where the estimate was found.

And amend the doc on L38 about the confirmation target range from 1 to 2 to avoid having to explain it in blocks:

0            {"conf_target", RPCArg::Type::NUM, RPCArg::Optional::NO, "Confirmation target in blocks (2 - 1008)"},

The error could be moved to the error description string:

0                {RPCResult::Type::ARR, "errors", /*optional=*/true, "Errors encountered during processing. An error is returned if not enough transactions and blocks have been observed to make an estimate for any number of blocks."

ismaelsadeeq commented at 5:10 pm on July 25, 2024:

Taken with some modification.

willcl-ark commented at 3:58 pm on July 25, 2024: member

Release note looks good to me.

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

ismaelsadeeq force-pushed on Jul 25, 2024

DrahtBot added the label CI failed on Jul 25, 2024

DrahtBot commented at 5:14 pm on July 25, 2024: contributor

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

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.

DrahtBot removed the label CI failed on Jul 25, 2024

in src/rpc/fees.cpp:51 in c4671e9557 outdated

59-                {RPCResult::Type::NUM, "blocks", "block number where estimate was found\n"
60-                "The request target will be clamped between 2 and the highest target\n"
61-                "fee estimation is able to return based on how long it has been running.\n"
62-                "An error is returned if not enough transactions and blocks\n"
63-                "have been observed to make an estimate for any number of blocks."},
64+                {RPCResult::Type::NUM, "blocks", "The returned fee rate estimate will likely result transaction\n"

tdb3 commented at 3:02 am on July 26, 2024:

Seems like there might be missing words here.

likely result in the transaction starting to…

ismaelsadeeq commented at 9:22 am on July 26, 2024:

fixed, thanks.

in src/common/messages.cpp:84 in c4671e9557 outdated

79+    for (const auto& fee_mode : FeeModeMap()) {
80+        modes += strprintf("\"%s\", ", fee_mode.first);
81+        info += FeeModeInfo(fee_mode);
82+    }
83+    if (!FeeModeMap().empty()) {
84+        modes = modes.substr(0, modes.size() - 2) + "\n";

tdb3 commented at 3:08 am on July 26, 2024:

nitty nit (feel free to ignore): If changing this file, could add a comment for clarity, although it’s a small enough function for a reader to pick up on the meaning of the magic number 2.

0modes = modes.substr(0, modes.size() - 2) + "\n"; // remove the ", " delimiter

Alternatively, could add a string to name the delimiter. Something like:

 0diff --git a/src/common/messages.cpp b/src/common/messages.cpp
 1index bf41d09bbd..76777dd8cd 100644
 2--- a/src/common/messages.cpp
 3+++ b/src/common/messages.cpp
 4@@ -76,12 +76,13 @@ std::string FeeModesDetail()
 5{
 6    std::string modes{"Available modes: "};
 7    std::string info;
 8+   std::string delimiter{", "};
 9    for (const auto& fee_mode : FeeModeMap()) {
10-        modes += strprintf("\"%s\", ", fee_mode.first);
11+        modes += "\"" + fee_mode.first + "\"" + delimiter;
12        info += FeeModeInfo(fee_mode);
13    }
14    if (!FeeModeMap().empty()) {
15-        modes = modes.substr(0, modes.size() - 2) + "\n";
16+        modes = modes.substr(0, modes.size() - delimiter.length()) + "\n";
17    }
18    return modes + info;
19}

ismaelsadeeq commented at 9:23 am on July 26, 2024:

Taken.

tdb3 commented at 3:09 am on July 26, 2024: contributor

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.

ismaelsadeeq force-pushed on Jul 26, 2024

in src/common/messages.cpp:60 in f38aaefbbf outdated

52@@ -53,6 +53,40 @@ const std::vector<std::pair<std::string, FeeEstimateMode>>& FeeModeMap()
53     return FEE_MODES;
54 }
55 
56+std::string FeeModeInfo(const std::pair<std::string, FeeEstimateMode>& mode)
57+{
58+    switch (mode.second) {
59+        case FeeEstimateMode::UNSET:
60+            return std::string("unset: no mode set; default mode will be used.\n");

kevkevinpal commented at 1:42 pm on July 26, 2024:

it might be useful to let the user know what the default is here

unset: no mode set; default mode (economical) will be used.

When using bitcoin-cli sendtoaddress I see the following snippit

08. estimate_mode            (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
1                            Available modes: "unset", "economical", "conservative"
2                            unset: no mode set; default mode will be used.
3                            Economical estimates use a shorter time horizon, making them more
4                            responsive to short-term drops in the prevailing fee market. This mode
5                            potentially returns a lower fee rate estimate.
6                            Conservative estimates use a longer time horizon, making them
7                            less responsive to short-term drops in the prevailing fee market. This mode
8                            potentially returns a higher fee rate estimate.

ismaelsadeeq commented at 2:40 pm on July 31, 2024:

This might not be a good idea because the wallet RPC sendtoaddress and friends default depends on whether the RPC call signals RBF or not. If it doesn’t, conservative will be used.

see

https://github.com/bitcoin/bitcoin/blob/9eb57d1ab6ea5ae642b6d6cc2e74c3046984230c/src/wallet/fees.cpp#L52

I will update the documentation to indicate the “unset” behavior for estimatesmartfee is economical whereas on wallet RPC’s it’s economical but will be changed to conservative if the user did not to opt into RBF.

ismaelsadeeq commented at 3:14 pm on July 31, 2024:

pushed in https://github.com/bitcoin/bitcoin/pull/30525/commits/d38a4d1f61f6b8c523f690eeb8f3a2671afaf9da, thanks for review

tdb3 approved

tdb3 commented at 2:29 am on July 27, 2024: contributor

ACK f38aaefbbfbc3be3751eff61ea3995ce30d303ba

ismaelsadeeq force-pushed on Jul 31, 2024

in src/common/messages.cpp:81 in 4f5ded6905 outdated

76+{
77+    std::string info;
78+    for (const auto& fee_mode : FeeModeMap()) {
79+        info += FeeModeInfo(fee_mode, default_info);
80+    }
81+    return strprintf("%s \n %s", FeeModes(", "), info);

tdb3 commented at 1:11 am on August 1, 2024:

nitty nit: Any reason for the spaces around the newline here?

ismaelsadeeq commented at 9:54 am on August 1, 2024:

will remove the space, If there is need to retouch.

ismaelsadeeq commented at 2:44 pm on August 2, 2024:

fixed.

tdb3 approved

tdb3 commented at 1:11 am on August 1, 2024: contributor

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.

in doc/release-notes-30275.md:13 in 063677237c outdated

 8+  to short-term drops in the prevailing fee market.
 9+
10+- Since users typically rely on the default mode, this change is expected to reduce overestimation for many users.
11+  The `economical` mode aligns with current users behavior, where users often opt-in to Replace-by-Fee (RBF),
12+  prefer not to overestimate fees, and use fee bumping when necessary. For users requiring high confidence
13+  in their fee estimates at the cost of potentially overestimating, the `conservative` mode remains available.

glozow commented at 1:18 pm on August 2, 2024:

Seems like some duplicate information? The user can use help estimatesmartfee

0- The default mode for the `estimatesmartfee` RPC has been updated from `conservative` to `economical`,
1  which is expected to reduce overestimation for many users, particularly if Replace-by-Fee is an option.
2  For users that require high confidence
3  in their fee estimates at the cost of potentially overestimating, the `conservative` mode remains available.

ismaelsadeeq commented at 2:45 pm on August 2, 2024:

taken

in src/rpc/fees.cpp:55 in 4f5ded6905 outdated

57-                "have been observed to make an estimate for any number of blocks."},
58+                {RPCResult::Type::NUM, "blocks", "The returned fee rate estimate will likely result in the\n"
59+                "transaction starting to confirm within the returned \"blocks\" value.\n"
60+                "If an estimate for the exact conf_target cannot be provided,\n"
61+                "the nearest possible target for which an estimate can be given will be returned.\n"
62+                "Fee estimation is able to return based on how long it has been running."},

glozow commented at 2:30 pm on August 2, 2024:

Not really sure if this is better. The last sentence seems to be fragmented off?

ismaelsadeeq commented at 3:03 pm on August 2, 2024:

https://github.com/bitcoin/bitcoin/pull/30525/commits/e387642f36ba01e45656875d8bde7b021ed28556 is an attempt to clarify the blocks returned value help text since I am modifying the file.

On master:

0  "blocks" : n      (numeric) Block number where the estimate was found.
1                    The request target will be clamped between 2 and the highest target
2                    fee estimation can return based on how long it has been running.
3                    An error is returned if not enough transactions and blocks
4                    have been observed to make an estimate for any number of blocks.

With this PR:

0  "blocks" : n      (numeric) The returned fee rate estimate will likely result in the
1                    transaction starting to confirm within the returned "blocks" value.
2                    If an estimate for the exact conf_target cannot be provided, the
3                    nearest possible target for which an estimate can be given
4                    will be returned.

I removed the last sentence, I think thats covered in the errors part.

glozow commented at 12:43 pm on August 6, 2024:

I think the previous text is clear and not inaccurate, so I’m wondering whether this last commit is necessary.

ismaelsadeeq commented at 1:46 pm on August 6, 2024:

Thanks I removed the last commit to focus this PR on #30275 follow-up.

[doc]: add `30275` release notes 6e7e620864

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

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

fa2f26960e

ismaelsadeeq force-pushed on Aug 2, 2024

in src/common/messages.cpp:81 in e387642f36 outdated

76+{
77+    std::string info;
78+    for (const auto& fee_mode : FeeModeMap()) {
79+        info += FeeModeInfo(fee_mode, default_info);
80+    }
81+    return strprintf("%s \n%s", FeeModes(", "), info);

tdb3 commented at 9:55 pm on August 5, 2024:

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

ismaelsadeeq commented at 4:58 am on August 7, 2024:

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

ismaelsadeeq force-pushed on Aug 6, 2024

glozow commented at 1:54 pm on August 6, 2024: member

ACK fa2f26960ee084971ab09959b213a9b8104482e5

DrahtBot requested review from tdb3 on Aug 6, 2024

willcl-ark approved

willcl-ark commented at 2:55 pm on August 6, 2024: member

ACK fa2f26960ee

This changes the help text from

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

to

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

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

 0diff --git a/src/common/messages.cpp b/src/common/messages.cpp
 1index da464106777..3666d4953b3 100644
 2--- a/src/common/messages.cpp
 3+++ b/src/common/messages.cpp
 4@@ -46,9 +46,9 @@ std::string StringForFeeReason(FeeReason reason)
 5 const std::vector<std::pair<std::string, FeeEstimateMode>>& FeeModeMap()
 6 {
 7     static const std::vector<std::pair<std::string, FeeEstimateMode>> FEE_MODES = {
 8-        {"unset", FeeEstimateMode::UNSET},
 9-        {"economical", FeeEstimateMode::ECONOMICAL},
10-        {"conservative", FeeEstimateMode::CONSERVATIVE},
11+        {"'unset'", FeeEstimateMode::UNSET},
12+        {"'economical'", FeeEstimateMode::ECONOMICAL},
13+        {"'conservative'", FeeEstimateMode::CONSERVATIVE},
14     };
15     return FEE_MODES;
16 }
17@@ -83,7 +83,9 @@ std::string FeeModesDetail(std::string default_info)
18 
19 std::string FeeModes(const std::string& delimiter)
20 {
21-    return Join(FeeModeMap(), delimiter, [&](const std::pair<std::string, FeeEstimateMode>& i) { return i.first; });
22+    std::string modes = "Available modes: ";
23+    modes += Join(FeeModeMap(), delimiter, [&](const std::pair<std::string, FeeEstimateMode>& i) { return i.first; });
24+    return modes;
25 }
26 
27 std::string InvalidEstimateModeErrorMessage()

which would render as:

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

tdb3 approved

tdb3 commented at 1:28 am on August 7, 2024: contributor

re ACK fa2f26960ee084971ab09959b213a9b8104482e5

achow101 commented at 3:00 am on August 7, 2024: member

ACK fa2f26960ee084971ab09959b213a9b8104482e5

achow101 merged this on Aug 7, 2024

achow101 closed this on Aug 7, 2024

ismaelsadeeq deleted the branch on Aug 7, 2024

doc, rpc : #30275 followups #30525

Code Coverage

Reviews

doc, rpc : `#30275` followups #30525