[Consensus] Linearizer to use commit up to gc_round #20250

akichidis · 2024-11-13T13:25:08Z

Description

Currently when committing/collecting the sub dag we always stop the recursion when we come across the highest committed round for an authority. This works well, but with the upcoming Fast Path changes it can lead to uncommitted FP certificates in the presence of blocks that don't form a proper hash chain.

This could be done if for example an authority A produces blocks A100, A101, A102 , but block A102 uses as ancestor block A100 and never references A101. A101 gets broadcasted and participate in the form of an FP certificate and referenced by others. With the current logic in Linearizer, if A102 gets committed first, then A101 will never get committed as part of subsequent DAG sequencing as we always stop our recursion to the highest committed round. With the new logic we ensure that A101 will get committed as well if it's > gc_round. Having Garbage Collection allows us to limit the recursion depth making the process efficient enough and knowing that we'll not attempt to keep committing blocks too far in the past. Using a reasonable depth (ex 50 - 60) for production shouldn't cause performance troubles.

To achieve the above DagState has been refactored to keep a global commit state of blocks. When we linearise the DAG across commits we do not re-commit previously committed blocks.

The new changes are guarded behind a feature flag and can only be enabled if GC is enabled.

Test plan

CI/PT

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-11-13T13:25:13Z

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	Dec 19, 2024 4:02pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 19, 2024 4:02pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 19, 2024 4:02pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 19, 2024 4:02pm

akichidis · 2024-11-13T13:26:04Z

consensus/core/src/linearizer.rs

+        if context
+            .protocol_config
+            .consensus_linearizer_collect_subdag_v2()
+        {


Decided to split completely the new/previous logic to make things safer during transition. Will clean up after deploy.

Not a big deal as you will clean it up, but from a diff perspective changing the two spots alone that use the new behavior does seem safer/easier to review and you can keep the comments.

Yeah I thought that it would be cleaner to review the logic as whole separately before/after rather than having to navigate through various points that enable different code paths. But if you believe it's tougher to review then I am happy to reconsider on this.

akichidis · 2024-11-13T13:28:32Z

consensus/core/src/dag_state.rs

+                    // When reaching zero will exist anyways as all commits are indexed starting from 1.
+                    index = index.saturating_sub(1);
+                }
+            }


Alternatively, we could retrieve all committed sub-dags within the index range (last_commit_index - gc_depth ... last_commit_index) to restore the commit status for blocks above the gc_round. This approach would avoid iterations and would function correctly without additional computation.
However, the reason this approach guarantees sufficient data restoration is less intuitive, and may be more error-prone in the event of future refactoring. Therefore, I opted for the above approach, which is more straightforward to reason about and ensures clearer correctness. Performance gains with the alternative method are likely negligible.

I think you should just read from store in range and then do the same processing in memory.

I think you should just read from store in range

how far back though? I can do some range reads and process in batch, but I am not sure of the gains.

arun-koshy

This could be done if for example an authority A produces blocks A100, A101, A102 , but block A102 uses as ancestor block A100 and never references A101

This should be considered as Byzantine behavior if we do see this as we currently always link the previous rounds block?

arun-koshy · 2024-11-26T03:07:05Z

consensus/core/src/dag_state.rs

+                    // When reaching zero will exist anyways as all commits are indexed starting from 1.
+                    index = index.saturating_sub(1);
+                }
+            }


I think you should just read from store in range and then do the same processing in memory.

arun-koshy · 2024-11-26T03:10:48Z

consensus/core/src/dag_state.rs

@@ -970,6 +1037,21 @@ impl DagState {
    }
 }

