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

Separate Alonzo genesis file #2743

Merged
merged 6 commits into from
May 26, 2021
Merged

Conversation

newhoggy
Copy link
Contributor

No description provided.

@newhoggy newhoggy force-pushed the newhoggy/separate-alonzo-genesis-file branch 2 times, most recently from 75875d2 to 3d1c320 Compare May 25, 2021 05:23
@newhoggy newhoggy force-pushed the newhoggy/separate-alonzo-genesis-file branch from 3d1c320 to 6efb28c Compare May 25, 2021 06:35
@@ -8,6 +8,7 @@

ByronGenesisFile: byron/genesis.json
ShelleyGenesisFile: shelley/genesis.json
AlonzoGenesisFile: alonzo/genesis.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New entry in the configuration.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

If we include a new genesis file shouldn't we also include a hash in that file?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can but it's not mandatory.

@newhoggy newhoggy requested a review from erikd May 25, 2021 06:58
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.

Looking good! We need to incorporate the Alonzo genesis hash check similarly to:
https://github.com/input-output-hk/cardano-node/blob/ad76ebe63e138f5bdaa91117f7583b145b687b85/cardano-node/src/Cardano/Node/Protocol/Shelley.hs#L107

And then update the mainnet configuration files in the node.

cardano-node/src/Cardano/Node/Configuration/POM.hs Outdated Show resolved Hide resolved
scripts/byron-to-alonzo/mkfiles.sh Outdated Show resolved Hide resolved
scripts/byron-to-alonzo/mkfiles.sh Outdated Show resolved Hide resolved
@@ -560,10 +590,6 @@ if [ "$1" = "alonzo" ]; then
sed -i ${ROOT}/configuration.yaml \
-e 's/LastKnownBlockVersion-Major: 1/LastKnownBlockVersion-Major: 5/'

# Update shelley genesis with required Alonzo fields.
alonzogenesisparams='.+ {adaPerUTxOWord: 0, executionPrices: {prMem: 1, prSteps: 1}, maxTxExUnits: {exUnitsMem: 1, exUnitsSteps: 1}, maxBlockExUnits: {exUnitsMem: 1, exUnitsSteps: 1}, maxValueSize: 1000, collateralPercentage: 100, maxCollateralInputs: 1}'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@newhoggy newhoggy force-pushed the newhoggy/separate-alonzo-genesis-file branch 2 times, most recently from c988b80 to e61ca38 Compare May 26, 2021 00:30
, "maxValueSize" .= Aeson.toJSON @Int 1000
, "collateralPercentage" .= Aeson.toJSON @Int 100
, "maxCollateralInputs" .= Aeson.toJSON @Int 1
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cardano-cli genesis create now creates <rootdir>/genesis.alonzo.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

should create-staked also do that?

@Jimbo4350 Jimbo4350 self-requested a review May 26, 2021 07:15
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.

All the comments below are now outdated as I've resolved them all.

Comment on lines 764 to 768
data AlonzoGenesisError
= AlonzoGenesisReadError !FilePath !Text
| AlonzoGenesisHashMismatch !GenesisHashAlonzo !GenesisHashAlonzo -- actual, expected
| AlonzoGenesisDecodeError !FilePath !Text
deriving Show
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a new type and all this duplication? Can't we just change the existing ShelleyGenesisError to be just GenesisError and cover both eras? Even the error messages are essentially the same.

Comment on lines 247 to 250
primary <- v .:? "AlonzoGenesisFile"
npcAlonzoGenesisFile <- case primary of
Just g -> return g
Nothing -> fail $ "Missing required field: AlonzoGenesisFile"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be simplified to just this?

npcAlonzoGenesisFile <- v .: "AlonzoGenesisFile"

Comment on lines 119 to 120
-- We choose to include the Alonzo relevant fields in the Shelley genesis
-- and therefore avoid creating a separate Alonzo genesis file
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment has to be updated.

alonzoGenesis <- firstExceptT CardanoProtocolInstantiationErrorAlonzo
$ readAlonzoGenesis shelleyGenFile
$ readAlonzoGenesis alonzoGenFile
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be changed to be more like Shelley.readGenesis and take the optional hash, and check it.

We should either:

  1. essentially copy and paste Shelley.readGenesis over the existing readAlonzoGenesis but with the necessary adaptations for the Alonzo-specific errors; or
  2. generalising the type of Shelley.readGenesis so that instead of using ShelleyGenesis StandardShelley it uses genesis for any FromJSON genesis =>. The errors would need to be separated. Specifically the first three constructors from ShelleyProtocolInstantiationError would need to be moved into a separate ReadGenesisError type. Then we would delete readAlonzoGenesis and use the Shelley.readGenesis in both places but at different types.

2 is preferable of course.

@@ -366,6 +366,26 @@ runGenesisCreate (GenesisDir rootdir)
(Lovelace 10) (Lovelace 1, Lovelace 1) (1,1) (1,1) 1 1 1

writeShelleyGenesis (rootdir </> "genesis.json") finalGenesis

liftIO $ LBS.writeFile (rootdir </> "genesis.alonzo.json") $ Aeson.encode $
Copy link
Contributor

Choose a reason for hiding this comment

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

At minimum we should leave a TODO here to make the naming scheme sane, so it's symmetric between the eras.

@@ -759,22 +762,21 @@ renderShelleyGenesisError sge =
]
where
renderHash :: GenesisHashShelley -> Text
renderHash (GenesisHashShelley h) = Text.decodeUtf8 $ Base16.encode (Cardano.Crypto.Hash.Class.hashToBytes h)
renderHash (GenesisHashShelley h) = Text.decodeUtf8 (Cardano.Crypto.Hash.Class.hashToBytesAsHex h)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already a version that returns it as text. We should use that.

dcoutts and others added 5 commits May 26, 2021 20:24
Split out the reusable phases of the genesis handling so they can be
reused more easily in the Cardano mode, and so they can also be
partially reused for Alonzo genesis file loading.
This means we benefit from the hash checking and error types.
Only in the node itself, CLI and scripts still need updating.

Co-authored-by: John Ky <john.ky@iohk.io>
CLI that generates genesis files and scripts.

Co-authored-by: John Ky <john.ky@iohk.io>
@dcoutts dcoutts force-pushed the newhoggy/separate-alonzo-genesis-file branch from 1fcd036 to a9583e5 Compare May 26, 2021 19:25
@disassembler disassembler force-pushed the newhoggy/separate-alonzo-genesis-file branch from a9583e5 to f69e248 Compare May 26, 2021 20:21
@disassembler
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 26, 2021

@iohk-bors iohk-bors bot merged commit 62d2675 into master May 26, 2021
@iohk-bors iohk-bors bot deleted the newhoggy/separate-alonzo-genesis-file branch May 26, 2021 20:32
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.

6 participants