Fee Estimation: change `estimatesmartfee` default mode to `economical`

ismaelsadeeq commented at 3:19 pm on June 12, 2024: member

This PR changes the estimatesmartfee default mode to economical.

This was also suggested on IRC https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-04-26#1021609

conservative mode: This is the estimatesmartfee RPC mode which considers a longer history of blocks. It potentially returns a higher fee rate and is more likely to be sufficient for the desired target, but it is not as responsive to short-term drops in the prevailing fee market.
economical mode: This is the estimatesmartfee RPC mode where estimates are potentially lower and more responsive to short-term drops in the prevailing fee market.

Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.

For an in-depth analysis of how significantly the conservative mode overestimates, see https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.

DrahtBot commented at 3:19 pm on June 12, 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, instagibbs

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30079 (Fee Estimation: Ignore all transactions that are CPFP’d by ismaelsadeeq)

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.

glozow commented at 9:35 am on June 19, 2024: member

Concept ACK 5a4a84a6809919561aec46f272f08ae57c0e386e. Slightly surprised no tests need to be changed.

Aside from seeming to overestimate much less, I think “economical” is much closer to what users would expect from a fee estimator. From #10589:

The logic used here is that any time a transaction signals opt-in-RBF and uses automatic fee estimation then it will use the non-conservative estimate. Transactions which do not signal opt-in-RBF will still use the default conservative estimate.

So accuracy of estimates aside, interpreting these settings as:

“conservative” = I want to have very high confidence that this feerate is sufficient for my confirmation target, even if it means overestimating
“economical” = I want to be reasonably confident about this feerate. I prefer not to overestimate and expect to be able to update it later with a fee-bump if needed

