rpc: Add dumpcoinstats #19792

fjahr · 2020-08-24T19:01:38Z

Requires #19521 (this PR only adds the last commit).

The rpc dumpcoinstats dumps the content of the coinstats index into a CSV file for auditing purposes.

DrahtBot · 2020-08-24T21:25:41Z

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
Concept ACK	laanwj, promag
Approach ACK	Zero-1729
Stale ACK	PierreRochard, w0xlt, Sjors

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:

#27607 (init: verify blocks data existence only once for all the indexers by furszy)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

fjahr · 2020-08-25T21:15:04Z

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.

PierreRochard · 2020-08-27T02:54:23Z

Tested ACK 21c42c2

laanwj · 2020-08-27T12:00:57Z

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.

fjahr · 2020-09-22T21:28:45Z

Rebased on top of the latest changes in #19521

Sjors · 2022-05-30T09:29:18Z

re-tACK 3b3e3d9

maflcko

Not sure about the cs_main locking assumptions

src/rpc/blockchain.cpp

maflcko · 2022-05-30T12:04:23Z

src/rpc/blockchain.cpp

+            NodeContext& node = EnsureAnyNodeContext(request.context);
+            ChainstateManager& chainman = EnsureChainman(node);
+            CChainState& active_chainstate = chainman.ActiveChainstate();
+            active_chainstate.ForceFlushStateToDisk();


Pretty sure flushing and getting the cursor will need to happen in the same cs_main scope, otherwise the flush seems best-effort at best?

Yeah, I would have described it as best effort so I think it can be omitted here as well. And this made me realize that we don't actually need coins_view and blockman, so I adapted this as well.

src/rpc/blockchain.cpp

maflcko · 2022-05-30T12:10:38Z

test/functional/feature_coinstatsindex.py

+            else:
+                file = "test.csv"
+
+            path = os.path.join(get_datadir_path(self.options.tmpdir, 1), file)


Probably time to introduce a helper and use that:

diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 7d2db391b6..aa9ff9831f 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -381,9 +381,13 @@ class TestNode(): def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT): wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor) + @property + def datadir_path(self) -> Path: + return Path(self.datadir) + @property def chain_path(self) -> Path: - return Path(self.datadir) / self.chain + return self.datadir_path / self.chain @property def debug_log_path(self) -> Path:

Suggested change

path = os.path.join(get_datadir_path(self.options.tmpdir, 1), file)

path = self.nodes[1].datadir_path / file

I like the helper but using the / operator would mean file would also need to be a Path object iiuc. So, instead I opted for a slightly different approach that I liked a little better using another helper datadir_file_path. What do you think?

but using the / operator would mean file would also need to be a Path object iiuc

Does it? (It doesn't for me locally)

Hm, seems like I misinterpreted the error, actually the next line fails. And I am now also seeing it fail with the Path object but I am sure it previously passed at least twice with it. Very strange. Maybe a python version or platform issue, need to look deeper into this.

2022-06-11T16:28:24.998000Z TestFramework (INFO): Test dumpcoinstats RPC 2022-06-11T16:28:24.998000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main self.run_test() File "/Users/FJ/projects/clones/bitcoin/test/functional/feature_coinstatsindex.py", line 58, in run_test self._test_dumpcoinstats_rpc() File "/Users/FJ/projects/clones/bitcoin/test/functional/feature_coinstatsindex.py", line 319, in _test_dumpcoinstats_rpc self.nodes[1].dumpcoinstats(path, cumulative) File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__ postdata = json.dumps(self.get_request(*args, **argsn), default=EncodeDecimal, ensure_ascii=self.ensure_ascii) File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 131, in get_request json.dumps(args or argsn, default=EncodeDecimal, ensure_ascii=self.ensure_ascii), File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/__init__.py", line 234, in dumps return cls( File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/encoder.py", line 199, in encode chunks = self.iterencode(o, _one_shot=True) File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/encoder.py", line 257, in iterencode return _iterencode(o, 0) File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 66, in EncodeDecimal raise TypeError(repr(o) + " is not JSON serializable") TypeError: PosixPath('/var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_ru27w0qw/node1/ctest.csv') is not JSON serializable

You could do str(path) before passing it as json?

Yeah, that works too and I just pushed a version with that. But I find it a little less elegant overall.

test/functional/rpc_users.py

aureleoules

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,"

src/rpc/blockchain.cpp

fjahr · 2022-12-22T15:44:20Z

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.

fjahr · 2022-12-22T15:51:48Z

Thanks @fanquake , lightning speed :)

This RPC dumps the full content of the coinstats index together with some other statistics into a CSV file.

DrahtBot · 2023-05-17T19:26:41Z

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

DrahtBot · 2023-08-15T01:40:20Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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.

fjahr · 2023-08-15T14:03:24Z

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.

DrahtBot added Build system RPC/REST/ZMQ UTXO Db and Indexes Validation labels Aug 24, 2020

fjahr force-pushed the dumpcoinstats branch from 1dc0728 to d7cae20 Compare August 25, 2020 21:13

fjahr marked this pull request as ready for review August 25, 2020 21:13

DrahtBot mentioned this pull request Aug 26, 2020

validation: UTXO snapshot activation #19806

Merged

DrahtBot added the Needs rebase label Aug 26, 2020

