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:

1. 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)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. 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.

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.

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

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

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.

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

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

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.