rpc: Add dumpcoinstats #19792
rpc: Add dumpcoinstats #19792
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
Taking this out of draft status since I am interested in conceptual feedback on something like this but of course, #19521 still has a long way to go before this can be seriously considered. |
|
Tested ACK 21c42c2 |
|
Concept ACK, I wish we had gotten streaming to work though so that the file can be downloaded (see my experiment in #7759). RPCs that creates a server-local file, although there are already some others, are not that great from a security and usability point of view. |
c2d0006 to
86b2fb4
Compare
|
Rebased on top of the latest changes in #19521 |
c8b5d0a to
42763a2
Compare
There was a problem hiding this comment.
If the index has not fully been synced yet, the error thrown would be:
Failed to get coins stats at height X. You may have to remove an incomplete dump file before running this command again.
It's not very clear that the index has not finished syncing.
Also, the csv file would be created before this error is thrown, so the file would have to be removed manually.
I would suggest checking if the index is synced before dumping and create the file only if the index is ready+enabled, with something like this:
Diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 74877d7da..7bb6e2927 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1039,16 +1039,21 @@ static RPCHelpMan dumpcoinstats()
throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.u8string() + " already exists. If you are sure this is what you want, move it out of the way first");
}
+ if (!g_coin_stats_index) {
+ throw JSONRPCError(RPC_MISC_ERROR, "Coinstats index is not enabled");
+ }
+
+ const auto summary{g_coin_stats_index->GetSummary()};
+ if (!summary.synced) {
+ throw JSONRPCError(RPC_MISC_ERROR, strprintf("coinstatsindex is not synced. Current best block height: %d", summary.best_block_height));
+ }
+
std::ofstream file;
file.open(filepath);
if (!file.is_open()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open dump file");
}
- if (!g_coin_stats_index) {
- throw JSONRPCError(RPC_MISC_ERROR, "Coinstats index is not enabled");
- }
-
const bool cumulative{request.params[1].isNull() ? false : request.params[1].get_bool()};
file << strprintf("height,"|
Addressed comments from @aureleoules , thanks for the review! Further note: I can't change the status back it seems, but please treat this as a draft for now. This PR will need to be updated and rebased soon when I have resolved the overflow issue noted in #26362. EDIT: Rebased as well since there was a silent merge conflict. |
|
Thanks @fanquake , lightning speed :) |
This RPC dumps the full content of the coinstats index together with some other statistics into a CSV file.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
There doesn't seem to be much interest in this feature currently. If someone needs it, feel free to ping me here and I will reorg/rebase for you. |
Requires #19521 (this PR only adds the last commit).
The rpc
dumpcoinstatsdumps the content of the coinstats index into a CSV file for auditing purposes.