[BUG] - envelopeChecks ignoring header's protocol major version?

[KtorZ]

Internal/External: External

Summary
The consensus layer is responsible for validating block headers (at least, some elements of it). This happens through the envelopeChecks:

ouroboros-consensus/ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Protocol/Praos.hs

Lines 111 to 125 in 2ca6c74

	envelopeChecks cfg lv hdr = do
	unless (m <= maxpv) $ throwError (ObsoleteNode m maxpv)
	unless (bhviewHSize bhv <= fromIntegral @Word16 @Int maxHeaderSize) $
	throwError $
	HeaderSizeTooLarge (bhviewHSize bhv) maxHeaderSize
	unless (bhviewBSize bhv <= maxBodySize) $
	throwError $
	BlockSizeTooLarge (bhviewBSize bhv) maxBodySize
	where
	pp = praosParams cfg
	(MaxMajorProtVer maxpv) = praosMaxMajorPV pp
	(ProtVer m _) = lvProtocolVersion lv
	maxHeaderSize = lvMaxHeaderSize lv
	maxBodySize = lvMaxBodySize lv
	bhv = mkHeaderView hdr

One of the check refers to the protocol major version:

    unless (m <= maxpv) $ throwError (ObsoleteNode m maxpv)

However:

• maxpv is a configuration parameter provided at runtime
• m comes from the ledger view and reflects the current protocol version major

Notice that the protocol version from the header is totally ignored.

Steps to reproduce

See this block on Preview which declares a protocol major version 12 when the network just transitioned into 11.

Expected behavior

I am unclear whether this is desired or a genuine oversight? It seems to me that this check only ensures that the consensus and ledger libraries agree on what is the maximum protocol version; but then, why perform this check on every header?

If this is the intended behavior, then so be it. If not, let me know 😶

System info (please complete the following information):

• OS Name: irrelevant
• OS Version: irrelevant
• Consensus version: ==3.0.1.0

Screenshots and attachments

Additional context
Add any other context about the problem here.

[KtorZ]

@nfrisby @jasagredo anyone here 🙈 ?

[jasagredo]

EDIT: TL;DR: the protocol version in the header is irrelevant, except for well-constructed and within known bounds (checked on deserialization). It is just to signal that your node can understand version X.

The story with the protocol version in the header is a bit more convoluted that it looks like.

We can differentiate headerVersion, ledgerVersion and maxKnownVersion.

---

The headerVersion is given to the node as part of a header. Nodes set this to their maxKnownVersion:

ouroboros-consensus/ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Ledger/Forge.hs

Line 77 in 2ca6c74

protocolVersion = shelleyProtocolVersion $ configBlock cfg

ouroboros-consensus/ouroboros-consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/Node.hs

Lines 778 to 781 in 2ca6c74

	Shelley.mkShelleyBlockConfig
	cardanoProtocolVersion
	genesisShelley
	(shelleyBlockIssuerVKey <$> credssShelleyBased)

(notice all BlockConfigs there use the same cardanoProtocolVersion)

The maxKnownVersion is static information:

https://github.com/IntersectMBO/cardano-node/blob/f098c09d233b021395b123ac77787b9dc4c2595d/cardano-node/src/Cardano/Node/Protocol/Cardano.hs#L181-L183

This above indicates "what is the maximum protocol version that a node knows it exists". Should a node say (in the block that it forges and broadcasts) that it can understand version X+2 but my node only understands X+1, that should be irrelevant for the validity of the block, as long as the block itself is in X+1 or older.

This already gives an indication that we don't really need to check the protocol version of the header at all. It is there just for informative purposes, in particular to estimate how much stake has upgraded and is ready to perform a Hard Fork, that starts the bureaucracy for the Hard Fork to be enacted.

---

The ledgerVersion is the protocol version on the ledger state after ticking (and therefore after performing any transitions to new eras/protocol versions). In particular, the last version my node can possibly understand is the MaxVersion, defined currently as 13:

https://github.com/IntersectMBO/cardano-ledger/blob/a624de4c8db7286a6c065da149679ea55f7d5629/libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Version.hs#L61-L63

The why is explained in this comment:

https://github.com/IntersectMBO/cardano-ledger/blob/master/libs/cardano-ledger-core/internal/Cardano/Ledger/Internal/Definition/Era.hs#L42-L45

I am not entirely knowledgeable on this part as that is mostly a Cardano-ledger concern and there have been changes recently, but my recollection is that we need to have a +2 room on the current expected version running on mainnet and the last era version. @lehins can probably clarify this a bit more.

---

There are currently however limitations on what can be in the headerVersion of a block. We only accept blocks whose protocol version is known to exist, currently at most 13:

https://github.com/IntersectMBO/cardano-ledger/blob/fc173c70043f1b4cb217c7fcfb924cd9c7a943b8/libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Version.hs#L110-L127

https://github.com/IntersectMBO/cardano-ledger/blob/fc173c70043f1b4cb217c7fcfb924cd9c7a943b8/libs/cardano-ledger-binary/src/Cardano/Ledger/Binary/Decoding/Decoder.hs#L431-L433

Also when validating a block, we supply the header and we reject headers where the declared version is higher than the next current version:

https://github.com/IntersectMBO/cardano-ledger/blob/fc173c70043f1b4cb217c7fcfb924cd9c7a943b8/eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Bbody.hs#L293-L312

This effectively means that when on Conway PV11, we only accept blocks with PV11 or blocks that signal that PV12 is understood.

It makes no sense to advertise PV13 if not even PV12 is released so this is correct.

Also hard fork proposals can only increment the PV by 1, so all these pieces work together as expected.

---

The check that you found (which previously lived in the Ledger but I think not anymore or it is redundant) indeed does not check anything about the header, but it does check if the node can continue running the current chain.

For example now a node will use PV11 as MaxKnownVersion, but the code actually has logic for running also PV12. Moreover, such a node should accept as valid a proposal to move to PV12 even if it won't be able to do such a transition itself, otherwise a HF proposal would fork the network immediately. So a HF proposal to PV12 can be enacted and then the whole network moves to PV12 in some slot, and so does our PV11 node, but when it tries to apply the first block that transitions to PV12, the check you found will fail. ledgerVersion at that point would be 12 but MaxKnownVersion would be 11, and this node will start emitting ObsoleteNode warnings in the log and stop adopting new-era blocks.

---

This is my current understanding, and currently this is, as I said above, a concern that the Ledger team has managed and in particular @lehins.

[jasagredo]

There is also this issue+proposal that Alexey shared IntersectMBO/cardano-ledger#5763

which in short means that the version in the header could be instead:

• highestKnownMajorProtVer :: Word32, which will be forced to be at least as large as current major protocol version in the ledger state
• nodeSoftwareTag :: Word32, which will have no validation whatsoever, besides being a Word32