Fee estimation: avoid serving stale fee estimate

ismaelsadeeq commented at 10:16 pm on May 10, 2023: member

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 commented at 10:17 pm on May 10, 2023: contributor

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.

fanquake commented at 9:27 am on May 11, 2023: member

cc @instagibbs @TheBlueMatt

in src/policy/fees.cpp:15 in a2ad5e475f outdated

11@@ -12,6 +12,7 @@
12 #include <policy/feerate.h>
13 #include <primitives/transaction.h>
14 #include <random.h>
15+#include <scheduler.h>

glozow commented at 2:51 pm on May 11, 2023:

I think it’s a bit strange for policy/fees to have a dependency on scheduler. Maybe register FlushFeeEstimates() with the scheduler within AppInitMain() instead?

ismaelsadeeq commented at 9:42 pm on May 11, 2023:

Sure :+1: , fixed.

glozow added the label TX fees and policy on May 11, 2023

in src/util/time.h:56 in a2ad5e475f outdated

52@@ -53,6 +53,7 @@ constexpr auto TicksSinceEpoch(Timepoint t)
53 {
54     return Ticks<Duration>(t.time_since_epoch());
55 }
56+constexpr int64_t count_hours(std::chrono::hours t) { return t.count(); }

MarcoFalke commented at 3:24 pm on May 11, 2023:

nit (feel free to ignore): An alternative to adding a function here would be to call the already existing Ticks<std::chrono::hours>(a) over count_hours(a), but either seems fine.

in test/functional/feature_fee_estimation.py:303 in a2ad5e475f outdated

298+        # Restart node to ensure fee_estimate.dat file is read
299+        self.stop_node(0)
300+        self.start_node(0)
301+        assert_equal(self.nodes[0].estimatesmartfee(1)["feerate"], fee_rate)
302+
303+        fee_dat = os.path.join(self.nodes[0].datadir, self.chain, "fee_estimates.dat")

MarcoFalke commented at 3:26 pm on May 11, 2023:

nit:

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

in test/functional/feature_fee_estimation.py:323 in a2ad5e475f outdated

318+            # If it is, it indicates that fee estimates have been successfully flushed to disk.
319+            with self.nodes[0].assert_debug_log(expected_msgs=["Flushed fee estimates to fee_estimates.dat."]):
320+                # Mock the scheduler for an hour, sleep for a short period to allow the flush to finish
321+                hour = 3600
322+                self.nodes[0].mockscheduler(hour)
323+                time.sleep(1)

MarcoFalke commented at 3:30 pm on May 11, 2023:

nit: I am not a fan of uninterruptible sleep in tests. assert_debug_log can be used to wait, if you supply a timeout argument (or rely on the default timeout)

MarcoFalke commented at 6:48 pm on May 11, 2023:

I think you forgot to remove the sleep?

ismaelsadeeq commented at 6:53 pm on May 11, 2023:

My bad, thanks.

MarcoFalke approved

MarcoFalke commented at 3:32 pm on May 11, 2023: member

lgtm. Left some nits

ismaelsadeeq force-pushed on May 11, 2023

in src/policy/fees.h:26 in 0d2329aeca outdated

21 #include <string>
22 #include <vector>
23 
24+
25+// How often to flush fee estimates to fee_estimates.dat.
26+static constexpr std::chrono::milliseconds FLUSH_INTERVAL{std::chrono::hours{1}};

instagibbs commented at 6:59 pm on May 11, 2023:

s/FLUSH_INTERVAL/FEE_FLUSH_INTERVAL/ ?

ismaelsadeeq commented at 7:05 pm on May 11, 2023:

FEE_FLUSH_INTERVAL is better. I think.

ismaelsadeeq force-pushed on May 11, 2023

in src/policy/fees.cpp:929 in bbb0750bd8 outdated

