Protocol polymorphism and the Praos protocol #3595

nc6 · 2022-01-31T21:50:55Z

This PR prepares the consensus side of the Vasil H/F. The main components of this are:

The introduction of a new protocol (hence referred to as simply Praos) in place of the TPraos protocol which has been in use since Shelley.
It adds support for multiple protocols within the Shelley era, and adds significant support to enhance dealing with multiple protocols in general.
It introduces the Babbage era from the ledger.

This code is in a number of disparate commits, which I'm happy to split into multiple PRs as per your desire.

Firstly, it makes a ledger version bump.
Secondly, it introduces the Praos protocol. It introduces the block header type to use with the new Praos protocol. This is deliberately disjoint from the protocol itself, but should be scrutinised since this is what will be sent over the wire. Perhaps we should add it to a CDDL spec somewhere.
Fourthly, it makes a few preparatory changes to prepare for protocol polymorphism. These are small and in individual commits.
Fifthly, it adds infrastructure for translating between protocols. Some of this did already exist, in an ad-hoc way, between PBFT and TPraos. This PR adds it in more generality for transitioning between "Shelley..." protocols. It could (should?) potentially be extended to cover the PBFT/TPraos transition, but this PR is big enough already.
Sixth, it abstracts ShelleyBlock over the protocol in addition to the era. Various classes are introduced (in ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol/Abstract.hs) which define various ledger functionality abstracted only over the protocol (and not the block). Some of these should arguably move to the ouroboros-consensus-protocol package - again, not done in this PR. It redefine the core ShelleyBasedEra constraint to ShelleyCompatible, which now also ties the era and protocol together. One additional constrain bubbles out at the top - LedgerSupportsProtocol. I'm open to suggestions to hide this.

nfrisby

I've reviewed and marked as Viewed all but the four largest files (Node, Node.TPraos, Ledger.Ledger, and Ledger.Block) in commit 85d2dc6. This was my way of starting to chip away at the iceberg.

Overall, your comments are fantastic and deeply appreciated. We're very thankful you're driving this -- you have much better command of this code than we do.

None of this set of comments is major, but the translation question might start a discussion.

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

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

nfrisby · 2022-02-15T03:37:37Z

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


  -- | Only for debugging purposes, we make no effort to ensure binary
  -- compatibility (cf the comment on 'GetCBOR').
  DebugChainDepState
-    :: BlockQuery (ShelleyBlock era) (SL.ChainDepState (EraCrypto era))
+    :: BlockQuery (ShelleyBlock proto era) (ChainDepState proto)


Need we mention this type change anywhere (such as in https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-consensus/docs/interface-CHANGELOG.md ) ? Or is the Debug* query set understood to be volatile in this way?

