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

Gossip dynamic local listener ports to peers #2277

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 10, 2021

Motivation

Previously, Zebra would either:

  • gossip port 0, which is an invalid port for outbound connections, or
  • skip gossiping its own dynamically allocated listener port, because the value 0 is invalid.

These changes help test PRs #2273, #2275, and #2276.

API Reference

Binding with a port number of 0 will request that the OS assigns a port to this listener. The port allocated can be queried via the local_addr method.

https://docs.rs/tokio/1.6.1/tokio/net/struct.TcpListener.html#method.bind

Solution

  • Configure the address book with the actual listener port, not the configured one
  • Improve "no configured peers" warning
  • Test that dynamic and fixed ports propagate correctly to the address book

This fix changes the following parts of Zebra's startup order:

  • open the listener
    • do the wrong port check
  • use its actual address to initialize the TimestampCollector and AddressBook
  • (unchanged startup tasks)
  • create the listen guard with the previously opened listener
  • (more unchanged startup tasks)

Review

@jvff can review this change.

This is a low-priority change, but it's useful for local testing of PRs #2273, #2275, and #2276.

This PR is based on #2276, it should automatically rebase on main once that PR merges.

Reviewer Checklist

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

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-Low I-usability Zebra is hard to understand or use A-network Area: Network protocol updates or fixes labels Jun 10, 2021
@teor2345 teor2345 added this to the 2021 Sprint 11 - Zcon2 milestone Jun 10, 2021
@teor2345 teor2345 requested a review from jvff June 10, 2021 06:30
@teor2345 teor2345 self-assigned this Jun 10, 2021
@teor2345
Copy link
Contributor Author

I need to write a unit test that initializes zebra-network, then checks the port in the local listener address.

@mpguerra mpguerra removed this from the 2021 Sprint 11 - Zcon2 milestone Jun 14, 2021
@teor2345 teor2345 force-pushed the local-listener-fix branch 10 times, most recently from c644df2 to 4aa6b00 Compare June 21, 2021 03:39
@teor2345 teor2345 force-pushed the dynamic-local-listener-to-peers branch 2 times, most recently from c1e80dd to cead481 Compare June 21, 2021 04:19
@teor2345
Copy link
Contributor Author

There was a mainnet large blocks sync failure, but it might have been due to bad peers?
https://github.com/ZcashFoundation/zebra/pull/2277/checks?check_run_id=2871967768

I restarted all the CI jobs.

Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice refactor in the peer_set/initialize.rs 🎉

zebra-network/src/meta_addr/tests/prop.rs Show resolved Hide resolved
zebra-network/src/peer_set/initialize.rs Outdated Show resolved Hide resolved
zebra-network/src/peer_set/initialize.rs Outdated Show resolved Hide resolved
jvff
jvff previously approved these changes Jun 22, 2021
@teor2345
Copy link
Contributor Author

I need to write a unit test that initializes zebra-network, then checks the port in the local listener address.

Unfortunately I still need to write this test!

But it should be quick.

Base automatically changed from local-listener-fix to main June 22, 2021 02:17
teor2345 and others added 3 commits June 22, 2021 13:11
Previously, Zebra would either gossip port `0`, which is invalid, or skip
gossiping its own dynamically allocated listener port.
Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
@teor2345 teor2345 force-pushed the dynamic-local-listener-to-peers branch from dcb70da to 126e9d9 Compare June 22, 2021 03:13
@teor2345 teor2345 marked this pull request as ready for review June 22, 2021 03:13
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 I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants