p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling #29664

1. 
   mzumsande commented at 10:27 pm on March 15, 2024: contributor

   Problem: For stalling at the tip, we have a parallel download mechanism for compact blocks that was added in #27626. For stalling during IBD, we have a lookahead window of 1024 blocks, and if that is exceeded, we disconnect the stalling peer. However, if we are close to but not at the tip (<=1024 blocks), neither of these mechanisms apply. We can’t do compact blocks yet, and the stalling mechanism doesn’t work because the 1024 window cannot be exceeded.

   As a result, we have to resort to BLOCK_DOWNLOAD_TIMEOUT_BASE which only disconnects a peer after 10 minutes (plus 5 minutes more for each additional peers we currently have blocks in flight). This is too long in my opinion, especially since peers get assigned up to 16 blocks (MAX_BLOCKS_IN_TRANSIT_PER_PEER) and could repeat this process to stall us even longer if they send us a block after 10 minutes. This issue was observed in #29281 and #12291 (comment) with broken peers that didn’t send us blocks.

   Proposed solution: If we are 1024 or less blocks away from the tip and haven’t requested or received a block from any peer for 30 seconds, add another peer to download the critical block from that would help us advance our tip. Add up to two additional peers this way.

   Other thoughts

   • I also considered the alternative of extending the existing stalling mechanism that disconnects instead of introducing parallel downloads. This could be potentially less wasteful, but we might be over-eager to disconnect peers when really close to the tip, plus this might lead to cycling through lots of peers in extreme situations where we have a very slow internet connection.
   • The chosen timeout of 30 seconds could lead to inefficiencies / bandwidth waste when we have a really slow internet connection. Maybe it could make sense to track the last successful download times from existing peers and use a dynamic timeout according to that statistics, instead of setting it to a fixed value.
   • I will leave this PR in draft until I have tested it a bit more in the wild.

   Fixes #29281

2. 
   DrahtBot commented at 10:27 pm on March 15, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	glozow
   Stale ACK	naumenkogs

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label P2P on Mar 15, 2024
4. DrahtBot added the label CI failed on Mar 16, 2024
5. mzumsande force-pushed on Mar 16, 2024
6. mzumsande renamed this:
   p2p: When close to the tip, download blocks in pararallel from additional peers to prevent stalling
   p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling
   on Mar 16, 2024
7. DrahtBot removed the label CI failed on Mar 16, 2024
8. 
   nanlour commented at 2:04 am on March 17, 2024: contributor

   Maybe it could make sense to track the last successful download times from existing peers and use a dynamic timeout according to that statistics, instead of setting it to a fixed value.

   Is it possible for other peers to make me believe I have a slow internet connection?

9. 
   mzumsande commented at 8:06 pm on April 29, 2024: contributor

   Is it possible for other peers to make me believe I have a slow internet connection?

   Sorry, I missed this question. For single peers yes, but the idea is that the data would be taken from multiple outbound peers - it is hard for an attacker to control multiple of these, see eclipse attacks.

   Will soon come back to this PR and take it out of draft.

10. mzumsande force-pushed on May 31, 2024
11. mzumsande marked this as ready for review on May 31, 2024
12. 
    mzumsande commented at 7:36 pm on May 31, 2024: contributor

    Cleaned up a bit, rebased and marked as ready for review!
13. 
    glozow commented at 11:08 am on July 4, 2024: member

    concept ACK
14. 
    naumenkogs commented at 12:59 pm on September 10, 2024: member

    Concept ACK
15. DrahtBot added the label CI failed on Sep 11, 2024
16. DrahtBot removed the label CI failed on Sep 15, 2024
17. DrahtBot added the label Needs rebase on Sep 16, 2024
18. mzumsande force-pushed on Sep 17, 2024
19. 
    mzumsande commented at 10:48 pm on September 17, 2024: contributor

    824c125 to 1bcfcf2: Rebased
