Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fee estimation: avoid serving stale fee estimate #27622

Merged
merged 4 commits into from Jun 20, 2023
Merged

Fee estimation: avoid serving stale fee estimate #27622

merged 4 commits into from Jun 20, 2023

Conversation

ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented May 10, 2023 •
edited

Fixes #27555

The issue arises when an old fee_estimates.dat file is sometimes read during initialization.
Or after an unclean shutdown, the latest fee estimates are not flushed to fee_estimates.dat.
If the fee estimates in the old file are old, they can cause transactions to become stuck in the mempool.
This PR ensures that nodes do not use stale estimates from the old file during initialization. If fee_estimates.dat
has not been updated for 60 hours or more, it is considered stale and will not be read during initialization. To avoid
having old estimates, the fee_estimates.dat file will be flushed periodically every hour. As mentioned #27555

"The immediate improvement would be to store fee estimates to disk once an hour or so to reduce the chance of having an old file. From there, this case could probably be detected, and refuse to serve estimates until we sync."

In addition, I will follow-up PR to persist the mempoolminfee across restarts.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 10, 2023 •
edited

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK willcl-ark, instagibbs, glozow
Concept ACK RandyMcMillan
Stale ACK pinheadmz

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:

  • #25380 (Detect and ignore transactions that were CPFP'd in the fee estimator by darosior)
  • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

@DrahtBot DrahtBot mentioned this pull request May 11, 2023
Closed
@fanquake
Copy link
Member

cc @instagibbs @TheBlueMatt

@DrahtBot DrahtBot mentioned this pull request May 11, 2023
Closed
glozow
glozow reviewed May 11, 2023
View reviewed changes
src/policy/fees.cpp Outdated Show resolved Hide resolved
maflcko
maflcko approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Left some nits

src/util/time.h Outdated Show resolved Hide resolved
test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved
test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch from a2ad5e4 to 0d2329a Compare May 11, 2023 18:43
instagibbs
instagibbs reviewed May 11, 2023
View reviewed changes
src/policy/fees.h Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch 2 times, most recently from 0d2329a to d24f874 Compare May 11, 2023 23:09
instagibbs
instagibbs reviewed May 17, 2023
View reviewed changes
src/policy/fees.cpp Outdated Show resolved Hide resolved
src/policy/fees.h Outdated Show resolved Hide resolved
test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch from aecd92e to 3a9fee2 Compare May 18, 2023 11:01
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch from 3a9fee2 to b02e3fc Compare May 18, 2023 13:14
pinheadmz
pinheadmz reviewed May 18, 2023
View reviewed changes
Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept ACK

This should solve the stale fee issue, the test is clever and effective. I ensured the test fails without the patch and also checked the test failed when the mtime was set < 12 hours. Reviewed code and left some notes / questions

src/policy/fees.cpp Outdated Show resolved Hide resolved
src/policy/fees.cpp Outdated Show resolved Hide resolved
src/policy/fees.h Outdated Show resolved Hide resolved
test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch 2 times, most recently from ef2ee18 to 473946b Compare May 18, 2023 23:02
glozow
glozow reviewed May 19, 2023
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

src/policy/fees.cpp Outdated Show resolved Hide resolved
src/policy/fees.h Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch from 473946b to 1315c9f Compare May 19, 2023 16:49
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch 3 times, most recently from 5f79a92 to cd9828f Compare May 20, 2023 08:54
@ismaelsadeeq ismaelsadeeq force-pushed the 05-2023-avoid-serving-stale-fee-estimate branch 2 times, most recently from de80c10 to 948edc9 Compare May 26, 2023 13:44
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@ismaelsadeeq @luke-jr
tx fees, policy: read stale fee estimates with a regtest-only option
2983373 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@ismaelsadeeq @luke-jr
test: ensure old fee_estimate.dat not read on restart and flushed
8bea323 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
glozow added a commit to bitcoin-core/gui that referenced this pull request Aug 22, 2023
@glozow
Merge bitcoin/bitcoin#28157: test doc: tests `acceptstalefeeestimates…
a84dade 
…` option is only supported on regtest chain

ee5a036 test: ensure acceptstalefeeestimates is supported only on regtest chain (ismaelsadeeq)
22d5d4b tx fees, policy: doc: update and delete unnecessary comment (ismaelsadeeq)

Pull request description:

  This PR Follow up comments from [#27622](bitcoin/bitcoin#27622)

  It test that the new `regtest-only` option `acceptstalefeeestimates` is not supported on [main, signet and test chains](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235218268), removes an unnecessary [comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235204323), and update fee estimator  `MAXFILEAGE` [description comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1233887314).

ACKs for top commit:
  jonatack:
    ACK ee5a036
  glozow:
    utACK ee5a036

Tree-SHA512: 4755f25b08db62f37614ea768272b12580ee0d481fb7fa339379901a6132c66828777c6747d3fe67490ceace3a6ff248bf13bdf65720f6e5ba8642eb762acd3c
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
@glozow
Merge bitcoin#28157: test doc: tests acceptstalefeeestimates option…
6efaec6 
… is only supported on regtest chain

ee5a036 test: ensure acceptstalefeeestimates is supported only on regtest chain (ismaelsadeeq)
22d5d4b tx fees, policy: doc: update and delete unnecessary comment (ismaelsadeeq)

Pull request description:

  This PR Follow up comments from [bitcoin#27622](bitcoin#27622)

  It test that the new `regtest-only` option `acceptstalefeeestimates` is not supported on [main, signet and test chains](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235218268), removes an unnecessary [comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235204323), and update fee estimator  `MAXFILEAGE` [description comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1233887314).

ACKs for top commit:
  jonatack:
    ACK ee5a036
  glozow:
    utACK ee5a036

Tree-SHA512: 4755f25b08db62f37614ea768272b12580ee0d481fb7fa339379901a6132c66828777c6747d3fe67490ceace3a6ff248bf13bdf65720f6e5ba8642eb762acd3c
@fanquake fanquake mentioned this pull request Sep 24, 2023
Merged
@fanquake
Copy link
Member

Added to #28487 for backporting to 25.1.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 24, 2023
@ismaelsadeeq @fanquake
tx fees, policy: periodically flush fee estimates to fee_estimates.dat
55cd4f1 
This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: bitcoin#27622
Rebased-From: 5b886f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 24, 2023
@ismaelsadeeq @fanquake
tx fees, policy: do not read estimates of old fee_estimates.dat
71832b1 
Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: bitcoin#27622
Rebased-From: 3eb241a
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 24, 2023
@ismaelsadeeq @fanquake
tx fees, policy: read stale fee estimates with a regtest-only option
8284c09 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 24, 2023
@ismaelsadeeq @fanquake
test: ensure old fee_estimate.dat not read on restart and flushed
19cd633 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: periodically flush fee estimates to fee_estimates.dat
56edeb8 
This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: bitcoin#27622
Rebased-From: 5b886f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: do not read estimates of old fee_estimates.dat
59c346b 
Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: bitcoin#27622
Rebased-From: 3eb241a
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: read stale fee estimates with a regtest-only option
c23bb6d 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
test: ensure old fee_estimate.dat not read on restart and flushed
f009d26 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
@fanquake
Copy link
Member

Pushed into #28535 for backporting to 24.x as well.

@fanquake fanquake mentioned this pull request Sep 26, 2023
Merged
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: periodically flush fee estimates to fee_estimates.dat
d00e770 
This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: bitcoin#27622
Rebased-From: 5b886f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: do not read estimates of old fee_estimates.dat
57f6242 
Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: bitcoin#27622
Rebased-From: 3eb241a
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: read stale fee estimates with a regtest-only option
27d11ec 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
test: ensure old fee_estimate.dat not read on restart and flushed
f882b40 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: periodically flush fee estimates to fee_estimates.dat
77979e0 
This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: bitcoin#27622
Rebased-From: 5b886f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: do not read estimates of old fee_estimates.dat
1c98029 
Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: bitcoin#27622
Rebased-From: 3eb241a
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
tx fees, policy: read stale fee estimates with a regtest-only option
01f8ee4 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 26, 2023
@ismaelsadeeq @fanquake
test: ensure old fee_estimate.dat not read on restart and flushed
cb5512d 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 2, 2023
@ismaelsadeeq @fanquake
tx fees, policy: periodically flush fee estimates to fee_estimates.dat
c4dd598 
This reduces chances of having old estimates in fee_estimates.dat.

Github-Pull: bitcoin#27622
Rebased-From: 5b886f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 2, 2023
@ismaelsadeeq @fanquake
tx fees, policy: do not read estimates of old fee_estimates.dat
16bb916 
Old fee estimates could cause transactions to become stuck in the
mempool. This commit prevents the node from using stale estimates
from an old file.

Github-Pull: bitcoin#27622
Rebased-From: 3eb241a
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 2, 2023
@ismaelsadeeq @fanquake
tx fees, policy: read stale fee estimates with a regtest-only option
37764d3 
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.

Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.

Github-Pull: bitcoin#27622
Rebased-From: cf219f2
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 2, 2023
@ismaelsadeeq @fanquake
test: ensure old fee_estimate.dat not read on restart and flushed
910c362 
This commit adds tests to ensure that old fee_estimates.dat files
are not read and that fee_estimates are periodically flushed to the
fee_estimates.dat file.

Additionaly it tests the -regtestonly option -acceptstalefeeestimates.

Github-Pull: bitcoin#27622
Rebased-From: d2b39e0
fanquake added a commit that referenced this pull request Oct 4, 2023
@fanquake
Merge #28487: [25.1] Final backports
9f8d501 
45a5fcb http: bugfix: track closed connection (stickies-v)
752a456 http: log connection instead of request count (stickies-v)
ae86ada http: refactor: use encapsulated HTTPRequestTracker (stickies-v)
f31899d gui: macOS, make appMenuBar part of the main app window (furszy)
64ffa94 gui: macOS, do not process dock icon actions during shutdown (furszy)
e270f3f depends: fix unusable memory_resource in macos qt build (fanquake)
a668394 build, macos: Fix `qt` package build with new Xcode 15 linker (Hennadii Stepanov)
b3517cb test: Test loading wallets with conflicts without a chain (Andrew Chow)
d63478c wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow)
5e51a9c ci: Nuke Android APK task, Use credits for tsan (MarcoFalke)
910c362 test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq)
37764d3 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq)
16bb916 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq)
c4dd598 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq)
c36770c test: wallet, verify migration doesn't crash for an invalid script (furszy)
0d2a33e wallet: disallow migration of invalid or not-watched scripts (furszy)
2c51a07 Do not use std::vector = {} to release memory (Pieter Wuille)

Pull request description:

  Further backports for the `25.x` branch. Currently:
  * #27622
  * #27834
  * #28125
  * #28452
  * #28542
  * #28543
  * #28551
  * #28571
  * bitcoin-core/gui#751

ACKs for top commit:
  hebasto:
    re-ACK 45a5fcb, only #28551 has been backported with since my recent [review](#28487 (review)).
  dergoegge:
    reACK 45a5fcb
  willcl-ark:
    reACK 45a5fcb

Tree-SHA512: 0f5807aa364b7c2a2039fef11d5cd5e168372c3bf5b0e941350fcd92e7db4a1662801b97bb4f68e29788c77d24bbf97385a483c4501ca72d93fa25327d5694fa
fanquake added a commit that referenced this pull request Oct 6, 2023
@fanquake
Merge #28535: [24.x] Further backports
1416d09 
9077f21 depends: fix unusable memory_resource in macos qt build (fanquake)
dccacf0 build, macos: Fix `qt` package build with new Xcode 15 linker (Hennadii Stepanov)
4359649 ci: Switch to `amd64` container in "ARM" task (Hennadii Stepanov)
805f98b ci: Nuke Android APK task, Use credits for tsan (MarcoFalke)
cb5512d test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq)
01f8ee4 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq)
1c98029 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq)
77979e0 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq)
67b6d99 Do not use std::vector = {} to release memory (Pieter Wuille)
defdc15 ci: Use podman stop over podman kill (MarcoFalke)
7f1357d ci: Use podman for persistent workers (MarcoFalke)
0db69a3 ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN (MarcoFalke)

Pull request description:

  Backports to the 24.x branch. Currently:
  * #27622
  * #27777
  * #27834
  * #27844
  * #27886
  * #28452
  * #28543
  * #28571

ACKs for top commit:
  stickies-v:
    ACK 9077f21

Tree-SHA512: abaafc9a048b67b494993134fd332457ea52695ec007b963c283f962ec40c3b6b3a7e98407481be55d3271a595088a0281cc84b79dad4f24d260381ea0153076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko approved these changes

@glozow glozow glozow left review comments

@instagibbs instagibbs instagibbs approved these changes

@willcl-ark willcl-ark willcl-ark approved these changes

@pinheadmz pinheadmz Awaiting requested review from pinheadmz

Assignees
No one assigned
Labels
TX fees and policy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid serving stale fees