WIP: add support to save fee estimates without having to shut down the node

greenaddress commented at 4:58 pm on October 29, 2021: contributor

This PR adds a new RPC method called savefeeestimates in a similar fashion to savemempool RPC.

The rational behind this is that currently there is no way to save the fee estimates without shutting down the node and this makes it harder/slower to bring up new instances of bitcoin core.

For example in https://github.com/Blockstream/esplora/ when we do new deployments or when we have higher traffic we need to bring up new instances, currently the procedure uses a snapshot of the blockchain (say a few weeks old) brings it up to the tip, then stops it, calls savemempool on a long running instance, copies that file over and restarts the new node so that the new instance has all the transactions in mempool even though it wasn’t live to receive them when they occurred.

However the fee estimation of that new node are not up to date and as such the new node will report bad fee estimates.

This PR will make it possible to start a new node with reasonable fee estimates from an old running node without having to shut down said old running node.

Any feedback welcome and thanks for reviewing/reading.

darosior commented at 5:04 pm on October 29, 2021: member

Concept ACK, will review.

greenaddress force-pushed on Oct 29, 2021

in test/functional/feeestimates_persist.py:42 in 0f41652093 outdated

37+        assert not os.path.isfile(fee_estimatesdat)
38+        self.nodes[0].savefeeestimates()
39+        self.log.debug('Verify the fee_estimates.dat file exists after calling savefeeestimates RPC')
40+        assert os.path.isfile(fee_estimatesdat)
41+        self.log.debug("Prevent bitcoind from writing fee_estimates.dat to disk. Verify that `savefeeestimates` fails")
42+        os.chmod(fee_estimatesdat, S_IREAD|S_IRGRP|S_IROTH)

greenaddress commented at 5:38 pm on October 29, 2021:

It looks like os.chmod(fee_estimatesdat, S_IREAD|S_IRGRP|S_IROTH) is not the ideal way to make core fail to write the file in a way that works across platforms (works locally but doesn’t seem to work on the 32-bit + dash[gui][CentOS8] Cirrus CI test

Ideas? Should I make writing the fee_estimates.dat file similar to mempool.dat where we write to a new file and then move over the old file and fail if the new file already exist?

in test/functional/test_runner.py:136 in 0f41652093 outdated

132@@ -133,6 +133,7 @@
133     'feature_reindex.py',
134     'feature_abortnode.py',
135     # vv Tests less than 30s vv
136+    'feeestimates_persist.py',

brunoerg commented at 5:55 pm on October 29, 2021:

I recommend you to follow the naming guideline, see docs (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md).

greenaddress commented at 6:30 pm on October 29, 2021:

@brunoerg do you mean fee_estimates_persist.py would be a better name or? I used the same as mempool_persist.py but I agree I should add an underscore between fee and estimates

DrahtBot added the label RPC/REST/ZMQ on Oct 29, 2021

DrahtBot added the label TX fees and policy on Oct 29, 2021

greenaddress force-pushed on Oct 29, 2021

JeremyRubin commented at 7:12 pm on October 29, 2021: contributor

concept ack; implementation NACK.

IMO the RPC should instead return a JSON format of the fee estimate file which can be paired with a loadfeeestimates rpc on the new node rather than encouraging copying files.

greenaddress force-pushed on Oct 29, 2021

in src/rpc/misc.cpp:344 in ecd40a92fb outdated

337@@ -337,6 +338,33 @@ static RPCHelpMan verifymessage()
338     };
339 }
340 
341+static RPCHelpMan savefeeestimates()
342+{
343+    return RPCHelpMan{"savefeeestimates",
344+                "\nDumps the fee estimates to disk. It will fail until the previous dump is fully loaded.\n",

lsilva01 commented at 3:56 am on October 30, 2021:

nit: The user has no way of knowing where the file will be saved. More information needed.

luke-jr commented at 4:33 pm on October 30, 2021:

Seems that’s an internal implementation detail?

lsilva01 approved

lsilva01 commented at 4:12 am on October 30, 2021: contributor

tACK ecd40a9 on Ubuntu 20.04.

This current approach solves the problem in similar way to savemempool. Maintaining a pattern of behavior and results between RPC methods is a positive aspect.

Not sure this method should return JSON format, as there’s already exist the fee_estimates.dat file to store this information and this would create 2 data structures for the same information.

But I think a follow-up PR can change this file to fee_estimates.json and implement a load RPC based on this JSON. It would maintain a single structure for fee estimates.

in src/rpc/misc.cpp:359 in ecd40a92fb outdated

354+    LOCK(dump_mutex);
355+    CBlockPolicyEstimator& fee_estimator = EnsureAnyFeeEstimator(request.context);
356+
357+    fs::path est_filepath = gArgs.GetDataDirNet() / CBlockPolicyEstimator::FEE_ESTIMATES_FILENAME;
358+    CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION);
359+    if (est_file.IsNull() || !fee_estimator.Write(est_file)) {

luke-jr commented at 4:33 pm on October 30, 2021:

IMO better to abstract this to fees.cpp

greenaddress commented at 8:33 pm on November 1, 2021:

Done, I called the method Write, without parameters - does it make sense to you to overload the method?

luke-jr commented at 1:02 am on November 3, 2021:

No opinion on that

in test/functional/test_runner.py:707 in ecd40a92fb outdated

703@@ -703,7 +704,7 @@ def was_successful(self):
704 def check_script_prefixes():
705     """Check that test scripts start with one of the allowed name prefixes."""
706 
707-    good_prefixes_re = re.compile("^(example|feature|interface|mempool|mining|p2p|rpc|wallet|tool)_")
708+    good_prefixes_re = re.compile("^(example|feature|fee|interface|mempool|mining|p2p|rpc|wallet|tool)_")

luke-jr commented at 4:34 pm on October 30, 2021:

I’m not sure this is a good prefix. Maybe feature_feeestimates_persist?

greenaddress commented at 8:33 pm on November 1, 2021:

Yep, sounds reasonable, done

JeremyRubin commented at 6:02 pm on October 30, 2021: contributor

the json format is required for a return value since it’s a JSON RPC, but that could just be e.g. a hex encoded string of the file contents.

add support to save fee estimates without shutting down the node d5b41e6b2e

greenaddress force-pushed on Nov 1, 2021

DrahtBot commented at 9:30 pm on November 1, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24959 (Remove not needed clang-format off comments by MarcoFalke)
#13990 (Allow fee estimation to work with lower fees by ajtowns)

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.

luke-jr referenced this in commit dd24671400 on Nov 4, 2021

DrahtBot commented at 2:24 pm on April 26, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Apr 26, 2022

DrahtBot commented at 9:59 am on August 1, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

achow101 commented at 6:44 pm on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

bitcoin locked this on Oct 12, 2023

WIP: add support to save fee estimates without having to shut down the node #23387

Conflicts