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

Async DNS seeder lookups #1662

Merged
merged 9 commits into from
Feb 3, 2021
Merged

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jan 31, 2021

Motivation

I am trying to replace to_socket_addrs() with my crated function resolve(). This is work in progress for #1613.

So i basically will want to replace https://github.com/oxarbitrage/zebra/blob/8a9a32b2f4f3e1e735def4ade717884885184243/zebra-network/src/config.rs#L46 with https://github.com/oxarbitrage/zebra/blob/8a9a32b2f4f3e1e735def4ade717884885184243/zebra-network/src/config.rs#L42 where replace is a new custom function that should return the same data.

to_socket_addrs() returns Result<Iterator> but replace() returns Option<Iterator> which are not the same. This can be a first issue.

Additionally i think there is something else, according to some hints i got from @teor2345 in discord something like this will need to be added to the combination:

.collect::<futures::stream::FuturesOrdered<_>>()

Which makes the implementation even more complicated for my skills. Looking for some additional hints to make more progress here.

Solution

Share the problem with others.

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

I will like @yaahc or @teor2345 to take a look.

Related Issues

Closes #1613

Note: #1613 is about DNS seeders, #1631 is about listener ports. This PR fixes the DNS seeders.

Follow Up Work

None by now.

@teor2345 teor2345 self-requested a review February 1, 2021 02:08
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Hi @oxarbitrage I've made some suggestions to help move you forward.

I've also tagged @yaahc in some questions that she's better able to answer.

zebra-network/src/peer_set/initialize.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Outdated Show resolved Hide resolved
zebra-network/src/config.rs Show resolved Hide resolved
@teor2345 teor2345 changed the title [WIP] Async DNS lookups [WIP] Async DNS seeder lookups Feb 1, 2021
@oxarbitrage oxarbitrage marked this pull request as ready for review February 1, 2021 23:13
@oxarbitrage oxarbitrage changed the title [WIP] Async DNS seeder lookups Async DNS seeder lookups Feb 2, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm good with this now, but I'd like @oxarbitrage to check the changes I just made.

Copy link
Contributor Author

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Changes looks good to me.

@teor2345 teor2345 merged commit 221512c into ZcashFoundation:main Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a timeout to DNS seeder requests to avoid hangs
3 participants