Implement "getchains" RPC command to monitor blockchain forks. #4536

domob1812 commented at 3:51 PM on July 15, 2014: contributor

Port over https://github.com/chronokings/huntercoin/pull/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.

domob1812 commented at 3:52 PM on July 15, 2014: contributor

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.

petertodd commented at 4:38 PM on July 15, 2014: contributor

"getchaintips" seems more intuitive to me; I've wanted this feature before myself.

sipa commented at 5:20 PM on July 15, 2014: member

Just iterate over the mapBlockIndex entries, removing the ones that are the pprev of another mapBlockIndex entry.

jgarzik commented at 5:28 PM on July 15, 2014: contributor

ACK concept + @sipa suggestion

in src/rpcblockchain.cpp:None in 0d12743c48 outdated

 478 | +Value getchains (const Array& params, bool fHelp)
 479 | +{
 480 | +    if (fHelp || params.size () != 0)
 481 | +        throw runtime_error(
 482 | +            "getchains\n"
 483 | +            "Return status of all known chains.\n"

gavinandresen commented at 5:34 PM on July 15, 2014:

"Returns status" is not very helpful. More detail on what information is returned, please.

domob1812 commented at 5:37 PM on July 15, 2014:

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).

domob1812 commented at 5:38 PM on July 15, 2014: contributor

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. :)

sipa commented at 5:39 PM on July 15, 2014: member

Yes, no problem with such an RPC.

in src/rpcblockchain.cpp:None in 0d12743c48 outdated

 561 | +                /* We are on the main chain if there's a next pointer.  */
 562 | +                if (chainActive.Contains (pcur))
 563 | +                    break;
 564 | +            }
 565 | +
 566 | +            obj.push_back (Pair ("branch_len", len));

gavinandresen commented at 5:41 PM on July 15, 2014:

branch_len should be branchlength to be consistent with other RPC results (e.g. see getblockchaininfo or getblocktemplate).

in src/rpcblockchain.cpp:None in 0d12743c48 outdated

 565 | +
 566 | +            obj.push_back (Pair ("branch_len", len));
 567 | +            obj.push_back (Pair ("branch", branch));
 568 | +        }
 569 | +        else
 570 | +            obj.push_back (Pair ("branch_len", 0));

gavinandresen commented at 5:43 PM on July 15, 2014:

Convention for RPC results is to not sometimes return a field and sometimes not.

So returning an empty branch Array here would be better.

Is branch_len always the length of the branch array? If yes, then it is redundant and should be removed.

domob1812 commented at 5:47 PM on July 15, 2014:

Ok. Yes, it is always the length - I included it originally for convenience when reading the data manually in a terminal or something. But I can remove it, as it is indeed redundant. I realise there are in general a lot of rough edges in the patch, I'll update it tomorrow to a hopefully cleaner one.

gavinandresen commented at 5:46 PM on July 15, 2014: contributor

Also: a regression test that exercises this new RPC call would be spiffy (see qa/rpc-tests/ for examples).

domob1812 commented at 7:46 PM on July 16, 2014: contributor

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.

domob1812 commented at 5:34 AM on July 17, 2014: contributor

@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 make distclean). Also make check works fine. Any hints?

jgarzik commented at 6:04 AM on July 17, 2014: contributor

Currently pulltester is broken.

domob1812 commented at 8:46 AM on July 18, 2014: contributor

Basic regtest added. Since the test framework chain doesn't contain orphans, it only checks that the "isbest:true" result is as expected. Ok?

in src/rpcblockchain.cpp:None in dc290df1cb outdated

 504 | +            + HelpExampleRpc ("getchaintips", "")
 505 | +        );
 506 | +
 507 | +    /* Lock everything.  Not sure if this is needed for the whole duration
 508 | +       of the call, but better be safe than sorry.  */
 509 | +    LOCK(cs_main);

laanwj commented at 9:10 AM on July 18, 2014:

As the command is not marked as threadSafe in the RPC command table, acquiring any locks is redundant.

domob1812 commented at 9:18 AM on July 18, 2014:

I see. So just removing the LOCK in the call is fine?

laanwj commented at 9:33 AM on July 18, 2014:

Yes. The calling code acquires the cs_main lock and cs_wallet locks if that bit is not set.

