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

Perform listener DNS lookups asynchronously, with a timeout #1631

Closed
teor2345 opened this issue Jan 24, 2021 · 2 comments
Closed

Perform listener DNS lookups asynchronously, with a timeout #1631

teor2345 opened this issue Jan 24, 2021 · 2 comments
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jan 24, 2021

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

Zebra uses serde to convert its listener addresses into SocketAddrs. Serde uses Rust's standard to_socket_addrs function to convert strings to IP addresses. If the strings are hostnames, to_socket_addrs uses DNS for the lookup.

If a user configures a DNS name as one of their listener addresses, and DNS takes a long time to resolve, Zebra can hang when loading the config. Since these DNS calls are blocking, and tokio uses cooperative multitasking for async code, this can cause arbitrary parts of Zebra to hang.

Describe the solution you'd like

Zebra should declare all its configured addresses as strings, then convert them using tokio::net::lookup_host().await?:
https://docs.rs/tokio/1.1.0/tokio/net/fn.lookup_host.html

lookup_host correctly wraps the DNS call in spawn_blocking, which avoids blocking other tasks.

We should also apply a timeout of at least 5 seconds to each DNS lookup:
https://tools.ietf.org/rfcmarkup?doc=1123#section-6.1.3.3

Describe alternatives you've considered

We could do nothing, leading to a poor user experience for users that use dynamic DNS and similar protocols.

We could change the function that serde uses to deserialise, but I'm not sure how serde interacts with async code.

Additional context

We want to do a similar fix for the DNS seeder addresses in PR #1662.
Also see this comment: https://github.com/ZcashFoundation/zebra/pull/1535/files#r563402763

This task is a lower priority, because most users use IP addresses for listener ports.

@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 labels Jan 24, 2021
@teor2345 teor2345 added P-High and removed S-needs-triage Status: A bug report needs triage labels Jan 28, 2021
@teor2345 teor2345 added P-Low and removed P-High labels Feb 1, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Feb 1, 2021

This ticket is about listener ports, most people don't use DNS to configure them.

#1613 is about initial peers, which use DNS seeders by default.

teor2345 added a commit to teor2345/zebra that referenced this issue Feb 19, 2021
We disabled these tests pending ZcashFoundation#1613. But the comment incorrectly said
we were waiting for ZcashFoundation#1631.
teor2345 added a commit to teor2345/zebra that referenced this issue Feb 19, 2021
We disabled these tests pending ZcashFoundation#1613. But the comment incorrectly said
we were waiting for ZcashFoundation#1631.
teor2345 added a commit to teor2345/zebra that referenced this issue Feb 19, 2021
We disabled these tests pending ZcashFoundation#1613. But the comment incorrectly said
we were waiting for ZcashFoundation#1631.
teor2345 added a commit that referenced this issue Feb 19, 2021
We disabled these tests pending #1613. But the comment incorrectly said
we were waiting for #1631.
@teor2345
Copy link
Contributor Author

This doesn't seem to be an issue in practice, we can fix it if it becomes one.

@mpguerra mpguerra closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
mpguerra added a commit that referenced this issue May 19, 2023
mergify bot pushed a commit that referenced this issue May 23, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
None yet
Development

No branches or pull requests

2 participants