It is pretty normal/default to expect to be able to RBF the transaction later (for example, Core wallet defaults to signaling BIP125 since #25610), so it makes sense that the fee estimation settings should reflect this expectation.

in src/rpc/fees.cpp:40 in 5a4a84a680 outdated

40-            "target, but is not as responsive to short term drops in the\n"
41+            {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"economical"}, "The fee estimate mode.\n"
42+            "Whether to return a more economical estimate\n"
43+            "An economical estimate potentially returns a\n"
44+            "lower feerate that is estimated to be sufficient for the desired\n"
45+            "target, it is also a bit more responsive to short term drops in the\n"

ajtowns commented at 3:41 am on June 20, 2024:

The old text here seems like a better explanation of why you would change this from the new default?

ismaelsadeeq commented at 8:09 am on June 20, 2024:

I Added it to the PR description

ajtowns commented at 9:25 am on June 20, 2024:

Not sure if you understood what I meant, apologies if you did already: I mean if I’m looking at the estimatesmartfee help, trying to figure out if I want to change from the (new) default of “economical”, the existing explanation of what “conservative” does seems more helpful than an explanation of what the default option does. So just changing the default without changing the rest of the help text seems slightly better, doesn’t it?

ismaelsadeeq commented at 10:06 am on June 20, 2024:

I now understand thanks for clarifying. yes it does it will even be better to document the short description of both modes in the help text and indicate economical is the default ?

ismaelsadeeq commented at 5:18 pm on June 27, 2024:

I added a commit, that add more information about the modes.

On master b27afb7fb774809c9c075d56e44657e0b607a00c This is the help output

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"

With this PR

 02. estimate_mode    (string, optional, default="economical") The fee estimate mode.
 1                    "unset" 
 2                    no mode set
 3                    "economical" 
 4                    Economical estimates potentially return a lower
 5                    feerate that is estimated to be sufficient for the
 6                    desired target. It is also more responsive to
 7                    short-term drops in the prevailing fee market.
 8                    "conservative" 
 9                    Conservative estimates satisfy a longer history.
10                    A conservative estimate potentially returns a higher
11                    feerate and is more likely to be sufficient for
12                    the desired target, but is not as responsive
13                    to short-term drops in the prevailing fee market.
14

The first commit https://github.com/bitcoin/bitcoin/pull/30275/commits/2ee3d774815f5e1840cb37d2a92007f19c217e4c doc change change affect other RPC’s that require estimation mode.

glozow commented at 10:38 am on July 18, 2024:

Maybe for this PR, it would be cleaner to drop the first commit and just change the default, so we can focus the review on the setting instead of the help text?

2ee3d774815f5e1840cb37d2a92007f19c217e4c seems helpful but can be a separate PR.

ismaelsadeeq commented at 11:20 am on July 18, 2024:

I dropped the first commit and use default help text as @ajtowns suggested.

Yes I think it will be beneficial to improve the documentation I will open up https://github.com/bitcoin/bitcoin/commit/2ee3d774815f5e1840cb37d2a92007f19c217e4c in a separate pull.

Thanks all for your reviews.

willcl-ark commented at 10:18 am on July 24, 2024:

I agree that updating the help text (in a followup) would be nice. I think (a slightly modified version of) the explanation from the 0.15.0 release notes would be a good improvement, as IMO it’s clearer than wording like “is more likely to be sufficient for the desired target”:

02. estimate_mode    (string, optional, default="economical") The fee estimate mode.
1                    *Conservative* estimates use longer time horizons to produce an estimate which
2                    is less susceptible to rapid changes in fee conditions. *Economical* estimates
3                    use shorter time horizons and will be more affected by short-term changes in
4                    fee conditions. Economical estimates may be considerably lower during periods
5                    of low transaction activity but may result in transactions being unconfirmed if
6                    prevailing fees increase rapidly.

ismaelsadeeq force-pushed on Jun 27, 2024

glozow added the label TX fees and policy on Jul 18, 2024

[fees]: change `estimatesmartfee` default mode to `economical` 41a2545046

ismaelsadeeq force-pushed on Jul 18, 2024

glozow commented at 11:32 am on July 18, 2024: member

ACK 41a2545046bce315af697a3c6baf6e3fb2e824c2

Checked that default estimatesmartfee result is using economical.

achow101 added this to the milestone 28.0 on Jul 18, 2024

instagibbs commented at 3:31 pm on July 18, 2024: member

concept ACK

could we return a new field that says the mode, then add a test for that to prevent regressions?

ismaelsadeeq commented at 3:45 pm on July 18, 2024: member

Could we return a new field that indicates the mode?

Yes, I plan on addressing this https://github.com/bitcoin/bitcoin/commit/2ee3d774815f5e1840cb37d2a92007f19c217e4c, in a follow-up PR: as suggested here #30275 (review) along with some documentation updates proposed in #18217 I will indicate the mode as well in that follow-up

then add a test for that to prevent regressions?

Could you please clarify about the test what specific regressions you’re concerned about? Thanks!

instagibbs commented at 5:24 pm on July 18, 2024: member

Could you please clarify about the test what specific regressions you’re concerned about? Thanks!

A boolean flip where the intended behavior isn’t followed and we are giving conservative again?

ismaelsadeeq commented at 11:12 am on July 22, 2024: member

Slightly surprised no tests need to be changed.

Because the fee estimation functional test for estimatesmartfee is not deterministic (transaction fee rates are generated randomly), the check_raw_estimates function tests for a range, ensuring the returned fee rate is reasonable.

A boolean flip where the intended behavior isn’t followed and we are conservative again?

I added a simple deterministic test which test the default mode is economical in 307ceb46587c83a7ab0f587915cdcbf99bfb48ee

DrahtBot commented at 1:00 pm on July 22, 2024: contributor

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

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 added the label CI failed on Jul 22, 2024

in test/functional/feature_fee_estimation.py:417 in 307ceb4658 outdated

412+        high_feerate = Decimal("0.005")
413+        weight = 1000
414+        tx_count = 24
415+        # Broadcast and mine high fee (1k weight) transactions for the first 5 blocks.
416+        for _ in range(5):
417+            self.broadcast_and_mine(self.nodes[1], self.nodes[2], high_feerate, weight, tx_count)

instagibbs commented at 1:33 pm on July 22, 2024:

why two loops to make 12 blocks?

ismaelsadeeq commented at 2:24 pm on July 22, 2024:

no need, updated.

in test/functional/feature_fee_estimation.py:429 in 307ceb4658 outdated

424+
425+        # We now track 12 blocks; short horizon stats will start decaying.
426+        # Broadcast and mine low fee (1k weight) transactions for the next 4 blocks.
427+        for _ in range(4):
428+            self.broadcast_and_mine(self.nodes[1], self.nodes[2], low_feerate, weight, tx_count)
429+        # conservative mode will consider longer time horizons while economical mode do not

instagibbs commented at 1:34 pm on July 22, 2024:

0        # conservative mode will consider longer time horizons while economical mode does not

ismaelsadeeq commented at 2:24 pm on July 22, 2024:

fixed

in test/functional/feature_fee_estimation.py:404 in 307ceb4658 outdated

399+        for _ in range(count):
400+            self.wallet.send_self_transfer(from_node=broadcaster, fee_rate=feerate, target_weight=weight)
401+        self.sync_mempools(wait=0.1)
402+        self.generate(miner, 3, sync_fun=self.no_op)
403+
404+    def check_fee_estimates_btw_modes(self, node, expected_conservative, expected_economical):

instagibbs commented at 1:34 pm on July 22, 2024:

doesn’t use self

ismaelsadeeq commented at 2:26 pm on July 22, 2024:

yes it does not.

instagibbs commented at 1:36 pm on July 22, 2024: member

better test than what I was asking for, thanks

DrahtBot removed the label CI failed on Jul 22, 2024

ismaelsadeeq force-pushed on Jul 22, 2024

instagibbs commented at 2:26 pm on July 22, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/30275/commits/48faceb91ec06ab4959038237d5e69448677855d

DrahtBot requested review from glozow on Jul 22, 2024

in test/functional/feature_fee_estimation.py:406 in 48faceb91e outdated

401+    def broadcast_and_mine(self, broadcaster, miner, feerate, weight, count):
402+        """Broadcast and mine some number of transactions with a specified fee rate and weight."""
403+        for _ in range(count):
404+            self.wallet.send_self_transfer(from_node=broadcaster, fee_rate=feerate, target_weight=weight)
405+        self.sync_mempools(wait=0.1)
406+        self.generate(miner, 3, sync_fun=self.no_op)

glozow commented at 9:54 am on July 24, 2024:

Why don’t we want the nodes to sync? We can’t expect the fee estimates to be updated until the blocks have been received.

ismaelsadeeq commented at 2:14 pm on July 24, 2024:

we do! fixed.

in test/functional/feature_fee_estimation.py:413 in 48faceb91e outdated

408+    def test_estimation_modes(self):
409+        low_feerate = Decimal("0.001")
410+        high_feerate = Decimal("0.005")
411+        weight = 1000
412+        tx_count = 24
413+        # Broadcast and mine high fee (1k weight) transactions for the first 12 blocks.

glozow commented at 10:00 am on July 24, 2024:

This comment seems wrong - broadcast_and_mine generates 3 blocks at a time, so this is 36 blocks not 12?

ismaelsadeeq commented at 2:15 pm on July 24, 2024:

yes, updated. thanks.

in test/functional/feature_fee_estimation.py:146 in 48faceb91e outdated

144-            [],
145-            ["-blockmaxweight=68000"],
146-            ["-blockmaxweight=32000"],
147+            ["-datacarriersize=1000"],
148+            ["-blockmaxweight=68000", "-datacarriersize=1000"],
149+            ["-blockmaxweight=32000", "-datacarriersize=1000"],

glozow commented at 10:04 am on July 24, 2024:

The test still works if no target weight is specified, and I can’t think of why it would be necessary - is there a need for it? Otherwise, removing that + minimizing the extra config options would be preferable.

ismaelsadeeq commented at 2:16 pm on July 24, 2024:

The rationale was to fill up node[2] block template before mining, but you are right, this is not needed. removed.

in test/functional/feature_fee_estimation.py:405 in 48faceb91e outdated

400+
401+    def broadcast_and_mine(self, broadcaster, miner, feerate, weight, count):
402+        """Broadcast and mine some number of transactions with a specified fee rate and weight."""
403+        for _ in range(count):
404+            self.wallet.send_self_transfer(from_node=broadcaster, fee_rate=feerate, target_weight=weight)
405+        self.sync_mempools(wait=0.1)

glozow commented at 10:04 am on July 24, 2024:

Why wait=0.1?

ismaelsadeeq commented at 2:17 pm on July 24, 2024:

No any rational for this, it’s just to reduce the wait time. removed to use the default.

in test/functional/feature_fee_estimation.py:136 in 48faceb91e outdated

127@@ -128,16 +128,22 @@ def make_tx(wallet, utxo, feerate):
128         fee_rate=Decimal(feerate * 1000) / COIN,
129     )
130 
131+def check_fee_estimates_btw_modes(node, expected_conservative, expected_economical):
132+    fee_est_conservative = node.estimatesmartfee(1, estimate_mode="conservative")['feerate']
133+    fee_est_economical = node.estimatesmartfee(1)['feerate']
134+    assert_equal(fee_est_conservative, expected_conservative)
135+    assert_equal(fee_est_economical, expected_economical)

