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

Filtering DNS seed by features #54

Conversation

vincenzopalazzo
Copy link
Collaborator

Fixes #50

Working in progress

  • Refactoring all the logic inside a dns module (a better name can be seeds module?)
  • Check Who support what
  • Unit testing
  • Put inside the node client

@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/filter_dns branch from 7496441 to 0a17fab Compare September 28, 2021 13:50
@cloudhead cloudhead changed the title Filetering DNS seed by features Filtering DNS seed by features Sep 28, 2021
NODE_WITNESS,
/// xd (aka x13), all included in x9 and x5
FULL_NODE,
}
Copy link
Owner

Choose a reason for hiding this comment

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

How about struct SeedServices { flags: bitcoin::ServiceFlags }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think, much better, thank to point me to this feature of rust-bitcoin

}
seeds_addr
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

For PRs I tend to prefer as minimal a diff as possible, as it makes it easier for me to review. Eg. I don't want to have to re-review my own code. So I'd suggest not moving existing code around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for clarifying this @cloudhead, in other words, do you want that I put the check of sub-seed in the nakamoto_comm::network?

I usually maintain the code struct of the repository, and I'm sorry if I move your code out of your class, but I did that because I think have a DNS seed module makes easy future change.

BTW, if you want I can revert the change

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for clarifying this @cloudhead, in other words, do you want that I put the check of sub-seed in the nakamoto_comm::network?

Yes, whatever results in the smallest diff.

I usually maintain the code struct of the repository, and I'm sorry if I move your code out of your class, but I did that because I think have a DNS seed module makes easy future change.

BTW, if you want I can revert the change

Yeah, that would be better for now. It makes my job easier to review code when less of it changes :)
It's okay if doing that means that some code is not in the optimal place. This is something that can be changed later on.

Generally, I try to not mix code refactors with change in functionality, as you have to then review two things at once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally, I try to not mix code refactors with change in functionality, as you have to then review two things at once.

I agree with your point of view, thanks. I will change this PR, or close it and open a new one with the change in the nakamoto_comm::network, thanks

@vincenzopalazzo vincenzopalazzo force-pushed the vincenzopalazzo/filter_dns branch from 0a17fab to 48f2845 Compare October 3, 2021 11:15
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Oct 3, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
Copy link
Collaborator Author

Closing in favor of #59

vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Oct 8, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Oct 10, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Oct 19, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Nov 2, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Nov 2, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Nov 8, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Nov 15, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Nov 27, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Dec 4, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
vincenzopalazzo added a commit to vincenzopalazzo/nakamoto that referenced this pull request Dec 6, 2021
This include a rework of the PR cloudhead#54, but not include the refactoring in other module.

Signed-off-by: Vincenzo Palazzo <[email protected]>
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.

Support service bit filtering
2 participants