[Consensus] process sync committed sub dags #20956

akichidis · 2025-01-23T14:16:59Z

Description

The PR is changing the way we are processing the synchronized committed sub dags when GC is enabled. This was a necessity given that it's possible during GC for committed sub dags to drop dependencies that are necessary to accept vote and decision round blocks, effectively leading us to having to fetch the missing blocks via the synchronizer. By empirical examination there is a correlation between the missing blocks and the gc depth - the lower the gc depth the higher the number of missing blocks that need to be fetched.

With this new approach we attempt to commit the synchronized committed sub dag without waiting for the commit rule to run (hence wait to have accepted blocks to form the voting & decision rounds). Attention needs to be given that to avoid edge cases we need to make sure that the synchronized sub dags needs to be processed first on their own , and then run on a next iteration the commit rule for any other leaders. That guarantees that any accepted blocks due to commit sync will not run into "missing ancestor" issues.

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 · 2025-01-23T14:17:04Z

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	Feb 19, 2025 6:26pm

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Feb 19, 2025 6:26pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Feb 19, 2025 6:26pm

akichidis · 2025-01-28T19:31:27Z

consensus/core/src/core.rs

+            if decided_leaders.is_empty() {
+                // TODO: limit commits by commits_until_update, which may be needed when leader schedule length is reduced.
+                decided_leaders = self.committer.try_decide(self.last_decided_leader);


Something to note here; we could avoid even linearizing the DAG and just produce the necessary outputs, persist etc. However right now:

uitlising the same path saves us from having to specially handle the storing of committed sub dags, move GC rounds etc

provides some extra layer of confirmation as we can - and we do in the end - make sure that the same sub dag is produced with the current state

mwtian

The high level strategy lgtm. This should be simpler than skipping & rewriting commit_observer.handle_commit().

mwtian · 2025-01-29T00:57:16Z

consensus/core/src/authority_service.rs

@@ -724,6 +726,7 @@ mod tests {
        }
    }

+    #[async_trait]


mwtian · 2025-01-29T00:57:39Z

consensus/core/src/block_manager.rs

