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

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

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.

DrahtBot added the label P2P on Mar 15, 2024

DrahtBot added the label CI failed on Mar 16, 2024

mzumsande force-pushed on Mar 16, 2024

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

DrahtBot removed the label CI failed on Mar 16, 2024

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?

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.

mzumsande force-pushed on May 31, 2024

mzumsande marked this as ready for review on May 31, 2024

mzumsande commented at 7:36 pm on May 31, 2024: contributor

Cleaned up a bit, rebased and marked as ready for review!

glozow commented at 11:08 am on July 4, 2024: member

concept ACK

naumenkogs commented at 12:59 pm on September 10, 2024: member

Concept ACK

DrahtBot added the label CI failed on Sep 11, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

DrahtBot added the label Needs rebase on Sep 16, 2024

mzumsande force-pushed on Sep 17, 2024

mzumsande commented at 10:48 pm on September 17, 2024: contributor

824c125 to 1bcfcf2: Rebased

DrahtBot removed the label Needs rebase on Sep 17, 2024

DrahtBot added the label CI failed on Sep 18, 2024

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.

DrahtBot removed the label CI failed on Sep 18, 2024

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…

DrahtBot added the label CI failed on Sep 18, 2024

mzumsande force-pushed on Sep 18, 2024

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.

DrahtBot removed the label CI failed on Sep 19, 2024

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).

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.

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

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)?

mzumsande force-pushed on Oct 8, 2024

DrahtBot added the label CI failed on Oct 8, 2024

mzumsande force-pushed on Oct 8, 2024

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).

DrahtBot removed the label CI failed on Oct 8, 2024

naumenkogs commented at 10:56 am on October 10, 2024: member

ACK ab45a0654c189da3013ef0c8d30478601052d922

DrahtBot requested review from glozow on Oct 10, 2024

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.

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

mzumsande force-pushed on Oct 10, 2024

DrahtBot added the label CI failed on Oct 10, 2024

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.

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.

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.

DrahtBot removed the label CI failed on Oct 11, 2024

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/

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

test: add sub-test for near-tip stalling c3d98815cc

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

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

mzumsande force-pushed on Oct 11, 2024

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?

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.

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.

mzumsande marked this as a draft on Dec 6, 2024

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

Code Coverage

Reviews

Conflicts