Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need to access epoch nonce from ChainDepState #3864

Closed
Jimbo4350 opened this issue May 16, 2022 · 10 comments · Fixed by IntersectMBO/ouroboros-network#3758
Closed

Need to access epoch nonce from ChainDepState #3864

Jimbo4350 opened this issue May 16, 2022 · 10 comments · Fixed by IntersectMBO/ouroboros-network#3758
Labels
consensus Related to the Consensus Mechanism era: babbage type: blocker Major issue, the tagged version cannot be released type: internal feature Non user-facing functionality user type: internal Created by an IOG employee Vasil_blocker

Comments

@Jimbo4350
Copy link
Contributor

We need a way to access the epoch nonce from the ChainDepState while using TPraos and Praos

@nfrisby
Copy link
Contributor

nfrisby commented May 17, 2022

Copied from my Slack message:

What's the summary of the need for cardano-node to access it? Testing? Logging? etc? That'd inform how Consensus would add such an observer function to our API.

@nfrisby nfrisby added the consensus Related to the Consensus Mechanism label May 17, 2022
@nfrisby
Copy link
Contributor

nfrisby commented May 17, 2022

It looks like nextEpochEligibleLeadershipSlots is the only one that uses the nonce from the ChainDepState. Is that right? @Jimbo4350

@nfrisby
Copy link
Contributor

nfrisby commented May 17, 2022

@nc6 assuming nextEpochEligibleLeadershipSlots is the only one, then I think what we need is a new class along the lines of "has Praos nonces".

If it's in Consensus, I imagine it'd be named LedgerSupportsNode.

Sound right? Or should it be in Ledger/is there already something like that in Ledger? Or maybe there are plans to lift this getMyLeadershipSlots function out of node and that's what we should be abstracting over?

@Jimbo4350
Copy link
Contributor Author

Jimbo4350 commented May 17, 2022

nextEpochEligibleLeadershipSlots and currentEpochEligibleLeadershipSlots both use the epoch nonce.

If it's in Consensus, I imagine it'd be named LedgerSupportsNode

Yes I imagined a type class that had a method to retrieve the nonce.

Or should it be in Ledger/is there already something like that in Ledger?

So AFAIU the ChainDepState type family has been moved to consensus, so I would imagine the type class should live there as well. That is a design decision you will need to take up with the ledger team as I don't have all the context.

Or maybe there are plans to lift this getMyLeadershipSlots function out of node and that's what we should be abstracting over?

There is getLeaderSchedule in the ledger which I based my functions on. So far there are no plans to lift my functions out of the node into the ledger.

@nfrisby
Copy link
Contributor

nfrisby commented May 17, 2022

I'm at least going to begin with a focus on nextEpochEligibleLeadershipSlots. https://github.com/input-output-hk/cardano-node/blob/902fc77dee9e0f40c36e2c8b905dc8d5c51e97d9/cardano-api/src/Cardano/Api/LedgerState.hs#L1289

It's polymorphic over a type variable era, which will be one of the token empty types declared in cardano-node's Cardano.Api.Eras module. Consensus has plenty of era type variables as well, so it's easy to get mixed up, but those are instantiated by the cardano-ledger types, eg Cardano.Ledger.Alonzo.AlonzoEra.

I think step 1 will be to declare another family along side in cardano-node Cardano.Api.Eras.ShelleyLedgerEra that maps from the cardano-node era type variable to the protocol that it uses (it seems likely you already have this on your integration branch?). In particular, BabbageEra will be mapped to Ouroboros.Consensus.Protocol.Praos.Praos and all the other Shelley eras will be mapped to Ouroboros.Consensus.Protocol.TPraos.TPraos. That's step 1 because Consensus's Ouroboros.Consensus.Protocol.Abstract.ChainDepState type family is indexed over protocol, not over era (be it node's or ledger's). It's word soup, at this point, but I think ShelleyConsensusProtocol might be a good name -- I'll use that in the rest of this comment.

On the Consensus side, I think we'll define this:

data PraosNonces = PraosNonces {
    evolvingNonce       :: !Nonce,
    candidateNonce      :: !Nonce,
    epochNonce          :: !Nonce,
    -- | Nonce constructed from the hash of the Last Applied Block
    labNonce            :: !Nonce,
    -- | Nonce corresponding to the LAB nonce of the last block of the previous
    -- epoch
    previousLabNonce :: !Nonce
}

class ConsensusProtocol p => ProtocolSupportsNode p where
  getNodeNonces :: ChainDepState p -> NodeNonces
  
