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

Warn the protocol version is too low if we have ExperimentalHardForksEnabled #5883

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cardano-node/ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- `--mempool-capacity-override` and `--no-mempool-capacity-override` can be set in the configuration file via the key `MempoolCapacityBytesOverride`.
- `--snapshot-interval` can be set in the configuration file via the key `SnapshotInterval`.
- `--num-of-disk-snapshots` can be set in the configuration file via the key `NumOfDiskSnapshots`.
- Add warning for when the protocol version is too low and we have set `ExperimentalHardForksEnabled`.

## 8.2.1 -- August 2023

Expand Down
1 change: 1 addition & 0 deletions cardano-node/cardano-node.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ library
, io-classes >= 1.4
, iohk-monitoring
, iproute
, lens
, lobemo-backend-aggregation
, lobemo-backend-ekg
, lobemo-backend-monitoring
Expand Down
53 changes: 50 additions & 3 deletions cardano-node/src/Cardano/Node/Protocol/Cardano.hs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
{-# LANGUAGE TypeApplications #-}

{-# OPTIONS_GHC -Wno-orphans #-}
{-# LANGUAGE BangPatterns #-}
Copy link
Contributor

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


module Cardano.Node.Protocol.Cardano
( mkSomeConsensusProtocolCardano
Expand All @@ -17,10 +18,12 @@ module Cardano.Node.Protocol.Cardano

import Cardano.Api
import Cardano.Api.Byron as Byron
import Cardano.Api.Ledger (ppProtocolVersionL)

import qualified Cardano.Chain.Update as Update
import qualified Cardano.Ledger.Api.Transition as Ledger
import Cardano.Ledger.BaseTypes (natVersion)
import Cardano.Ledger.BaseTypes (getVersion, natVersion)
import Cardano.Ledger.Crypto (StandardCrypto)
import qualified Cardano.Node.Protocol.Alonzo as Alonzo
import qualified Cardano.Node.Protocol.Byron as Byron
import qualified Cardano.Node.Protocol.Conway as Conway
Expand All @@ -40,6 +43,8 @@ import qualified Ouroboros.Consensus.Shelley.Node.Praos as Praos

import Prelude

import Control.Lens.Getter ((^.))
import Control.Monad (when)
import Data.Maybe

------------------------------------------------------------------------------
Expand Down Expand Up @@ -139,8 +144,7 @@ mkSomeConsensusProtocolCardano NodeByronProtocolConfiguration {

--TODO: all these protocol versions below are confusing and unnecessary.
-- It could and should all be automated and these config entries eliminated.
return $!
SomeConsensusProtocol CardanoBlockType $ ProtocolInfoArgsCardano $ CardanoProtocolParams {
let !cardanoProtocolParams = CardanoProtocolParams {
paramsByron =
Consensus.ProtocolParamsByron {
byronGenesis = byronGenesis,
Expand Down Expand Up @@ -314,11 +318,54 @@ mkSomeConsensusProtocolCardano NodeByronProtocolConfiguration {
, checkpoints = emptyCheckpointsMap
}

-- Check that the versions for the experimental hard forks are consistent
warnAboutProtocolVersionsForExperimentalHardForks npcExperimentalHardForksEnabled
cardanoProtocolParams shelleyGenesis

return $! SomeConsensusProtocol CardanoBlockType $
ProtocolInfoArgsCardano cardanoProtocolParams

----------------------------------------------------------------------
-- WARNING When adding new entries above, be aware that if there is an
-- intra-era fork, then the numbering is not consecutive.
----------------------------------------------------------------------

-- | Warn about the protocol versions for experimental hard forks
warnAboutProtocolVersionsForExperimentalHardForks :: ()
=> Bool
-> ProtocolParams (CardanoBlock StandardCrypto)
-> ShelleyGenesis StandardCrypto
-> ExceptT CardanoProtocolInstantiationError IO ()
warnAboutProtocolVersionsForExperimentalHardForks npcExperimentalHardForksEnabled
CardanoProtocolParams
{ hardForkTriggers = CardanoHardForkTriggers'
{ triggerHardForkShelley = thfShelley, triggerHardForkAllegra = thfAllegra
, triggerHardForkMary = thfMary, triggerHardForkAlonzo = thfAlonzo
, triggerHardForkBabbage = thfBabbage, triggerHardForkConway = thfConway
}
} shelleyGenesis =
when npcExperimentalHardForksEnabled (do
let shelleyProtocolVersion = sgProtocolParams shelleyGenesis ^. ppProtocolVersionL
checkHardForkVersion "Shelley" thfShelley shelleyProtocolVersion
checkHardForkVersion "Allegra" thfAllegra shelleyProtocolVersion
checkHardForkVersion "Mary" thfMary shelleyProtocolVersion
checkHardForkVersion "Alonzo" thfAlonzo shelleyProtocolVersion
checkHardForkVersion "Babbage" thfBabbage shelleyProtocolVersion
checkHardForkVersion "Conway" thfConway shelleyProtocolVersion)
where
checkHardForkVersion :: ()
=> String
-> Consensus.TriggerHardFork
-> ProtVer
-> ExceptT CardanoProtocolInstantiationError IO ()
checkHardForkVersion era (Consensus.TriggerHardForkAtVersion hfMajor) (ProtVer actualMajorVer _) =
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jun 26, 2024

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:

  1. A mapping of Era -> Protocol Version
  2. 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.

Copy link
Contributor Author

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.

let actualMajor = getVersion actualMajorVer in
when (hfMajor < actualMajor)
Copy link
Contributor

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?

(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
Copy link
Contributor

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?


------------------------------------------------------------------------------
-- Errors
--
Expand Down
Loading