-
Notifications
You must be signed in to change notification settings - Fork 12
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
move ledger into separate crate #606
Conversation
@@ -0,0 +1,13 @@ | |||
[package] | |||
name = "cryptarchia-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.
curious, why a separate crate and not a ledger
module?
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 was already a ledger module before 🤔
The idea is to make it clear we would like to have a distinction between the two, in the sense that consensus should have limited influence on ledger choices (e.g. transactions should probably have the same format regardless of the consensus alg, they should work with both carnot and cryptarchia). As a result, those items might better be defined in their own separate crate (for now I've not created a common crate/module since we don't have those components yet).
Of course, some things will have to change (like the header format and leadership proof), and for this reason we have specific ledger modules/crates.
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.
So to summarize:
- consensus/cryptarchia-engine deals with forks and branch selection
- ledger/cryptarchis-ledger deals contents of blocks (header and body)
with the dependency going ledger -> consensus
.
Sounds reasonable.
I guess we are still missing the bit that ties together the two crates
let current_epoch = self.slot.epoch(config); | ||
let new_epoch = slot.epoch(config); | ||
let current_epoch = config.epoch(self.slot); | ||
let new_epoch = config.epoch(slot); |
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.
👍
pub struct Cryptarchia { | ||
local_chain: Branch, | ||
branches: Branches, | ||
ledger: 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.
Prior to this PR, we had this Cryptarchia
struct tying together ledger and branches.
It was making sure that whenever branches was updated, the ledger was also updated (and vice-versa).
This functionality seems to have been dropped in this PR? Or I'm just now seeing where it moved.
It seems we need something like this Cryptarchia
struct in ledger
to keep branches and ledger in sync.
The idea is that this is only true for the format of some components that might be part of the ledger (e.g. a leader proof that needs to access ledger properties like stake).
True, this would be moved to a nomos service. The goal is that we shoudn't need to change the ledger much if we change consensus |
As I understand it, the rest of the nomos service should only be exposed to consensus indirectly through the ledger api right? so in that sense, the ledger should probably be the one that is driving consensus and ensuring it's state is in sync with the consensus state? So what if you modified the Ledger struct to hold an instance of Cryptrachia and when there is a ledger state upate, it goes and updates cryptarchia as well. |
As I see it there are two separate things:
The goal of this separation is that we can develop the two somewhat independently. If an execution zone would like to use another consensus (like Carnot), it should not have to dramatically change the ledger, just change the way it selects which state to consider. I don't see a need for the ledger to track the state of consensus. |
Ok, got it, yeah I agree now, having the consensus instance inside Ledger doesn't make much sense. |
No description provided.