glozow commented at 10:07 am on July 24, 2024:

nit: this could be more future-proof and self-documenting:

0    fee_est_conservative = node.estimatesmartfee(1, estimate_mode="conservative")['feerate']
1    fee_est_economical = node.estimatesmartfee(1, estimate_mode="economical")['feerate']
2    fee_est_default = node.estimatesmartfee(1)['feerate']
3    assert_equal(fee_est_conservative, expected_conservative)
4    assert_equal(fee_est_economical, expected_economical)
5    assert_equal(fee_est_default, expected_economical)

ismaelsadeeq commented at 2:18 pm on July 24, 2024:

updated to your suggestion. Thanks!

glozow commented at 10:08 am on July 24, 2024: member

Nice test! I just have a few comments

DrahtBot requested review from glozow on Jul 24, 2024

in test/functional/feature_fee_estimation.py:394 in 48faceb91e outdated

387@@ -382,6 +388,41 @@ def test_acceptstalefeeestimates_option(self):
388         self.start_node(0,extra_args=["-acceptstalefeeestimates"])
389         assert_equal(self.nodes[0].estimatesmartfee(1)["feerate"], fee_rate)
390 
391+    def clear_estimates(self):
392+        self.log.info("Restarting node with fresh estimation")
393+        self.stop_node(0)
394+        fee_dat = os.path.join(self.nodes[0].chain_path, "fee_estimates.dat")