910+void CBlockPolicyEstimator::FlushFeeEstimates() {
911     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")};
912     if (est_file.IsNull() || !Write(est_file)) {
913         LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
914     }
915+    LogPrintf("Flushed fee estimates to fee_estimates.dat.\n");

instagibbs commented at 7:55 pm on May 17, 2023:

this log line fires even if it actually failed?

ismaelsadeeq commented at 11:02 am on May 18, 2023:

Yes, fixed thank you.

pinheadmz commented at 5:09 pm on May 18, 2023:

Mine as well use this constant here for the filename?

https://github.com/bitcoin/bitcoin/blob/6cc136bbd36f859a16e469bb5c016d06c19bcd50/src/policy/fees_args.cpp#L10

in src/policy/fees.h:191 in 5a41661a11 outdated

185@@ -186,6 +186,10 @@ class CBlockPolicyEstimator
186     static constexpr double FEE_SPACING = 1.05;
187 
188     const fs::path m_estimation_filepath;
189+
190+    /** Fee estimate.dat must not be more than 12 hours old. */
191+    static constexpr std::chrono::hours MAX_FILE_AGE{12};

instagibbs commented at 7:58 pm on May 17, 2023:

Seems ok, but would have to think about this more. I’d be ok with longer too, to start.

ismaelsadeeq commented at 11:10 am on May 18, 2023:

Yeah, to enhance flexibility, I can create a follow-up PR to introduce a configuration variable for MAX_FILE_AGE, This way users can specify the maximum age of estimates they do not want to accept?

glozow commented at 3:18 pm on May 19, 2023:

What is the use case for being able to configure the maximum file age?

ismaelsadeeq commented at 5:19 pm on May 19, 2023:

I was thinking that introducing flexibility would enable users to define their preferred threshold for considering fee estimates as stale. As @instagibbs suggested extending the default 12 hours further. what is your view on it?

willcl-ark commented at 10:56 am on May 30, 2023:

Another use case could be so that testers can purposefully load (very) old fee estimate files to populate estimatesmartfee? #27415 (comment)

glozow commented at 5:03 pm on June 1, 2023:

Not sure if we should load old (i.e. potentially bad) data as it can skew the fee estimates both at startup and after more data has been collected. For testing, they could mock the time right? Just worried about adding a config option that’s a bit footgunny.

ismaelsadeeq commented at 10:24 pm on June 1, 2023:

Yes, I agree with you. stale estimates should not be read. I have updated the MAX_FILE_AGE to 60 hours, 2.5 days, as @instagibbs suggested that longer would be okay

willcl-ark commented at 10:05 am on June 2, 2023:

Can you explain the rationale behind the 60 hours chosen? Also perhaps you can update OP as it still mentions 12 hours :)

ismaelsadeeq commented at 1:21 pm on June 2, 2023:

Estimates that are 2.5 days old have nearly half the decay weight of the most recent block. Thank you I updated the PR description.

in test/functional/feature_fee_estimation.py:323 in aecd92e8e1 outdated

314+
315+
316+    def test_estimate_dat_is_flushed_periodically(self):
317+            # Check if the string "Flushed fee estimates to fee_estimates.dat." is present in the debug log file.
318+            # If it is, it indicates that fee estimates have been successfully flushed to disk.
319+            with self.nodes[0].assert_debug_log(expected_msgs=["Flushed fee estimates to fee_estimates.dat."], timeout=1):

instagibbs commented at 8:00 pm on May 17, 2023:

checking for log is good, but checking it was actually written is better in case there’s a bug in the logging logic

ismaelsadeeq commented at 11:04 am on May 18, 2023:

Yes thanks for pointing this out, I added the check.

ismaelsadeeq force-pushed on May 18, 2023

DrahtBot added the label CI failed on May 18, 2023

ismaelsadeeq force-pushed on May 18, 2023

DrahtBot removed the label CI failed on May 18, 2023

in src/policy/fees.cpp:914 in 4614a64bbf outdated

909 
910+void CBlockPolicyEstimator::FlushFeeEstimates() {
911     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "wb")};
912     if (est_file.IsNull() || !Write(est_file)) {
913         LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
914+        return;

pinheadmz commented at 5:59 pm on May 18, 2023:

could also simplify a bit:

0    if (est_file.IsNull() || !Write(est_file))
1        LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
2    else
3        LogPrintf("Flushed fee estimates to fee_estimates.dat.\n");

ismaelsadeeq commented at 10:16 pm on May 18, 2023:

Thanks for your review, I have fixed it.

ismaelsadeeq commented at 11:22 pm on May 18, 2023:

Mine as well use this constant here for the filename?

:thinking: it is an anon namespace, not defined in policy/fees_args.h, I think it is okay as is, or else

policy/fees_args.h

0const char* GetFeeEstimatesFilename();

policy/fees.args.cpp

0const char* GetFeeEstimatesFilename() {
1    return FEE_ESTIMATES_FILENAME;
2}

policy/fees.cpp

0LogPrintf("Flushed fee estimates to %s.\n", GetFeeEstimatesFilename());

but if it’s possible another way I will be glad to learn how.

pinheadmz commented at 2:15 pm on May 19, 2023:

Ok good point I see, well maybe just then…

 0diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp
 1index 085f938f80..adb0ebc383 100644
 2--- a/src/policy/fees.cpp
 3+++ b/src/policy/fees.cpp
 4@@ -925,7 +925,7 @@ void CBlockPolicyEstimator::FlushFeeEstimates() {
 5     if (est_file.IsNull() || !Write(est_file)) {
 6         LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
 7     } else {
 8-        LogPrintf("Flushed fee estimates to fee_estimates.dat.\n");
 9+        LogPrintf("Flushed fee estimates to %s.", fs::PathToString(m_estimation_filepath));
10     }
11 }
12

ismaelsadeeq commented at 4:52 pm on May 19, 2023:

thanks fixed with

0LogPrintf("Flushed fee estimates to %s.", fs::PathToString(m_estimation_filepath.filename()));

so as to get the filename only not the dir

in src/policy/fees.h:26 in 4614a64bbf outdated

21 #include <string>
22 #include <vector>
23 
24+
25+// How often to flush fee estimates to fee_estimates.dat.
26+static constexpr std::chrono::milliseconds FEE_FLUSH_INTERVAL{std::chrono::hours{1}};

pinheadmz commented at 6:00 pm on May 18, 2023:

I don’t actually know for sure but couldn’t this just be …?

0static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};

