-
Notifications
You must be signed in to change notification settings - Fork 751
cardano-testnet executable: take node config file as optional input #6103
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
Conversation
3894926 to
8e37423
Compare
Jimbo4350
left a comment
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.
How do you plan to deal with conflicts in the node configuration yaml file? I think this is the wrong approach. We don't want to be forced to reconcile configuration file differences. All we should need the is the node configuration yaml file.
63c60a5 to
6ca79c5
Compare
|
For future reference, if we leave the call to > cabal run cardano-testnet -- cardano --output-dir cl-output-dir --node-config cl-example/configuration.json --testnet-magic 44
✗ <interactive> failed at src/Testnet/Start/Cardano.hs:283:3
after 1 test.
shrink path: 1:
forAll0 =
Process exited with non-zero exit-code: 1
━━━━ stdout ━━━━
Checking byron genesis file: cl-example/byron.genesis.spec.json
━━━━ stderr ━━━━
Error reading node configuration at: cl-example/byron.genesis.spec.json: Incorrect schema for GenesisData.
Error: expected field protocolConstsIt seems the code is requiring too much data here when deserializing the Byron genesis. This data is actually not required to start the node [edit] Oh well, it seems the node doesn't like it either actually: ✗ <interactive> failed at src/Testnet/Start/Cardano.hs:342:5
after 1 test.
shrink path: 1:
forAll0 =
Cardano node process did not start: CardanoProtocolInstantiationError (CardanoProtocolInstantiationErrorByron (GenesisReadError "/home/churlin/dev/cardano-node/cl-example/byron.genesis.spec.json" (GenesisDataSchemaError (SchemaError {seExpected = "field protocolConsts", seActual = Nothing}))))
cardano-node: There was an error parsing the genesis file: "/home/churlin/dev/cardano-node/cl-example/byron.genesis.spec.json" Error: GenesisDataSchemaError (SchemaError {seExpected = "field protocolConsts", seActual = Nothing}) |
The node itself, when it starts, doesn't require hashes of genesis files. So it seems overkill to require it to start a testnet. See also #6103 (comment)
6ca79c5 to
d67ff61
Compare
The node itself, when it starts, doesn't require hashes of genesis files. So it seems overkill to require it to start a testnet. See also #6103 (comment)
d67ff61 to
c78c34f
Compare
The node itself, when it starts, doesn't require hashes of genesis files. So it seems overkill to require it to start a testnet. See also #6103 (comment)
c78c34f to
1f7c3d8
Compare
The node itself, when it starts, doesn't require hashes of genesis files. So it seems overkill to require it to start a testnet. See also #6103 (comment)
18c0d3e to
50849e1
Compare
The node itself, when it starts, doesn't require hashes of genesis files. So it seems overkill to require it to start a testnet. See also #6103 (comment)
8117ffc to
3671f83
Compare
@Jimbo4350> Updated version of this PR now does that 👍 |
… node configuration file (it was unused) This will be done differently in #6103
… node configuration file (it was unused) This will be done differently in #6103
c25acc3 to
6dc7fb4
Compare
Jimbo4350
left a comment
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 is an improvement 👍 . Have a look at my comments.
| <> OA.showDefault | ||
| <> OA.value 1) | ||
| , UserNodeOptions | ||
| <$> strOption ( long "node-config" |
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.
So this is an improvement however the resultant type should be [TestnetNodeOptions]. Users (or QA) may want to for example create forking conditions whereby nodes have different configurations. This will have to be done in another PR at some point.
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.
Yes I agree, but as you say, this ought to be done later. This PR does a lot of changes already.
| Nothing -> error $ "Genesis files not specified in node configuration file: " <> nodeInputConfigFile | ||
| Just x -> x | ||
| adjustedProtocolConfig = | ||
| -- Make all the files be relative to the location of the config file. |
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.
👍
cardano-testnet/src/Parsers/Run.hs
Outdated
| alonzoGenesisFile | ||
| conwayGenesisFile | ||
| where | ||
| getShelleyGenesises (NodeProtocolConfigurationCardano _byron shelley alonzo conway _hardForkConfig) = |
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.
We also want to confirm the validity of the byron genesis but that can be a separate PR in the future.
| where | ||
| nSpoNodes = | ||
| case cardanoNodes of | ||
| UserNodeOptions _ -> 1 |
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.
Hardcoding this isn't great. This is related to my comment above regarding [TestnetNodeOptions]
| -- to run a testnet. "create-staked" is not a good way to do this especially because it | ||
| -- makes assumptions about where things should go and where genesis template files should be. | ||
| -- See all of the ad hoc file creation/renaming/dir creation etc below. | ||
| H.failMessage GHC.callStack "Specifying node configuration files per node not supported yet." |
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.
We need to note that we want to support configuration files per node. However it's not urgent to do this now.
| cardanoNumRelays CardanoTestnetOptions{cardanoNodes} = | ||
| NumRelays . length $ filter isRelayNodeOptions cardanoNodes | ||
| cardanoNumRelays CardanoTestnetOptions{cardanoNodes=_} = | ||
| undefined |
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.
?
| => HasCallStack | ||
| => CardanoTestnetOptions -- ^ The options to use | ||
| -> Conf | ||
| -> UserNodeConfig -- ^ The node configuration file to use. If omitted it's generated. |
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 is an indication that TestnetNodeOptions ought to be renamed to TestnetNodeConfiguration.
A type like the following would be more indicative:
data TestnetNodeConfiguration
= UserSpecifiedConfiguration FilePath
| DefaultConfiguration [AutomaticNodeOption]
However what we really want is:
data TestnetNodeConfiguration
= UserSpecifiedConfiguration FilePath
| DefaultSpoConfiguration [String]
| DefaultRelayConfiguration [String]
This gives users the most flexibility in that they can spin up a testnet with a mixture of nodes. Haddocks are desperately needed to indicate that [String] types are command line executable modifiers.
ad8dbd6 to
70c0e00
Compare
1b6c518 to
c4aba77
Compare
70c0e00 to
9cd0ae0
Compare
c4aba77 to
4d424b6
Compare
9cd0ae0 to
72ea5dc
Compare
4d424b6 to
fa3a62e
Compare
89d6e24 to
d922fb3
Compare
d922fb3 to
47f3da4
Compare
47f3da4 to
2d20baf
Compare
| adjustFilePaths (takeDirectory nodeInputConfigFile </>) protocolConfig | ||
| (shelley, alonzo, conway) = getShelleyGenesises adjustedProtocolConfig | ||
| -- The paths to the genesis files have to exist in the node configuration file, | ||
| -- but the files themselves don't have to exist. So if the user wants to pass |
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.
If the user wants to call cardanoTestnet and mix and match their genesis files then fine but this should not be possible at the executable level. If all the genesis files do not exist in the user provided configuration we fail hard. We do not provide them with our defaults. They only use our defaults if they provide no configuration.
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.
but this should not be possible at the executable level.
Why not? I'm not sure there's much value in requiring all configuration files from the user.
Let's continue discussion here: #6137 (comment)
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.
@Jimbo4350> design you prefer implemented here: #6148
Approve the one you want merged!
|
Closed in favor of #6148 |
Description
Fixes #6069
Fixes #6137
This PR makes it possible for callers of the
cardano-testnetbinary to pass a custom node configuration file, as well as custom genesis files. To do so, a new--node-configflag is added to thecardano-testnetexecutable:As explained above, this flag also allows to pass custom genesis files.
How to trust this PR
Execute the new test
The new behavior is tested in the new test CommandLineExecutable.hs which you can execute locally as follows:
Note how the test finishes in 60 seconds, which is a duration chosen in
CommandLineExecutable.hs, during which we check that nodes have been produced.Execute the
cardano-testnetbinary directlyYou can also run the
cardano-testnetbinary directly as follows (here using a custom node configuration file and a custom alonzo and conway genesis, but leaving the shelley genesis (the hardest to tweak) to be autogenerated:At this point the testnet is running and you can kill this process using⚠️ This will need to be fixed soon as proposed below. Also we will want to unplug ourselves from the test monad in this scenario and this is already tracked here: #6122
CTRL-C, but it doesn't kill thecardano-nodeprocess.In the meantime, users can either do
killall cardano-nodeto clean up or use a combination ofpidof cardano-nodeandkill. I didn't want to do in this PR, as this one is quite big already.Follow-up issues to create
@Jimbo4350> please tick them if you agree 🙂 I'll create them
pTestnetNodeOptionsreturn a list ofTestnetNodeOptionandTestnetNodeOptionbeUserProvidedNodeOptions Filepath | SpoNodeOptions [Srting] | RelayNodeOptions [ String]. In other words merge the typesTestnetNodeOptionsandAutomaticNodeOption. This will generalize the behavior so that the user can create multiple nodes, no matter whether they are using custom options or using default behavior (SpoorRelay). This will require a bit of generalization in Cardano.hs but not much.cardanoTestnet: if the user passes a custom node configuration file and a custom genesis file (as a Haskell value right now), we should check that the genesis file pointed to by the node configuration file (if any) contains the same Haskell value as the custom genesis file.cardanoTestnetto takeFilepathfor genesis files instead of Haskell values, maybe?runTestnet: don't tie it with H.Integration #6122Checklist