in src/rpcblockchain.cpp:None in dc290df1cb outdated

 535 | +    Array res;
 536 | +    for (std::vector<uint256>::const_iterator j = vTips.begin ();
 537 | +         j != vTips.end (); ++j)
 538 | +    {
 539 | +        i = mapBlockIndex.find (*j);
 540 | +        assert (i != mapBlockIndex.end ());

laanwj commented at 9:14 AM on July 18, 2014:

I'd prefer throwing a JSONRPCError when unexpected things happen in this function instead of asserting.

domob1812 commented at 9:19 AM on July 18, 2014:

This assert should never really happen, and it shouldn't be possible to trigger it by invalid user input or something like that. For me, this is precisely a situation where one wants to assert(). But if it is more in line with Bitcoin Core philosophy to throw a JSONRPCError, I can, of course, do that.

laanwj commented at 9:37 AM on July 18, 2014:

Well - fine with me to keep it as assert in that case.

sipa commented at 8:06 PM on July 18, 2014:

Agree on using an assert.

Diapolo commented at 1:16 PM on July 21, 2014:

If it should not happen in any way, why is it there ;)?

laanwj commented at 9:15 AM on July 18, 2014: member

Useful information, ACK on concept.

in src/rpcblockchain.cpp:None in fa69eaa41b outdated

 465 | +/* Comparison function for sorting the getchaintips heads.  */
 466 | +static bool compareBlocksByHeight (const uint256& a, const uint256& b)
 467 | +{
 468 | +    std::map<uint256, CBlockIndex*>::const_iterator ia, ib;
 469 | +
 470 | +    ia = mapBlockIndex.find (a);

sipa commented at 8:04 PM on July 18, 2014:

Coding style nit: no space before ()

domob1812 commented at 11:36 AM on July 19, 2014:

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?

in src/rpcblockchain.cpp:None in fa69eaa41b outdated

 493 | +            "  {\n"
 494 | +            "    \"height\": xxxx,\n"
 495 | +            "    \"hash\": \"xxxx\",\n"
 496 | +            "    \"isbest\": false,\n"
 497 | +            "    \"branch\": [             (array) blocks connecting the tip to the main chain\n"
 498 | +            "      \"xxxx\"                (string) hashes of the connecting blocks\n"

sipa commented at 8:08 PM on July 18, 2014:

This wasn't clear to me from the documentation. How about just include the length of the branch (how many blocks are not in the main chain)? You can always find the actual chain through getblock -> prev.

domob1812 commented at 11:37 AM on July 19, 2014:

That's true, but my original motivation was to include the information already here because it makes life easier. If you want the branch, you don't have to use repeated "getblock" calls. It doesn't hurt much to have that here instead of the length, IMHO. But if you prefer the length, I will change it of course.

domob1812 commented at 9:54 AM on July 21, 2014: contributor

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?

laanwj commented at 9:56 AM on July 21, 2014: member

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.

domob1812 commented at 11:02 AM on July 21, 2014: contributor

Squashed commit of the last version.

sipa commented at 2:51 PM on July 27, 2014: member

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).

in src/rpcblockchain.cpp:None in 3e1af92c1b outdated

 555 | +                pcur = pcur->pprev;
 556 | +                ++branchLen;
 557 | +            }
 558 | +            while (!chainActive.Contains(pcur));
 559 | +        }
 560 | +        obj.push_back(Pair("branchlen", branchLen));

sipa commented at 2:53 PM on July 27, 2014:

You can use pcur->nHeight - chainActive.FindFork(pcur)->nHeight; instead of this loop (which uses the recently added CBlockIndex* skiplist).

domob1812 commented at 5:05 PM on July 29, 2014: contributor

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.

