Disconnect from outdated peers on network upgrade #3108

jvff · 2021-11-28T11:58:23Z

Motivation

Zebra should disconnect from outdated peers when a network upgrade is activated.

Specifications

Overwinter nodes should take steps to:

[...]

disconnect any existing connections to pre-Overwinter nodes.

https://zips.z.cash/zip-0201#rejecting-pre-overwinter-connections

Solution

create a LoadTrackedClient that wraps a load tracked client service and keeps track of the peer's reported protocol version
create a PeerDiscoverer type that prepares discovered peers into LoadTrackedClients
disconnect from outdated peers stored in ready_services once the minimum peer version changes
avoid adding unready services for outdated peers
avoid adding ready services for outdated peers

Review

@teor2345 can review this. It's still in draft because I haven't written any tests yet.

Reviewer Checklist

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

Follow Up Work

teor2345

This looks great, thanks for your hard work on this!

The new design avoids the issues with smuggling the version in the Discover::Key. (I was concerned I would have to review every existing key use, to make sure the hidden version was handled correctly. But this design avoids that problem entirely.)

I've suggested some behaviour and module changes that we should fix before writing the tests. Or feel free to skip them.

I also have a bunch of minor questions and nitpicks.
Edit: I've marked some minor issues as resolved, so we can focus on the changes that might cause test rework.

zebra-network/src/discoverer.rs

zebra-network/src/peer_set/initialize.rs

zebra-network/src/peer_set/set.rs

teor2345 · 2021-11-29T02:13:09Z

I think we can close #2262 without any disconnection rate limits, because:

ready peer connections are closed immediately
unready peer connections are closed when their requests complete

So that provides enough natural rate-limiting on busy peers. And on peers that aren't busy, the rate-limits don't matter, they'll just slowly re-open connections as needed.

But we should document this behaviour somewhere, so we know why it's ok.

What do you think?

zebra-network/src/discoverer.rs

jvff · 2021-12-01T02:38:50Z

Before Tests Question: This type is really complicated, I'm wondering if we could use a stream mapping instead?

Thanks for this, it's a great idea! It allowed me to simplify the code quite a bit :)

teor2345

Looks good, I think we're getting down to the last few bugs.

I've un-resolved some minor nitpicks as well.

zebra-network/src/constants.rs

zebra-network/src/peer_set/initialize.rs

zebra-network/src/peer_set/set.rs

teor2345

Looks great!

We're just tweaking comments now, so feel free to merge once you're happy with the tests.

teor2345

Looks good, let me know if you'd like help with the tests

Remove the assumption that a `Remove` variant would never be created with type changes that allow the compiler to guarantee that assumption.

Keep track of the peer's reported protocol version.

A `peer::Client` type wrapper that implements `Load`. This helps with the creation of a client service that has extra peer information to be accessed without having to send requests.

Ensure that `PeerSet` receives `LoadTrackedClient`s so that it will be able to query the peer's protocol version later on.

Replace the generic type with a concrete `LoadTrackedClient` so that we can query its version.

A type to track the current minimum protocol version for connected peers based on the current block height.

Keep the code to obtain the current minimum peer protocol version in a central place.

Prepare it to be able to disconnect from outdated peers based on the current minimum supported peer protocol version.

When the minimum peer protocol version is detected to have changed (because of a network upgrade), remove all ready services of peers that became outdated.

Only add an unready service if it's for a peer that has a supported protocol version. Otherwise, add it but drop the cancel handle so that the `UnreadyService` can execute and detect that it was cancelled.

If a service becomes ready but it's for a connection to an outdated peer, drop it.

Describe an edge case that is also handled but was not explicit. Co-authored-by: teor <teor@riseup.net>

Given an arbitrary best chain tip height, check that the calculated minimum peer protocol version is the expected value.

Apply an arbitrary list of chain tip height updates and check that for each update the minimum peer version is calculated correctly.

Simulate a series of best chain tip height updates, and check for minimum peer version updates at least once between them. Changes should only be reported once.

Used to create and then track a mock `Client` instance.

An extension method useful for tests, that contains some shared boilerplate code.

Give a 50% chance for an arbitrary `Version` to be in the range of previously used values the Zcash network.

Helps with the creation of mocked client services with arbitrary protocol versions.

