Do not send (potentially) invalid headers in response to getheaders #11580

1. 
   TheBlueMatt commented at 2:45 pm on October 30, 2017: member

   Nowhere else in the protocol do we send headers which are for blocks we have not fully validated except in response to getheaders messages with a null locator. On my public node I have not seen any such request (whether for an invalid block or not) in at least two years of debug.log output, indicating that this should have minimal impact.
2. fanquake added the label P2P on Oct 30, 2017
3. in src/net_processing.cpp:1985 in a2f9b47b2f outdated

   1981@@ -1986,8 +1982,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
   1982                 return true;
   1983             pindex = (*mi).second;
   1984 
   1985-            if (!chainActive.Contains(pindex) &&
   1986-                !StaleBlockRequestAllowed(pindex, chainparams.GetConsensus())) {
   1987+            if (!StaleBlockRequestAllowed(pindex, chainparams.GetConsensus())) {
   

   ---

   ---

   

   promag commented at 2:52 pm on October 30, 2017:

   So it was missing IsValid(BLOCK_VALID_SCRIPTS) here?

   ---

   

   TheBlueMatt commented at 2:53 pm on October 30, 2017:

   “Missing” is a strong word given there isn’t exactly a spec for the P2P protocol, but, yes, I’d say it should be there.

   ---

   

   promag commented at 2:55 pm on October 30, 2017:

   Should be tagged backport then?

   ---

   

   TheBlueMatt commented at 11:00 pm on October 30, 2017:

   Meh, dont think it matters too much.
4. in src/net_processing.cpp:763 in a2f9b47b2f outdated

   758@@ -759,7 +759,8 @@ void Misbehaving(NodeId pnode, int howmuch)
   759 static bool StaleBlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams)
   760 {
   761     AssertLockHeld(cs_main);
   762-    return (pindexBestHeader != nullptr) &&
   763+    if (chainActive.Contains(pindex)) return true;
   

   ---

   ---

   

   jimpo commented at 2:58 pm on October 30, 2017:

   Now that this function is expected to accept main chain blocks, maybe change the function name to BlockRequestAllowed?
5. in src/net_processing.cpp:764 in a2f9b47b2f outdated

   758@@ -759,7 +759,8 @@ void Misbehaving(NodeId pnode, int howmuch)
   759 static bool StaleBlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams)
   760 {
   761     AssertLockHeld(cs_main);
   762-    return (pindexBestHeader != nullptr) &&
   763+    if (chainActive.Contains(pindex)) return true;
   764+    return pindex->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != nullptr) &&
   

   ---

   ---

   

   jimpo commented at 2:58 pm on October 30, 2017:

   Update the function comment to describe this condition.
6. 
   jimpo commented at 2:59 pm on October 30, 2017: contributor

   Concept ACK
7. Do not send (potentially) invalid headers in response to getheaders

   Nowhere else in the protocol do we send headers which are for
   blocks we have not fully validated except in response to getheaders
   messages with a null locator. On my public node I have not seen any
   such request (whether for an invalid block or not) in at least two
   years of debug.log output, indicating that this should have minimal
   impact.

   3788a8479b
8. TheBlueMatt force-pushed on Oct 30, 2017
9. TheBlueMatt force-pushed on Oct 30, 2017
10. 
    TheBlueMatt commented at 11:00 pm on October 30, 2017: member

    Addressed @jimpo’s comments.
11. 
    gmaxwell commented at 0:56 am on November 1, 2017: contributor

    utACK
12. 
    ryanofsky commented at 11:01 pm on November 1, 2017: member

    I wrote a test for this change which you could include here: e79a9c92bf0158b1a8676cffd0cab9ab928160c0.

    ACK 3788a8479b4efd481f3e91419bcf347113375112

13. [test] Verify node doesn't send headers that haven't been fully validated 725b79a9cf
14. 
    TheBlueMatt commented at 5:49 pm on November 2, 2017: member

    Thanks @ryanofsky!
15. 
    laanwj commented at 6:58 pm on November 9, 2017: member

    utACK 725b79a, thanks for adding the test @ryanofsky
16. laanwj merged this on Nov 9, 2017
17. laanwj closed this on Nov 9, 2017

    ---

18. laanwj referenced this in commit 1f4375f8e7 on Nov 9, 2017
19. laanwj referenced this in commit 37ffa16933 on Dec 11, 2017
20. codablock referenced this in commit 321b3de619 on Mar 12, 2019
21. codablock referenced this in commit 9344dee8aa on Mar 12, 2019
22. PastaPastaPasta referenced this in commit f4fb2c1906 on Jan 26, 2020
23. ckti referenced this in commit 9b18bc0bc0 on Mar 28, 2021
24. gades referenced this in commit 7c35c8d250 on Jun 30, 2021
25. DrahtBot locked this on Sep 8, 2021

Contributors
TheBlueMatt promag jimpo gmaxwell ryanofsky laanwj

Labels
P2P