-
Notifications
You must be signed in to change notification settings - Fork 242
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
chore: add fleet ENRs to the list of discv5 nodes #5184
Conversation
Jenkins BuildsClick to see older builds (12)
|
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.
LGTM
apart from a Minor comment
WakuNodes: { | ||
"enrtree://AI4W5N5IFEUIHF5LESUAOSMV6TKWF2MB6GU2YK7PU4TYUGUNOCEPW@boot.staging.shards.nodes.status.im", | ||
}, | ||
DiscV5BootstrapNodes: { |
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.
Why not use the config from config/cli/fleets-<name>.json
rather than again hardcoding in the code?
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.
it's a good question. These .json files seem to be used only when running status-go as a service. I imagine we could refactor status-go so the default values come from the config files instead of having them hardcoded.
wdyt, @cammellos ?
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 mind myself, I don't see the value of having them in JSON files to be honest if they are only used in a single place, this looks more explicit to me and doesn't require a go generate
(not sure if that's the case with the JSON files), but no strong opinion on my side
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.
would you say it's better to get rid of the json files instead?
* chore_: add fleet ENRs to the list of discv5 nodes * fix_: handle scenario in which there are dnsDiscoveryURLs and enrs in the DiscV5Bootnode list
I noticed that we were using the same nodes/dnsdiscovery URLs for both the
WakuNodes
andDiscV5Bootstrap
nodes when setting up the default config. While that's okay, for discv5 i also suggest adding the ENRs, for the scenarios in which dnsdiscovery fails for some reason.With this change, discv5 will use the fixed list of enrs, and eventually, once we get a response from dnsDiscovery, discv5 will be reseeded with both the static bootnodes from the configuration and the new ones obtained from dnsDiscovery. (This is to handle cases in which the list of enrs change).