MarcoFalke commented at 6:26 pm on May 18, 2023:

It could also be constexpr auto FEE_FLUSH_INTERVAL{1h};

ismaelsadeeq commented at 11:02 pm on May 18, 2023:

Still a nub on cpp, constexpr auto FEE_FLUSH_INTERVAL{1h}; does not compile I added instead static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};

in test/functional/feature_fee_estimation.py:300 in b02e3fc91f outdated

295+        # Get the initial fee rate while node is running
296+        fee_rate = self.nodes[0].estimatesmartfee(1)["feerate"]
297+
298+        # Restart node to ensure fee_estimate.dat file is read
299+        self.stop_node(0)
300+        self.start_node(0)

pinheadmz commented at 6:09 pm on May 18, 2023:

I think you can use self.restart_node(0) here

ismaelsadeeq commented at 11:03 pm on May 18, 2023:

Thanks fixed

pinheadmz commented at 6:15 pm on May 18, 2023: member

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

ismaelsadeeq force-pushed on May 18, 2023

DrahtBot added the label CI failed on May 18, 2023

DrahtBot removed the label CI failed on May 19, 2023

in src/policy/fees.cpp:553 in accf995cf0 outdated

550     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "rb")};
551-    if (est_file.IsNull() || !Read(est_file)) {
552+
553+    // Whenever the fee estimation file is not present return early
554+    if (est_file.IsNull()) {
555+        LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));

glozow commented at 3:11 pm on May 19, 2023:

in accf995cf095d7ff3d10c0403e70bdbc1273e852

Seems like the error message here should be something like “there is no fee estimates file” instead of “failed to read” ?

ismaelsadeeq commented at 5:01 pm on May 19, 2023:

Thanks fixed

glozow commented at 3:20 pm on May 19, 2023: member

Approach ACK

ismaelsadeeq force-pushed on May 19, 2023

DrahtBot added the label CI failed on May 19, 2023

ismaelsadeeq force-pushed on May 19, 2023

ismaelsadeeq force-pushed on May 20, 2023

DrahtBot removed the label CI failed on May 20, 2023

ismaelsadeeq force-pushed on May 26, 2023

ismaelsadeeq force-pushed on Jun 1, 2023

DrahtBot added the label CI failed on Jun 1, 2023

DrahtBot removed the label CI failed on Jun 1, 2023

in test/functional/feature_fee_estimation.py:319 in 8c51cd00fb outdated

314+
315+    def test_estimate_dat_is_flushed_periodically(self):
316+            fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
317+
318+            # Verify that fee_estimates.dat does not exist
319+            assert_equal(os.path.isfile(fee_dat), False)

instagibbs commented at 1:49 pm on June 2, 2023:

to make this test less order-dependent, you can attempt to delete the file, then run the test

ismaelsadeeq commented at 11:02 pm on June 2, 2023:

Thank you, I added the delete attempt

in test/functional/feature_fee_estimation.py:329 in 8c51cd00fb outdated

324+                # Mock the scheduler for an hour, fee estimates will be flushed to fee_estimate.dat
325+                hour = 3600
326+                self.nodes[0].mockscheduler(hour)
327+
328+            # Verify that fee estimates were flushed and fee_estimates.dat file is created
329+            assert_equal(os.path.isfile(fee_dat), True)

instagibbs commented at 1:56 pm on June 2, 2023:

might be good to test that it does it every hour, and that the output files should be matching provided we don’t see blocks in the interim, also, check that what is written on node shutdown also matches?

That way we can have higher assurance on the timing of the flushing and the contents matching what we already do.

ismaelsadeeq commented at 11:03 pm on June 2, 2023:

Sure, thank you, I updated the test.

instagibbs commented at 1:58 pm on June 2, 2023: member

Looking good, just a couple of test suggestions.

Also wondering if we should include a hidden argument to allow older files for testing reasons, re: #27415

