WIP: add support to save fee estimates without having to shut down the node #23387
Conversation
greenaddress
force-pushed
the
dump_fee_estimates
branch
from
de06a90 to
0f41652
Compare
test/functional/test_runner.py
Outdated
There was a problem hiding this comment.
I recommend you to follow the naming guideline, see docs (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md).
There was a problem hiding this comment.
@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
greenaddress
force-pushed
the
dump_fee_estimates
branch
2 times, most recently
from
b482500 to
8d1ff47
Compare
|
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
the
dump_fee_estimates
branch
from
8d1ff47 to
ecd40a9
Compare
lsilva01
left a comment
lsilva01
left a comment
There was a problem hiding this comment.
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.
src/rpc/misc.cpp
Outdated
There was a problem hiding this comment.
nit: The user has no way of knowing where the file will be saved. More information needed.
There was a problem hiding this comment.
Seems that's an internal implementation detail?
src/rpc/misc.cpp
Outdated
There was a problem hiding this comment.
IMO better to abstract this to fees.cpp
There was a problem hiding this comment.
Done, I called the method Write, without parameters - does it make sense to you to overload the method?
test/functional/test_runner.py
Outdated
There was a problem hiding this comment.
I'm not sure this is a good prefix. Maybe feature_feeestimates_persist?
There was a problem hiding this comment.
Yep, sounds reasonable, done
|
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. |
greenaddress
force-pushed
the
dump_fee_estimates
branch
from
ecd40a9 to
d5b41e6
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Github-Pull: bitcoin#23387 Rebased-From: d5b41e6
|
🐙 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". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
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. |
8 participants
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.