+struct BlockInfo {


BlockInfo seems odd because it contains the block itself? Unless you plan on adding more to this struct maybe BlockWithCommitStatus or BlockWithStatus?

I am aware that @mwtian will most probably re-use this struct with additional info for Fast Path, that's why kept the BlockInfo

arun-koshy · 2024-11-26T03:23:42Z

consensus/core/src/linearizer.rs

+        if context
+            .protocol_config
+            .consensus_linearizer_collect_subdag_v2()
+        {


Not a big deal as you will clean it up, but from a diff perspective changing the two spots alone that use the new behavior does seem safer/easier to review and you can keep the comments.

arun-koshy · 2024-11-26T03:27:29Z

crates/sui-protocol-config/src/lib.rs

+    // Enables the new logic for collecting the subdag in the consensus linearizer. The new logic does not stop the recursion at the highest
+    // committed round for each authority, but allows to commit uncommitted blocks up to gc round (excluded) for that authority.
+    #[serde(skip_serializing_if = "is_false")]
+    consensus_linearizer_collect_subdag_v2: bool,


nit: consensus_linearize_subdag_v2

mwtian · 2024-12-02T17:10:59Z

consensus/core/src/commit_observer.rs

@@ -53,8 +53,12 @@ impl CommitObserver {
        leader_schedule: Arc<LeaderSchedule>,
    ) -> Self {
        let mut observer = Self {
+            commit_interpreter: Linearizer::new(


nit: why reorder this field?

In order to be able to clone first (so could provide it in Linearizer) and pass it as parameter later

mwtian · 2024-12-02T17:19:08Z

consensus/core/src/dag_state.rs

+                        .scan_commits((index..=index).into())
+                        .unwrap_or_else(|e| panic!("Failed to read from storage: {:?}", e));
+                    let Some(commit) = commits.first() else {
+                        info!("Recovering finished, no more commits to recover");


index should be logged as well.

mwtian · 2024-12-02T17:21:39Z

consensus/core/src/dag_state.rs

+                    });
+
+                    // When reaching zero will exist anyways as all commits are indexed starting from 1.
+                    index = index.saturating_sub(1);


Just break if index is <= 1?

mwtian · 2024-12-03T06:07:45Z

consensus/core/src/linearizer.rs

@@ -46,16 +59,19 @@ pub(crate) struct Linearizer {
    /// In memory block store representing the dag state
    dag_state: Arc<RwLock<DagState>>,
    leader_schedule: Arc<LeaderSchedule>,
+    context: Arc<Context>,


nit: let's keep context first in fields / args by default.

mwtian · 2024-12-03T06:15:39Z

consensus/core/src/linearizer.rs

+
+                for ancestor in ancestors {
+                    buffer.push(ancestor.clone());
+                    assert!(


Why not combine the dag_state.set_committed(&ancestor.reference()) asserts and call it after to_commit.push(x.clone());?

Could you please clarify a little bit more on this? Do you suggest moving the dag_state.set_committed(&ancestor.reference()) after the to_commit.push(x.clone()); ? That would not give desired results given that we do BFS here. I mean, if we don't mark the blocks as committed here then we'll revisit them and try to re-commit them. Unless I am totally misunderstanding what you mean.

Sounds good.

mwtian · 2024-12-03T06:21:30Z

consensus/core/src/linearizer.rs

+            leader_block.clone(),
+            last_committed_rounds,
+            &mut dag_state,
+        );


I think we discussed this before but it seems better to drop dag_state here.

mwtian · 2024-12-09T06:37:11Z

consensus/core/src/linearizer.rs

+
+                for ancestor in ancestors {
+                    buffer.push(ancestor.clone());
+                    assert!(


Sounds good.

mystenmark · 2024-12-16T20:11:11Z

crates/sui-protocol-config/src/lib.rs

+        let res = self.feature_flags.consensus_linearize_subdag_v2;
+        assert!(
+            !res || self.gc_depth() > 0,
+            "The consensus linearize sub dag V2 requires GC to be enabled"


mystenmark

protocol changes LGTM

…s committed.

…est committed round per authority but only once we reach gc_round.

…all refactorings.

akichidis requested a review from mystenmark as a code owner November 13, 2024 13:25

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 13, 2024 13:25 — with GitHub Actions Inactive

akichidis commented Nov 13, 2024

View reviewed changes

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 13, 2024 14:19 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 13, 2024 14:21 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 13, 2024 14:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 13, 2024 14:29 View deployment

akichidis had a problem deploying to sui-typescript-aws-kms-test-env November 18, 2024 10:10 — with GitHub Actions Failure

vercel bot deployed to Preview – sui-docs November 18, 2024 10:11 View deployment

akichidis requested review from mwtian and arun-koshy November 18, 2024 10:14

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 10:33 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 18, 2024 10:35 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 18, 2024 11:00 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 18, 2024 11:01 View deployment

arun-koshy approved these changes Nov 26, 2024

View reviewed changes

akichidis force-pushed the akichidis/consensus-commit-rule-modify branch from 502e96f to 7474966 Compare November 27, 2024 13:40

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 27, 2024 13:40 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 27, 2024 13:45 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 27, 2024 17:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 27, 2024 17:48 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env November 27, 2024 18:17 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs November 27, 2024 18:18 View deployment

mwtian approved these changes Dec 3, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs December 5, 2024 16:06 View deployment

vercel bot deployed to Preview – sui-docs December 6, 2024 10:38 View deployment

mwtian approved these changes Dec 9, 2024

View reviewed changes

consensus/core/src/linearizer.rs

for ancestor in ancestors {

buffer.push(ancestor.clone());

assert!(

Copy link

Member

mwtian Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

mystenmark reviewed Dec 16, 2024

View reviewed changes

akichidis added 12 commits December 19, 2024 15:47

[feat] introducing the BlockInfo struct in DagState. Marking blocks a…

ab09fbb

…s committed.

[refactor] Linearizer commit rule so we do not stop recursion on high…

7f14192

…est committed round per authority but only once we reach gc_round.

[feat] fix issue with BlockInfo. Adding more tests and fix others. Sm…

fc7d47c

…all refactorings.

[fix] we don't need to go through already committed blocks.

142ffac

[refactor] small refactoring

526a3a4

[comments] improve comments

5963694

[fix] format

25381b5

[review] address review comments

ee95c63

[fix] format

25ae5f4

[review] address comments

3a66ea0

[review] drop the storage write lock after DAG gets linearized.

bf7e7f4

[review] address review comments

a9b2f85

akichidis force-pushed the akichidis/consensus-commit-rule-modify branch from c3c0502 to a9b2f85 Compare December 19, 2024 15:58

akichidis requested a review from a team as a code owner December 19, 2024 15:58

akichidis temporarily deployed to sui-typescript-aws-kms-test-env December 19, 2024 15:58 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 19, 2024 16:02 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus] Linearizer to use commit up to gc_round #20250

[Consensus] Linearizer to use commit up to gc_round #20250

akichidis commented Nov 13, 2024 •

edited

Loading

vercel bot commented Nov 13, 2024 •

edited

Loading

akichidis Nov 13, 2024 •

edited

Loading

arun-koshy Nov 26, 2024

akichidis Nov 27, 2024

akichidis Nov 13, 2024 •

edited

Loading

arun-koshy Nov 26, 2024

akichidis Nov 27, 2024

arun-koshy left a comment

arun-koshy Nov 26, 2024

arun-koshy Nov 26, 2024

akichidis Nov 27, 2024

arun-koshy Nov 26, 2024

arun-koshy Nov 26, 2024

mwtian Dec 2, 2024

akichidis Dec 5, 2024

mwtian Dec 2, 2024

mwtian Dec 2, 2024

mwtian Dec 3, 2024

mwtian Dec 3, 2024

akichidis Dec 5, 2024

mwtian Dec 9, 2024

mwtian Dec 3, 2024

mwtian Dec 9, 2024

mystenmark Dec 16, 2024

mystenmark left a comment

[Consensus] Linearizer to use commit up to gc_round #20250

Are you sure you want to change the base?

[Consensus] Linearizer to use commit up to gc_round #20250

Conversation

akichidis commented Nov 13, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Nov 13, 2024 • edited Loading

akichidis Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akichidis Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arun-koshy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystenmark left a comment

Choose a reason for hiding this comment

akichidis commented Nov 13, 2024 •

edited

Loading

vercel bot commented Nov 13, 2024 •

edited

Loading

akichidis Nov 13, 2024 •

edited

Loading

akichidis Nov 13, 2024 •

edited

Loading