Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actually rate-limit new outbound peer connections #2216

Closed
2 tasks
teor2345 opened this issue May 27, 2021 · 0 comments · Fixed by #2251
Closed
2 tasks

Actually rate-limit new outbound peer connections #2216

teor2345 opened this issue May 27, 2021 · 0 comments · Fixed by #2251
Assignees
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network

Comments

@teor2345
Copy link
Collaborator

teor2345 commented May 27, 2021

Is your feature request related to a problem? Please describe.

Zebra has an outbound connection rate-limit in the CandidateSet. But if there haven't been many connections for a while, it can allow a very large burst of connections.

Describe the solution you'd like

  • Set the current_deadline to the maximum of the current time and the next_peer_min_wait.deadline():
    let current_deadline = self.next_peer_min_wait.deadline();
    let mut sleep = sleep_until(current_deadline + Self::MIN_PEER_CONNECTION_INTERVAL);

This makes sure that the next sleep is at least 100 milliseconds from the current time. (It can be be up to 200 milliseconds, if there's just been a connection, and we're about to sleep for 100 milliseconds.)

  • Write proptests with random attempts and waits
    • 10 random addresses in an arbitrary AddressBook
    • 0..4 initial candidates, an async sleep from 0..400 milliseconds, and then 0..4 extra candidates
    • the first candidate should yield immediately, but we can't really test for that due to VM delays
    • each subsequent candidate should yield at least 100 milliseconds after the last candidate
    • the whole test should complete within 10 seconds (use a timeout and select)

Describe alternatives you've considered

We could change the CandidateSet.next_peer_min_wait from a Sleep to an Instant. This simplifies the code, because we're not storing a Sleep future any more. See #2160 for an example of this change.

Additional context

This change will also rate-limit the crawler task, so we should do it before #2212.

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage 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 May 27, 2021
@teor2345 teor2345 added this to the 2021 Sprint 11 - Zcon2 milestone May 27, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label May 31, 2021
@jvff jvff self-assigned this Jun 3, 2021
jvff added a commit to jvff/zebra that referenced this issue Jun 4, 2021
Set the rate-limiting sleep timer to use a delay added to the maximum
between the next peer connection instant and now. This ensures that the
timer always sleeps at least the time used for the delay.

This change fixes rate-limiting new outbound peer connections, since
before there could be a burst of attempts until the deadline progressed
to the current instant.

Fixes ZcashFoundation#2216
jvff added a commit to jvff/zebra that referenced this issue Jun 4, 2021
Set the rate-limiting sleep timer to use a delay added to the maximum
between the next peer connection instant and now. This ensures that the
timer always sleeps at least the time used for the delay.

This change fixes rate-limiting new outbound peer connections, since
before there could be a burst of attempts until the deadline progressed
to the current instant.

Fixes ZcashFoundation#2216
teor2345 pushed a commit that referenced this issue Jun 7, 2021
* Rate-limit new outbound peer connections

Set the rate-limiting sleep timer to use a delay added to the maximum
between the next peer connection instant and now. This ensures that the
timer always sleeps at least the time used for the delay.

This change fixes rate-limiting new outbound peer connections, since
before there could be a burst of attempts until the deadline progressed
to the current instant.

Fixes #2216

* Create `MetaAddr::alternate_node_strategy` helper

Creates arbitrary `MetaAddr`s as if they were network nodes that sent
their listening address.

* Test outbound peer connection rate limiting

Tests if connections are rate limited to 10 per second, and also tests
that sleeping before continuing with the attempts still respets the rate
limit and does not result in a burst of reconnection attempts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-security Category: Security issues I-remote-node-overload Zebra can overload other nodes on the network
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants