[Consensus 2.0] Calculate committed sub dag digest #17765
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
There was a problem hiding this comment.
Great work figuring out why prologue transaction digest was not changing with forked consensus commits!
For the commit digest without reputation score, my suggestion is to use CommitRef in place of CommitIndex in CommittedSubDag, and take the digest from the ref. Then we don't need to maintain the separate digest computation, and there is one less type of digest. If we ever want to use checkpoint to certify consensus commit, this is the digest to use as well.
Thanks @mwtian . Yes I’ve considered using CommitRef, it was actually my first attempt , but we kind of have a cyclic dependency here. We first construct the sub dag and then the Commit - which calculates its digest . In this case we would have to go back and update the sub dag commit ref which is not ideal. Let me try again and see if there is an elegant way, but those two structs they kind of seem a bit independent and related in the same time. |
|
@mwtian @arun-koshy please take another look after the latest changes |
| } | ||
|
|
||
| impl Display for CommittedSubDag { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| write!( | ||
| f, | ||
| "CommittedSubDag(leader={}, index={}, blocks=[", | ||
| self.leader, self.commit_index | ||
| self.leader, self.commit_ref |
There was a problem hiding this comment.
Update the message above to ref={}?
consensus/core/src/commit.rs
Outdated
| )?; | ||
| for (idx, block) in self.blocks.iter().enumerate() { | ||
| if idx > 0 { | ||
| write!(f, ", ")?; | ||
| } | ||
| write!(f, "{}", block.digest())?; | ||
| } | ||
| write!(f, "])") | ||
| write!(f, "], digest={})", self.commit_ref.digest) |
| ) -> CommittedSubDag { | ||
| ) -> (CommittedSubDag, TrustedCommit) { | ||
| // Grab latest commit state from dag state | ||
| let dag_state = self.dag_state.read(); |
There was a problem hiding this comment.
Eventually I hope we can place where the current commit state is read from DagState, and where commits are buffered in DagState, inside the same function. It give readers of the code more confidence that commit state gets updated by the last commit. We can follow up with another PR after the score refactor. In the short term, maybe we can just add an assertion that the last commit index is current index - 1.
There was a problem hiding this comment.
Eventually I hope we can place where the current commit state is read from DagState, and where commits are buffered in DagState, inside the same function. It give readers of the code more confidence that commit state gets updated by the last commit
Hhm I am not sure I understand the above comment. Could you please elaborate a bit more?
There was a problem hiding this comment.
This does not block the PR. But when reading last commit from DagState, it is unintuitive which exact commit is being read unless pushing commits to DagState also happened in the same function, or there is verification that the last commit from DagState indeed has index - 1.
There was a problem hiding this comment.
sui-core changes are good, but we should enable it in a new proto version for devnet at least.
having this disabled is actually as big a risk as having it disabled. if we enable it and it causes a fork, that just means that we caught the fork as early as possible
| // When disabled the default digest is used instead. It's important to have this guarded behind | ||
| // a flag as it will lead to checkpoint forks. | ||
| #[serde(skip_serializing_if = "is_false")] | ||
| mysticeti_use_committed_subdag_digest: bool, |
There was a problem hiding this comment.
don't we want to enable this, at least for ChainId::Unknown?
There was a problem hiding this comment.
oh sure, was just planning to do as separate PR
|
In fact I would even go further and say I'd really like to see this enabled before we launch on mainnet |
… the blocks in construction. There shouldn't be any other rule sorting them differently
…t. User the Commit digest into the CommittedSubdag. Use the Commit digest for the checkpoint commit info
akichidis
force-pushed
the
akichidis/consensus-mysticeti-subdag-digest
branch
from
f516e8d
to
0c4f61f
Compare
Description
Currently the default digest is being used on the mysticeti committed subdag making the fork detection in checkpoints weaker as included in the
ConsensusCommitInfo. This PR is calculating the committed sub dag digest and enabling it behind a feature flag. It has to be noted that the reputation scores are not included in the sub dag digest as during crash recovery we don't restore the actual scores for every committed sub dag, but only restoring the last ones and sent as part of the last committed sub dag.Test plan
CI
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.