Skip to content
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

Yaml version of mainnet config #3269

Merged
merged 2 commits into from Oct 12, 2021
Merged

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Oct 5, 2021

Introduces a YAML version of the Mainnet configuration file that is better documented than the JSON version. This includes a test to ensure the YAML and JSON versions agree.

To see how the configuration file used by mainnet currently compares to how it was back in the Byron era, see this diff: b9287a9

@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch 2 times, most recently from 6d060c4 to b9287a9 Compare October 5, 2021 11:46
@newhoggy newhoggy marked this pull request as ready for review October 5, 2021 11:47
@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch from b9287a9 to ec6ad20 Compare October 5, 2021 11:55
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include in the commit message why there are code changes in the second commit and not just comments in a config file.

@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch from ec6ad20 to 791f49e Compare October 5, 2021 12:08
@newhoggy
Copy link
Contributor Author

newhoggy commented Oct 5, 2021

Can you include in the commit message why there are code changes in the second commit and not just comments in a config file.

Done!

@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch from 791f49e to 30516b7 Compare October 5, 2021 12:09
@Jimbo4350
Copy link
Contributor

There is a windows failure in CI still.

@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch 2 times, most recently from 9464e40 to eff9e3c Compare October 8, 2021 09:33
@newhoggy newhoggy requested a review from dcoutts October 8, 2021 10:11
@@ -167,7 +167,8 @@ test-suite cardano-cli-test
main-is: cardano-cli-test.hs
type: exitcode-stdio-1.0

build-depends: bech32 >= 1.1.0
build-depends: aeson
, bech32 >= 1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random question, is the bech32 version bounds necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the background for why there is a lower bound here, but it is unchanged from before.

projectBase <- H.note =<< H.evalIO . IO.canonicalizePath =<< H.getProjectBase
result <- H.evalIO $ runExceptT $ initialLedgerState $ projectBase </> "configuration/cardano/mainnet-config.json"
case result of
Right (_, _) -> return ()
Left e -> H.failWithCustom GHC.callStack Nothing (T.unpack (renderInitialLedgerStateError e))

hprop_configMainnetYaml :: Property
hprop_configMainnetYaml = H.propertyOnce $ do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're testing the equivalence of yaml and aeson decoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a YAML config file that contains helpful comments and a JSON config file which is generated by nix. They are meant to have the same config, so the test ensures they never diverge.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

##### Update system parameters #####

# This protocol version number gets used by block producing nodes as part
# of the system for agreeing on and synchronising protocol updates.
LastKnownBlockVersion-Major: 0
LastKnownBlockVersion-Minor: 2
LastKnownBlockVersion-Major: 3
Copy link
Contributor

@Jimbo4350 Jimbo4350 Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to link to: https://github.com/input-output-hk/cardano-node/blob/master/cardano-node/src/Cardano/Node/Protocol/Cardano.hs#L199 for the maximum protocol version. I'm not sure where the max block version exists though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@newhoggy newhoggy force-pushed the yaml-version-of-mainnet-config branch from eff9e3c to ae0d335 Compare October 12, 2021 08:15
@newhoggy
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 12, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 36e886a into master Oct 12, 2021
@iohk-bors iohk-bors bot deleted the yaml-version-of-mainnet-config branch October 12, 2021 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants