p2p: make block download logic aware of limited peers threshold #28120

furszy · 2023-07-21T13:55:22Z

Even when the node believes it has IBD completed, need to avoid
requesting historical blocks from network-limited peers.
Otherwise, the limited peer will disconnect right away.

The simplest scenario could be a node that gets synced, drops
connections, and stays inactive for a while. Then, once it re-connects
(IBD stays completed), the node tries to fetch all the missing blocks
from any peer, getting disconnected by the limited ones.

Note:
Can verify the behavior by cherry-picking the test commit alone on
master. It will fail there.

DrahtBot · 2023-07-21T13:55:25Z

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
ACK	vasild, mzumsande, pinheadmz, achow101
Stale ACK	andrewtoth

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.

mzumsande

Maybe getting disconnected could be beneficial in this rare situation? If our tip is that much behind but we're not in IBD, we can't help them and they can't help us, so our priority should be to get the missing blocks asap, and getting disconnected by useless peers seems like it's not such a bad thing towards that goal.

What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

furszy

Maybe getting disconnected could be beneficial in this rare situation? If our tip is that much behind but we're not in IBD, we can't help them and they can't help us, so our priority should be to get the missing blocks asap, and getting disconnected by useless peers seems like it's not such a bad thing towards that goal.

Agree but, while the outcome might be the desired one, the approach to achieve it isn't the best. It seems unwise to have the node sending a message through the wire, knowing it will fail, just to force the other peer to disconnect.
The node should be smarter than that and not rely on external factors to take decisions.

Moreover, in cases where the other peer chooses to ignore the message without disconnecting (maybe due to using different software than bitcoin-core), the node will end up waiting for a response that will never arrive, wasting time and resources, which could have been easily avoided.

And also, while the node has more connection slots available, I don't see why should disconnect a peer that is behaving properly.

Instead, I think that we should try to keep processes separated. The eviction process should be in charge of disconnecting peers that aren't/will not provide any meaningful data. And the block downloading logic should only be responsible of deciding which blocks request to each peer and which ones not.

naumenkogs · 2023-07-25T09:40:13Z

What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

I agree with this part. I think the solution should be expanded to give a chance in such cases, and it should be merged within the same PR.

luke-jr · 2023-07-25T18:33:06Z

Agree with @mzumsande and @naumenkogs. Maybe not the same PR if it's substantially different, but the new eviction logic should be merged before this fix.

furszy

What would be bad is if all our current peers were limited, and we wouldn't try to request the needed blocks from anyone, but also wouldn't try to exchange them for better peers, so that we would be stuck.

Right now, this will hardly happen. The reason is CheckForStaleTipAndEvictPeers. The logic is as follows:

When the node doesn't update the tip for a period longer than 30 minutes (TipMayBeStale()), it triggers the extra outbound connection process. Which instructs the net layer to bypass the maximum outbound connection restriction and create another full relay outbound connection to sync up the missing blocks.

Then, considering the worst-case scenario: when the extra outbound connection is also a limited peer. In that case, 45 seconds after connecting to the extra peer (scheduler task timeout), CheckForStaleTipAndEvictPeers will call EvictExtraOutboundPeers, which will evict the peer that least recently announced a block header.
Then, in case of no chain movement; the process starts again, adding an extra outbound connection, and so on, until it finds a peer that can provide the missing blocks.

Note:
While this sooner or later corrects the stalling situation, it's not ideal to continue establishing connections to limited peers, on the extra outbound connection process, when the peer is stalled. So, to address this issue, have created #28170. Which basically disallows connections to limited peers when the node is behind enough to know that limited peers will not provide any of the required historical blocks (although they will still provide valid headers).

furszy

rebased. Conflicts solved.

src/net_processing.cpp

vasild

Approach ACK c1612ea

src/net_processing.cpp

test/functional/p2p_node_network_limited.py

furszy

Updated per feedback, thanks for the review!

