-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Configs] Add seed peers for testnet and mainnet. #9801
Conversation
const MAINNET_SEED_PEER_PUBLIC_KEYS: [&str; 0] = []; | ||
|
||
// Testnet seed peer addresses and keys | ||
const TESTNET_SEED_PEER_ADDRESSES: [&str; 4] = [ |
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 wish there was either a loud comment "these three are one thing and must be the same length and correspond by index", or (really this) I wish it was one list of something with three elements. maybe [[&str; 3]; 4] or vec![] of (str, str, str) tuples?
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.
SGTM! I've gone for an array of tuples of (str, str, str) 😄
// Mainnet seed peer addresses and keys | ||
const MAINNET_SEED_PEER_ADDRESSES: [&str; 0] = []; | ||
const MAINNET_SEED_PEER_NETWORK_ADDRESSES: [&str; 0] = []; | ||
const MAINNET_SEED_PEER_PUBLIC_KEYS: [&str; 0] = []; |
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 would be better to use a more specific type that
- Ties together these 3 properties
- Parses the strings into something more appropriate, like a bignum
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.
- Ties together these 3 properties
See above. We now have an array of tuples, which makes it more obvious 😄
- Parses the strings into something more appropriate, like a bignum
Can you elaborate? I'm not sure what you mean 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.
The address is not a string, it's a large integer, so it should be parsed as such. The string is only its printed representation.
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.
Aah, gotcha 😄
- The peer address here is a hex formatted string. When we build the seed from it, we use
PeerId::from_hex(...)
, which appears to be the standardized way of handling addresses across our code. Likewise, it's the format expected by the serde implementation for the object (e.g., if you were to define these in the node config manually). - I also think it's much more readable than storing a large number in the code. For example, I can directly look it up in our explorer 😄
So, I'm inclined to leave this as it is. I think it's superior to a large number 🤔
f295924
to
c4f3c05
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This PR adds dedicated seed peer support for testnet and mainnet. This is useful for nodes that wish to sync to the network with only the genesis blob.
Test Plan
Existing test infrastructure.