-
Notifications
You must be signed in to change notification settings - Fork 720
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
Warn the protocol version is too low if we have ExperimentalHardForksEnabled
#5883
base: master
Are you sure you want to change the base?
Conversation
@@ -7,6 +7,7 @@ | |||
{-# LANGUAGE TypeApplications #-} | |||
|
|||
{-# OPTIONS_GHC -Wno-orphans #-} | |||
{-# LANGUAGE BangPatterns #-} |
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.
Should go with the other LANGUAGE
pragma
(liftIO $ putStrLn $ "Warning: experimental hard fork version " ++ show hfMajor ++ | ||
" for " ++ era ++ " is less than the actual protocol version " ++ show actualMajor ++ | ||
" specified for Shelley on its genesis file.") | ||
checkHardForkVersion _ _ _ = return () -- No need to check for epoch-based hard forks |
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.
Can you make the _
on Consensus.TriggerHardFork
explicit? So that, if TriggerHardFork
receives a new case in the future, we get a non-exhaustive pattern warning?
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 think I don't understand the change. Could you clarify it a bit? When the warning should be printed? When we're in babbage and experimental hard forks are enabled? Or when we're in bootstrap Conway and the flag is enabled? Or in any case?
-> ExceptT CardanoProtocolInstantiationError IO () | ||
checkHardForkVersion era (Consensus.TriggerHardForkAtVersion hfMajor) (ProtVer actualMajorVer _) = | ||
let actualMajor = getVersion actualMajorVer in | ||
when (hfMajor < actualMajor) |
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.
Wouldn't that result in printing the warning for each previous era?
-> Consensus.TriggerHardFork | ||
-> ProtVer | ||
-> ExceptT CardanoProtocolInstantiationError IO () | ||
checkHardForkVersion era (Consensus.TriggerHardForkAtVersion hfMajor) (ProtVer actualMajorVer _) = |
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 the problem in the slack thread was hard forking to Conway at epoch 0. So we're actually (at this point) not interested in TriggerHardForkAtVersion
but we care about TriggerHardForkAtEpoch
. We need two things:
- A mapping of Era -> Protocol Version
- Functions that check for the presence of the various
npcTest${era}HardForkAtEpoch
values.
Then you can compare the defined protocol version in the Shelley genesis vs the expected protocol version.
See if you can use the Era
class in cardano-ledger
to avoid hard coding protocol versions.
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 I am comparing the Eras with the protocol version. I am using the numbers already hardcoded in the function, and those can be overriden, apparently. So having a mapping wouldn't work? But I don't understand how the epochs fit on this. How do we link the epochs to versions? I may be completely wrong.
This PR is stale because it has been open 45 days with no activity. |
Description
This PR makes
cardano-node
show a warning when theExperimentalHardForksEnabled
flag is set, and the specified version is less than the one specified forShelley
in its genesis file.Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7