Randomize message processing peer order #22144

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202106_rand_peers changing 1 files +6 −0

sipa commented at 5:34 pm on June 3, 2021: member

Right now, the message handling loop iterates the list of nodes always in the same order: the order they were connected in (see the vNodes vector). For some parts of the net processing logic, this order matters. Transaction requests are assigned explicitly to peers since #19988, but many other parts of processing work on a “first-served-by-loop-first” basis, such as block downloading. If peers can predict this ordering, it may be exploited to cause delays.

As there isn’t anything particularly optimal about the current ordering, just make it unpredictable by randomizing.

Reported by Crypt-iQ.
Randomize message processing peer order 79c02c88b3
fanquake added the label P2P on Jun 3, 2021
practicalswift commented at 7:32 pm on June 3, 2021: contributor

Concept ACK
theStack commented at 7:45 pm on June 3, 2021: member

Concept ACK
ghost commented at 7:51 pm on June 3, 2021: none

Concept ACK

Randomize the order in which we process messages from/to our peers. This prevents attacks in which an attacker exploits having multiple consecutive connections in the vNodes list.

I am assuming it improves privacy and security.
Crypt-iQ commented at 10:16 pm on June 3, 2021: contributor

Concept ACK
DrahtBot commented at 3:27 am on June 4, 2021: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21943 (Dedup and RAII-fy the creation of a copy of CConnman::vNodes by vasild)
- #21878 (Make all networking code mockable by vasild)
- #20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
jnewbery commented at 10:39 am on June 4, 2021: member

ACK 79c02c88b3

We shouldn’t build any assumptions into net_processing about the order that we serve peers. Shuffling the order on each loop of the message handler is a good way to make that requirement more explicit.

I think in the future, we want to move away from a strictly equally-weighted algorithm to service our peers in the message handling thread. If a peer is providing us with lots of useful data, we shouldn’t have to call ProcessMessages()/SendMessages() for every peer each time we process a message from the useful peer. Likewise, if a peer is causing us to use a lot of resources, we should be able to throttle back on serving it. That would mean a non-stable ordering of servicing peers, which would again mean that net_processing can’t assume a stable ordering. Merging this PR should give us confidence that there aren’t any undocumented assumptions about ordering in net_processing and that those future changes are safe.

In summary, I think this is a useful change now, and is consistent with the way we should think about the net-net_processing interface.
kristapsk commented at 10:58 am on June 4, 2021: contributor

Concept ACK
Crypt-iQ commented at 3:27 pm on June 4, 2021: contributor

ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
sipa commented at 5:06 pm on June 4, 2021: member

FWIW, shuffling 1000 pointers takes around 10 microseconds here, if anyone was concerned about the CPU cost (in a microbenchmark).
jonatack commented at 5:25 pm on June 4, 2021: member

FWIW, shuffling 1000 pointers takes around 10 microseconds here, if anyone was concerned about the CPU cost (in a microbenchmark).

Thanks, I was curious what the impact would be.

I’ve been running a node with this patch rebased to master (to also continue testing the torv2 removal) since a few minutes after this was opened, as a sanity check, and all seems nominal.

ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
sdaftuar commented at 4:53 pm on June 7, 2021: member

utACK 79c02c88b347f1408a2db307db2654917f9b0bcc
vasild approved
vasild commented at 1:30 pm on June 9, 2021: member

ACK 79c02c88b347f1408a2db307db2654917f9b0bcc

The patch looks harmless and will achieve what it claims. Maybe it is just me, but I don’t see the attack vector.
luke-jr commented at 6:26 pm on June 12, 2021: member

Is there a risk this could make things worse? Right now, longer-connected peers can’t be “disrupted” (in this context) by later-connected peers, but with a random order, disruption opportunities also open up to later peers where they might not have previously been.
sipa commented at 6:40 pm on June 12, 2021: member

@luke-jr Interruption isn’t changed, we always progress from one peer to the next one in vNodes to the next one in the list, at predefined times (when all its messages have been processed, or after certain things like getdata for blocks). What this changes is removing the predictability of that order.
jamesob commented at 9:43 pm on June 12, 2021: member

Concept ACK
theStack approved
theStack commented at 4:36 pm on June 14, 2021: member

ACK 79c02c88b347f1408a2db307db2654917f9b0bcc

Built and ran the PR branch on mainnet for the last few hours, didn’t notice any problems.
sipa added this to the milestone 22.0 on Jun 14, 2021
achow101 commented at 6:02 pm on June 15, 2021: member

Code Review ACK 79c02c88b347f1408a2db307db2654917f9b0bcc
jamesob commented at 6:04 pm on June 15, 2021: member

crACK https://github.com/bitcoin/bitcoin/pull/22144/commits/79c02c88b347f1408a2db307db2654917f9b0bcc
fanquake commented at 3:27 am on June 16, 2021: member

Edited op to remove @ mention.
fanquake merged this on Jun 16, 2021
fanquake closed this on Jun 16, 2021
sidhujag referenced this in commit 0134f50ed5 on Jun 16, 2021
vasild commented at 3:56 pm on June 21, 2021: member

Should this shuffling also be done in CConnman::SocketHandler()? I will do that in #21943 mainly because it is most clean to create the nodes’ snapshot shuffled.
gwillen referenced this in commit ebffc84d07 on Jun 1, 2022
DrahtBot locked this on Aug 18, 2022