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 :: !(SL.ByronTranslationContext era)
    }

where in the ledger we have:

type family ByronTranslationContext era

type instance ByronTranslationContext ShelleyEra = SL.ParedDownShelleyGenesis
type instance ByronTranslationContext AllegraEra = ()
type instance ByronTranslationContext MaryEra    = ()
type instance ByronTranslationContext AlonzoEra  = ()
type instance ByronTranslationContext BabbageEra = ()
type instance ByronTranslationContext ConwayEra  = ()

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 #3878 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 :+1: 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: https://github.com/input-output-hk/cardano-ledger/pull/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: https://github.com/input-output-hk/cardano-ledger/pull/3053#discussion_r1015751613