willcl-ark commented at 10:23 am on July 24, 2024:

nit: if you retouch Pathlib lets you use the / operator to create child paths:

0        fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"

ismaelsadeeq commented at 2:18 pm on July 24, 2024:

fixed. thanks

willcl-ark commented at 10:58 am on July 24, 2024: member

Looks good to me, left a micro style nit if you end up touching to address @glozow ’s comments.

It makes sense to default to more economical fee estimation by default, since users can use RBF (or “conservative” estimation) in those cases when it may be required. RBF opt-in has been enabled by default in Bitcoin Core wallets since ~2022 and there is a proposal to enable “full” RBF as default as mempool policy currently open: #30493 (previously #28132)

ismaelsadeeq force-pushed on Jul 24, 2024

[test]: ensure `estimatesmartfee` default mode is `economical` 25bf86a225

ismaelsadeeq force-pushed on Jul 24, 2024

glozow commented at 4:43 pm on July 24, 2024: member

ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988

DrahtBot requested review from instagibbs on Jul 24, 2024

willcl-ark approved

willcl-ark commented at 7:17 pm on July 24, 2024: member

ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988

Looks good to me with those nits fixed 👍🏼

instagibbs commented at 8:36 pm on July 24, 2024: member

reACK https://github.com/bitcoin/bitcoin/pull/30275/commits/25bf86a225b0df3f48ade1016b47f5ee1636b988

tests make me feel much better about this thanks

fanquake added the label Needs release note on Jul 25, 2024

fanquake commented at 9:40 am on July 25, 2024: member

Will need a release note.

fanquake merged this on Jul 25, 2024

fanquake closed this on Jul 25, 2024

ismaelsadeeq commented at 9:47 am on July 25, 2024: member

Will need a release note.

Thanks, I will add it in the follow-up!

ismaelsadeeq deleted the branch on Jul 25, 2024

fanquake commented at 3:05 pm on July 25, 2024: member

Release note being added in #30525.

fanquake removed the label Needs release note on Jul 25, 2024

Fee Estimation: change estimatesmartfee default mode to economical #30275

Code Coverage

Reviews

Conflicts

Fee Estimation: change `estimatesmartfee` default mode to `economical` #30275