CBlockLocator improvements & move to core #3083

pull sipa wants to merge 2 commits into bitcoin:master from sipa:chainlocator changing 6 files +86 −144
  1. sipa commented at 1:27 pm on October 12, 2013: member

    This moves the chain-related logic in CBlockLocator to CChain (CBlockLocator is a P2P data structure, its implementation shouldn’t depend on validation engine globals), and then moves CBlockLocator itself from main.h to core.h.

    This means construction of CBlockLocator objects now happens in O(log nHeight) instead of O(nHeight), as it can use CChain’s height-based index.

    Depends on #3077.

  2. jgarzik commented at 2:33 am on October 15, 2013: contributor

    untested ACK.

    ACK, presuming getblocks and getheaders style downloads from older clients has been tested.

  3. Reimplement CBlockLocator's chain-related logic in CChain.
    This removes a few unused CBlockLocator methods, and moves the
    construction and fork-finding logic to CChain (which can do these
    more efficiently, as it has a height-indexable chain available).
    It also makes CBlockLocator independent from the validation code.
    e4daecda0b
  4. Move CBlockLocator to core.h
    As CBlockLocator is a P2P data structure, and independent from the
    validation logic, it can be moved to core.
    f9b15a4fc9
  5. in src/main.cpp: in fb9993b3d9 outdated
    199-    std::map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.find(hashBlock);
    200-    if (mi != mapBlockIndex.end())
    201-        Set((*mi).second);
    202+CBlockIndex *CChain::SetTip(CBlockIndex *pindex) {
    203+    if (pindex == NULL) {
    204+        std::vector<CBlockIndex*>().swap(vChain);
    


    gavinandresen commented at 3:56 am on October 15, 2013:
    Not a merge-stopping issue… but why not vChain.clear() ? I doubt SetTip(NULL) called often enough to justify the memory saved by the more obscure swap-with-an-empty-vector coding idiom.

    sipa commented at 8:41 pm on October 15, 2013:
    Fair enough. Fixed.
  6. in src/core.h: in fb9993b3d9 outdated
    660@@ -661,4 +661,38 @@ class CBlock : public CBlockHeader
    661     void print() const;
    662 };
    663 
    664+
    665+/** Describes a place in the block chain to another node such that if the
    666+ * other node doesn't have the same branch, it can find a recent common trunk.
    667+ * The further back it is, the further before the fork it may be.
    668+ */
    669+struct CBlockLocator
    


    gavinandresen commented at 4:02 am on October 15, 2013:

    clang warns (repeatedly):

    0./core.h:669:1: warning: 'CBlockLocator' defined as a struct here but previously declared as a class [-Wmismatched-tags]
    

    … because db.h contains ‘class CBlockLocator;’


    sipa commented at 8:41 pm on October 15, 2013:
    Fixed.
  7. BitcoinPullTester commented at 9:48 am on October 15, 2013: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f9b15a4fc94cdd4b535a2f7b1eccc04332367d00 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.
  8. sipa commented at 8:47 pm on October 15, 2013: member
    @jgarzik We should test that, indeed. @gmaxwell Feel like testing sync by/from a node with this patch? :p
  9. Diapolo commented at 8:51 pm on October 15, 2013: none
    Sometimes I miss the practical pros such pulls give, like 5% faster in IBD or something easily understandable. Can you give an insight if this is the case here :)?
  10. sipa commented at 9:25 pm on October 15, 2013: member

    There’s two improvements here.

    The first is just a cleanup. The CBlockLocator datastructure is part of the P2P protocol (so something very basic), but its methods directly refer to some of the validation engine’s globals. That’s really ugly, as a user of that class you won’t expect that using some of its methods imply you need to do locking on cs_main, or that it would depend on global data in the first place. In short, it doesn’t belong there - and now that we have a CChain data structure that explicitly encapsulates the block chain, it’s much more natural to ask the chain “give a locator that points to you”, rather than asking locator “make yourself into something that points to… well, you know”. The advantage is really just making the code easier to understand and reuse.

    The second is an actual performance improvement. Currently, when constructing a CBlockLocator, we iterate the entire block chain index in memory (just benchmarked that, takes ~20ms), just to find the few ones we need. Since we have CChain that allows looking up entries by height, this becomes trivial (i expect a few microseconds). 20ms may not seem much, but we do this any time we’re asking for blocks from a peer, and it’s really just silly not to use the height-index now that it exists.

    EDIT: It also reduces the number of lines of code!

  11. Diapolo commented at 9:37 pm on October 15, 2013: none
    ACK description wise, sounds like a very good idea! And thanks for taking the time to explain it to me :+1:.
  12. gavinandresen commented at 10:59 pm on October 15, 2013: contributor

    ACK.

    Code changes look good, and I tested following combinations against new code:

    old code, partial chain, main network old code, partial chain, test network old code, fresh sync, test network

  13. gavinandresen referenced this in commit 9e70bff67a on Oct 15, 2013
  14. gavinandresen merged this on Oct 15, 2013
  15. gavinandresen closed this on Oct 15, 2013

  16. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-25 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me