[nild] Argument --chain adds test/main-net defaults #813
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a --chain argument to the nild command to allow chain-specific default configurations for archive and network nodes, while adding validations in generating devnet clusters.
- Added chain-specific default settings and validations in defaults.go and defaults_test.go
- Extended configuration loading and devnet generation with relay validations and the new chain flag support
- Updated helper functions in internal network and cobrax packages to support chain-related parameters
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| nil/services/nilservice/defaults_test.go | Added tests for chain-specific default validations |
| nil/services/nilservice/defaults.go | Introduced default configurations for testnet and mainnet |
| nil/internal/network/addr_info.go | Added function for constructing AddrInfoSlice from strings |
| nil/internal/collate/syncer.go | Added NodeVersion.String method for debug/log output |
| nil/internal/cobrax/config.go | Added chain flag and getter functions |
| nil/cmd/nild/main.go | Integrated chain flag in configuration loading |
| nil/cmd/nild/devnet.go | Updated devnet generation with relay validation and chain-specific config |
| ChainNameMainnet = "mainnet" | ||
| ) | ||
|
|
||
| type ChainDefaults struct { |
There was a problem hiding this comment.
I don't really understand how we're going to maintain this. As far as I understand it, in devnet (where is it by the way? We don't have testnet or mainnet right now, and they are the only ones represented here) we drop the configuration from time to time and validator identities can change. In that sense, I don't think a static hardcode is going to help us. Plus there's a chance of getting a chicken and egg problem when we're preparing a release in which we plan to deploy a new configuration, and we don't yet know the identifiers that we want to zip into the nild version we'll be releasing.
Why don't we put a method in the JSON-RPC API that will output actual validator addresses that can be used as bootstrapPeers? Then we could make a command that takes an RPC URL, goes to it, and gets the actual values. This would work for any deployments.
There was a problem hiding this comment.
We'll have to hard-code the url then. And also somehow guarantee that the service was not compromised.
libp2p guarantees identity verification and also allows to connect to the network without any mediators like RPC.
Here we have just a short list of peers that will be used by the bootstrap. I reduced it to the single archive node in a follow-up push (I don't think that we want to open validator nodes right away).
We won't use this on dev, I suppose.
And on prod we're not supposed to clear network keys a lot. (I doubt that we clear them regularly on dev either.)
I'm using nil-dev for tests now.
I'll leave devnet (or testnet?) probably in the end if that's how our chain is known to the public.
There was a problem hiding this comment.
First, let's synchronize in terminology. As I understand it,
mainnet — not present
testnet — not present
devnet == nil-prod
🤷♂️ == nil-dev
Do you understand these terms in the same way?
How justified is it to worry about JSON-RPC being compromised? We have HTTPS there. If we assume the user will download nild from our site and trust it, why shouldn't they trust another HTTPS request?
There was a problem hiding this comment.
I've updated the chain names.
Concerning the RPC, let's discuss on Thursday.
Also, we can always change this solution to RPC later.
|
Will be implemented in a different fashion |
Short Summary
An archive node outside of the cluster will be run simply with
nild archive --chain devnet.The PR is a draft until the relay is up.
What Changes Were Made
--chaininnildand default config depending on it.--chain test.nild gen-configswith--chain. E.g., each bootstrap identity and each relay from the defaults must be present in the cluster configs.Related Issue
Fixes #786
Future Tasks
More defaults, including zero state.