Improve handling of unconnecting headers #8305

1. 
   sdaftuar commented at 6:02 PM on July 5, 2016: member

   I found a situation on testnet where a peer could send a header that wouldn't be accepted locally, because of the 2-hour-in-the-future rule. This would then cause BIP-130 headers sync to fail, because the sender doesn't realize that the recipient didn't accept the header, and so continues to send headers (one-by-one) that build on top of the rejected header, until eventually the sender is disconnected.

   This PR tries to improve upon the situation: if the first header in a received headers message doesn't connect, try once to request connecting headers.

   If we get two headers messages in a row that don't connect, give up on trying to request connecting headers (to avoid infinite looping with the remote peer), until our peer delivers a headers message that does connect.

   If this approach is acceptable, I think we should backport to 0.12 (looks like it doesn't merge cleanly, but I can open a new PR for the backport).

   cc: @TheBlueMatt @sipa

2. laanwj added the label P2P on Jul 6, 2016
3. 
   laanwj commented at 5:44 AM on July 6, 2016: member

   Needs rebase after #8306

4. sdaftuar force-pushed on Jul 6, 2016
5. 
   sdaftuar commented at 1:00 PM on July 6, 2016: member

   Rebased.

6. in src/main.cpp:None in ecfaf957f0 outdated

   5784 | +            // fLastHeadersConnected is set to true (below) whenever we succeed
   5785 | +            // in processing headers from a peer.
   5786 | +            if (nodestate->fLastHeadersConnected) {
   5787 | +                nodestate->fLastHeadersConnected = false;
   5788 | +                pfrom->PushMessage(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256());
   5789 | +                return true;
   

   ---

   ---

   

   mrbandrews commented at 8:07 PM on July 6, 2016:

   Print a log message here?

7. 
   pstratem commented at 10:55 PM on July 6, 2016: contributor

   @sdaftuar can you split this into two commits, one with changes and a second with tests?

8. sdaftuar force-pushed on Jul 7, 2016
9. 
   sdaftuar commented at 1:17 AM on July 7, 2016: member

   Added logging, and split into two commits.

10. 
    pstratem commented at 1:33 AM on July 7, 2016: contributor

    @sdaftuar thank you

11. jonasschnelli added this to the milestone 0.13.0 on Jul 7, 2016
12. 
    mrbandrews commented at 3:32 PM on July 11, 2016: contributor

    ACK

13. 
    sipa commented at 5:57 PM on July 11, 2016: member

    I've tried to reason through what would happen if two subsequent blocks happen with a very small time between them, before the peer has time to respond to the getheaders sent as response to the first block, but couldn't find any problem. I believe that convergence would only fail in case every future pair of blocks happens with a sufficiently short time between them.

14. 
    sipa commented at 5:57 PM on July 11, 2016: member

    ACK

15. 
    theuni commented at 11:40 PM on July 11, 2016: member

    Is it necessary to recover from this more than once per remote-node? As-is, it looks like someone could just respond with alternating connecting/non-connecting headers forever since a ban score isn't set. I can't imagine any motivation for doing so, though...

    Also, I don't think this should be allowed during IBD? Seems the approach here could be unified with the one when a non-connecting CMPCTBLOCK is received.

16. Improve handling of unconnecting headers

    When processing a headers message that looks like a block announcement,
    send peer a getheaders if the headers message won't connect.
    
    Apply DoS points after too many consecutive unconnecting headers messages.

    96fa95361f
17. Add test for handling of unconnecting headers e91cf4b210
18. 
    sdaftuar commented at 5:34 PM on July 12, 2016: member

    Discussed with @gmaxwell on IRC yesterday. He pointed out one drawback to this approach is that if we were to receive 2 blocks in short succession, such that the first block was still not valid according to the 2hr rule when the second one was received, then we would still fall out of sync.

    So I'm going to push an alternate approach, where instead of just trying once to recover, we do the following: when receiving a HEADERS message with at most a few headers, if the first header doesn't connect, increment a counter and send the peer a getheaders. If the counter gets too big, assign DoS points, but if a connecting header comes in before that happens, reset the counter to 0. @theuni Since DoS points don't ever decay back to 0, I was reluctant to assign DoS points at all when this type of thing happens, because otherwise if you were up and running long enough, you'd eventually disconnect your peers as these situations accumulated points over time.

    One difference between this and the CMPCTBLOCK handling is that we have to worry about potential infinite looping if our peer is broken, since we're sending a getheaders in response to a headers message.

19. sdaftuar force-pushed on Jul 12, 2016
20. 
    sdaftuar commented at 5:39 PM on July 12, 2016: member

    Updated with new approach. @gmaxwell How does this look?

21. 
    gmaxwell commented at 8:32 AM on July 13, 2016: contributor

    This looks reasonable to me. I am testing it.

22. 
    sipa commented at 7:06 PM on July 14, 2016: member

    re-utACk

23. 
    gmaxwell commented at 7:08 PM on July 14, 2016: contributor

    ACK

24. 
    laanwj commented at 5:46 AM on July 18, 2016: member

    utACK e91cf4b

25. laanwj merged this on Jul 18, 2016
26. laanwj closed this on Jul 18, 2016

    ---

27. laanwj referenced this in commit 37303934fe on Jul 18, 2016
28. in src/main.cpp:None in e91cf4b210

    5775 | @@ -5773,6 +5776,35 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
    5776 |              return true;
    5777 |          }
    5778 |  
    5779 | +        CNodeState *nodestate = State(pfrom->GetId());
    

    ---

    ---

    

    sipa commented at 10:08 AM on August 25, 2016:

    Can you please stop ask basic programming questions? You can learn these things online, or on various other forums (including stackoverflow or chat channels) that don't require the developers of the project to waste their time teaching you.

    You're free to ask for rationale behind design decisions, but this is getting unreasonable.

    One last time: it's generally good style to use accessors instead of field variables directly. No, we don't do that everywhere yet. No, that doesn't mean we should continue that practice. No, that also doesn't mean we'll always complain when someone access the field directly.

29. rebroad referenced this in commit 3d32c51a5c on Dec 7, 2016
30. codablock referenced this in commit 7f13010d0b on Sep 19, 2017
31. codablock referenced this in commit 221462beb8 on Dec 27, 2017
32. codablock referenced this in commit bc257c1a08 on Dec 28, 2017
33. andvgal referenced this in commit c8d89ebf8b on Jan 6, 2019
34. DrahtBot locked this on Sep 8, 2021

Milestone
0.13.0