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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions common/src/dns.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
//! Seed resolution.
use log::debug;
use std::net::{SocketAddr, ToSocketAddrs};

use crate::network::Network;

/// DNS feature
#[derive(Debug, Copy, Clone)]
pub enum SeedFeature {
/// x1, Full Node
NODE_NETWORK,
/// x5, all include in x1 plus Node network
NODE_BLOOM,
/// x9, all included in x1 plus Node Witness
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


impl Default for SeedFeature {
fn default() -> Self {
Self::NODE_NETWORK
}
}

impl SeedFeature {
pub fn as_code(&self) -> &'static str {
match self {
Self::NODE_NETWORK => &"x1",
Self::NODE_BLOOM => &"x5",
Self::NODE_WITNESS => &"x9",
Self::FULL_NODE => &"xd",
}
}
}

/// Seeds implementation
pub struct Seeds<'a> {
/// The network to work load the dns
network: Network,
/// The default port of the node
port: u16,
/// List of feature supported by nakamoto
features: &'a [SeedFeature],
}

impl<'a> Seeds<'a> {
/// Create a new seed with the requirements
pub fn new(network: Network, port: u16, features: &'a [SeedFeature]) -> Self {
Seeds {
network,
port,
features,
}
}

pub fn load(&self) -> Vec<SocketAddr> {
let seeds = self.from_network();
self.filter_by_feature(seeds)
}

fn from_network(&self) -> &[&str] {
match self.network {
Network::Mainnet => &[
"seed.bitcoin.sipa.be", // Pieter Wuille
"dnsseed.bluematt.me", // Matt Corallo
"dnsseed.bitcoin.dashjr.org", // Luke Dashjr
"seed.bitcoinstats.com", // Christian Decker
"seed.bitcoin.jonasschnelli.ch", // Jonas Schnelli
"seed.btc.petertodd.org", // Peter Todd
"seed.bitcoin.sprovoost.nl", // Sjors Provoost
"dnsseed.emzy.de", // Stephan Oeste
"seed.bitcoin.wiz.biz", // Jason Maurice
"seed.cloudhead.io", // Alexis Sellier
],
Network::Testnet => &[
"testnet-seed.bitcoin.jonasschnelli.ch",
"seed.tbtc.petertodd.org",
"seed.testnet.bitcoin.sprovoost.nl",
"testnet-seed.bluematt.me",
],
Network::Regtest => &[], // No seeds
}
}

fn filter_by_feature(&self, seed_list: &[&str]) -> Vec<SocketAddr> {
let mut seeds_addr = Vec::new();
for feature in self.features {
let feature_code = feature.as_code();
debug!("Filtering by feature {}", feature_code);

for seed in seed_list {
let sub_seed = format!("{}.{}:{}", feature_code, seed, self.port);
debug!("Sub seed {} checking ...", sub_seed);
if let Ok(hosts) = (*sub_seed).to_socket_addrs() {
for host in hosts {
seeds_addr.push(host);
}
} else {
// we admit the failure at this point because not all the seed
// support all the feature.
debug!("Seed {} doesn't support feature {}", seed, feature_code);
}
}
}
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

2 changes: 1 addition & 1 deletion common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
#![deny(missing_docs, unsafe_code)]
pub mod block;
pub mod collections;
mod dns;
pub mod network;
pub mod p2p;

pub use nonempty;

/// Return the function path at the current source location.
Expand Down
1 change: 1 addition & 0 deletions common/src/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use bitcoin_hashes::hex::FromHex;
use bitcoin_hashes::sha256d;

use crate::block::Height;
use crate::dns;

/// Peer services supported by nakamoto.
#[derive(Debug, Copy, Clone)]
Expand Down
2 changes: 2 additions & 0 deletions common/src/p2p/peer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ pub trait Store {
seeds: impl Iterator<Item = S>,
source: Source,
) -> io::Result<()> {
// TODO(vincenzopalazzo): move this logic inside the dns module in comm
// and use this method to add only the list of the socket addrs.
let mut error = None;
let mut success = false;

Expand Down