An auxiliary type to a `PeerSet` instance created for testing. It keeps track of any dummy endpoints of channels created and passed to the `PeerSet` instance.

Helps to reduce the code when preparing a `PeerSet` test instance.

Simulate a set of discovered peers being sent to the `PeerSet`. Ensure that only up-to-date peers are kept by the `PeerSet` and that outdated peers are dropped.

A helper type that allows the creation of arbitrary block height pairs, where one value is before and the other is at or after the activation height of an arbitrary network upgrade.

Simulate a network upgrade, and check that peers that become outdated are dropped by the `PeerSet`.

zebra-network/src/peer_set/set/tests.rs

zebra-network/src/peer_set/set/tests/prop.rs

zebra-network/src/protocol/external/arbitrary.rs

teor2345

Looks good, merging now to unblock the notfound work in this sprint.

I have a minor suggestion that we can fix in a follow-up PR or ticket.

…grade

zfnd-bot bot assigned jvff Nov 28, 2021

teor2345 reviewed Nov 29, 2021

View reviewed changes

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

mpguerra linked an issue Nov 29, 2021 that may be closed by this pull request

After network upgrade activation, close existing connections to outdated peers #2262

Closed

8 tasks

jvff force-pushed the disconnect-from-outdated-peers-on-network-upgrade branch from 037da4b to ef0d790 Compare December 1, 2021 02:34

jvff requested review from teor2345 December 1, 2021 02:38

teor2345 reviewed Dec 1, 2021

View reviewed changes

jvff mentioned this pull request Dec 2, 2021

UnreadyService ignores dropped cancel handles #3130

Closed

jvff force-pushed the disconnect-from-outdated-peers-on-network-upgrade branch 2 times, most recently from 783a891 to 32f0417 Compare December 2, 2021 01:22

teor2345 reviewed Dec 2, 2021

View reviewed changes

zebra-network/src/peer_set/set.rs Outdated Show resolved Hide resolved

teor2345 reviewed Dec 2, 2021

View reviewed changes

jvff force-pushed the disconnect-from-outdated-peers-on-network-upgrade branch from 32f0417 to 77d55ad Compare December 3, 2021 01:51

jvff requested a review from teor2345 December 3, 2021 01:51

teor2345 reviewed Dec 3, 2021

View reviewed changes

teor2345 added the P-Medium label Dec 8, 2021

jvff force-pushed the disconnect-from-outdated-peers-on-network-upgrade branch from 0f1e426 to 51831c5 Compare December 8, 2021 22:36

jvff added 10 commits December 9, 2021 00:54

Replace usage of discover::Change with a tuple

a9c82b3

Remove the assumption that a `Remove` variant would never be created with type changes that allow the compiler to guarantee that assumption.

Add a version field to the Client type

0852a12

Keep track of the peer's reported protocol version.

Create LoadTrackedClient type

3b12430

A `peer::Client` type wrapper that implements `Load`. This helps with the creation of a client service that has extra peer information to be accessed without having to send requests.

Use LoadTrackedClient in initialize

9d2e163

Ensure that `PeerSet` receives `LoadTrackedClient`s so that it will be able to query the peer's protocol version later on.

Require LoadTrackedClient in PeerSet

9128b09

Replace the generic type with a concrete `LoadTrackedClient` so that we can query its version.

Create MinimumPeerVersion helper type

a298fa3

A type to track the current minimum protocol version for connected peers based on the current block height.

Use MinimumPeerVersion in handshakes

a84226b

Keep the code to obtain the current minimum peer protocol version in a central place.

Add a MinimumPeerVersion instance to PeerSet

4cf4f82

Prepare it to be able to disconnect from outdated peers based on the current minimum supported peer protocol version.

Disconnect from ready services for outdated peers

07f79a8

When the minimum peer protocol version is detected to have changed (because of a network upgrade), remove all ready services of peers that became outdated.

Cancel added unready services of outdated peers

cc597b7

Only add an unready service if it's for a peer that has a supported protocol version. Otherwise, add it but drop the cancel handle so that the `UnreadyService` can execute and detect that it was cancelled.

jvff and others added 14 commits December 9, 2021 00:54

Avoid adding ready services for outdated peers

3002239

If a service becomes ready but it's for a connection to an outdated peer, drop it.

Improve comment inside crawl_and_dial