I’ve used it for systems I’ve built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.

For testing, they could mock the time right?

Depends on your test setup, really. It might interfere during integration tests which require a realistic clock?

glozow commented at 2:38 pm on June 2, 2023: member

Also wondering if we should include a hidden argument to allow older files for testing reasons, re: #27415

I’ve used it for systems I’ve built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.

For testing, they could mock the time right?

Depends on your test setup, really. It might interfere during integration tests which require a realistic clock?

Makes sense, seems fine as a regtest-only option for testing.

ismaelsadeeq force-pushed on Jun 2, 2023

in test/functional/feature_fee_estimation.py:307 in 451ab8b513 outdated

302+        fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
303+
304+        # Stop the node and backdate the fee_estimates.dat file for 61 hours
305+        self.stop_node(0)
306+        hour = 60 * 60
307+        last_modified_time = time.time() - 61 * hour

willcl-ark commented at 8:33 am on June 6, 2023:

I know we only use it once in the test, but if you re-touch this file it could be nice to use a named variable for the 60 hours, to make it clear where it comes from? (e.g. MAX_FILE_AGE would shadow the name used in fees.h)

ismaelsadeeq commented at 12:53 pm on June 6, 2023:

I agree it’s better than the magic number, I have updated it.

in test/functional/feature_fee_estimation.py:347 in 451ab8b513 outdated

342+        # Verify that the estimates remain the same after shutdown with no blocks before shutdown
343+        self.restart_node(0)
344+        fee_dat_current_content = open(fee_dat, "rb").read()
345+        assert_equal(fee_dat_current_content, fee_dat_initial_content)
346+
347+        # Verify that the estimates are not the same if there are no blocks in the flush interval

willcl-ark commented at 8:37 am on June 6, 2023:

0        # Verify that the estimates are not the same if new blocks were produced in the flush interval

ismaelsadeeq commented at 12:54 pm on June 6, 2023:

Fixed Thank you.

willcl-ark approved

willcl-ark commented at 8:39 am on June 6, 2023: contributor

ACK 451ab8b513

Left a few comments on the test in case you touch that file again, but nothing blocking.

Not loading very old fee estimates makes good sense to me to further avoid creating stuck transactions.

ismaelsadeeq force-pushed on Jun 6, 2023

instagibbs approved

instagibbs commented at 1:04 pm on June 6, 2023: member

code review ACK 6d36f3edbe7549cfaadc48b43c09dd3e581e92ff

I’d really prefer we have a regtest-only option to skip the check to impact integration tests the least.

DrahtBot requested review from willcl-ark on Jun 6, 2023

DrahtBot added the label CI failed on Jun 6, 2023

DrahtBot removed the label CI failed on Jun 6, 2023

ismaelsadeeq force-pushed on Jun 7, 2023

DrahtBot added the label CI failed on Jun 7, 2023

ismaelsadeeq commented at 8:52 am on June 8, 2023: member

I’d really prefer we have a regtest-only option to skip the check to impact integration tests the least.

I added a support for skipping the check only on regtest in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06 by passing -acceptstaleestimates option and it’s tested in dea3accb1f2eeef74f293262c168d68c0ec444cb

in src/policy/fees.h:29 in 2fec7f362d outdated

24@@ -25,6 +25,9 @@
25 // How often to flush fee estimates to fee_estimates.dat.
26 static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};
27 
28+// Stale fee estimates are not going to be read on mainnet
29+static constexpr bool ACCEPT_STALE_FEE_ESTIMATES = false;

glozow commented at 9:36 am on June 8, 2023:

in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06:

prefix with DEFAULT_, make doxygen compatible, use braced initialization. Documentation about where the option can be used belongs where that code lives, not here.

0/** Whether we allow importing a fee_estimates file older than MAX_FILE_AGE. */
1static constexpr bool DEFAULT_ACCEPT_STALE_FEE_ESTIMATES{false};

ismaelsadeeq commented at 12:46 pm on June 8, 2023:

Thank you, I updated it.

in src/init.cpp:589 in dea3accb1f outdated

585@@ -586,6 +586,7 @@ void SetupServerArgs(ArgsManager& argsman)
586     argsman.AddArg("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !testnetChainParams->RequireStandard()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
587     argsman.AddArg("-incrementalrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define cost of relay, used for mempool limiting and replacement policy. (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_INCREMENTAL_RELAY_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
588     argsman.AddArg("-dustrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define dust, the value of an output such that it will cost more than its value in fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
589+    argsman.AddArg("-acceptstalefeeestimates", "Read fee estimates even if they are stale supported only in regtest", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

glozow commented at 9:49 am on June 8, 2023:

in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06 Please also include here (1) the default value (2) how old “stale” is specifically, using the constants.

ismaelsadeeq commented at 12:47 pm on June 8, 2023:

I modify as you suggested 👍🏾

in src/policy/fees.h:198 in 2fec7f362d outdated

194@@ -192,9 +195,11 @@ class CBlockPolicyEstimator
195      */
196     static constexpr std::chrono::hours MAX_FILE_AGE{60};
197 
198+    bool m_read_stale_estimates;

glozow commented at 9:50 am on June 8, 2023:

2fec7f362d8540cda5ff7e3e4dfb2cb751364e06

this should be const

ismaelsadeeq commented at 12:48 pm on June 8, 2023:

Thank you

in test/functional/feature_fee_estimation.py:318 in dea3accb1f outdated

313+        assert_equal(self.nodes[0].estimatesmartfee(1)["errors"], ["Insufficient data or no feerate found"])
314+
315+
316+    def test_estimate_dat_is_flushed_periodically(self):
317+        fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
318+        hour = 3600

glozow commented at 9:53 am on June 8, 2023:

nit in dea3accb1f2eeef74f293262c168d68c0ec444cb

maybe replace this hour and the one in the other tests with a constant at the top of the file, SECONDS_PER_HOUR

in test/functional/feature_fee_estimation.py:332 in dea3accb1f outdated

329+            self.nodes[0].mockscheduler(hour)
330+
331+        # Verify that fee estimates were flushed and fee_estimates.dat file is created
332+        assert_equal(os.path.isfile(fee_dat), True)
333+
334+        # Verify that the estimates remain the same if there are no blocks in the flush interval

glozow commented at 9:55 am on June 8, 2023:

in dea3accb1f2eeef74f293262c168d68c0ec444cb

Given that you comment “there are no blocks” you should probably also assert that is true, e.g. assert_equal the result from getbestblockhash before and after

ismaelsadeeq commented at 12:48 pm on June 8, 2023:

sure fixed

glozow commented at 9:56 am on June 8, 2023: member

Looks pretty good, thanks for adding the option. Please clean up the commit messages. dea3accb1f2eeef74f293262c168d68c0ec444cb claims to add the regtest option, but that is done in 2fec7f362d8540cda5ff7e3e4dfb2cb751364e06.

ismaelsadeeq force-pushed on Jun 8, 2023

instagibbs commented at 1:16 pm on June 8, 2023: member

tsan failure looks unrelated: https://cirrus-ci.com/task/5970681108627456

in src/init.cpp:1261 in f91d089a76 outdated

1254@@ -1253,7 +1255,18 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1255     assert(!node.fee_estimator);
1256     // Don't initialize fee estimation with old data if we don't relay transactions,
1257     // as they would never get updated.
1258-    if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(FeeestPath(args));
1259+    if (!ignores_incoming_txs) {
1260+        bool read_stale_estimates = args.GetBoolArg("-acceptstalefeeestimates", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES);
1261+        if (read_stale_estimates && (chainparams.GetChainType() != ChainType::REGTEST)) {
1262+            LogPrintf("Reading stale fee estimates is only supported in regtest \n");

instagibbs commented at 1:32 pm on June 8, 2023:

I think this should fail rather than being ignored?

ismaelsadeeq commented at 10:18 pm on June 8, 2023:

I agree, fixed it.

DrahtBot removed the label CI failed on Jun 8, 2023

ismaelsadeeq force-pushed on Jun 8, 2023

in src/policy/fees.h:28 in efe5f373e1 outdated

23 
24+
25+// How often to flush fee estimates to fee_estimates.dat.
26+static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};
27+
28+/** fee_estimates.dat that are more than 60 hours old, 2 and half days will not be read,

willcl-ark commented at 7:24 am on June 9, 2023:

clarity nit:

0/** fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,

pinheadmz commented at 6:51 pm on June 13, 2023:

I think if you retouch there is some style nit too:

0- /** fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,
1+ /**
2+  * fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,

ismaelsadeeq commented at 8:51 am on June 15, 2023:

Updated, thank you @willcl-ark

willcl-ark approved

willcl-ark commented at 7:44 am on June 9, 2023: contributor

re-ACK efe5f373e1

Changes look good. Left a nit not to be addressed unless you re-touch :)

I also checked how much time the new tests were adding to feature_fee_estimation.py, as this is often one of the longest-running tests on my machine. It looks like it’s <=1second, so that seems fine.

DrahtBot requested review from instagibbs on Jun 9, 2023

instagibbs commented at 12:56 pm on June 9, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27622/commits/efe5f373e12329104f1c706145eee75c2d2079a7

DrahtBot removed review request from instagibbs on Jun 9, 2023

in src/policy/fees.h:202 in 29814149f2 outdated

198+    const bool m_read_stale_estimates;
199+
200 public:
201     /** Create new BlockPolicyEstimator and initialize stats tracking classes with default values */
202-    CBlockPolicyEstimator(const fs::path& estimation_filepath);
203+    CBlockPolicyEstimator(const fs::path& estimation_filepath, const bool& read_stale_estimates);

glozow commented at 8:56 pm on June 11, 2023:

in 29814149f29ce1858ee3f6ea2fbd37ec411070d6: for primitive types like bools, it makes more sense to pass by value

ismaelsadeeq commented at 8:53 am on June 15, 2023:

Updated 💯

in test/functional/feature_fee_estimation.py:308 in efe5f373e1 outdated

303+
304+        fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
305+
306+        # Stop the node and backdate the fee_estimates.dat file more than MAX_FILE_AGE
307+        self.stop_node(0)
308+        last_modified_time = time.time() - (MAX_FILE_AGE + 1) * SECONDS_PER_HOUR

glozow commented at 9:13 pm on June 11, 2023:

in efe5f373e12329104f1c706145eee75c2d2079a7

Is this 61 hours? Or 1 second past the 60hour mark?

ismaelsadeeq commented at 11:45 am on June 14, 2023:

61 hours.

in src/policy/fees.cpp:531 in 29814149f2 outdated

527@@ -528,8 +528,9 @@ bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock)
528     }
529 }
530 
531-CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath)
532-    : m_estimation_filepath{estimation_filepath}
533+CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath, const bool& read_stale_estimates)

pinheadmz commented at 6:54 pm on June 13, 2023:

Question: isn’t bool small enough to pass by value instead of reference? (i.e. remove the & ?)

ismaelsadeeq commented at 8:52 am on June 15, 2023:

Yes, thank you. I updated it

in src/policy/fees.h:198 in 29814149f2 outdated

193@@ -191,9 +194,12 @@ class CBlockPolicyEstimator
194     static constexpr double FEE_SPACING = 1.05;
195 
196     const fs::path m_estimation_filepath;
197+
198+    const bool m_read_stale_estimates;

pinheadmz commented at 6:56 pm on June 13, 2023:

Does this need to be a class property since we only use it one time, during object construction?

ismaelsadeeq commented at 8:53 am on June 15, 2023:

No, It does not thank you.

pinheadmz approved

pinheadmz commented at 7:13 pm on June 13, 2023: member

ACK efe5f373e12329104f1c706145eee75c2d2079a7

Great work on this! A few non-breaking questions below. Ran all tests locally, ensured all 3 new tests fail on master. Reviewed each commit, confirmed regtest-only argument is only accepted on regtest!

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK efe5f373e12329104f1c706145eee75c2d2079a7
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmSIvz0ACgkQ5+KYS2KJ
 7yTrL8w//coGfk9oOwTpePMIqxFQxlhxZP9hX6wur36aKPLtR7e07DYelDAvdQnZn
 8U3zLpMVDhoaWoAysQUinMyblQ06JCaNmPhDtu9b3gs6kPMGIhmhqM1SgcHtosvUe
 9r05sZGVZVlhz3lsHQ4ebXqzQwq6LuAxvXniKeTj7Afu2IhAsycJCFoXTSzTbMWlG
102GyJdkA1PQnaEMV5uZVFI7/oosG0ifNsoH9jeRYbqRoZjXQC3rAtP4a+T07IiNgv
11K4lI1HrekW3M0+JpvoV38cYGuIehpKEoMS9ISTxa3o3zTNn8KjxpXZTOXq9o6IAv
125p7VpMHFVWXYBD+V21Jk2zni/BS6x2KtPLWhMNlTUoxnsw28R8pRbyeUEYBjIXEX
13Z19BgwMOsZqWaz8DU0SedRC1ngpbidOrbgYQZaTXXP0BZX1YZDjTMH/ojV/gVWqo
14VDhpT8RG/PetW6xvOAp9pAfDdNENvSCOIQVjCg04kxmqlrAvbNS9UHX+dvuCHEGB
15njArT/p1TjCD3Pq6hO3YSH8ANVRnrb4Vg3qoB0WzT7DcHfySmLiydDnvmOut2HEC
16G2D4vor14YXr5/NNipImX0IO0aTMXr5drswhwfXPoBjSExG4UQ6f4Lev2/AHkibH
175tSMfX40NagvSx9+xNqKjpZrKk+89rFhddfA9iB+XZidAzYSoVc=
18=49Ly
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

achow101 commented at 7:33 pm on June 14, 2023: member

@ismaelsadeeq Are you planning on making any further changes here?

ismaelsadeeq commented at 8:34 pm on June 14, 2023: member

@ismaelsadeeq Are you planning on making any further changes here?

Yes I will add all recent feedbacks in next push.