20. DrahtBot removed the label Needs rebase on Sep 17, 2024
21. DrahtBot added the label CI failed on Sep 18, 2024
22. 
    maflcko commented at 2:35 pm on September 18, 2024: member

    The CI failed, but the logs are missing :(

    https://github.com/bitcoin/bitcoin/pull/29664/checks?check_run_id=30286710796

    0�[0m�[0;31mp2p_ibd_stalling.py --v2transport                        | ✖ Failed  | 2465 s
    

    I’ll re-run 10 times.

23. DrahtBot removed the label CI failed on Sep 18, 2024
24. 
    mzumsande commented at 3:12 pm on September 18, 2024: contributor

    The CI failed, but the logs are missing :( I’ll re-run 10 times.

    Failed log from CI: https://api.cirrus-ci.com/v1/task/5540180679983104/logs/ci.log

    I’m still not sure why it failed, tried to bump a mocktime with the last push, but poking in the dark since it doesn’t fail locally for me…

25. DrahtBot added the label CI failed on Sep 18, 2024
26. mzumsande force-pushed on Sep 18, 2024
27. 
    instagibbs commented at 6:24 pm on September 18, 2024: member

    IIUC(? could be off-base), this could also fire at tip if there are no hb peers and the first peer requested stalls out for 30s? Once parallel compact blocks is warmed up, the slots will likely be taken by those efforts.

    If so, I wonder if this could allow us to f.e., not require a parallel compact slot be taken by an outbound, if we know we will try an outbound connection 30s later to defeat stalling behavior?

    Having tests for the interplay would be interesting too.

28. DrahtBot removed the label CI failed on Sep 19, 2024
29. 
    mzumsande commented at 10:25 pm on September 19, 2024: contributor

    IIUC(? could be off-base), this could also fire at tip if there are no hb peers and the first peer requested stalls out for 30s? Once parallel compact blocks is warmed up, the slots will likely be taken by those efforts.

    Yes, I agree that it should also happen at the tip, both in the cases of low-bandwidth compact relay and in the case of no compact block relay - which seems like a good thing in most cases?! However, one possible downside might be that if you run with an extremely slow connection for which blocks typically take longer than 30s to download, it could be wasteful to add more peers.

    If so, I wonder if this could allow us to f.e., not require a parallel compact slot be taken by an outbound, if we know we will try an outbound connection 30s later to defeat stalling behavior?

    I’d suspect we probably wouldn’t want that, given that the additional peers introduced here are also likely to be inbound peers (if full), so attacker controlling many of our inbound slots could stall us effectively.

    Having tests for the interplay would be interesting too.

    I will work on adding tests for this situation. I might also push some non-merge commits over the next days to hopefully find out why the test currently fails intermittently (still can’t reproduce locally).

30. 
    instagibbs commented at 2:31 pm on September 20, 2024: member

    I’d suspect we probably wouldn’t want that, given that the additional peers introduced here are also likely to be inbound peers (if full), so attacker controlling many of our inbound slots could stall us effectively.

    If we were to go that direction, I suspect we would force the “last slot” to always be an outbound, just like the parallel compact block download attempts do now. The parallel cb attempts could instead be a bit more courteous to outbound connections in almost every case.

    Just batting the idea around in my head, I look forward to more test cases to reason through.

31. in src/net_processing.cpp:1652 in 26e3686138 outdated

    1646+                    }
    1647+                    if (nMaxHeight <= nWindowEnd && // we have 1024 or less blocks left to download
    1648+                        GetTime<std::chrono::microseconds>() > m_last_block_requested.load() + BLOCK_NEARTIP_TIMEOUT_MAX &&
    1649+                        GetTime<std::chrono::microseconds>() > m_last_block_received.load() + BLOCK_NEARTIP_TIMEOUT_MAX &&
    1650+                        !already_requested_from_peer &&
    1651+                        mapBlocksInFlight.count(pindex->GetBlockHash()) <= 2) {
    

    ---

    ---

    

    instagibbs commented at 2:35 pm on September 20, 2024:

    MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK seems like a variable we could rename to be more general and use here, especially if both mechanisms could fire
32. 
    rebroad commented at 2:39 pm on September 28, 2024: contributor

    Rather than the crude 10 minute timeout - why not actually check if the block is being downloaded from the chosen peer - i.e. work out the throughput and make a decision based on this (compared to typical throughput from other peers)?
33. mzumsande force-pushed on Oct 8, 2024
34. mzumsande force-pushed on Oct 8, 2024
35. DrahtBot added the label CI failed on Oct 8, 2024
36. mzumsande force-pushed on Oct 8, 2024
37. 
    mzumsande commented at 10:50 pm on October 8, 2024: contributor

    I’m still not sure why it failed, tried to bump a mocktime with the last push, but poking in the dark since it doesn’t fail locally for me…

    Hopefullly fixed now, I believe that there was a missing sync in the added test.

    Rather than the crude 10 minute timeout - why not actually check if the block is being downloaded from the chosen peer - i.e. work out the throughput and make a decision based on this (compared to typical throughput from other peers)?

    I’m not sure what exactly you mean with throughput here - do you keeping track of historical block download times, and use this as input for an algorithm for detecting stalling? If so, I’m thinking about that option (and how to exactly design an algorithm using it), and I think it could be applied to all kinds of stalling, also when deep in IBD - but it also has some downsides (more complexity, more data to keep track of, have to be careful it’s not easily gameable). So I’m thinking about it but not completely sure if that is the best way to go.

    In any case the current approach is more in line with the current IBD stalling detection (fixed timeouts).

38. DrahtBot removed the label CI failed on Oct 8, 2024
39. 
    naumenkogs commented at 10:56 am on October 10, 2024: member

    ACK ab45a0654c189da3013ef0c8d30478601052d922
40. DrahtBot requested review from glozow on Oct 10, 2024
41. in test/functional/p2p_ibd_stalling.py:200 in ab45a0654c outdated

    194+        self.log.info("Check that after another 20 minutes, all stalling peers are disconnected")
    195+        # 10 minutes BLOCK_DOWNLOAD_TIMEOUT_BASE + 2*5 minutes BLOCK_DOWNLOAD_TIMEOUT_PER_PEER
    196+        self.mocktime += 20 * 60
    197+        node.setmocktime(self.mocktime)
    198+        for peer in peers:
    199+            peer.wait_for_disconnect()
    

    ---

    ---

    

    instagibbs commented at 3:25 pm on October 10, 2024:

    It’d be better if this test(and others) actually complete the “chain sync” successfully up to known headers to e2e the test case. Perhaps it requests the next stalled block correctly, but it could fall over after.

    ---

    

    mzumsande commented at 6:28 pm on October 11, 2024:

    added, by connecting a peer that provides the missing block in the end, and checking that the sync finishes.
42. 
    instagibbs commented at 6:05 pm on October 10, 2024: member

    I took a bit of time to work on test coverage, with a rough branch here: https://github.com/instagibbs/bitcoin/commits/mzumsande_202403_near_tip_stalling/

    last commit needs some clean up, but demonstrates some of the interplay of parallel blocks both compact and non-compact

43. mzumsande force-pushed on Oct 10, 2024
44. mzumsande force-pushed on Oct 10, 2024
45. DrahtBot added the label CI failed on Oct 10, 2024
46. 
    DrahtBot commented at 7:32 pm on October 10, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31374303124

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

47. 
    mzumsande commented at 7:36 pm on October 10, 2024: contributor

    I took a bit of time to work on test coverage, with a rough branch here: https://github.com/instagibbs/bitcoin/commits/mzumsande_202403_near_tip_stalling/

    last commit needs some clean up, but demonstrates some of the interplay of parallel blocks both compact and non-compact

    Thanks! I also worked on this yesterday/today and wrote a similar test, but added that as an at_tip_stalling subtest to p2p_ibd_stalling.py - while your test covering the logic in a similar way is in p2p_compactblocks.py. In which location do you think that it fits better (or even both?)?

    Your first commit looks great, will definitely take that (but haven’t yet with the latest push).

    One more thing: I added the condition m_last_block_requested.load() > 0s so that the near-tip stalling logic can’t hit right after startup when we never requested a block from anyone.

48. 
    instagibbs commented at 8:00 pm on October 10, 2024: member

    while your test covering the logic in a similar way is in p2p_compactblocks.py. In which location do you think that it fits better (or even both?)?

    I think your location is also fine, we’re not testing that it works, just that it’s being requested pretty much.

49. DrahtBot removed the label CI failed on Oct 11, 2024
50. 
    instagibbs commented at 3:51 pm on October 11, 2024: member

    Just for brainstorming, also made a modified branch that reduces parallel compact block downloads to a count of 2, which would allow one or more backoff blocks at tip at a less aggressive pace: https://github.com/instagibbs/bitcoin/commits/mzumsande_202403_near_tip_stalling_2/
51. test: make p2p_ibd_stalling.py more modular

    This is in preparation to add more subtests.
    Also adjust waiting condition to replace the
    total_bytes_recv_for_blocks magical number hack.

    75668d079c
52. test: add sub-test for near-tip stalling c3d98815cc
53. p2p: Add additional peers to download from when close to the tip

    If we are 1024 or less blocks away from the tip and haven't requested or received
    a block from any peer for 30 seconds, add another peer to download the critical
    block from. Add up to two additional peers this way.
    
    Also adds test for the new behavior (co-authored by Greg Sanders)
    
    Co-authored-by: Greg Sanders <gsanders87@gmail.com>

    9019c08e4e
54. test: Add functional test for stalling at tip

    When at the tip, the near-tip stalling still applies, but
    it will interact with compact block requests. Add tests for this.

    5e1ff82251
55. mzumsande force-pushed on Oct 11, 2024
56. 
    mzumsande commented at 6:27 pm on October 11, 2024: contributor

    973cd01 to 5e1ff82:

    • added more detailed coverage by @instagibbs (thanks!)
    • provided missing block and check that sync finishes in the end in near_tip_stalling subtest
    • remove unused total_bytes_recv_for_blocks in first commit

    Just for brainstorming, also made a modified branch that reduces parallel compact block downloads to a count of 2, which would allow one or more backoff blocks at tip at a less aggressive pace:

    That sounds reasonable. Is it a problem that on slow connections or new connections with unfilled mempools the parallel compact blocks download (which is triggered immediately instead of having a timeout) would hit more often than necessary?

57. 
    instagibbs commented at 6:46 pm on October 11, 2024: member

    Is it a problem that on slow connections or new connections with unfilled mempools the parallel compact blocks download (which is triggered immediately instead of having a timeout) would hit more often than necessary?

    I think that’s already the case, a saving grace is that since we only trigger parallel cb with high-bandwidth cb peers it won’t happen until you’ve seen a few blocks at tip at least.

58. 
    rebroad commented at 0:45 am on October 13, 2024: contributor

    I’m still not sure why it failed, tried to bump a mocktime with the last push, but poking in the dark since it doesn’t fail locally for me…

    Hopefullly fixed now, I believe that there was a missing sync in the added test.

    Rather than the crude 10 minute timeout - why not actually check if the block is being downloaded from the chosen peer - i.e. work out the throughput and make a decision based on this (compared to typical throughput from other peers)?

    I’m not sure what exactly you mean with throughput here - do you keeping track of historical block download times, and use this as input for an algorithm for detecting stalling? If so, I’m thinking about that option (and how to exactly design an algorithm using it), and I think it could be applied to all kinds of stalling, also when deep in IBD - but it also has some downsides (more complexity, more data to keep track of, have to be careful it’s not easily gameable). So I’m thinking about it but not completely sure if that is the best way to go.

    In any case the current approach is more in line with the current IBD stalling detection (fixed timeouts).

    I mean the current download speed - averaged out over the last 30 seconds should be sufficient to make a reasonably quick decision.

Contributors
mzumsande DrahtBot nanlour glozow naumenkogs maflcko instagibbs rebroad

Review Requested
glozow

Labels
P2P