I think that yes, the changes in this PR will need to be heavily documented there (albeit this query is possible the least relevant change, since the Debug thing is not guaranteed stable.

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

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

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

nfrisby · 2022-04-19T19:34:29Z

Should the commit message on 5e65dc1 say which has been in use since Shelley instead of since Genesis?

Edit: Resolved.

This allows removing all of the NP2 machinery. It comes at a slight cost - this function now has to be modified when we add a new era. But we will still be guided by the types here, and this is not really any worse than having to add to `ShelleyErasAndProtos`. So overall I think this is a simplification.

nc6 · 2022-05-02T07:44:46Z

Rebased after review, so all the fixup commits are now folded in and this sits directly atop current master.

@nfrisby Duncan has approved his changes. You indicated approval. Are we good to merge?

nfrisby

Approving in order to remove my Request Changes.

nc6 · 2022-05-02T15:26:17Z

bors merge

3595: Protocol polymorphism and the Praos protocol r=nc6 a=nc6 This PR prepares the consensus side of the Vasil H/F. The main components of this are: - The introduction of a new protocol (hence referred to as simply `Praos`) in place of the `TPraos` protocol which has been in use since Shelley. - It adds support for multiple protocols within the Shelley era, and adds significant support to enhance dealing with multiple protocols in general. - It introduces the Babbage era from the ledger. This code is in a number of disparate commits, which I'm happy to split into multiple PRs as per your desire. - Firstly, it makes a ledger version bump. - Secondly, it introduces the Praos protocol. It introduces the block header type to use with the new Praos protocol. This is deliberately disjoint from the protocol itself, but should be scrutinised since this is what will be sent over the wire. Perhaps we should add it to a CDDL spec somewhere. - Fourthly, it makes a few preparatory changes to prepare for protocol polymorphism. These are small and in individual commits. - Fifthly, it adds infrastructure for translating between protocols. Some of this did already exist, in an ad-hoc way, between PBFT and TPraos. This PR adds it in more generality for transitioning between "Shelley..." protocols. It could (should?) potentially be extended to cover the PBFT/TPraos transition, but this PR is big enough already. - Sixth, it abstracts `ShelleyBlock` over the protocol in addition to the era. Various classes are introduced (in ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol/Abstract.hs) which define various ledger functionality abstracted only over the protocol (and not the block). Some of these should arguably move to the `ouroboros-consensus-protocol` package - again, not done in this PR. It redefine the core `ShelleyBasedEra` constraint to `ShelleyCompatible`, which now also ties the era and protocol together. One additional constrain bubbles out at the top - `LedgerSupportsProtocol`. I'm open to suggestions to hide this. Co-authored-by: Nicholas Clarke <nick@topos.org.uk> Co-authored-by: Nicholas Clarke <nicholas.clarke@tweag.io> Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

iohk-bors · 2022-05-02T17:19:28Z

Build failed:

build (8.10.7, windows-latest)

nc6 · 2022-05-03T07:16:13Z

bors retry

3595: Protocol polymorphism and the Praos protocol r=nc6 a=nc6 This PR prepares the consensus side of the Vasil H/F. The main components of this are: - The introduction of a new protocol (hence referred to as simply `Praos`) in place of the `TPraos` protocol which has been in use since Shelley. - It adds support for multiple protocols within the Shelley era, and adds significant support to enhance dealing with multiple protocols in general. - It introduces the Babbage era from the ledger. This code is in a number of disparate commits, which I'm happy to split into multiple PRs as per your desire. - Firstly, it makes a ledger version bump. - Secondly, it introduces the Praos protocol. It introduces the block header type to use with the new Praos protocol. This is deliberately disjoint from the protocol itself, but should be scrutinised since this is what will be sent over the wire. Perhaps we should add it to a CDDL spec somewhere. - Fourthly, it makes a few preparatory changes to prepare for protocol polymorphism. These are small and in individual commits. - Fifthly, it adds infrastructure for translating between protocols. Some of this did already exist, in an ad-hoc way, between PBFT and TPraos. This PR adds it in more generality for transitioning between "Shelley..." protocols. It could (should?) potentially be extended to cover the PBFT/TPraos transition, but this PR is big enough already. - Sixth, it abstracts `ShelleyBlock` over the protocol in addition to the era. Various classes are introduced (in ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Protocol/Abstract.hs) which define various ledger functionality abstracted only over the protocol (and not the block). Some of these should arguably move to the `ouroboros-consensus-protocol` package - again, not done in this PR. It redefine the core `ShelleyBasedEra` constraint to `ShelleyCompatible`, which now also ties the era and protocol together. One additional constrain bubbles out at the top - `LedgerSupportsProtocol`. I'm open to suggestions to hide this. Co-authored-by: Nicholas Clarke <nick@topos.org.uk> Co-authored-by: Nicholas Clarke <nicholas.clarke@tweag.io> Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

iohk-bors · 2022-05-03T09:28:12Z

Timed out.

nc6 · 2022-05-03T10:08:06Z

bors retry

nc6 · 2022-05-03T10:56:44Z

bors merge

iohk-bors · 2022-05-03T10:56:45Z

Already running a review

nc6 · 2022-05-03T12:03:33Z

bors cancel

iohk-bors · 2022-05-03T12:03:35Z

Canceled.

nc6 · 2022-05-03T12:03:42Z

bors merge

dcoutts · 2022-05-03T12:16:06Z

ouroboros-consensus-protocol/src/Ouroboros/Consensus/Protocol/Praos.hs

+          praosStateOCertCounters =
+            Map.insert hk n $ praosStateOCertCounters cs


Micro-optimisation opportunity (perhaps as a TODO): 99% of the time this update will be an identity. If we do a lookup first, we will save a few allocations.

iohk-bors · 2022-05-03T14:17:56Z

Build succeeded:

3754: Bugfix in HFC: do not consider the last known era to be eternal r=nfrisby a=nfrisby This supersedes PR #3750. And it unblocks the Vasil HF. This PR fixes a bug in the Consensus Hard Fork Combinator (HFC). The bug is that certain parts of the HFC before this PR assume that the final era the code is aware of (ie the rightmost era in the `xs` argument to `HardForkBlock xs`) will never end. At face value, this assumption seems very reasonable. If the final era could end, then that means we wrote the code that knows how to end the final era but didn't simultaneously add the code for the following era, which is pretty clearly a bad idea unless you indeed want your system-wide chain to stop growing. The patterns we have in `protocolInfoCardano` and in the related call in [input-output-hk/cardano-node](https://github.com/input-output-hk/cardano-node) ensure that mistake would be quite obvious in review of such a PR. Despite that assumption seeming reasonable, merely adding Babbage in the recent PR #3595 revealed this assumption as a bug: the new code considered some Alonzo transactions on the historical chains to now be invalid. Together, this PR and PR IntersectMBO/cardano-ledger#2785 fix the bug and also allows those Alonzo transactions to remain valid. The recent PR #3595 added the Babbage era, changing `type CardanoBlock = HardForkBlock [ByronBlock, ShelleyBlock, AllegraBlock, MaryBlock, AlonzoBlock]` to `type CardanoBlock = HardForkBlock [ByronBlock, ShelleyBlock, AllegraBlock, MaryBlock, AlonzoBlock, BabbageBlock]`. From that change alone, due to the bug, the code stopped considering the Alonzo era as eternal, since it was no longer the final era. We now classify this assumption as a bug because it's clear that the (inevitable) addition of a new final era causes, via the bug, a non-mononotic change in behavior: for Alonzo (ie before we transition to Babbage), the Babbage-aware HFC now refuses to translate some slot<->times that it happily translated when Alonzo was the final era in the list. The eras prior to Alonzo are unaffected because Alonzo introduced Plutus scripts and with them the requirement that the validity interval (specified as an interval between two slots) on an Alonzo transaction that contains Plutus scripts must be translatable to POSIX times, because the Plutus interpreter exposes the interval to the script as POSIX times, not as slots. The translation between slots and times is the responsibility of the HFC, because it depends on the slot duration, which is allowed to change during era transitions (eg it changed from 20s to 1s when the chain transitioned from Byron to Shelley; it has not yet changed a second time). The HFC is very careful with that translation, as you can see in the Time chapter in the Hard Fork Combinator section of the [The Cardano Consensus and Storage Layer](https://hydra.iohk.io/job/Cardano/ouroboros-network/native.consensus-docs.x86_64-linux/latest/download/1) report. In particular, that chapter explains that the HFC refuses to translate slots<->times unless the answer would always remain correct regardless of any possible rollbacks (ie would be the same for any extension of our immutable tip, which is `k` blocks back from the tip of our currently selected chain). The assumption that the final era does not end is in direct violation of that rule: if we assume the final era won't end, then we might translate a slot/time that is (currently!) 1000 years into the future -- and it's obvious that future activity on this chain is likely to change the correspondence between slots and times at some point during the next 1000 years! It wasn't until Alonzo's transaction validity interval check that this mattered, because that's the first (and so far only) slot<->time translation in the ledger rules that involves a user-defined slot (ie what they set as the transaction's validity interval bounds) -- all other translations are fixed by the ledger rules and are by design always within the range the HFC will translate (even after this PR's bugfix). Thus, as a result of this PR, Babbage and subsequent eras will never be considered eternal, thereby satisfying the rule about all successful slot<->time translations being deterministic with respect to the selection's immutable tip. And PR IntersectMBO/cardano-ledger#2785 will intentionally violate that rule only during Alonzo, so that the historical transactions already on-chain remain valid. Because the consequences are currently limited to transaction validity intervals, there's no harm in that. So-called "clients", such as `db-sync`, the wallet, the Cardano cli tools, etc may also exhibit a change in behavior due to this PR, but those at worst will be less convenient than they seemed before: any features of those tools that allowed the user to translate slot<->time well into the future will now refuse to do so. This PR changes no types, so that downstream code already has code paths that handle the HFC's refusal to translate a slot/time; they merely weren't being exercised for as many arguments as they should have been. Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

4050: Bug fix: base `maxMajorProtVer` off of Babbage r=amesgen a=amesgen # Description This bases the `maxMajorProtVer` of the Cardano protocol off the Babbage (instead of Alonzo) protocol version, which was overlooked in #3595 (also see [`AddingAnEra.md`][AddingAnEra]). This has not been a problem in the node as it sets the protocol version of Alonzo to `7` (see [here][NodeAlonzoMajProtVer]), in contrast to db-analyser (see [here][DbAnaAlonzoMajProtVer]), where this issue manifests as an `ObsoleteNode 7 6` exception thrown on the first Babbage block. This relates to #3925 (in particular #3925 (comment), where `@JaredCorduan` already noted this bug), as it shows that the current process of managing protocol versions is quite tricky. [AddingAnEra]: https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-consensus/docs/AddingAnEra.md [NodeAlonzoMajProtVer]: https://github.com/input-output-hk/cardano-node/blob/40458eefa87c7c3abd8c6ba542d9e931d8b2ecb8/cardano-node/src/Cardano/Node/Protocol/Cardano.hs#L206 [DbAnaAlonzoMajProtVer]: https://github.com/input-output-hk/ouroboros-network/blob/9249a70ed9e2365f3963e47cb31b4b1589bca8f6/ouroboros-consensus-cardano-tools/src/Cardano/Tools/DBAnalyser/Block/Cardano.hs#L302 Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>

nc6 requested a review from nfrisby January 31, 2022 21:50

nc6 force-pushed the nc/praos-protocol branch 5 times, most recently from 51912dd to de24624 Compare February 7, 2022 08:00

nc6 force-pushed the nc/praos-protocol branch from de24624 to ab394f5 Compare February 14, 2022 12:44

nfrisby reviewed Feb 15, 2022

View reviewed changes

nc6 force-pushed the nc/praos-protocol branch 4 times, most recently from 3ff1cef to ba55ff3 Compare February 21, 2022 14:29

nc6 force-pushed the nc/praos-protocol branch 7 times, most recently from ac51358 to 49c966d Compare March 23, 2022 10:17

This was referenced Mar 31, 2022

New UPDN rule IntersectMBO/cardano-ledger#2476

Closed

New OVERLAY rule IntersectMBO/cardano-ledger#2475

Closed

Praos block header IntersectMBO/cardano-ledger#2474

Closed

nc6 force-pushed the nc/praos-protocol branch 3 times, most recently from 4220b86 to 9ef2b5b Compare April 6, 2022 10:26

nc6 assigned dcoutts Apr 6, 2022

Jimbo4350 mentioned this pull request Apr 12, 2022

Vasil CLI/API integration IntersectMBO/cardano-node#3739

Closed

38 tasks

nc6 force-pushed the nc/praos-protocol branch 2 times, most recently from ee017a3 to 37c209b Compare April 19, 2022 07:11

nc6 added 2 commits May 2, 2022 09:43

Invert the arguments to CanMock.

fc10f18

nc6 force-pushed the nc/praos-protocol branch from 01362a6 to fc10f18 Compare May 2, 2022 07:44

nfrisby approved these changes May 2, 2022

View reviewed changes

dcoutts reviewed May 3, 2022

View reviewed changes

iohk-bors bot merged commit 0a3a61a into master May 3, 2022

iohk-bors bot deleted the nc/praos-protocol branch May 3, 2022 14:18

JaredCorduan mentioned this pull request May 5, 2022

Minimal OCert counter increments in Praos IntersectMBO/cardano-ledger#2686

Closed

nfrisby mentioned this pull request May 20, 2022

Bugfix in HFC: do not consider the last known era to be eternal #3754

Merged

This was referenced Nov 30, 2023

Improve name of function additionalEnvelopeChecks IntersectMBO/ouroboros-consensus#576

Open

Remove intermediate instance when Praos types no longer rely on TPraos types IntersectMBO/ouroboros-consensus#571

Open

amesgen mentioned this pull request Sep 30, 2022

Bug fix: base maxMajorProtVer off of Babbage #4050

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol polymorphism and the Praos protocol #3595

Protocol polymorphism and the Praos protocol #3595

nc6 commented Jan 31, 2022 •

edited

nfrisby left a comment •

edited

nfrisby Feb 15, 2022

nc6 Feb 21, 2022

nfrisby commented Apr 19, 2022 •

edited

nc6 commented May 2, 2022 •

edited

nfrisby left a comment

nc6 commented May 2, 2022

iohk-bors bot commented May 2, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

dcoutts May 3, 2022

iohk-bors bot commented May 3, 2022

		praosStateOCertCounters =
		Map.insert hk n $ praosStateOCertCounters cs

Protocol polymorphism and the Praos protocol #3595

Protocol polymorphism and the Praos protocol #3595

Conversation

nc6 commented Jan 31, 2022 • edited

nfrisby left a comment • edited

Choose a reason for hiding this comment

nfrisby Feb 15, 2022

Choose a reason for hiding this comment

nc6 Feb 21, 2022

Choose a reason for hiding this comment

nfrisby commented Apr 19, 2022 • edited

nc6 commented May 2, 2022 • edited

nfrisby left a comment

Choose a reason for hiding this comment

nc6 commented May 2, 2022

iohk-bors bot commented May 2, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

iohk-bors bot commented May 3, 2022

nc6 commented May 3, 2022

dcoutts May 3, 2022

Choose a reason for hiding this comment

iohk-bors bot commented May 3, 2022

nc6 commented Jan 31, 2022 •

edited

nfrisby left a comment •

edited

nfrisby commented Apr 19, 2022 •

edited

nc6 commented May 2, 2022 •

edited