[Consensus 2.0] Small refactoring in scores for CommittedSubDag #17768

akichidis · 2024-05-15T23:31:16Z

Description

Initialise the consensus scores in committed sub dag during construction. Also always pre-sort the blocks , that should be the default anyways either during commit or recover.

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.

vercel · 2024-05-15T23:31:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 28, 2024 0:57am

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	May 28, 2024 0:57am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	May 28, 2024 0:57am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	May 28, 2024 0:57am

mwtian

I know mutating CommittedSubDag just to set reputation scores is a bit weird. But sending reputation scores to linearizer just to construct CommittedSubDag does not seem the most ideal either. Have we settled on always sending a copy of reputation score with CommittedSubDag, or do we still want to only send a version of scores once? The setter way seems easier to only set the scores once.

akichidis · 2024-05-16T05:54:13Z

I know mutating CommittedSubDag just to set reputation scores is a bit weird. But sending reputation scores to linearizer just to construct CommittedSubDag does not seem the most ideal either. Have we settled on always sending a copy of reputation score with CommittedSubDag, or do we still want to only send a version of scores once? The setter way seems easier to only set the scores once.

My preference is always to have as less mutation as possible as we seemed to touch the sub dag a couple of times after its construction and lots of things can go wrong there - I’ve realised that when I tried to refactor that code. Even if we decide to set the scores only once it’s still fine. Alternatively we can inject the leader schedule in the linearizer.

arun-koshy · 2024-05-16T06:04:22Z

I like the safety provided by not mutating the subdag but I would still vote for keeping the setter method especially because I left a todo so that we only set the scores on leader schedule change. If we do set the scores only on each new version then passing the leader schedule in to the linearizer also would not work as we would have to do it at the commit observer level (that is how I saw it playing out originally atleast)

akichidis · 2024-05-16T06:08:18Z

I like the safety provided by not mutating the subdag but I would still vote for keeping the setter method especially because I left a todo so that we only set the scores on leader schedule change. If we do set the scores only on each new version then passing the leader schedule in to the linearizer also would not work as we would have to do it at the commit observer level (that is how I saw it playing out originally atleast)

Yeah I thought that we can pass this info to the linearizer and set it it self only once when the scores change. Ok , shall I attempt to do that and if you guys still don’t like it we can pass on it?

akichidis · 2024-05-16T08:13:01Z

I like the safety provided by not mutating the subdag but I would still vote for keeping the setter method especially because I left a todo so that we only set the scores on leader schedule change. If we do set the scores only on each new version then passing the leader schedule in to the linearizer also would not work as we would have to do it at the commit observer level (that is how I saw it playing out originally atleast)

Yeah I thought that we can pass this info to the linearizer and set it it self only once when the scores change. Ok , shall I attempt to do that and if you guys still don’t like it we can pass on it?

Oh I also realised that I somehow nuked your TODO @arun-koshy and never put it back - although I did have it on my mind. Apologies for that!

mwtian · 2024-05-16T12:39:33Z

I think the main issue we have here and in the other PR (commit digest) is this mismatch: for output, we are using Commit as the internal commit data structure for persistence and certification, and CommittedSubDag as the in-memory only commit representation with blocks sent to Sui. But during construction, linearizer handle_commit() creates committed subdag first and returns it as output. I think we can do a refactor where linearizer returns Commit and maybe blocks. Then CommitObserver constructs CommittedSubDag before sending it to Sui. Doing this in linearizer may work too. I will take a look at the PR.

akichidis · 2024-05-16T13:08:00Z

I think the main issue we have here and in the other PR (commit digest) is this mismatch: for output, we are using Commit as the internal commit data structure for persistence and certification, and CommittedSubDag as the in-memory only commit representation with blocks sent to Sui. But during construction, linearizer handle_commit() creates committed subdag first and returns it as output. I think we can do a refactor where linearizer returns Commit and maybe blocks. Then CommitObserver constructs CommittedSubDag before sending it to Sui. Doing this in linearizer may work too. I will take a look at the PR.

Yep that's the issue. I've ended up to similar conclusions and refactored how we construct the Commit & the committed subdag to be more coherent (see #17765). Hopefully this would make more sense now.

For this PR I've ended up moving things mostly to the linearizer. Please have another look @mwtian @arun-koshy

arun-koshy

This works for me 👍🏽

…mmitted sub dag

…anges

akichidis · 2024-05-24T11:36:29Z

@mwtian any comments before I get this merged?

mwtian · 2024-05-24T15:03:28Z

consensus/core/src/dag_state.rs

+                            vec![],
+                        ); // We don't mind recovering the reputation scores


Suggested change

vec![],

); // We don't mind recovering the reputation scores

// We don't need to recover reputation scores for unscored_committed_subdags

vec![],

);

mwtian · 2024-05-24T15:07:11Z

consensus/core/src/leader_schedule.rs

+    /// Checks whether the dag state unscored sub dags list is empty. If yes then that means that
+    /// either (1) the system has just started and there is no unscored sub dag available (2) the
+    /// schedule has updated - new scores have been calculated. Both cases we consider as valid cases
+    /// where the schedule has been updated.


I'm not sure (1) is a valid reason. DagState has to fully recover unscored commits before leader schedule can start to make decisions. Otherwise there can be a fork. Or are you seeing something else when starting consensus?

Yes, but if it's a fresh system then there are not sub dags to recover.

mwtian · 2024-05-24T15:10:56Z

consensus/core/src/leader_schedule.rs

+    /// either (1) the system has just started and there is no unscored sub dag available (2) the
+    /// schedule has updated - new scores have been calculated. Both cases we consider as valid cases
+    /// where the schedule has been updated.
+    pub(crate) fn leader_schedule_updated(&self, dag_state: Arc<RwLock<DagState>>) -> bool {


Suggested change

pub(crate) fn leader_schedule_updated(&self, dag_state: Arc<RwLock<DagState>>) -> bool {

pub(crate) fn leader_schedule_updated(&self, dag_state: &RwLock<DagState>) -> bool {

mwtian · 2024-05-24T15:11:30Z

consensus/core/src/linearizer.rs

+        // part of the first sub dag.
+        let schedule_updated = self
+            .leader_schedule
+            .leader_schedule_updated(self.dag_state.clone());


Suggested change

.leader_schedule_updated(self.dag_state.clone());

.leader_schedule_updated(&self.dag_state);

akichidis requested review from arun-koshy and mwtian May 15, 2024 23:31

mwtian reviewed May 16, 2024

View reviewed changes

akichidis force-pushed the akichidis/consensus-mysticeti-update-scores-refactor branch from fac6cf4 to ce92ac3 Compare May 16, 2024 12:31

vercel bot deployed to Preview – sui-docs May 16, 2024 12:32 View deployment

vercel bot deployed to Preview – sui-docs May 16, 2024 13:47 View deployment

arun-koshy approved these changes May 16, 2024

View reviewed changes

akichidis force-pushed the akichidis/consensus-mysticeti-subdag-digest branch from f516e8d to 0c4f61f Compare May 23, 2024 19:53

akichidis requested a review from mystenmark as a code owner May 23, 2024 19:53

Base automatically changed from akichidis/consensus-mysticeti-subdag-digest to main May 23, 2024 22:49

akichidis added 3 commits May 24, 2024 12:29

[refactor] do not update consensus leader scores separately on the co…

bd58be7

…mmitted sub dag

[refactor] update reputation scores only once when leader schedule ch…

aa1bce1

…anges

[test] add test for score calculation

d5e2df7

akichidis force-pushed the akichidis/consensus-mysticeti-update-scores-refactor branch from 4867ca9 to d5e2df7 Compare May 24, 2024 11:32

vercel bot deployed to Preview – sui-docs May 24, 2024 11:34 View deployment

mwtian approved these changes May 24, 2024

View reviewed changes

[review] address review comments

724c589

vercel bot deployed to Preview – sui-docs May 28, 2024 12:57 View deployment

akichidis merged commit 71c997e into main May 28, 2024
48 checks passed

akichidis deleted the akichidis/consensus-mysticeti-update-scores-refactor branch May 28, 2024 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus 2.0] Small refactoring in scores for CommittedSubDag #17768

[Consensus 2.0] Small refactoring in scores for CommittedSubDag #17768

akichidis commented May 15, 2024

vercel bot commented May 15, 2024 •

edited

mwtian left a comment

akichidis commented May 16, 2024

arun-koshy commented May 16, 2024 •

edited

akichidis commented May 16, 2024

akichidis commented May 16, 2024

mwtian commented May 16, 2024 •

edited

akichidis commented May 16, 2024 •

edited

arun-koshy left a comment

akichidis commented May 24, 2024

mwtian May 24, 2024

mwtian May 24, 2024

akichidis May 28, 2024

mwtian May 24, 2024

mwtian May 24, 2024

	pub(crate) fn leader_schedule_updated(&self, dag_state: Arc<RwLock<DagState>>) -> bool {
	pub(crate) fn leader_schedule_updated(&self, dag_state: &RwLock<DagState>) -> bool {

	.leader_schedule_updated(self.dag_state.clone());
	.leader_schedule_updated(&self.dag_state);

[Consensus 2.0] Small refactoring in scores for CommittedSubDag #17768

[Consensus 2.0] Small refactoring in scores for CommittedSubDag #17768

Conversation

akichidis commented May 15, 2024

Description

Test plan

Release notes

vercel bot commented May 15, 2024 • edited

mwtian left a comment

Choose a reason for hiding this comment

akichidis commented May 16, 2024

arun-koshy commented May 16, 2024 • edited

akichidis commented May 16, 2024

akichidis commented May 16, 2024

mwtian commented May 16, 2024 • edited

akichidis commented May 16, 2024 • edited

arun-koshy left a comment

Choose a reason for hiding this comment

akichidis commented May 24, 2024

mwtian May 24, 2024

Choose a reason for hiding this comment

mwtian May 24, 2024

Choose a reason for hiding this comment

akichidis May 28, 2024

Choose a reason for hiding this comment

mwtian May 24, 2024

Choose a reason for hiding this comment

mwtian May 24, 2024

Choose a reason for hiding this comment

vercel bot commented May 15, 2024 •

edited

arun-koshy commented May 16, 2024 •

edited

mwtian commented May 16, 2024 •

edited

akichidis commented May 16, 2024 •

edited