Advertize all new block hashes in a reorganization

sipa commented at 9:18 pm on November 18, 2014: member

Currently we only inv the hash of the new tip, which means that several round-trips are needed to first discover the hashes of all blocks in between. We can just announce all new hashes.

Just a minor improvement, not intended for 0.10.

sipa force-pushed on Nov 18, 2014

sipa force-pushed on Nov 25, 2014

laanwj added the label P2P on Nov 28, 2014

in src/main.cpp: in 3ed7f8e070 outdated

2085@@ -2086,14 +2086,16 @@ static bool ActivateBestChainStep(CValidationState &state, CBlockIndex *pindexMo
2086 // or an activated best chain. pblock is either NULL or a pointer to a block
2087 // that is already loaded (to avoid loading it again from disk).
2088 bool ActivateBestChain(CValidationState &state, CBlock *pblock) {
2089-    CBlockIndex *pindexNewTip = NULL;
2090     CBlockIndex *pindexMostWork = NULL;
2091     do {
2092         boost::this_thread::interruption_point();
2093 
2094+        CBlockIndex *pindexNewTip;

laanwj commented at 7:27 am on March 26, 2015:

In the moved line, this variable was initialized to NULL, but no more. Maybe better to do so just in case so that potential bugs result in more predictable outcomes (same for pindexFork on the line below).

sipa commented at 5:40 pm on April 8, 2015:

I’m not sure I agree with this as best practice (initializing variables may hide bugs too that uninitialized memory and eg valgrind could show), but: done.

sipa force-pushed on Apr 8, 2015

sipa commented at 5:40 pm on April 8, 2015: member

Updated.

sdaftuar commented at 11:26 pm on April 10, 2015: member

Did some basic testing, and looks good to me.

sipa commented at 10:02 am on April 11, 2015: member

Mental note: perhaps this should limit itself to 8-16 new entries or so. If it’s more, rely on peer’s incremental synchronization to catch up (inspired by a comment by @sdaftuar in #5982).

EDIT: implemented

Advertize all new block hashes in a reorganization 90eda4d376

sipa force-pushed on Apr 12, 2015

sdaftuar commented at 5:01 pm on April 16, 2015: member

I guess one side effect of this is that when announcing a reorg to an 0.10 peer, this increases the overhead. Say there are N blocks announced in the inv generated here. Then an 0.10 peer will generate N getheaders messages (with the same locator in each, but with a different hashstop), and the 0.10 peer will therefore receive N headers responses, with {0,1,…,(N-1)} headers in each message.

The total overhead is still not that large in absolute terms I think, but with N^2 behavior here, maybe this argues for the cap being more like 4 or 6, rather than 16?

Also I think in the case that we are truncating the inv’s, perhaps it would be better to revert to only inv’ing the tip, rather than announcing up to our cap (whatever we pick for that value), since 0.10 nodes would otherwise be requesting the same long chain of headers multiple times.

sipa commented at 3:21 pm on April 17, 2015: member

@sdaftuar Hmm, maybe that’s a reason to only send all hashes when using ‘headers’ announcements, not ‘inv’ announcement (#5982)?

sdaftuar commented at 6:49 pm on April 29, 2015: member

@sipa Yes I think that’s reasonable, and that’s the approach I’m trying for #5982 – when inv-ing, only announce the tip; but when announcing headers, be willing to announce everything back to the last fork point (with some limits, as discussed in that issue).

jgarzik commented at 6:40 pm on July 23, 2015: contributor

concept and light review ACK.

Main comment: If many nodes on the network are re-org’ing at once, doesn’t this cause traffic to spike?

I’ll see if I can queue up some worst case local testing in a week or so, if nobody beats me to it.

sipa commented at 4:40 pm on September 22, 2015: member

Superseded by #6494.

sipa closed this on Sep 22, 2015

MarcoFalke locked this on Sep 8, 2021

Advertize all new block hashes in a reorganization #5307