ouroboros-consensus icon indicating copy to clipboard operation
ouroboros-consensus copied to clipboard

Simplify the `protVer` arguments

Open nfrisby opened this issue 1 year ago • 9 comments

Without loss of generality, focus on Allegra. Start from the binding of protVerAllegra in protocolInfoCardano. EG at https://github.com/input-output-hk/ouroboros-consensus/blob/0079ee65f6c49d1c05a8d30ffa492b20c46b12b8/ouroboros-consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/Node.hs#L658

There, protVerAllegra:

  • (A) is passed to mkShelleyBlockConfig
  • (B) influences maxMajorProtVer

Also, maxMajorProtVer:

  • (C) is passed to tpraosParams and used to construct praosParams, to eventually be an upper bound in the ProtocolHeaderSupportsEnvelope envelopeChecks method (to throw the "obsolete node" errors).
  • (D) is passed to mkPartialLedgerConfigShelley which eventually uses it to constructor SL.Globals
mkShelleyBlockConfig ::
     ShelleyBasedEra era
  => SL.ProtVer
  -> SL.ShelleyGenesis (EraCrypto era)
  -> [SL.VKey 'SL.BlockIssuer (EraCrypto era)]
  -> BlockConfig (ShelleyBlock proto era)
mkShelleyBlockConfig protVer genesis blockIssuerVKeys = ShelleyConfig {
      shelleyProtocolVersion  = protVer
    , shelleySystemStart      = SystemStart  $ SL.sgSystemStart  genesis
    , shelleyNetworkMagic     = NetworkMagic $ SL.sgNetworkMagic genesis
    , shelleyBlockIssuerVKeys = Map.fromList
        [ (SL.hashKey k, k)
        | k <- blockIssuerVKeys
        ]
    }
data instance BlockConfig (ShelleyBlock proto era) = ShelleyConfig {
      -- | The highest protocol version this node supports. It will be stored
      -- the headers of produced blocks.
      shelleyProtocolVersion  :: !SL.ProtVer
-- | Praos parameters that are node independent
data PraosParams = PraosParams
    ...
    -- | All blocks invalid after this protocol version, see
    -- 'Globals.maxMajorPV'.
    praosMaxMajorPV        :: !MaxMajorProtVer,
data Globals = Globals
  ...
  , maxMajorPV :: !Version
  -- ^ All blocks invalid after this protocol version

  • (D) I just opened https://github.com/input-output-hk/cardano-ledger/issues/3682, which would eliminate this use.

  • (A) ultimately enables the node to write this version to a header field that is never scrutinized by the entire Cardano node code base. It is only used for SPOs to indicate in their blocks that they have upgraded their software in preparation for an imminent transition to the next ledger era.

  • (C) This use is motivated in a short paragraph in the the ledger spec, here: https://github.com/input-output-hk/cardano-ledger/blob/63d98c3f8e9eb2878cec3ab71c54fc40c798ac01/eras/shelley/formal-spec/protocol-parameters.tex#L162-L165 It is worth noting that the value scrutinized here doesn't come from the header, it instead comes from the ledger state (view). And most crucially: it is not the header from (A).

  • (B) The influence of allegraProtVer on maxMajorProtVer is via the latter's definition:

https://github.com/input-output-hk/ouroboros-consensus/blob/0079ee65f6c49d1c05a8d30ffa492b20c46b12b8/ouroboros-consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/Node.hs#L754-L775

Note that this use is exactly contrary to the warning comments where allegraProtVer is actually set, such as:

https://github.com/input-output-hk/cardano-node/blob/70dde196367e86c977fbe72167f4eb303b3ffa88/cardano-node/src/Cardano/Node/Protocol/Cardano.hs#L185-L194

        Consensus.ProtocolParamsAllegra {
          -- This is /not/ the Allegra protocol version. It is the protocol
          -- version that this node will declare that it understands, when it
          -- is in the Allegra era. That is, it is the version of protocol
          -- /after/ Allegra, i.e. Mary.
          allegraProtVer =
            ProtVer (natVersion @4) 0,
          allegraMaxTxCapacityOverrides =
            TxLimits.mkOverrides TxLimits.noOverridesMeasure
        }

It is currently my suspicion that all of these per-era *ProtVer arguments should be removed (thereby eliminating (B)), and replaced by some logic that defines maxMajorProtVer by analyzing the last given transitionIntraShelleyTrigger. And the per-era values used in (A) should instead all simply use maxMajorProtVer.

nfrisby avatar Aug 27 '23 23:08 nfrisby