Limit the number of outbound peer connections #2944

teor2345 · 2021-10-25T00:19:15Z

Motivation

We want to limit Zebra's outbound peer connections to avoid whole-of-network denial of service attacks.

Solution

Implementation:

Limit the number of outbound connections in the crawler
Bias Zebra towards outbound connections
Make zebra-network channel bounds depend on config.peerset_initial_target_size

Bug fixes:

Avoid cooperative async task starvation in the peer crawler and listener

Unit tests:

Add zebra_network::initialize tests with limited numbers of peers
Test the crawler with small connection limits and connection errors
Test the crawler with small connection limits and connection success then close
Test the crawler with small connection limits and connection success then stay open
Test the crawler with the default connection limit and connection success then stay open

Runtime checks:

Panic if Zebra exceeds its connection limit #2947

Local tests:

I've run the node for a few days and it seems stable, the connection limits seem to work
I've run the unit tests repeatedly, and they seem reliable on my machine

Closes #1850.

Review

@jvff is a good person to review this PR.

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

jvff

Looks good, just added some minor questions/nit-picks.

zebra-network/src/config.rs

zebra-network/src/constants.rs

zebra-network/src/peer_set/initialize.rs

teor2345

@conradoplg here are some notes on testing.

You might need the task starvation fixes in 5d245cf to avoid test hangs.
(Commit name: Avoid cooperative async task starvation...)

And you might need the new Config methods in 0b5f82e.
(Bias Zebra towards outbound connections.)

zebra-network/src/peer_set/initialize/tests/vectors.rs

…rget_size

And turn connection limits into `Config` methods.

Includes: - `fastmod OUTBOUND_PEER_BIAS_FRACTION OUTBOUND_PEER_BIAS_DENOMINATOR zebra*` - clarify connection limit documentation

If we don't yield in these loops, they can run for a long time before tokio forces them to yield.

And use the multi-threaded runtime to avoid long hangs.

Adds yields after inbound successes and initial peer connections.

teor2345 · 2021-10-27T01:42:08Z

I've checked the "network health" Grafana dashboard. It shows the peer count dropping to ~75 when I implemented the outbound connection limit.

oxarbitrage

I think this looks good and suggestions from @jvff were applied.

jvff

Looks good. The tests are very similar to the ones in #2961 (or maybe it should be the other way round 😅 ), so some review comments from there might also be useful here.

But we can merge as is and take a look at improving all of them later 👍

zebra-network/src/peer_set/initialize/tests/vectors.rs

…d by #2969

* ZIPs were updated to remove ambiguity, this was tracked in #1267. * #2105 was fixed by #3039 and #2379 was closed by #3069 * #2230 was a duplicate of #2231 which was closed by #2511 * #3235 was obsoleted by #2156 which was fixed by #3505 * #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969 * We migrated to Rust 2021 edition in Jan 2022 with #3332 * #1631 was closed as not needed * #338 was fixed by #3040 and #1162 was fixed by #3067 * #2079 was fixed by #2445 * #4794 was fixed by #6122 * #1678 stopped being an issue * #3151 was fixed by #3934 * #3204 was closed as not needed * #1213 was fixed by #4586 * #1774 was closed as not needed * #4633 was closed as not needed * Clarify behaviour of difficulty spacing Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour when retrying block downloads Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify when we might want to fix Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify what we might want to change in future Co-authored-by: teor <teor@riseup.net> * Clarify benefits of how we do block verification Co-authored-by: teor <teor@riseup.net> * Fix rustfmt errors --------- Co-authored-by: teor <teor@riseup.net>

teor2345 added P-High C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network A-network Area: Network protocol updates or fixes labels Oct 25, 2021

teor2345 requested a review from jvff October 25, 2021 00:19

teor2345 self-assigned this Oct 25, 2021

This was referenced Oct 25, 2021

Limit the initial crawler demand for outbound peers, based on seed peer connections #2902

Closed

Panic if Zebra exceeds its connection limit #2947

Merged

Rate-limit initial seed peer connections #2943

Merged

jvff previously approved these changes Oct 25, 2021

View reviewed changes

zebra-network/src/config.rs Outdated Show resolved Hide resolved

zebra-network/src/constants.rs Outdated Show resolved Hide resolved

zebra-network/src/peer_set/initialize.rs Show resolved Hide resolved

teor2345 dismissed jvff’s stale review via 3839027 October 26, 2021 00:12

teor2345 force-pushed the limit-outbound-peers branch 3 times, most recently from 8ec9c93 to 6188022 Compare October 26, 2021 07:03

teor2345 commented Oct 26, 2021

View reviewed changes

zebra-network/src/peer_set/initialize/tests/vectors.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

teor2345 force-pushed the limit-outbound-peers branch from 6188022 to e51afcb Compare October 27, 2021 01:20

teor2345 marked this pull request as ready for review October 27, 2021 01:24

teor2345 added 12 commits October 27, 2021 11:25

Limit the number of outbound connections in the crawler

f140f7d

Make zebra-network channel bounds depend on config.peerset_initial_ta…

03f3e12

…rget_size

Bias Zebra towards outbound connections

4fcf5a6

And turn connection limits into `Config` methods.

Downgrade some connection logs to debug

1f81c20

Remove verbose or outdated fields in tracing logs

6390788

Clarify connection limits

94be242

Includes: - `fastmod OUTBOUND_PEER_BIAS_FRACTION OUTBOUND_PEER_BIAS_DENOMINATOR zebra*` - clarify connection limit documentation

Clarify inventory channel capacity

76dbaf2

Add zebra_network::initialize tests with limited numbers of peers

d3884eb

Avoid cooperative async task starvation in the peer crawler and listener

df3cc1a

If we don't yield in these loops, they can run for a long time before tokio forces them to yield.

Test the crawler with small connection limits

abb695c

And use the multi-threaded runtime to avoid long hangs.

Stop using the multi-threaded executor in tests where it's not needed

ad51903

Avoid starvation for every connection

ecf51e4

Adds yields after inbound successes and initial peer connections.

teor2345 added 2 commits October 27, 2021 11:25

Add a crawler peer connection success test

627712a

Add outbound connection limit tests

4315699

teor2345 force-pushed the limit-outbound-peers branch from e51afcb to 4315699 Compare October 27, 2021 01:25

teor2345 requested a review from jvff October 27, 2021 01:28

Improve outbound tests

0fd5259

teor2345 mentioned this pull request Oct 27, 2021

Limit the number of inbound peer connections #2961

Merged

3 tasks

oxarbitrage approved these changes Oct 27, 2021

View reviewed changes

Merge branch 'main' into limit-outbound-peers

0e0768c

teor2345 enabled auto-merge (squash) October 27, 2021 20:28

jvff approved these changes Oct 27, 2021

View reviewed changes

zebra-network/src/peer_set/initialize/tests/vectors.rs Show resolved Hide resolved

teor2345 merged commit 3e03d48 into main Oct 27, 2021

teor2345 deleted the limit-outbound-peers branch October 27, 2021 21:28

mpguerra added a commit that referenced this pull request May 19, 2023

#1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixe…

9b67e09

…d by #2969

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit the number of outbound peer connections #2944

Limit the number of outbound peer connections #2944

teor2345 commented Oct 25, 2021 •

edited

Loading

jvff left a comment

teor2345 left a comment

This comment has been minimized.

teor2345 commented Oct 27, 2021

oxarbitrage left a comment

jvff left a comment

Limit the number of outbound peer connections #2944

Limit the number of outbound peer connections #2944

Conversation

teor2345 commented Oct 25, 2021 • edited Loading

Motivation

Solution

Review

Reviewer Checklist

jvff left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

teor2345 commented Oct 27, 2021

oxarbitrage left a comment

Choose a reason for hiding this comment

jvff left a comment

Choose a reason for hiding this comment

teor2345 commented Oct 25, 2021 •

edited

Loading