cb0f2ee

Describe an edge case that is also handled but was not explicit. Co-authored-by: teor <teor@riseup.net>

Test if calculated minimum peer version is correct

d2a64d8

Given an arbitrary best chain tip height, check that the calculated minimum peer protocol version is the expected value.

Test if minimum version changes with chain tip

25b6989

Apply an arbitrary list of chain tip height updates and check that for each update the minimum peer version is calculated correctly.

Test minimum peer version changed reports

96597c3

Simulate a series of best chain tip height updates, and check for minimum peer version updates at least once between them. Changes should only be reported once.

Create a MockedClientHandle helper type

601cc65

Used to create and then track a mock `Client` instance.

Add MinimumPeerVersion::with_mock_chain_tip

a6c1fb8

An extension method useful for tests, that contains some shared boilerplate code.

Bias arbitrary Versions to be in valid range

399a7fc

Give a 50% chance for an arbitrary `Version` to be in the range of previously used values the Zcash network.

Create a PeerVersions helper type

08424c9

Helps with the creation of mocked client services with arbitrary protocol versions.

Create a PeerSetGuard helper type

1fd8247

An auxiliary type to a `PeerSet` instance created for testing. It keeps track of any dummy endpoints of channels created and passed to the `PeerSet` instance.

Create a PeerSetBuilder helper type

dacada2

Helps to reduce the code when preparing a `PeerSet` test instance.

Test if outdated peers are rejected by PeerSet

8a96a3a

Simulate a set of discovered peers being sent to the `PeerSet`. Ensure that only up-to-date peers are kept by the `PeerSet` and that outdated peers are dropped.

Create BlockHeightPairAcrossNetworkUpgrades type

4264bd1

A helper type that allows the creation of arbitrary block height pairs, where one value is before and the other is at or after the activation height of an arbitrary network upgrade.

Test if peers are dropped as they become outdated

b07a158

Simulate a network upgrade, and check that peers that become outdated are dropped by the `PeerSet`.

jvff force-pushed the disconnect-from-outdated-peers-on-network-upgrade branch from 51831c5 to cb0f2ee Compare December 9, 2021 00:57

jvff mentioned this pull request Dec 9, 2021

Test disconnection from outdated peers on network upgrade jvff/zebra#2

Merged

3 tasks

jvff marked this pull request as ready for review December 9, 2021 01:18

jvff requested a review from teor2345 December 9, 2021 01:19

teor2345 reviewed Dec 9, 2021

View reviewed changes

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

teor2345 reviewed Dec 9, 2021

View reviewed changes

zebra-network/src/peer_set/set/tests/prop.rs Outdated Show resolved Hide resolved

Remove dbg! macros

0de7402

teor2345 reviewed Dec 9, 2021

View reviewed changes

zebra-network/src/protocol/external/arbitrary.rs Show resolved Hide resolved

teor2345 previously approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'main' into disconnect-from-outdated-peers-on-network-up…

280943e

…grade

teor2345 dismissed their stale review via 280943e December 9, 2021 02:12

teor2345 enabled auto-merge (squash) December 9, 2021 02:12

teor2345 approved these changes Dec 9, 2021

View reviewed changes

teor2345 merged commit 0ad89f2 into ZcashFoundation:main Dec 9, 2021

jvff deleted the disconnect-from-outdated-peers-on-network-upgrade branch December 9, 2021 13:35

jvff mentioned this pull request Dec 9, 2021

Describe PeerSets behavior at a network upgrade's activation height #3181

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disconnect from outdated peers on network upgrade #3108

Disconnect from outdated peers on network upgrade #3108

jvff commented Nov 28, 2021 •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 commented Nov 29, 2021 •

edited

Loading

jvff commented Dec 1, 2021

teor2345 left a comment

teor2345 left a comment

teor2345 left a comment

teor2345 left a comment

Disconnect from outdated peers on network upgrade #3108

Disconnect from outdated peers on network upgrade #3108

Conversation

jvff commented Nov 28, 2021 • edited Loading

Motivation

Specifications

Solution

Review

Reviewer Checklist

Follow Up Work

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 commented Nov 29, 2021 • edited Loading

jvff commented Dec 1, 2021

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

jvff commented Nov 28, 2021 •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 commented Nov 29, 2021 •

edited

Loading