-
Notifications
You must be signed in to change notification settings - Fork 19
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
Use signet seeds to return utreexo nodes #77
Conversation
connmgr/seed.go
Outdated
@@ -41,7 +41,7 @@ func SeedFromDNS(chainParams *chaincfg.Params, reqServices wire.ServiceFlag, | |||
host = fmt.Sprintf("x%x.%s", uint64(reqServices), dnsseed.Host) | |||
} | |||
|
|||
go func(host string) { | |||
go func(host string, services wire.ServiceFlag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write up why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding this to tell which services are supported by each peer, so we can connect with utreexo peers right away. But it seems connmgr isn't using it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right since the Services
field is for alll the services that the address supports.
Lines 69 to 70 in affee16
func NewNetAddressTimestamp( | |
timestamp time.Time, services ServiceFlag, ip net.IP, port uint16) *NetAddress { |
If we add reqServices
to it, it's not entirely correct since it's not all the supported services that the address has. The services are set during the version negotiation in here: https://github.com/btcsuite/btcd/blob/80f5a0ffdf363cfff27d550f9e38aa262667a7f1/server.go#L477-L479
Since the services are set elsewhere, the change made here isn't really going to do much and I'd prefer to leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal here was to tell the daemon to connect with utreexo peers before any other. It's not like we won't set this up on handshake, but if the dnsseed
told this peer has a given flag, it's almost certain that it have. Therefore, we can use this as a hint to open utreexo connections at startup, without having to try many peers at random. But since openconn
don't use this, it makes no sense.
Is it reasonable to change the connection logic to try utreexo peers before non-utreexo peers if -utreexo
is set? I did this on Floresta, and it opens an utreexo
connection right at startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it reasonable to change the connection logic to try utreexo peers before non-utreexo peers if
-utreexo
is set?
Yeah that's reasonable.
My goal here was to tell the daemon to connect with utreexo peers before any other.
I'm not quite sure if that's what this code is achieving. Could you let me know where I should look to verify that passing in the services flag will make the node try a utreexo node first? (I'm not familiar with the peer connection logic here so you may be totally right)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding this to tell which services are supported by each peer, so we can connect with utreexo peers right away. But it seems connmgr isn't using it at all?
Took a look at the code and you're right, connmgr isn't really differentiating between nodes based on their services. Seems like it's something we can address later on but would it be ok to leave it out of this PR? I prefer if PRs achieved one singular goal and this PR's goal is to add an utreexo dns seed, not to change the connmgr logic
server.go
Outdated
@@ -2314,8 +2314,12 @@ func (s *server) peerHandler() { | |||
} | |||
|
|||
if !cfg.DisableDNSSeed { | |||
requiredFeatures := wire.SFNodeNetwork | wire.SFNodeWitness | |||
if cfg.Utreexo { | |||
requiredFeatures = wire.SFNodeUtreexo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. It'd be dropping SFNodeNetwork
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Fixed in 0772c48
e78af66
to
0772c48
Compare
0772c48
to
9277913
Compare
9277913
to
8538e08
Compare
DNS seeds allows you to filter addresses by features. This PR asks for utreexo peers (service bit 1 << 24 is set) if -utreexo is enabled. This helps to find bridge nodes for the first IBD. Also add seed.dlsouza.lol that supports this feature.
Weird, GitHub closed this automatically. Narrowed it down to only the dnsseed utreexo part. |
DNS seeds allows you to filter addresses by features. This PR asks for utreexo peers (service bit 1 << 24 is set) if
-utreexo
is enabled. This helps to find bridge nodes for the first IBD.Also add
seed.dlsouza.lol
that supports this feature.