tx fees, policy: periodically flush fee estimates to fee_estimates.dat

This reduces chances of having old estimates in fee_estimates.dat.

5b886f2b43

tx fees, policy: do not read estimates of old fee_estimates.dat

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.

3eb241a141

tx fees, policy: read stale fee estimates with a regtest-only option

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.

cf219f29f3

test: ensure old fee_estimate.dat not read on restart and flushed

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.

d2b39e09bc

ismaelsadeeq force-pushed on Jun 14, 2023

instagibbs commented at 10:33 am on June 15, 2023: member

@ismaelsadeeq just FYI if you don’t have to rebase on master due to a conflict, don’t. It’s extra review burden and makes getting re-ACKs harder.

ismaelsadeeq commented at 10:53 am on June 15, 2023: member

@ismaelsadeeq just FYI if you don’t have to rebase on master due to a conflict, don’t. It’s extra review burden and makes getting re-ACKs harder.

Ohh Sorry, Thank you for pointing this out. Its okay now I think and any further nit changes I would address in a follow-up.

willcl-ark commented at 11:18 am on June 15, 2023: contributor

Just noting my understanding of this and the followup pull and how they work together.

In this PR we will regularly persist fee estimations to fee_estimates.dat, providing more protection in an unclean shutdown scenario. We then reload these on startup, under then new condition that they are less than 2.5 days old.

In the case where the restart is instant/very fast this let’s us keep accurate fee estimation after a crash.
In the case where the restart is > 60 hours then we are improved in that we don’t serve known stale data.
In the case where fees start high (or low), and remain high (or low), this will help us with more accurate fee estimation after restart.
If fees start high and descrease then we may have too-high fee estimation, which still helps us avoid unwated stuck transactions.
If fees start low and increased then our fee estimation may report too-low. Although this pull doesn’t help directly in this case, it’s not worse than current behaviour and the expiry should make it strictly better.

In the followup pull, persisting mempool_min_fee to disk is much more sensitive than fee estimation data. If we used outdated rates here we may start rejecting valid tx from the p2p network from our mempool which may be undesirable. Therefore IMO it makes sense there to be much more strict about the lifetime of mempool_min_fee rates read from disk at startup; perhaps something more in the order of 1 hour?

RandyMcMillan commented at 5:49 pm on June 15, 2023: contributor

concept ACK

RandyMcMillan commented at 7:02 pm on June 15, 2023: contributor

In this scenario - the node is going to have to sync - whether it was shut down for 1 hour or 60+ hours. IMO it would be ideal to not have one more thing dependent upon system time when booting up the node (*There are other issues related to system time when booting up the node). Since we know that the node must connect and sync - I think the ideal is to delay the file age check later into the life cycle of the node.

*In the scenario where system time has been skewed - off by many hours - there is a possibility that the file age is “in the future”. You may want to use absolute value in the file age check… this will remove the possibility of a negative value for file age.

instagibbs commented at 7:06 pm on June 15, 2023: member

Since we know that the node must connect and sync - I think the ideal is to delay the file age check later into the life cycle of the node.

Not sure what the suggestion is. Can you be specific?

in src/policy/fees.h:29 in 3eb241a141 outdated