fjahr force-pushed the dumpcoinstats branch from d7cae20 to 21c42c2 Compare August 26, 2020 18:48

DrahtBot removed the Needs rebase label Aug 26, 2020

This was referenced Aug 28, 2020

Coinstats Index #19521

Merged

Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849

Merged

rpc: remove deprecated CRPCCommand constructor #18531

Merged

DrahtBot mentioned this pull request Sep 19, 2020

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #19953

Merged

DrahtBot added the Needs rebase label Sep 22, 2020

fjahr force-pushed the dumpcoinstats branch 2 times, most recently from c2d0006 to 86b2fb4 Compare September 22, 2020 21:28

maflcko approved these changes May 30, 2022

View reviewed changes

fjahr force-pushed the dumpcoinstats branch 2 times, most recently from c8b5d0a to 42763a2 Compare June 11, 2022 18:50

maflcko reviewed Jun 12, 2022

View reviewed changes

test/functional/rpc_users.py Outdated Show resolved Hide resolved

fjahr force-pushed the dumpcoinstats branch from 42763a2 to c8e63f8 Compare June 12, 2022 15:55

DrahtBot mentioned this pull request Jul 23, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

aureleoules reviewed Oct 14, 2022

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

fjahr mentioned this pull request Oct 23, 2022

signed overflow in coinstats index #26362

Open

fjahr force-pushed the dumpcoinstats branch from c8e63f8 to fe8aa34 Compare December 22, 2022 15:41

fanquake marked this pull request as draft December 22, 2022 15:45

fjahr force-pushed the dumpcoinstats branch from fe8aa34 to be8a3ca Compare December 22, 2022 15:49

fjahr force-pushed the dumpcoinstats branch from be8a3ca to 4171e12 Compare December 22, 2022 15:55

DrahtBot mentioned this pull request Mar 15, 2023

refactor: Extract util/fs from util/system #27254

Merged

DrahtBot added the Needs rebase label Apr 3, 2023

fjahr added 3 commits May 3, 2023 14:04

rpc: Add dumpcoinstats

906d54f

This RPC dumps the full content of the coinstats index together with some other statistics into a CSV file.

test: Add dumpcoinstats RPC functional test

8094ab6

test: Introduce datadir_path helper

9ec745c

fjahr force-pushed the dumpcoinstats branch from 4171e12 to 9ec745c Compare May 3, 2023 12:35

DrahtBot removed the Needs rebase label May 3, 2023

DrahtBot added the CI failed label May 14, 2023

DrahtBot mentioned this pull request May 17, 2023

index: make startup more efficient #27607

Merged

DrahtBot added the Needs rebase label May 17, 2023

fjahr closed this Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Add dumpcoinstats #19792

rpc: Add dumpcoinstats #19792

fjahr commented Aug 24, 2020 •

edited

DrahtBot commented Aug 24, 2020 •

edited

fjahr commented Aug 25, 2020

PierreRochard commented Aug 27, 2020

laanwj commented Aug 27, 2020

fjahr commented Sep 22, 2020

Sjors commented May 30, 2022

maflcko left a comment

maflcko May 30, 2022

fjahr Jun 11, 2022

maflcko May 30, 2022

fjahr Jun 11, 2022

maflcko Jun 11, 2022

fjahr Jun 11, 2022 •

edited

maflcko Jun 11, 2022

fjahr Jun 11, 2022

aureleoules left a comment •

edited

fjahr commented Dec 22, 2022 •

edited

fjahr commented Dec 22, 2022

DrahtBot commented May 17, 2023

DrahtBot commented Aug 15, 2023

fjahr commented Aug 15, 2023

	path = os.path.join(get_datadir_path(self.options.tmpdir, 1), file)
	path = self.nodes[1].datadir_path / file

rpc: Add dumpcoinstats #19792

rpc: Add dumpcoinstats #19792

Conversation

fjahr commented Aug 24, 2020 • edited

DrahtBot commented Aug 24, 2020 • edited

Reviews

Conflicts

fjahr commented Aug 25, 2020

PierreRochard commented Aug 27, 2020

laanwj commented Aug 27, 2020

fjahr commented Sep 22, 2020

Sjors commented May 30, 2022

maflcko left a comment

Choose a reason for hiding this comment

maflcko May 30, 2022

Choose a reason for hiding this comment

fjahr Jun 11, 2022

Choose a reason for hiding this comment

maflcko May 30, 2022

Choose a reason for hiding this comment

fjahr Jun 11, 2022

Choose a reason for hiding this comment

maflcko Jun 11, 2022

Choose a reason for hiding this comment

fjahr Jun 11, 2022 • edited

Choose a reason for hiding this comment

maflcko Jun 11, 2022

Choose a reason for hiding this comment

fjahr Jun 11, 2022

Choose a reason for hiding this comment

aureleoules left a comment • edited

Choose a reason for hiding this comment

fjahr commented Dec 22, 2022 • edited

fjahr commented Dec 22, 2022

DrahtBot commented May 17, 2023

DrahtBot commented Aug 15, 2023

fjahr commented Aug 15, 2023

fjahr commented Aug 24, 2020 •

edited

DrahtBot commented Aug 24, 2020 •

edited

fjahr Jun 11, 2022 •

edited

aureleoules left a comment •

edited

fjahr commented Dec 22, 2022 •

edited