in src/rpcblockchain.cpp:None in b92da981fe outdated

 502 | +    std::map<uint256, CBlockIndex*>::const_iterator i;
 503 | +    for (i = mapBlockIndex.begin(); i != mapBlockIndex.end(); ++i)
 504 | +        setTips.insert(i->second);
 505 | +    for (i = mapBlockIndex.begin(); i != mapBlockIndex.end(); ++i)
 506 | +    {
 507 | +        const CBlockIndex* pprev = i->second->pprev;

sipa commented at 11:54 PM on July 29, 2014:

No need for the find/erase. Just setTips.erase(pprev).

in src/rpcblockchain.cpp:None in b92da981fe outdated

 460 | @@ -461,3 +461,81 @@ Value getblockchaininfo(const Array& params, bool fHelp)
 461 |      obj.push_back(Pair("chainwork",             chainActive.Tip()->nChainWork.GetHex()));
 462 |      return obj;
 463 |  }
 464 | +
 465 | +/* Comparison function for sorting the getchaintips heads.  */
 466 | +static bool compareBlocksByHeight(const CBlockIndex* a, const CBlockIndex* b)

sipa commented at 11:55 PM on July 29, 2014:

Does this work? This will compare two blocks equal if they're of equal height. I'm not sure how std::sort behaves when the comparator and the equality operator are inconsistent.

domob1812 commented at 4:56 AM on July 30, 2014:

I think that the STL doesn't use the '==' at all in sort, it considers elements to be the same if neither "a < b" nor "b < a". But I can't guarantee that it keeps two "equal" elements by themselves, rather than copying one of them two times - I believe not, but I'll change the code to include ordering of the pointers as a secondary criterion to be safe.

in src/rpcblockchain.cpp:None in b92da981fe outdated

 512 | +                setTips.erase(j);
 513 | +        }
 514 | +    }
 515 | +
 516 | +    /* Get chain heads and sort them by height.  */
 517 | +    std::vector<const CBlockIndex*> vTips(setTips.begin(), setTips.end());

sipa commented at 11:57 PM on July 29, 2014:

If you make the comparator support multiple blocks at the same height, you could just use that comparator directly as internal sort for setTips, so you don't need the vector or the explicity sort, and can just iterate through setTips (use BOOST_FOREACH, btw).

domob1812 commented at 4:58 AM on July 30, 2014:

Yes, that is a good idea!

I don't really like BOOST_FOREACH personally, but I guess this is how the existing code does it - so I'll change this, too.

in src/rpcblockchain.cpp:None in b92da981fe outdated

 527 | +        Object obj;
 528 | +        obj.push_back(Pair("height", block.nHeight));
 529 | +        obj.push_back(Pair("hash", block.phashBlock->GetHex()));
 530 | +
 531 | +        const bool isMain = chainActive.Contains(&block);
 532 | +        obj.push_back(Pair("isbest", isMain));

sipa commented at 12:02 AM on July 30, 2014:

The name 'isbest' may be obvious now, but if #4468 would get merged, we may end up with multiple header tips that all descend from the current chainActive (while we're catching up).

I'm not sure how to deal with this, but something branchlen essentially conveys this information already (maybe a better name is 'reorg_distance' or 'fork_distance', or replace it with 'fork_height'?) - if it's 0, it extends our main branch.

EDIT: ah, not really; fork_distance would be chainActive.Height() - chainActive.FindFork(&block)->nHeight. 'isbest' would be equal to fork_distance == 0 then.

domob1812 commented at 5:01 AM on July 30, 2014:

Yes, branchlen==0 is equivalent to isbest. So if you prefer, we can drop the "isbest" field and just have "branchlen". The name was suggested by Gavin, IIRC, but I can change it to "fork_distance" if that is clearer?

laanwj added the label RPC on Jul 31, 2014

sipa commented at 10:40 AM on August 3, 2014: member

Tested ACK. Works nicely.

sipa commented at 10:40 AM on August 3, 2014: member

Please squash you commits, though.

laanwj commented at 12:09 PM on August 3, 2014: member

Another tested ACK (after squashing commits)

Implement "getchaintips" RPC command to monitor blockchain forks.

Port over https://github.com/chronokings/huntercoin/pull/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.

b33bd7a3be

domob1812 commented at 4:14 PM on August 3, 2014: contributor

Thanks for the review - I've updated the branch to a single squashed commit.

BitcoinPullTester commented at 4:27 PM on August 3, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4536_b33bd7a3be1cbcc8d255178307976b7762125b18/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa merged this on Aug 3, 2014

sipa closed this on Aug 3, 2014

sipa referenced this in commit 1d2a1d4bbc on Aug 3, 2014

domob1812 deleted the branch on Aug 4, 2014

MarcoFalke locked this on Sep 8, 2021