@@ -84,17 +84,15 @@ impl BlockManager {
    /// Tries to accept the provided blocks assuming that all their causal history exists. The method
    /// returns all the blocks that have been successfully processed in round ascending order, that includes also previously
    /// suspended blocks that have now been able to get accepted. Method also returns a set with the missing ancestor blocks.
+    /// When the `commit_sync_gc_round_override` is > 0 then the method will skip any missing ancestors that are <= `commit_sync_gc_round_override` round. This


mwtian · 2025-01-29T00:57:50Z

consensus/core/src/block_manager.rs

    pub(crate) fn try_accept_blocks(
        &mut self,
        mut blocks: Vec<VerifiedBlock>,
    ) -> (Vec<VerifiedBlock>, BTreeSet<BlockRef>) {
        let _s = monitored_scope("BlockManager::try_accept_blocks");

        blocks.sort_by_key(|b| b.round());
-        debug!(


Is this moved somewhere?

mwtian · 2025-01-29T00:59:41Z

consensus/core/src/commit_syncer.rs

+            match self
+                .inner
+                .core_thread_dispatcher
+                .add_commits(commits, blocks)


The change here is minimal which is nice.

mwtian · 2025-01-29T06:53:38Z

consensus/core/src/universal_committer.rs

+                .first()
+                .expect("Synced commits should not be empty");
+            if synced_commit.index() <= last_commit_index {
+                info!(


info seems too much logging here

arun-koshy

The direction of the PR LGTM!

arun-koshy · 2025-01-29T08:45:36Z

consensus/core/src/universal_committer.rs

+    // The function returns the list of decided leaders and updates in place the remaining synced commits. If empty vector is returned, it means that
+    // there are no synced commits to be committed as `synced_commits` is either empty or all of the synced commits are already committed.
+    #[tracing::instrument(skip_all)]
+    pub(crate) fn try_decide_synced(


This is cool! It feels like this is in general how we should handle the trusted commits regardless of GC. Did you notice a performance boost for catchup doing this instead of trying to run the commit rule for the synced commits?

I haven't benchmarked this but I'll do. So far didn't see any difference on the opposite direction at least.

arun-koshy · 2025-01-29T08:56:58Z

consensus/core/src/core.rs

+    ) -> ConsensusResult<BTreeSet<BlockRef>> {
+        let _scope = monitored_scope("Core::add_commit");
+
+        // We want to enable the commit process logic when GC is enabled.


why not just do this even if GC is not enabled?

Well to be fair we could enable this without GC. Not sure if I wouldn't still feature flag it though. Let me think a bit about it.

arun-koshy · 2025-01-29T09:04:26Z

consensus/core/src/core.rs

-            // is reduced.
-            let decided_leaders = self.committer.try_decide(self.last_decided_leader);
+            // Always try to process the synced commits first
+            let mut decided_leaders = self


one thing to consider here is that you moved some of the logic of leader schedule into universal committer where try_decide does not work like that and we kept the commit logic as a black box that just spits out decided leaders. What do you think about continuing to keep that separation of logic?

Yeah my initial iteration was without moving part of that logic in universal committer and keeping it in Core. We can certainly do that. It's just that the truncation of the leaders is a bit more complex and decided to do everything in the try_decide_synced method. I am open though to suggestions here.

One suggestion is to remove try_decide_certified from universal committer and keep it as a separate method in core i.e. get_certified_commit_leaders. We are actually bypassing the committer with your new logic so it makes sense to not use the component at all.

arun-koshy · 2025-01-29T09:06:23Z

consensus/core/src/core.rs

+                .try_decide_synced(&mut synced_commits, commits_until_update);
+
+            // If the synced `decided_leaders` is empty then try to run the decision rule.
+            if decided_leaders.is_empty() {


Why can't you update self.last_decided_leader from the synced decided leaders and then run the commit rule to fill whatever remaining space is left for the schedule?

We don't want to run the commit rule until the synced leaders have been committed and GC rounds have been moved. That's important to make sure that when we try to run the commit rule we have the most updated GC and there is no possibility to try retrieve causal history (ex when we figure out the certificate support) that doesn't exist.

mwtian · 2025-02-12T19:26:19Z

consensus/core/src/core.rs

+                    certified_commit.index(),
+                    last_commit_index
+                );
+                certified_commits.remove(0);


This seems inefficient in a loop. We may as well filter certified_commits, then check the 1st element for gap.

You are right about the inefficiency. Let me refactor this.

mwtian · 2025-02-12T19:29:50Z

consensus/core/src/core.rs

+                certified_commits.remove(0);
+            } else {
+                // Make sure that the first commit we find is the next one in line and there is no gap.
+                if certified_commit.index() != last_commit_index + 1 {


I think this check is pretty critical to the correctness of Core::add_certified_commits(). Personally I would be comfortable having it called first thing in Core::add_certified_commits() or included in the function itself. Otherwise we may loose or reorder this check after refactoring. I think try_decide_certified() can assume the CertifiedCommits have been filtered and validated, and can just assert_eq(...). Wdyt?

I see the point on the Core::add_certified_commits. It's just that the try_commit can be practically called from anywhere within the Core and the caller should always be mindful to do the validation. But given the limit scope we can continue with the assumption here and I can move the checks. Left me refactor this a bit.

mwtian · 2025-02-12T19:50:13Z

consensus/core/src/core.rs

+            return Vec::new();
+        }
+
+        let to_commit = if certified_commits.len() >= limit {


For logic above this line, I think they are better to be around core.rs. For logic below, they are indeed closer to commit rule. But tbh I'm not sure if we should actually move leader schedule commit loop to core as well instead. There are more refactor and we don't have to make things perfect in this PR.

Ok I'll keep the logic for now in Core.rs. I agree that overall we could work out some refactoring with the leader schedule loop etc. Let's do that on upcoming PR.

…above the last committed leader's gc.

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 23, 2025 14:17 — with GitHub Actions Inactive

akichidis requested review from mwtian and arun-koshy January 24, 2025 14:05

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 27, 2025 16:06 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 27, 2025 16:08 View deployment

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from 39f1a9b to fd30e73 Compare January 28, 2025 18:26

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 28, 2025 18:26 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 28, 2025 18:29 View deployment

akichidis commented Jan 28, 2025

View reviewed changes

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 28, 2025 19:58 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 28, 2025 19:59 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 28, 2025 20:06 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 28, 2025 20:07 View deployment

mwtian reviewed Jan 29, 2025

View reviewed changes

arun-koshy reviewed Jan 29, 2025

View reviewed changes

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 30, 2025 13:37 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 30, 2025 13:39 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 30, 2025 19:37 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 30, 2025 19:38 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env January 31, 2025 18:19 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 31, 2025 18:21 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 14:19 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 14:20 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 14:39 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 14:40 View deployment

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 15:22 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 15:23 View deployment

akichidis marked this pull request as ready for review February 3, 2025 15:25

akichidis requested a review from a team as a code owner February 3, 2025 15:25

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 12, 2025 13:39 — with GitHub Actions Inactive

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from eb9c357 to e0d776a Compare February 12, 2025 13:40

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 12, 2025 13:40 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 12, 2025 13:45 View deployment

mwtian requested review from mwtian and arun-koshy February 12, 2025 18:33

mwtian approved these changes Feb 12, 2025

View reviewed changes

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 13, 2025 13:13 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 13, 2025 13:14 View deployment

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from 8baad55 to 6209b58 Compare February 14, 2025 11:28

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 14, 2025 11:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 14, 2025 11:29 View deployment

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from 6209b58 to 76c6717 Compare February 17, 2025 17:43

vercel bot deployed to Preview – sui-docs February 17, 2025 17:44 View deployment

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from 76c6717 to c6b4dfb Compare February 17, 2025 18:02

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 17, 2025 18:02 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 17, 2025 18:04 View deployment

akichidis force-pushed the akichidis/commit-syncer-process-blocks branch from c6b4dfb to 537f02b Compare February 18, 2025 18:25

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 18, 2025 18:25 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 18, 2025 18:26 View deployment

[chore] adding metric for the gced blocks.

Verified

This commit was signed with the committer’s verified signature.

Julesssss Jules

GPG key ID: 9A9B6B5E39C41CFD

Verified
Learn about vigilant mode

Loading
Loading status checks…

abc2e8a

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 19, 2025 15:46 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 19, 2025 15:47 View deployment

[review] address review comments.

Verified

This commit was signed with the committer’s verified signature.

Julesssss Jules

GPG key ID: 9A9B6B5E39C41CFD

Verified
Learn about vigilant mode

Loading
Loading status checks…

41bc237

akichidis temporarily deployed to sui-typescript-aws-kms-test-env February 19, 2025 18:24 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 19, 2025 18:26 View deployment

akichidis merged commit 5c1fdc3 into main Feb 21, 2025
47 checks passed

akichidis deleted the akichidis/commit-syncer-process-blocks branch February 21, 2025 11:24

arun-koshy approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus] process sync committed sub dags #20956

[Consensus] process sync committed sub dags #20956

akichidis commented Jan 23, 2025

vercel bot commented Jan 23, 2025 •

edited

Loading

akichidis Jan 28, 2025

mwtian left a comment

mwtian Jan 29, 2025

mwtian Jan 29, 2025

mwtian Jan 29, 2025

mwtian Jan 29, 2025

mwtian Jan 29, 2025

arun-koshy left a comment

arun-koshy Jan 29, 2025

akichidis Jan 29, 2025

arun-koshy Jan 29, 2025

akichidis Jan 29, 2025

arun-koshy Jan 29, 2025

akichidis Jan 29, 2025 •

edited

Loading

arun-koshy Feb 7, 2025

arun-koshy Jan 29, 2025

akichidis Jan 29, 2025

mwtian Feb 12, 2025

akichidis Feb 13, 2025

mwtian Feb 12, 2025 •

edited

Loading

akichidis Feb 13, 2025

mwtian Feb 12, 2025

akichidis Feb 13, 2025

[Consensus] process sync committed sub dags #20956

[Consensus] process sync committed sub dags #20956

Conversation

akichidis commented Jan 23, 2025

Description

Test plan

Release notes

vercel bot commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

mwtian 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

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

akichidis Jan 29, 2025 • edited Loading

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

mwtian Feb 12, 2025 • edited Loading

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

vercel bot commented Jan 23, 2025 •

edited

Loading

akichidis Jan 29, 2025 •

edited

Loading

mwtian Feb 12, 2025 •

edited

Loading