Randomize message processing peer order

[sipa]

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.

[practicalswift]

Concept ACK

[theStack]

Concept ACK

[ghost]

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]

Concept ACK

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943 (Dedup and RAII-fy the creation of a copy of CConnman::vNodes by vasild)
• Make all networking code mockable #21878 (Make all networking code mockable by vasild)
• Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #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]

ACK 79c02c8

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]

Concept ACK

[Crypt-iQ]

ACK 79c02c8

[sipa]

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

[jonatack]

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 79c02c8

[sdaftuar]

utACK 79c02c8

[vasild]

ACK 79c02c8

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]

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]

@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]

Concept ACK

[theStack]

ACK 79c02c8

Built and ran the PR branch on mainnet for the last few hours, didn't notice any problems.

[achow101]

Code Review ACK 79c02c8

[jamesob]

crACK 79c02c8

[fanquake]

Edited op to remove @ mention.

[vasild]

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.