Use syncheight instead of startheight for broadcasting block invs

rebroad commented at 1:59 am on September 5, 2014: contributor

No need to use the outdated height when we have a more up to date height we can use.

Use syncheight instead of startheight for broadcasting block invs 582d7e8294

in src/main.cpp: in 582d7e8294

2051@@ -2052,8 +2052,9 @@ bool ActivateBestChain(CValidationState &state, CBlock *pblock) {
2052             int nBlockEstimate = Checkpoints::GetTotalBlocksEstimate();
2053             {
2054             LOCK(cs_vNodes);
2055-            BOOST_FOREACH(CNode* pnode, vNodes)
2056-                if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate))

rebroad commented at 2:02 am on September 5, 2014:

I contentiously removed the “- 2000”, which I suspect will get objections/NAKs. In fact, there’s probably a better way to determine whether to broadcast an inv than using the height number - i.e. broadcast if their height is lower OR if their latest block is not in our best chain. (Although actually, this is probably good to go, as we could broadcast our latest block inv the moment we realise their latest block is not in our best chain - if I add that code also, then this pull is probably then ready to merge).

(This pull is dependent on aa815647005bc8467f467c35a9e617794446cd64)

BitcoinPullTester commented at 2:03 am on September 5, 2014: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4846_582d7e8294926136389bf2cb65fade180f85ba0e/ for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

gmaxwell commented at 4:36 pm on September 5, 2014: contributor

We don’t always have a pindexBestKnownBlock— pretty much never immediately after startup, only when we’ve seen an inv for a block in common.

IIRC the substantial grace span is require to prevent gratuitous convergence delays/failures when there is a better chain with fewer blocks and was an intentional element of the design. The recognition heuristic sounds fine but doesn’t cover the case where we haven’t seen a block inv from them at all.

rebroad commented at 11:29 am on September 15, 2014: contributor

@gmaxwell in the case where we haven’t seen a block inv from them, it resorts to using nStartingHeight (provided in the version message).

laanwj commented at 10:36 am on September 22, 2014: member

@sipa can you please take a look at this change?

sipa commented at 1:03 am on September 27, 2014: member

I’m very wary about touching this code before headersfirst is rolled out; not because the change itself is bad, but because it may interact in unexpected ways with the current broken fetching logic on the other side.

Specifically, if we made this code perfect and only announced new blocks to peers that were already caught up, we risk having peers that are stuck (for the various existing reasons) not getting unstuck anymore, because they don’t learn about new invs at all anymore.

Post-headersfirst this makes sense given some safety margins.

rebroad commented at 3:18 pm on September 28, 2014: contributor

I agree with @sipa ’s comment above. headersfirst8 is a prerequisite for this pull. Should I close it until headersfirst8 is merged?

laanwj commented at 8:52 am on October 7, 2014: member

Yes, let’s close this until headers-first is merged. That has priority now.

sipa commented at 6:36 pm on October 7, 2014: member

Not just until it is merged. Until it is released and deployed. The reason for keeping the code as-is is because it may affect old peers.

sipa closed this on Oct 7, 2014

MarcoFalke locked this on Sep 8, 2021

Use syncheight instead of startheight for broadcasting block invs #4846