-
Notifications
You must be signed in to change notification settings - Fork 9
Add section about impact on ledger #566
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good for us to be pushing into these details! I made several comments.
|
||
Being part of the ledger state, the voting committee will be stored in ledger snapshots and hence on disk in course of Ledger-HD. Depending on how exactly keys will be registered, the ledger might need to be able to access block headers in order to read the BLS public keys from the operational certificate. As this is not the case today (only block bodies are processed by the ledger), this results in requirement: | ||
|
||
- **REQ-LedgerBlockHeaderAccess**: The ledger must be able to access block headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, this feels too leaky to me.
How about a different approach, in which the Ledger is parameterized over a "certificate validation function" that Consensus provides to the ledger whenever asking the ledger to validate a block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doable. Not going to specify functions in this document, but we could summarize this as:
- REQ-LedgerCertificateVerification: The ledger must be provided with a means to verify certificates in a given block body.
And amending the explanation above/below accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not as easy as that I realize .. IMO the ledger would be in charge of keeping track of the voting committee. So accessing the header is not only about verifying the certificate, but really extracting keys from the opcert to keep track of that?
- REQ-LedgerBLSFromOpCert: The ledger must be provided with the public BLS key from the opcert used in the given block header.
Feels more and more premature design to me.. what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not easy to discuss this without getting very precise.
Even trying for a high-level description (eg what Consensus and Ledger need to cooperatively achieve, however they cooperate), I'm already struggling to sketch it out---but I think that's because I can't think past the opcert details. In particular, are we sure we need a hot-cold split for Leios voting keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'll leave a note here (750d631) that this requirement might change a lot depending in how we manage the BLS keys and design the consensus/ledger interfaces.
are we sure we need a hot-cold split for Leios voting keys?
Is this implied by making BLS public keys part of the opcert? In my (limited) understanding, I was assuming the opcert is signed by an SPO's hot key (= KES key?) and for example that sequence number (a ratchet) can be bumped at any time only using the hot key to authorize it. Adding the BLS public key to it so it can be rotated whenever we can also bump that sequence number sounded meaningful.
Looking at the header_body structure in the Conway CDDL though the dependency graph of what authorizes what is maybe more difficult?
- The
vrf_vkey
seems to be authorized by thekes_signature
on the wholeheader_body
? - In the
operational_cert
:- which key is used for
sigma
? What is it signing? - is
sequence_number
authorized bysigma
or transitively by thekes_signature
?
- which key is used for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ch1bo I made this diagram a while ago https://ouroboros-consensus.cardano.intersectmbo.org/docs/references/block_diagrams_of_data/, I find it difficult to read, but easier than extrapolating from CDDL etc.
Last time I looked, I realized it might help to also list to various invariants that are encoded with colors here as explicit formulae.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that diagram, ocertSigma
is hbVk
signing the little triplet of ocert*
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of those triplet fields is ocertN
. So it's ultimately hbVk
that's authorizing the sequence number. (An illuminating related fact is that SPOs don't actually have to increment their sequence number if they're rotating the KES key exactly when it expires.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That block diagram is good stuff (and would be great cardano-blueprint material!).
Is there a concrete suggestion for this part? Is the note I have added since to this section good enough to get this merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, note it is good. Actually, I suggest moving it to the top of the section. It's plausible that nearly everything described here is ultimately the responsibility of the ouroboros-consensus
code.
The voting committee consists of persistent and non-persistent voters. The persistet voters are to be selected at epoch boundaries using a [Fait Accompli sortition scheme](https://github.com/cardano-scaling/CIPs/blob/leios/CIP-0164/README.md#votes-and-certificates). Hence: | ||
|
||
- **REQ-LedgerCommitteeSelection**: The ledger must select persistent voters in the voting committee at epoch boundaries using the Fait Accompli sortition scheme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that's a ledger requirement. I think it'd make more sense to package that together with all the other things related to the voting scheme, and the only thing that we may ask from the ledger would be to call the appropriate function & store the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had a similar thought since I last wrote here. I think the ledger just needs to make the right stake distribution snapshot available (which is already does for the Praos leader schedule). And then some logic (which the ledger doesn't care about the details), maps that into a leader schedule for voting (fait daccompli with sortition or iid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I think you are both overestimating the level of detail and maturity this impact analysis document aims for. Whether this logic is part of the ledger or another component that is introduced to remain separation of concerns is not that much of a difference. In essence, I just wanted to capture that we must do this committee selection somewhere.
I could make it a requirement for the consensus part, but that would be equally correct/wrong? Can either or both of you formulate a concrete thing you would like to see before this is merged on this part?
This currently covers serialization, new block structure, voting committee and protocol parameter considerations.
8e609cb
to
727f918
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done another pass. Looks good for an initial merge, especially if you include the minor fixups I'm suggesting in this new Review.
|
||
Being part of the ledger state, the voting committee will be stored in ledger snapshots and hence on disk in course of Ledger-HD. Depending on how exactly keys will be registered, the ledger might need to be able to access block headers in order to read the BLS public keys from the operational certificate. As this is not the case today (only block bodies are processed by the ledger), this results in requirement: | ||
|
||
- **REQ-LedgerBlockHeaderAccess**: The ledger must be able to access block headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, note it is good. Actually, I suggest moving it to the top of the section. It's plausible that nearly everything described here is ultimately the responsibility of the ouroboros-consensus
code.
Finally, block validation of the ledger can use the voting committee state to verify certificates contained in ranking blocks: | ||
|
||
- **REQ-LedgerCertificateVerification**: The ledger must verify certificates contained in ranking blocks using the voting committee state. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a very related, but missing requirement: the Ledger needs to provide enough information to Consensus for Consensus to be able to validate individual votes before relaying them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a similar Warning is due as the one about the serialization of votes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. could be seen as part of REQ-LedgerStateVotingCommittee?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's implied, yeah, but explicit seems good here.
Co-authored-by: Nicolas Frisby <[email protected]>
|
||
Where possible, `reapplyTx` is used when we know that the transaction has been fully validated before. For example when refreshing the mempool after adopting a new block. With Leios, a third level of validation is introduced: | ||
|
||
- **REQ-LedgerTxNoValidation** The ledger should provide a way to update the ledger state by just applying a transaction without validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality is already available in Ledger, there is just no specifically named function for it, like applyTx or reapplyTx, since it was never used by anyone.
In other words, this requirement is trivially satisfied with today's Ledger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got a chance to review this PR and it looks very sensible.
I'm still a bit fuzzy on certificates validation and management of the Voting Committee, but between Consensus and Ledger teams I am sure we'll be able to figure out how to best implement it and separate responsibilities.
This currently covers serialization, new block structure, voting committee and protocol parameter considerations.