Implement "getchains" RPC command to monitor blockchain forks. #4536
Conversation
|
Of course, the name of the RPC call is up for discussion - maybe "getchaintips" is better, or "getblockchainheads" or something like that. For now, I just used the original name. This command has been included (and tested) so far in Huntercoin, Namecoin and Motocoin. |
|
"getchaintips" seems more intuitive to me; I've wanted this feature before myself. |
|
Just iterate over the mapBlockIndex entries, removing the ones that are the pprev of another mapBlockIndex entry. |
|
ACK concept + @sipa suggestion |
| if (fHelp || params.size () != 0) | ||
| throw runtime_error( | ||
| "getchains\n" | ||
| "Return status of all known chains.\n" |
There was a problem hiding this comment.
"Returns status" is not very helpful. More detail on what information is returned, please.
There was a problem hiding this comment.
This was just meant as a "place-holder" for now. I will extend the help text, also including example output (similar to other help texts).
|
Thanks for the input - I'll try to incorporate everything and update the pull request. I wanted to know if there's interest in this feature at all first, and I'm glad that there is. :) |
|
Yes, no problem with such an RPC. |
| break; | ||
| } | ||
|
|
||
| obj.push_back (Pair ("branch_len", len)); |
There was a problem hiding this comment.
branch_len should be branchlength to be consistent with other RPC results (e.g. see getblockchaininfo or getblocktemplate).
|
Also: a regression test that exercises this new RPC call would be spiffy (see qa/rpc-tests/ for examples). |
|
I've now updated the code and help text. What do you think about it? @sipa, is this the algorithm you had in mind? I'll also build a new unit test for the call, possibly tomorrow. |
|
@TheBlueMatt: Not sure why the test failed - the log shows no conclusive error message (but I haven't got any experience with your script so far), and it builds perfectly fine for me on GNU/Linux (after a |
|
Currently pulltester is broken. |
|
Basic regtest added. Since the test framework chain doesn't contain orphans, it only checks that the "isbest:true" result is as expected. Ok? |
|
|
||
| /* Lock everything. Not sure if this is needed for the whole duration | ||
| of the call, but better be safe than sorry. */ | ||
| LOCK(cs_main); |
There was a problem hiding this comment.
As the command is not marked as threadSafe in the RPC command table, acquiring any locks is redundant.
There was a problem hiding this comment.
I see. So just removing the LOCK in the call is fine?
There was a problem hiding this comment.
Yes. The calling code acquires the cs_main lock and cs_wallet locks if that bit is not set.
|
Useful information, ACK on concept. |
| { | ||
| std::map<uint256, CBlockIndex*>::const_iterator ia, ib; | ||
|
|
||
| ia = mapBlockIndex.find (a); |
There was a problem hiding this comment.
Coding style nit: no space before ()
There was a problem hiding this comment.
I see. I'm used to GNU projects and tried to adapt my style to the surrounding code, but it seemed not to have worked 100%. I can, of course, change this if it matters. Should I?
|
I've now updated the whitespace and replaced the "branch" output in the JSON result with "branchlen", according to the last feedback. Should I provide a quashed commit, or is the series of commits also fine? |
|
A series of commits is fine if the commits are distinct changes, or a logical sequence of changes. In this case I'd suggest squashing as all of the changes apply to the same code. |
|
Squashed commit of the last version. |
|
I think the implementation can be made more efficient by using sets/maps of CBlockIndex* entries rather than uint256's. (it means you don't need a find in the comparator, and the data structures are 4 or 8 times smaller). |
| } | ||
| while (!chainActive.Contains(pcur)); | ||
| } | ||
| obj.push_back(Pair("branchlen", branchLen)); |
There was a problem hiding this comment.
You can use pcur->nHeight - chainActive.FindFork(pcur)->nHeight; instead of this loop (which uses the recently added CBlockIndex* skiplist).
|
Indeed, very good observations! I've updated the patch now. Let me know if it is ok like this, then I will again provide a squashed commit instead. |
| setTips.insert(i->second); | ||
| for (i = mapBlockIndex.begin(); i != mapBlockIndex.end(); ++i) | ||
| { | ||
| const CBlockIndex* pprev = i->second->pprev; |
There was a problem hiding this comment.
No need for the find/erase. Just setTips.erase(pprev).
|
Tested ACK. Works nicely. |
|
Please squash you commits, though. |
|
Another tested ACK (after squashing commits) |
Port over chronokings/huntercoin#19 from Huntercoin: This implements a new RPC command "getchaintips" that can be used to find all currently active chain heads. This is similar to the -printblocktree startup option, but it can be used without restarting just via the RPC interface on a running daemon.
|
Thanks for the review - I've updated the branch to a single squashed commit. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4536_b33bd7a3be1cbcc8d255178307976b7762125b18/ for binaries and test log. |
b33bd7a Implement "getchaintips" RPC command to monitor blockchain forks. (Daniel Kraft)
Port over chronokings/huntercoin#19 from
Huntercoin: This implements a new RPC command "getchains" that can be
used to find all currently active chain heads. This is similar to the
-printblocktree startup option, but it can be used without restarting
just via the RPC interface on a running daemon.