24@@ -25,6 +25,11 @@
25 // How often to flush fee estimates to fee_estimates.dat.
26 static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};
27 
28+/** fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,
29+ * as the estimates in the file are stale.

glozow commented at 10:54 am on June 19, 2023:

nit in 3eb241a141defa564c94cb95c5bbaf4c5bd9682e: I think the word “old” may be missing. Also could elaborate a tiny bit on why stale is bad.

0/** fee_estimates.dat that are more than 60 hours (2.5 days) old will not be read,
1 * as fee estimates are based on historical data and may be inaccurate if
2 * network activity has changed.

ismaelsadeeq commented at 7:48 am on July 26, 2023:

Fixed in #28157

glozow commented at 12:31 pm on June 19, 2023: member

In this scenario - the node is going to have to sync - whether it was shut down for 1 hour or 60+ hours. IMO it would be ideal to not have one more thing dependent upon system time when booting up the node

I don’t see how this is very problematic - we are checking the timestamp we put on our own file, which is only used for fee estimation. The data in that file isn’t really concerned with what the current time is, it’s tracking based on a relative number of blocks.

*In the scenario where system time has been skewed - off by many hours - there is a possibility that the file age is “in the future”. You may want to use absolute value in the file age check… this will remove the possibility of a negative value for file age.

If you manage to be off by more than 60 hours, your node will have much worse problems. We reject blocks if they are MAX_FUTURE_BLOCK_TIME i.e. 2 hours into the future. Clocks don’t need to be completely synced, but they’re not supposed to be wildly off.

in src/policy/fees.cpp:551 in 3eb241a141 outdated

545@@ -545,9 +546,22 @@ CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath
546     shortStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
547     longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
548 
549-    // If the fee estimation file is present, read recorded estimations
550     AutoFile est_file{fsbridge::fopen(m_estimation_filepath, "rb")};
551-    if (est_file.IsNull() || !Read(est_file)) {
552+
553+    // Whenever the fee estimation file is not present return early

willcl-ark commented at 12:37 pm on June 20, 2023:

nit: Personally I don’t think comments like this are particularly needed/useful, as they are just describing exactly the code below. I ususally try and save them for when something reads as unexpected or I want to add a reason why I am doing something.

In the comment 7 lines below it could be a useful comment (in my opinion) if it read something like: “serving stale fee estimates can cause transactions to become stuck in the bottom of the mempool in the worst case. Don’t do that.” As this explains why we expire fee estimates.

instagibbs commented at 1:25 pm on June 20, 2023:

Not blocking, but I agree. Comments are generally required when the code fails to self-document. This whole section is self-explanatory, which is good, meaning comments aren’t really required.

ismaelsadeeq commented at 7:49 am on July 26, 2023:

Fixed in #28157

in test/functional/feature_fee_estimation.py:368 in d2b39e09bc

363+        self.restart_node(0)
364+        fee_dat_current_content = open(fee_dat, "rb").read()
365+        assert fee_dat_current_content != fee_dat_initial_content
366+
367+
368+    def test_acceptstalefeeestimates_option(self):

willcl-ark commented at 12:49 pm on June 20, 2023:

nit: There’s no test to ensure that stale estimates aren’t accepted on non-regtest networks, and I’m not sure there particularly needs to be. But I verified manually that this was the case:

0$./src/bitcoind -testnet -acceptstalefeeestimates
12023-06-20T12:47:07Z Error: acceptstalefeeestimates is not supported on test chain.
22023-06-20T12:47:07Z Shutdown: In progress...
32023-06-20T12:47:07Z scheduler thread exit
42023-06-20T12:47:07Z Shutdown: done

ismaelsadeeq commented at 7:47 am on July 26, 2023:

Fixed in #28157

willcl-ark approved

willcl-ark commented at 12:55 pm on June 20, 2023: contributor

ACK d2b39e09bc

Looks good to me now.

Using stale fee estimates is a good way to get your transaction stuck in the mempool, and therefore not using those makes sense. I also agree with flushing the data periodically and not just on shutdown, which can help us have access to current estimates even after an unclean shutdown.

Left a few nits which don’t need addressing

DrahtBot requested review from instagibbs on Jun 20, 2023

DrahtBot requested review from pinheadmz on Jun 20, 2023

instagibbs approved

instagibbs commented at 1:30 pm on June 20, 2023: member

reACK https://github.com/bitcoin/bitcoin/pull/27622/commits/d2b39e09bc6a5982fc5cf4b538b7fdb0e3cae576

glozow commented at 2:09 pm on June 20, 2023: member

ACK d2b39e09bc6a5982fc5cf4b538b7fdb0e3cae576. One nit if you follow up.

glozow merged this on Jun 20, 2023

glozow closed this on Jun 20, 2023

dergoegge commented at 3:51 pm on June 20, 2023: member

This should probably be backported.

sidhujag referenced this in commit 5d0a209488 on Jun 21, 2023

luke-jr referenced this in commit 918cbb20e0 on Aug 16, 2023

luke-jr referenced this in commit e7cc1d0aa0 on Aug 16, 2023

luke-jr referenced this in commit 2983373445 on Aug 16, 2023

luke-jr referenced this in commit 8bea323e13 on Aug 16, 2023

glozow referenced this in commit a84dade1f9 on Aug 22, 2023

Frank-GER referenced this in commit 6efaec637c on Sep 8, 2023

fanquake commented at 5:38 pm on September 24, 2023: member

Added to #28487 for backporting to 25.1.

fanquake referenced this in commit 55cd4f1887 on Sep 24, 2023

fanquake referenced this in commit 71832b190d on Sep 24, 2023

fanquake referenced this in commit 8284c09ad7 on Sep 24, 2023

fanquake referenced this in commit 19cd63366c on Sep 24, 2023

fanquake referenced this in commit 56edeb8707 on Sep 26, 2023

fanquake referenced this in commit 59c346b9b1 on Sep 26, 2023

fanquake referenced this in commit c23bb6dd81 on Sep 26, 2023

fanquake referenced this in commit f009d26a99 on Sep 26, 2023

fanquake commented at 12:20 pm on September 26, 2023: member

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

Fee estimation: avoid serving stale fee estimate #27622

Reviews

Conflicts