Fixed the two blocks buffer extension (now is reduction). Small diff.

test/functional/p2p_node_network_limited.py

furszy

Updated per feedback, thanks for the review @vasild!

The node will now request blocks within the limited peers threshold when the block download window contemplates them and not only when the previous blocks arrived or are in-flight. Added test coverage exercising this behavior and verifying that the node can get synced even when the blocks arrived out of order.

test/functional/p2p_node_network_limited.py

andrewtoth · 2024-02-21T22:55:00Z

src/net_processing.cpp

@@ -1451,6 +1451,7 @@ void PeerManagerImpl::FindNextBlocks(std::vector<const CBlockIndex*>& vBlocks, c
 {
    std::vector<const CBlockIndex*> vToFetch;
    int nMaxHeight = std::min<int>(state->pindexBestKnownBlock->nHeight, nWindowEnd + 1);
+    bool is_limited_peer = IsLimitedPeer(peer);


nit: const bool is_limited_peer{IsLimitedPeer};

andrewtoth · 2024-02-21T23:29:12Z

src/net_processing.cpp

@@ -1475,6 +1476,11 @@ void PeerManagerImpl::FindNextBlocks(std::vector<const CBlockIndex*>& vBlocks, c
                return;
            }

+            // Don't request blocks that go further than what limited peers can provide
+            if (is_limited_peer && (state->pindexBestKnownBlock->nHeight - pindex->nHeight >= static_cast<int>(NODE_NETWORK_LIMITED_MIN_BLOCKS) - 2 /* two blocks buffer for possible races */)) {


I think what @naumenkogs is referring to here is that if this condition is true then pindex will hit this condition on the first pindex in the loop and every pindex up until this condition is no longer true. This can be more efficient by just bumping pindex to the first pindex that is past the network limited blocks threshold.

So, we can either change the loop from range based to regular for-loop and advance the index, or move this check out of this loop and into the loop on line 1464 and break early if the condition is met.

mzumsande

Code Review ACK c5b5843

pinheadmz

ACK c5b5843

Built and ran all functional and unit tests on x86 Ubuntu. Confirmed test fails on master, passes with the patch in the branch. Reviewed code and existing discussion. Regarding the decision to keep the pruning peer connected at all, I agree with the author that the affected code is only responsible for requesting available blocks from a peer and it is reasonable to take the limited flag into account. I like the clean up in the first commit as well, it's easier to read the loop with continue.

For sanity I added checks in the test to confirm blocks from the pruned node were "found" by the full node but had 'confirmations': -1, which makes sense because there is a gap between the full node's chain tip and the blocks available from the pruning peer.

Show Signature

pinheadmz's public key is on keybase

achow101 · 2024-03-11T12:09:07Z

ACK c5b5843

DrahtBot added the P2P label Jul 21, 2023

mzumsande reviewed Jul 21, 2023

View reviewed changes

furszy commented Jul 21, 2023

View reviewed changes

furszy mentioned this pull request Jul 27, 2023

p2p: adaptive connections services flags #28170

Merged

furszy commented Aug 1, 2023

View reviewed changes

furszy mentioned this pull request Aug 1, 2023

net: introduce block tracker to retry to download blocks after failure #27837

Draft

DrahtBot mentioned this pull request Aug 24, 2023

assumeutxo (2) #27596

Merged

achow101 requested review from vasild and sipa September 20, 2023 17:22

DrahtBot mentioned this pull request Sep 21, 2023

validation: assumeutxo params for testnet and signet #28516

Closed

DrahtBot mentioned this pull request Sep 30, 2023

validation: assumeutxo params mainnet #28553

Draft

DrahtBot added the Needs rebase label Oct 2, 2023

furszy mentioned this pull request Oct 16, 2023

Allow getblockfrompeer to use any peer #27652

Open

furszy force-pushed the 2023_net_limited_peers branch from 8e91439 to c1612ea Compare October 18, 2023 13:34

furszy commented Oct 18, 2023

View reviewed changes

DrahtBot removed the Needs rebase label Oct 18, 2023

andrewtoth reviewed Oct 29, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

vasild reviewed Nov 1, 2023

View reviewed changes

furszy force-pushed the 2023_net_limited_peers branch 2 times, most recently from 9eba981 to 534996a Compare November 2, 2023 22:33

furszy commented Nov 2, 2023

View reviewed changes

DrahtBot mentioned this pull request Nov 7, 2023

test: Make existing functional tests compatible with --v2transport #28805

Merged

vasild reviewed Nov 10, 2023

View reviewed changes

test/functional/p2p_node_network_limited.py Outdated Show resolved Hide resolved

furszy force-pushed the 2023_net_limited_peers branch from 534996a to adfc9c7 Compare November 11, 2023 00:33

furszy commented Nov 11, 2023

View reviewed changes

DrahtBot added the CI failed label Nov 11, 2023

furszy force-pushed the 2023_net_limited_peers branch from adfc9c7 to 930e531 Compare November 11, 2023 02:09

bitcoin deleted a comment from sicelo7 Feb 6, 2024

mzumsande reviewed Feb 15, 2024

View reviewed changes

test/functional/p2p_node_network_limited.py Show resolved Hide resolved

DrahtBot requested review from andrewtoth and removed request for andrewtoth February 15, 2024 17:37

andrewtoth reviewed Feb 21, 2024

View reviewed changes

DrahtBot requested review from andrewtoth and removed request for andrewtoth February 21, 2024 23:40

mzumsande reviewed Feb 26, 2024

View reviewed changes

DrahtBot requested review from andrewtoth and removed request for andrewtoth February 26, 2024 23:12

pinheadmz approved these changes Mar 9, 2024

View reviewed changes

DrahtBot requested review from andrewtoth and removed request for andrewtoth March 9, 2024 23:23

DrahtBot requested review from andrewtoth and removed request for andrewtoth March 11, 2024 12:09

achow101 merged commit 4a90374 into bitcoin:master Mar 11, 2024
16 checks passed

furszy deleted the 2023_net_limited_peers branch March 11, 2024 12:18

andrewtoth mentioned this pull request May 1, 2024

net: don't lock cs_main while reading blocks in net processing #26326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: make block download logic aware of limited peers threshold #28120

p2p: make block download logic aware of limited peers threshold #28120

furszy commented Jul 21, 2023 •

edited

DrahtBot commented Jul 21, 2023 •

edited

mzumsande left a comment

furszy left a comment •

edited

naumenkogs commented Jul 25, 2023

luke-jr commented Jul 25, 2023

furszy left a comment •

edited

furszy left a comment

vasild left a comment

furszy left a comment

furszy left a comment •

edited

andrewtoth Feb 21, 2024

andrewtoth Feb 21, 2024

mzumsande left a comment

pinheadmz left a comment

achow101 commented Mar 11, 2024

p2p: make block download logic aware of limited peers threshold #28120

p2p: make block download logic aware of limited peers threshold #28120

Conversation

furszy commented Jul 21, 2023 • edited

DrahtBot commented Jul 21, 2023 • edited

Code Coverage

Reviews

Conflicts

mzumsande left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

naumenkogs commented Jul 25, 2023

luke-jr commented Jul 25, 2023

furszy left a comment • edited

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

vasild left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

andrewtoth Feb 21, 2024

Choose a reason for hiding this comment

andrewtoth Feb 21, 2024

Choose a reason for hiding this comment

mzumsande left a comment

Choose a reason for hiding this comment

pinheadmz left a comment

Choose a reason for hiding this comment

achow101 commented Mar 11, 2024

furszy commented Jul 21, 2023 •

edited

DrahtBot commented Jul 21, 2023 •

edited

furszy left a comment •

edited

furszy left a comment •

edited

furszy left a comment •

edited