p2p: Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started #26355

1. 
   dergoegge commented at 5:55 pm on October 20, 2022: member

   This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don’t lead to a chain that surpasses our DoS limit.

   The issue is that we ignore the return value on the first IsContinuationOfLowWorkHeadersSync call after a new headers sync is started, which leads to us passing headers to ProcessNewBlockHeaders when that initial IsContinuationOfLowWorkHeadersSync call returns false. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different nBits value than the prior headers (which fails the pre-sync logic here). Those 2000 headers will be passed to ProcessNewBlockHeaders.

   I haven’t included a test here so far because we can’t test this without changing the default value for CRegTestParams::consensus.fPowAllowMinDifficultyBlocks or doing some more involved refactoring.

2. glozow added the label P2P on Oct 20, 2022
3. glozow added this to the milestone 24.0 on Oct 20, 2022
4. glozow requested review from sdaftuar on Oct 20, 2022
5. glozow requested review from sipa on Oct 20, 2022
6. in src/net_processing.cpp:2575 in 91b0fdd04f outdated

   2571+                // headers sync.
   2572+                headers = {};
   2573+                peer.m_headers_sync.reset(nullptr);
   2574+            }
   2575+
   2576+            return true;
   

   ---

   ---

   

   sipa commented at 8:26 pm on October 20, 2022:

   Perhaps it’s useful to move this return true; outside the enclosing if/else. I think it’s easy to see that this branch always needs a return true;: we haven’t met the work threshold, so we certainly shouldn’t release the headers for further processing.

   ---

   

   dergoegge commented at 10:29 am on October 21, 2022:

   Good idea, done!
7. maflcko added the label Needs backport (24.x) on Oct 21, 2022
8. [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started 7ad15d1100
9. dergoegge force-pushed on Oct 21, 2022
10. 
    sipa commented at 3:37 pm on October 21, 2022: member

    ACK 7ad15d11005eac36421398530da127333d87ea80
11. sipa approved
12. 
    glozow commented at 2:36 pm on October 24, 2022: member

    ACK 7ad15d1100
13. glozow merged this on Oct 24, 2022
14. glozow closed this on Oct 24, 2022

    ---

15. 
    glozow commented at 2:45 pm on October 24, 2022: member

    Backport in #26382
16. 
    mzumsande commented at 3:45 pm on October 24, 2022: contributor

    Post-Merge ACK 7ad15d11005eac36421398530da127333d87ea80

    Good catch!

17. glozow removed the label Needs backport (24.x) on Oct 24, 2022
18. in src/net_processing.cpp:2581 in 7ad15d1100

    2580         }
    2581+
    2582+        // The peer has not yet given us a chain that meets our work threshold,
    2583+        // so we want to prevent further processing of the headers in any case.
    2584+        headers = {};
    2585+        return true;
    

    ---

    ---

    

    mzumsande commented at 5:12 pm on October 24, 2022:

    nit: The doc of TryLowWorkHeadersSync:

    0     * [@return](/bitcoin-bitcoin/contributor/return/)      True if chain was low work and a headers sync was
    1     *              initiated (and headers will be empty after calling); false
    2     *              otherwise.
    

    got out of date - the function now always returns true and empties the headers if the chain is low work, irrespective of whether headers sync was initiated or not.

    ---

    

    dergoegge commented at 10:41 am on October 25, 2022:

    Fixed in #26387
19. sidhujag referenced this in commit edfabe8fd7 on Oct 25, 2022
20. in src/net_processing.cpp:2573 in 7ad15d1100

    2569+            if (!IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers)) {
    2570+                // Something went wrong, reset the headers sync.
    2571+                peer.m_headers_sync.reset(nullptr);
    2572+                LOCK(m_headers_presync_mutex);
    2573+                m_headers_presync_stats.erase(peer.m_id);
    2574+            }
    

    ---

    ---

    

    sdaftuar commented at 2:19 pm on October 25, 2022:

    It looks like we don’t need this block of code that clears out peer.m_headers_sync after all, because this is handled in IsContinuationOfLowWorkHeadersSync. So this could just be:

    0(void) IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers);
    

    with a comment explaining that failures are handled inside that function, and that we know there is nothing more to be done even if the function succeeds.

    ---

    

    dergoegge commented at 10:22 am on October 26, 2022:

    Done in the follow up PR #26387.
21. 
    sdaftuar commented at 2:21 pm on October 25, 2022: member

    Great catch, thanks again for fixing this. I think it should be possible to write a functional test to exercise the code path that was broken before, so I’ll give that a try.

    I think the code merged is correct but I think it could be cleaned up a little bit, left one comment.

22. glozow referenced this in commit d22cc74837 on Oct 27, 2022
23. fanquake referenced this in commit 43e813cab2 on Oct 31, 2022
24. sidhujag referenced this in commit 57f8033b7c on Oct 31, 2022
25. bitcoin locked this on Oct 26, 2023

Contributors
dergoegge sipa glozow mzumsande sdaftuar

Labels
P2P

Milestone
24.0