[consensus] Exclude low scoring ancestors in proposals with time based inclusion/exclusion #19605

arun-koshy · 2024-09-28T00:04:36Z

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-09-28T00:04:40Z

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	Nov 5, 2024 7:16am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Nov 5, 2024 7:16am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Nov 5, 2024 7:16am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 5, 2024 7:16am

## Description To be used in smart ancestor selection [PR#19605](#19605) --- ## 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. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

consensus/core/src/leader_scoring.rs

consensus/core/src/core.rs

consensus/core/src/round_prober.rs

consensus/core/src/ancestor.rs

mwtian · 2024-10-21T16:54:39Z

consensus/core/src/ancestor.rs

+        match ancestor_info.state {
+            // Check conditions to switch to EXCLUDE state
+            AncestorState::Include => {
+                if propagation_score <= low_score_threshold {


Have you considered / tried exclusion based on the gap between a peer's low and high quorum rounds? Maybe we can leave a TODO here.

I did consider this but thought excluding based on the quality (votes) of blocks distributed was also important but I can give this a try. Do you have a sense for a good threshold between low and high quorum rounds. I believe it should not be more than 1 round in the healthy cases but wanted to confirm. Maybe something like 5-10 rounds is a good threshold for exclusion?

consensus/core/src/core.rs

consensus/core/src/dag_state.rs

akichidis · 2024-10-22T20:58:24Z

consensus/core/src/metrics.rs

+            ).unwrap(),
+            included_excluded_proposal_ancestors_count_by_authority: register_int_counter_vec_with_registry!(
+                "included_excluded_proposal_ancestors_count_by_authority",
+                "Total number of included excluded ancestors per authority during proposal. Either weak or strong type",


"included excluded" , this could be a bit confusing (at least to me). Maybe something like Total number of ancestors per authority with 'excluded' status that got included in proposal. Either weak or strong type..

Yeah its a little confusing, changed the description but left the metric name as is. If you have a batter name idea for the metric let me know.

consensus/core/src/core.rs

mwtian

Sorry for the delay in reviewing.

consensus/core/src/dag_state.rs

mwtian · 2024-10-31T04:44:35Z

consensus/core/src/dag_state.rs

@@ -919,8 +968,8 @@ impl DagState {
        self.scoring_subdag.is_empty()
    }

-    pub(crate) fn calculate_scoring_subdag_scores(&self) -> ReputationScores {
-        self.scoring_subdag.calculate_scores()
+    pub(crate) fn calculate_scoring_subdag_distributed_vote_scores(&self) -> ReputationScores {


Since we have done the protocol upgrade, is it still worth renaming?

I renamed it back to calculate_scoring_subdag_scores but will go in and cleanup more of the leader scoring code now that we have done the protocol upgrade in a separate PR

consensus/core/src/core.rs

mwtian · 2024-10-31T05:22:01Z

consensus/core/src/core.rs

+            self.context.metrics.node_metrics.smart_selection_wait.inc();
+            debug!("Only found {} stake of good ancestors to include for round {clock_round}, will wait for more.", parent_round_quorum.stake());
+            return vec![];
+        }


Is it ok to continue when not smart_select?

Yes, when we are not in smart_select mode which means we are in the force=true try_propose mode we will just fall back to essentially the old behavior where we will include enough "EXCLUDE" ancestors until we have reached a quorum for the parent round.

consensus/core/src/core.rs

Add time based inclusion logic

mwtian · 2024-11-05T22:29:47Z

consensus/core/src/ancestor.rs

+    // Inclusion threshold is based on network quorum round
+    const INCLUSION_THRESHOLD_PERCENTAGE: u64 = 90;
+
+    pub(crate) fn new(context: Arc<Context>, propagation_scores: ReputationScores) -> Self {


Is it necessary to pass in propagation_scores, instead of calling set_propagation_scores()?

mwtian · 2024-11-06T22:57:22Z

consensus/core/src/ancestor.rs

+        // If round prober has not run yet and we don't have network quorum round,
+        // it is okay because network_low_quorum_round will be zero and we will
+        // include all ancestors until we get more information.
+        let network_low_quorum_round = self.calculate_network_low_quorum_round();


As we discussed, it may be safer to use a high quorum round instead for network round.

mwtian · 2024-11-06T23:13:14Z

consensus/core/src/core.rs

@@ -370,6 +384,43 @@ impl Core {
            }
        }

+        // TODO: produce the block for the clock_round. As the threshold clock can advance many rounds at once (ex
+        // because we synchronized a bulk of blocks) we can decide here whether we want to produce blocks per round


I think we are reasonably sure we only want to build the latest blocks. Earlier blocks do not contribute to voting, and they can be close to GC or voting limit.

arun-koshy requested review from akichidis and mwtian September 28, 2024 00:04

arun-koshy force-pushed the ak/timed-inclusion branch from 65183ba to e33f913 Compare September 30, 2024 20:01

vercel bot deployed to Preview – sui-docs September 30, 2024 20:05 View deployment

vercel bot deployed to Preview – sui-docs October 1, 2024 21:27 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from 57062c2 to c209697 Compare October 1, 2024 22:34

vercel bot deployed to Preview – sui-docs October 1, 2024 22:35 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from c209697 to 59ae4a5 Compare October 2, 2024 01:44

vercel bot deployed to Preview – sui-docs October 2, 2024 01:46 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from 59ae4a5 to 628d399 Compare October 2, 2024 07:34

vercel bot deployed to Preview – sui-docs October 2, 2024 07:35 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from 5f24a49 to f3dcbaa Compare October 2, 2024 23:32

vercel bot deployed to Preview – sui-docs October 2, 2024 23:34 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from f3dcbaa to 58bd29c Compare October 3, 2024 02:18

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

arun-koshy marked this pull request as ready for review October 3, 2024 16:53

arun-koshy mentioned this pull request Oct 4, 2024

[consensus] return quorum rounds from round prober #19703

Merged

8 tasks

arun-koshy force-pushed the ak/timed-inclusion branch from 58bd29c to 361d90d Compare October 5, 2024 00:43

vercel bot deployed to Preview – sui-docs October 5, 2024 00:44 View deployment

akichidis reviewed Oct 7, 2024

View reviewed changes

consensus/core/src/leader_scoring.rs Outdated Show resolved Hide resolved

consensus/core/src/core.rs Outdated Show resolved Hide resolved

consensus/core/src/core.rs Show resolved Hide resolved

consensus/core/src/core.rs Outdated Show resolved Hide resolved

arun-koshy force-pushed the ak/timed-inclusion branch from 361d90d to ee3bf25 Compare October 10, 2024 07:07

vercel bot deployed to Preview – sui-docs October 10, 2024 07:12 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from ee3bf25 to cbf0789 Compare October 21, 2024 16:25

vercel bot deployed to Preview – sui-docs October 21, 2024 16:26 View deployment

mwtian reviewed Oct 21, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs October 21, 2024 20:41 View deployment

akichidis reviewed Oct 22, 2024

View reviewed changes

arun-koshy requested a review from mystenmark as a code owner October 23, 2024 00:58

vercel bot deployed to Preview – sui-docs October 23, 2024 01:01 View deployment

mwtian requested review from akichidis and mwtian October 31, 2024 04:27

mwtian reviewed Oct 31, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs November 5, 2024 02:22 View deployment

vercel bot deployed to Preview – sui-docs November 5, 2024 05:52 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from a57e9df to 0ec03ed Compare November 5, 2024 05:54

vercel bot deployed to Preview – sui-docs November 5, 2024 05:55 View deployment

arun-koshy force-pushed the ak/timed-inclusion branch from 0ec03ed to 9967b40 Compare November 5, 2024 06:00

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

Exclude low scoring ancestors in proposals

49d995d

Add time based inclusion logic

arun-koshy force-pushed the ak/timed-inclusion branch from 9967b40 to 40f4a4a Compare November 5, 2024 06:25

arun-koshy temporarily deployed to sui-typescript-aws-kms-test-env November 5, 2024 06:25 — with GitHub Actions Inactive

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

include only ancestors sync'd 90% & no weak links for EXCLUDE ancestors

e0d5607

arun-koshy force-pushed the ak/timed-inclusion branch from 40f4a4a to e0d5607 Compare November 5, 2024 07:14

arun-koshy temporarily deployed to sui-typescript-aws-kms-test-env November 5, 2024 07:14 — with GitHub Actions Inactive

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

mwtian self-requested a review November 5, 2024 22:27

mwtian approved these changes Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[consensus] Exclude low scoring ancestors in proposals with time based inclusion/exclusion #19605

[consensus] Exclude low scoring ancestors in proposals with time based inclusion/exclusion #19605

arun-koshy commented Sep 28, 2024 •

edited

Loading

vercel bot commented Sep 28, 2024 •

edited

Loading

mwtian Oct 21, 2024

arun-koshy Oct 21, 2024

akichidis Oct 22, 2024

arun-koshy Oct 22, 2024

mwtian left a comment

mwtian Oct 31, 2024

arun-koshy Nov 5, 2024

mwtian Oct 31, 2024

arun-koshy Nov 5, 2024

mwtian Nov 5, 2024

mwtian Nov 6, 2024

mwtian Nov 6, 2024

[consensus] Exclude low scoring ancestors in proposals with time based inclusion/exclusion #19605

Are you sure you want to change the base?

[consensus] Exclude low scoring ancestors in proposals with time based inclusion/exclusion #19605

Conversation

arun-koshy commented Sep 28, 2024 • edited Loading

Release notes

vercel bot commented Sep 28, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arun-koshy commented Sep 28, 2024 •

edited

Loading

vercel bot commented Sep 28, 2024 •

edited

Loading