instance ProtocolSupportsNode TPraos
instance ProtocolSupportsNode Praos

I think if you add ProtocolSupportsNode (ShelleyConsensusProtocol era) to the context of nextEpochEligibleLeadershipSlots, then you'll be able to call getNodeNonces there and that'll unblock you.

@nc6
Copy link
Contributor

nc6 commented May 18, 2022

I would be tempted to avoid going down this route, and instead add a specific query GetEpochNonce, which would be supported in all cardano eras. The chain dep state is only accessible through a Debug query, which we should really be avoiding using in production. It would also be better to avoid digging through the internals in such a manner.

@Jimbo4350
Copy link
Contributor Author

Jimbo4350 commented May 18, 2022

add a specific query GetEpochNonce

This also works

@nfrisby
Copy link
Contributor

nfrisby commented May 20, 2022

At least nextEpochEligibleLeadershipSlots needs more than just the epoch nonce (at least its current code does). So GetEpochNonce wouldn't cut it.


Ah, I see now that the nextEpochEligibleLeadershipSlots call is being fed by QueryProtocolState and

toConsensusQueryShelleyBased erainmode QueryProtocolState =
    Some (consensusQueryInEraInMode erainmode (Consensus.GetCBOR Consensus.DebugChainDepState))

I suspect we'll need a broader plan for QueryLeadershipSchedule et al to eventually not rely on any queries that Consensus considers Debug* queries. I'd rather not make an hurried incremental attempt at beginning that now, involving serialization schemes etc, when I don't think I have the whole picture and no one who does has spare time.

So I'll prepare a PR that just adds the class for Jordan to use now. We can remove it later as part of the aforementioned plan.

nfrisby added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
Fixes Issue IntersectMBO/cardano-node#3864.

This is a continuation of the somewhat kludgy implementation of some of the
Cardano API queries via what Consensus at least currently considers `Debug*`
queries. So this new `PraosProtocolSupportsNode` class should likely be removed
once that situation is improved with a plan that is more stable/maintainable in
the long term.
nfrisby added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
Fixes Issue IntersectMBO/cardano-node#3864.

This is a continuation of the somewhat kludgy implementation of some of the
Cardano API queries via what Consensus at least currently considers `Debug*`
queries. So this new `PraosProtocolSupportsNode` class should likely be removed
once that situation is improved with a plan that is more stable/maintainable in
the long term.
nfrisby added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
Fixes Issue IntersectMBO/cardano-node#3864.

This is a continuation of the somewhat kludgy implementation of some of the
Cardano API queries via what Consensus at least currently considers `Debug*`
queries. So this new `PraosProtocolSupportsNode` class should likely be removed
once that situation is improved with a plan that is more stable/maintainable in
the long term.
nfrisby added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
Fixes Issue IntersectMBO/cardano-node#3864.

This is a continuation of the somewhat kludgy implementation of some of the
Cardano API queries via what Consensus at least currently considers `Debug*`
queries. So this new `PraosProtocolSupportsNode` class should likely be removed
once that situation is improved with a plan that is more stable/maintainable in
the long term.
@dcoutts
Copy link
Contributor

dcoutts commented May 20, 2022

I agree that "doing this properly" would mean not using debug queries to just grab the state, but introducing new queries. Probably those ought to be per-era queries, since it's not guaranteed that future eras are Praos and thus support all these things. The alternative is a query to actually calculate the leader schedule for the current / next era.

nfrisby added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
Fixes Issue IntersectMBO/cardano-node#3864.

This is a continuation of the somewhat kludgy implementation of some of the
Cardano API queries via what Consensus at least currently considers `Debug*`
queries. So this new `PraosProtocolSupportsNode` class should likely be removed
once that situation is improved with a plan that is more stable/maintainable in
the long term.
iohk-bors bot added a commit to IntersectMBO/ouroboros-network that referenced this issue May 20, 2022
3758: protocol: add PraosProtocolSupportsNode class r=nfrisby a=nfrisby

Fixes IntersectMBO/cardano-node#3864. Single commit; see its message.

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
@dorin100 dorin100 added type: internal feature Non user-facing functionality type: blocker Major issue, the tagged version cannot be released user type: internal Created by an IOG employee era: babbage labels Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus Related to the Consensus Mechanism era: babbage type: blocker Major issue, the tagged version cannot be released type: internal feature Non user-facing functionality user type: internal Created by an IOG employee Vasil_blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants