Remove ShelleyGenesis from ShelleyLedgerConfig

[nfrisby]

The difference between Babbage and Conway was intentionally as minimal as possible. This has a couple benefits.

• Clarke had orchestrated the integration of new eras. So it's nice that the first one we did without him is so minimal.

• The minimality helped us (especially in Clarke's absence) notice the aspects that we found confusing.

This Issue is about one such aspect: I don't think ShelleyGenesis (as CompactGenesis) should be in the ShelleyLedgerConfig. In other words, I'm proposing removing the following function.

Ouroboros.Consensus.Shelley.Ledger.Ledger.shelleyLedgerGenesis :: ShelleyLedgerConfig era -> SL.ShelleyGenesis era

I've catalogued its uses as follows:

• It's used in Ouroboros.Consensus.Cardano.CanHardFork for translating/forecasting from Byron to the first Shelley era. This is a necessary use.
• It's used in Ouroboros.Consensus.Shelley.Ledger.Inspect, Ouroboros.Consensus.Shelley.Ledger.Ledger, and Ouroboros.Consensus.Shelley.ShelleyHFC to get the value of k, f (only used to compute the stability window), quorum, slotLength, and epochLength. I think all of these could and should instead use the (already-existing!) shelleyLedgerGlobals :: SL.Globals field.

The second bullet point is for enabling the hard fork combinator, which every era needs to do. The first bullet point is instead for translating from Byron to Shelley, which only the first Shelley-based era needs to do. Moreover, I don't think we want to require that any Shelley-based era could be the successor Byron, do we? So I propose:

data ShelleyLedgerConfig era = ShelleyLedgerConfig {
      shelleyLedgerGlobals                     :: !SL.Globals
    , shelleyLedgerTranslationContext          :: !(Core.TranslationContext era)
    , shelleyLedgerTranslationContextFromByron :: !(StaticMaybe (SL.ByronTranslationContext era))
    }

data StaticMaybe :: Maybe Type -> Type where
  StaticJust :: !a -> StaticMaybe (Just a)
  StaticNothing :: StaticMaybe Nothing

(see IntersectMBO/cardano-ledger#3053 (comment) for motivating the StaticMaybe layer)

where in the ledger we have:

type family ByronTranslationContext era

type instance ByronTranslationContext ShelleyEra = SL.ParedDownShelleyGenesis
type instance ByronTranslationContext AllegraEra = Void -- or TypeError?         (I originally wrote () here but then I objected to it on one of Bart's PRs :face_palm)
type instance ByronTranslationContext MaryEra    = Void -- or TypeError?
type instance ByronTranslationContext AlonzoEra  = Void -- or TypeError?
type instance ByronTranslationContext BabbageEra = Void -- or TypeError?
type instance ByronTranslationContext ConwayEra  = Void -- or TypeError?

As a result of this, we should be able to remove all of the oxymoronic TranslateEra ShelleyGenesis instances. We'll have to replace them with TranslateEra Globals instances (or some translate-able data that determines the Globals). For example, these new translation instances won't even have a "genesis delegates" field, and so it'll avoid the existing confusing fact that the TranslateEra ShelleyGenesis ConwayEra instance does not need to update the sgGenDelegs field!

[nfrisby]

We recently assigned Issue #571 to @yogeshsajanikar -- we wanted an onboarding techdebt task that was directly altering the code because his onboarding work so far has been on infrastructure concerns. We wanted something for him that is more directly Consensus work.

I'm also assigning this one to him.

I think both Issues have a relatively narrow scope, won't require a burdensomely-large diff in the end, but aren't trivial. All of those are good qualities for this kind of onboarding. If Yogesh chooses one, maybe we'll give the other to Bart when Bart returns.

[bartfrenk]

The HasHardForkHistory of ShelleyBlock proto era requires creating a Summary from a ShelleyLedgerConfig and a ShelleyLedgerState. The summary itself requires the slot length and the epoch size, which are currently part of ShelleyGenesis. Given a slotNo and an epochNo it would be possible to get the slot length and the epoch size from the EpochInfo field in the Globals part of the ShelleyLedgerConfig, save for the fact that the EpochInfo functions ('epochInfoSize_, epochInfoSlotLength_`) may fail, since executing may require information derived from the blockchain itself.

The slotNo and the epochNo are (indirectly) part of the ShelleyLedgerState and that is something we have access for creating a summary. However, both are wrapped in a WithOrigin indicating that the tip might be Genesis. Whenever we have an Origin we cannot use the EpochInfo functions to get the slot length or the epoch size.

So there are two failure modes when we try to use existing information outside of ShelleyGenesis to get the slot length and the epoch size. I see two options:

1. Somehow infer from the context that these failure modes don't occur.
2. Make the epoch size and the slot length part of ParedDownShelleyGenesis.

Another interesting side note: The formal specification of the Shelley ledger (on page 11) seems to imply that at least the epoch size is considered to be a global constant for the Shelley ledger. Why is it not in Cardano.Ledger.BaseTypes.Globals (in cardano-ledger)?

[nfrisby]

the epoch size is considered to be a global constant for the Shelley ledger

The Shelley ledger formal spec is written from the perspective of a single era -- it doesn't consider the HFC. And within a single era, the epoch size (and k and slot length etc) must be constant.

[bartfrenk]

The external API used by the node makes use of the GetGenesisConfig query in the ledger. There seems to be no way to avoid breaking that query without keeping the ShelleyGenesis in ShelleyLedgerConfig. The relevant query is here.

This puts the status of this issue on blocked until further notice.

[nfrisby]

Hmm. Looks like that query GetGenesisConfig is only used to implement the internal node query QueryGenesisParameters, which is in turn only used to implement Cardano.CLI.Shelley.Run.Query.runQueryKesPeriodInfo, which only calls the following functions on the result.

• protocolParamMaxKESEvolutions
• protocolParamSlotsPerKESPeriod
• protocolParamSystemStart

Those are all selectors from this record type: https://github.com/input-output-hk/cardano-node/blob/df1dea590d9ad7a6caed58147ad8792ff98711db/cardano-api/src/Cardano/Api/GenesisParameters.hs#L37

• protocolParamMaxKESEvolutions :: Int
• protocolParamSlotsPerKESPeriod :: Int
• protocolParamSystemStart :: UTCTime

Those fields are also all present in the similar data type in the Ledger https://github.com/input-output-hk/cardano-ledger/blob/9a25a43f3970941a2cd64029f7ce667b48832635/libs/cardano-ledger-core/src/Cardano/Ledger/BaseTypes.hs#L585

• maxKESEvo :: !Word64
• slotsPerKESPeriod :: !Word64
• systemStart :: !SystemStart (which holds a UTCTime)

My proposed ShelleyLedgerConfig above does contain those data, via shelleyLedgerGlobals.

So this doesn't seem fully blocked to me. It's a matter of:

• confirming my analysis here (eg there's no other use of QueryGenesisParameters? Or no other client tool that relies on GetGenesisConfig? Maybe db-sync does?)
• determining the development cost of instead serving these data through a narrower query (eg perhaps a narrower GetKesInfo query would replace GetGenesisConfig?)
• convincing folks it's worth it (I really want to simplify the HFC story here, but I could technically be convinced it's not worthwhile)

cc @bartfrenk @dnadales

[dnadales]

Thank you for these pointers @nfrisby 👍 These three action points sound sensible. Let's try that.

[amesgen]

One idea would be to keep the GetGenesisConfig query while still removing ShelleyGenesis from the ledger cfg.

This could be done by adding it as a field to the BlockConfig (static block configuration actually does not seem like a bad place for it, and the Shelley BlockConfig already contains stuff from ShelleyGenesis; it is also already an argument to mkShelleyBlockConfig). Also, answerBlockQuery has access to the BlockConfig via ExtLedgerConfig/TopLevelConfig.

This would minimize the scope of the issue (shrinking the Shelley ledger cfg to what actually needs translating) without having to submit upstream PRs (IIRC, a PR to wallet would otherwise also be necessary). At the same time, it does not prevent tweaking/removing GetGenesisConfig in the future if we think that is desirable; we can still do that separately.

[nfrisby]

Bah! I could have sworn I already wrote a reply here... I'll attempt to replicate what I thought I wrote, but I'm a bit rushed at the moment.

We discussed this on a call. I think Esgen's suggestion is strictly-speaking an improvement (because it avoids the unnecessary translations---he emphasized on the call that we never translate BlockConfigs.) However, I think we should consider it a contingency plan. The current plan is more involved, but I think it has lots of value, including that exercise-like elements of @bartfrenk touching other repos and interacting with other teams, narrowing an (apparently) overly-broad query, and removing the duplication (Globals vs (Compact)ShelleyGenesis) within the *Config data types.

I think it's valuable as a contingency plan, if we (ie mostly Bart) determines that the current plan is an overwhelming amount of change.

[bartfrenk]

My plan is to do this in two steps, and separate blocks of PRs:

1. Remove ShelleyGenesis from the LedgerConfig, while keeping the query (using the aforementioned BlockConfig). This touches both ouroboros-consensus and cardano-ledger. There is already a draft PR for ledger here: Add context for translating ShelleyLedgerConfig cardano-ledger#3053.
2. Update cardano-node, wallet and potentially other repo's to remove usage of GetGenesisConfig and then remove it from ouroboros-consensus.

The benefits to this approach, as I see them, are that we keep a sense of progress, and that we separate the work involving mostly simple changes to the codebase, from the work (and the delays) involved in interacting with other teams.

The drawback that there is overhead in re-implementing the GetGenesisConfig query from the BlockConfig, but I think that is fairly minor.

[nfrisby]

Sounds good. It'll end up with some excessive churn in the commit diffs, but that's ultimately fine if it simplifies the work for you 👍

[nfrisby]

I updated the description of this ticket to reflect what I just now commented on here: IntersectMBO/cardano-ledger#3053 (comment)

[bartfrenk]

I am running into a conceptual complication executing the plan on removing Genesis from the ShelleyLedgerConfig. The issue is that there is another thing in ledger, the AdditionalGenesisConfig that is an associated type family for CanStartFromGenesis. From what I gather from the code it is the extra information that is required on top of a ShelleyGenesis value to create an initial ledger state (i.e., a NewEpochState) for that era, for Shelley-based era's. Obviously, for ShelleyEra c it instantiates to unit.

Now, the ShelleyBased type class has as a super class contraint that AdditionalGenesisConfig ~ TranslationContext, which did make some sense when we were still thinking about TranslationContext as a context for translation within Shelley-based era's, but is much less sensible when TranslationContext is the context for translating from any previous era, including Byron.

The obvious technical work-around is to keep the separation between Byron and Shelley-based era's in the TranslationContext, as discussed earlier, but it is a route I prefer not to take, because I like the idea of having a singular concept of translation context. Also, there is the following comment in Ouroboros.Consensus.Shelley.Eras that makes one think cleaning up the relation might be part of this PR:

-- TODO This constraint is a little weird. The translation context
-- reflects things needed in comparison to the previous era, whereas the
-- 'AdditionalGenesisConfig' is from Shelley. Ultimately we should drop
-- this and potentially add a new API for dealing with the relationship
-- between `GenesisConfig` and `TranslationContext`.

I'd be interested in discussing approaches, including the one where we take the easy way out, and leave cleaning up the relation for later.

Including @amesgen in the discussion, as we had some talks about this PR earlier.

[nfrisby]

Update: we've discussed a bit on Slack, Crucially, @amesgen noticed the relation to a change I made in this neighborhood when drafting the Conway PR. I think that decoupled the two type families in a way that unblocks Bart here, but hopefully @bartfrenk can confirm that and write a follow-up explanation here 👍

[lehins]

This ticket was automatically and wrongfully closed by github because it had the keyword Closes input-output-hk/ouroboros-consensus#409 in this PR: IntersectMBO/cardano-ledger#3164

[amesgen]

Marked this as "Ready" again. Current status:

The relevant upstream PRs (IntersectMBO/cardano-ledger#3164, IntersectMBO/cardano-ledger#3224) have landed and have been in the cardano-ledger releases for quite some time.

In particular, ShelleyGenesis is no longer parameterized by era (only by crypto), so its weird TranslateEra instances are already gone (see IntersectMBO/cardano-ledger#3224), so the main motivation of this issue has already been resolved.

The remaining work is to remove ShelleyGenesis (in the form of CompactGenesis) from ShelleyLedgerConfig, as we don't actually need all of it (as is already clear from the usage of CompactGenesis), and it is a bit weird to have a ShelleyGenesis in eg the Conway ledger config. This work is complicated a bit by the existing GetGenesisConfig query, so Bart's approach was to do this in two steps. See the commits by Bart in IntersectMBO/ouroboros-network#4091 for how the first step would look like.