Add `ExecutionTimeEstimate` mode for congestion control #20994

aschran · 2025-01-28T16:18:01Z

Description

ExecutionTimeObserver tracks local measurements of execution time and submits observations to consensus when moving average changes by more than a threshold.
ExecutionTimeEstimator records execution time observations received from consensus and computes stake-weighted medians for use in congestion control.

This PR contains the core implementation of the feature but is missing some important components required for use in production:

storage of received observations for crash recovery
propagation of observations to next epoch
metrics

The heuristics it uses are as simple as possible for a first pass and will likely require some tuning.

Test plan

Added & updated unit tests. Disabled for now by protocol config.

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-28T16:18:05Z

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	Jan 30, 2025 10:43pm

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Jan 30, 2025 10:43pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Jan 30, 2025 10:43pm

- `ExecutionTimeObserver` tracks local measurements of execution time and submits observations to consensus when moving average changes by more than a threshold. - `ExecutionTimeEstimator` records execution time observations received from consensus and computes stake-weighted medians for use in congestion control. This PR contains the core implementation of the feature but is missing some important components required for use in production: - storage of received observations for crash recovery - propagation of observations to next epoch - metrics The heuristics it uses are as simple as possible for a first pass and will likely require some tuning.

mystenmark

Some minor issues, feel free to make the TODOs instead of fixing them immediately, since we aren't enabling this yet anyway.

mystenmark · 2025-01-29T16:33:37Z

crates/sui-node/src/lib.rs

@@ -1364,6 +1365,12 @@ impl SuiNode {
            }
        }

+        ExecutionTimeObserver::spawn(
+            &epoch_store,
+            Box::new(consensus_adapter.clone()),


is this a Box of an Arc? Can we just do the Arc?

this is a Box<dyn SubmitToConsensus>

SubmitToConsensus trait is implemented for Arc<ConsensusAdapter>

mystenmark · 2025-01-29T16:38:59Z

crates/sui-core/src/consensus_validator.rs

+                    if obs.estimates.len()
+                        > epoch_store
+                            .protocol_config()
+                            .max_programmable_tx_commands()


even if max commands is a reasonable starting point for this value, lets put it behind an appropriately named method that just uses that value?

and - it seems unnecessarily high? you wouldn't expect to see a transaction with 1000 unique entry points

you wouldn't expect to normally, but the limit is 1024 commands, so in theory you could?

the current implementation has no code to cap the message size beyond the command limit, so I think it is exactly correct for now to use this limit here

however i agree we may want to add batching, or a smaller observation limit - i put a TODO for that, prob best to add the separate limit at the same time as the (future) code that implements it?

mystenmark · 2025-01-29T16:45:48Z

crates/sui-core/src/authority/authority_per_epoch_store.rs

+        if let Err(e) = tx_local_execution_time.try_send((ptb.clone(), timings, total_duration)) {
+            // This channel should not overflow, but if it does, don't wait; just log an error
+            // and drop the observation.
+            warn!("failed to send local execution time to observer: {e}");


we probably want to use a metric instead of a log for this

Yes, metrics are on the future-PR TODO list, but I put another one here specifically

mystenmark · 2025-01-29T16:52:27Z

crates/sui-core/src/authority/execution_time_estimator.rs

+            let key = ExecutionTimeObservationKey::from_command(command);
+            let local_observation =
+                self.local_observations
+                    .entry(key.clone())


key is heavy enough that it might be worth it to avoid doing unnecessary clones here.

from our chat for posterity: maybe start w metrics and measure utilization/throughput, as that would complicate the code here and not sure how big of a difference it makes?

mystenmark · 2025-01-29T16:58:03Z

crates/sui-core/src/authority/execution_time_estimator.rs

+                if let Err(e) = self
+                    .consensus_adapter
+                    .submit_to_consensus(&[transaction], &epoch_store)
+                    .await


this can block for some time, which will probably cause the channel to fill up.

It may be better to have a separate task that handles submissions. We can send observations to it via a channel, it can pull as many observations from the channel as it can without blocking, send them all in a batch, and repeat.

We probably also need to have some sender-side rate limits on how fast we submit things.

what in here would block for a long time? afaict it does some basic checks and then spawns a separate task for the submission in submit_unchecked?

added a todo for rate limit

looked more, and even though submit_to_consensus is an async trait, the real implementation appears to have zero awaits in it - the only thing that awaits is the mock

makes me think it might be worth making the trait non-async and then changing the mock to just panic if try_send fails (it already has a very large channel buffer)

sounds good - i thought that this had a potentially long delay but obviously i was wrong

mystenmark · 2025-01-29T17:01:44Z

crates/sui-core/src/authority/execution_time_estimator.rs

+            .enumerate()
+            .filter_map(|(i, (_, duration))| {
+                duration.map(|duration| {
+                    let authority_index: AuthorityIndex = i.try_into().unwrap();


i as u32 seems fine here too

almost certainly you are right, but it seems better as a default to use the safe version unless there is a reason not to?

mystenmark · 2025-01-29T17:04:00Z

crates/sui-core/src/authority/execution_time_estimator.rs

+
+            // Send a new observation through consensus if our current moving average
+            // differs too much from the last one we shared.
+            // TODO: Consider only sharing observations for entrypoints with congestion.


there is a further TODO to note, which is that we shouldn't bother sharing if the consensus estimate already agrees with our local estimate

that would require adding a dependency on the estimator to the observer, which is something I was trying to avoid, but yes it's something we def could consider.
updated the todo

mystenmark · 2025-01-30T20:50:55Z

crates/sui-core/src/authority/execution_time_estimator.rs

+        timings: &[ExecutionTiming],
+        total_duration: Duration,
+    ) {
+        assert_eq!(tx.commands.len(), timings.len());


I missed this before - this assert is not valid. Timings can be shorter due to an abort

Good catch, thanks

aschran requested review from a team and mystenmark as code owners January 28, 2025 16:18

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

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

aschran force-pushed the aschran/cc-estimator-2 branch from fc51819 to 8170065 Compare January 28, 2025 18:24

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

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

aschran force-pushed the aschran/cc-estimator-2 branch from 8170065 to 221fce6 Compare January 28, 2025 19:22

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

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

mystenmark approved these changes Jan 29, 2025

View reviewed changes

address comments

1c5e1fd

aschran requested a review from mystenmark January 30, 2025 17:32

mystenmark approved these changes Jan 30, 2025

View reviewed changes

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

aschran temporarily deployed to sui-typescript-aws-kms-test-env January 30, 2025 17:34 — with GitHub Actions Inactive

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

Merge remote-tracking branch 'origin/main' into aschran/cc-estimator-2

f54d36d

aschran force-pushed the aschran/cc-estimator-2 branch from 16ae058 to f54d36d Compare January 30, 2025 18:03

aschran temporarily deployed to sui-typescript-aws-kms-test-env January 30, 2025 18:03 — with GitHub Actions Inactive

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

mystenmark reviewed Jan 30, 2025

View reviewed changes

fix assert

d12d15c

aschran temporarily deployed to sui-typescript-aws-kms-test-env January 30, 2025 22:41 — with GitHub Actions Inactive

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

aschran merged commit d6ed8a5 into main Jan 31, 2025
47 checks passed

aschran deleted the aschran/cc-estimator-2 branch January 31, 2025 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ExecutionTimeEstimate` mode for congestion control #20994

Add `ExecutionTimeEstimate` mode for congestion control #20994

aschran commented Jan 28, 2025

vercel bot commented Jan 28, 2025 •

edited

Loading

mystenmark left a comment

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

aschran Jan 30, 2025

mystenmark Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 29, 2025

aschran Jan 30, 2025

mystenmark Jan 30, 2025

aschran Jan 30, 2025

Add ExecutionTimeEstimate mode for congestion control #20994

Add ExecutionTimeEstimate mode for congestion control #20994

Conversation

aschran commented Jan 28, 2025

Description

Test plan

Release notes

vercel bot commented Jan 28, 2025 • edited Loading

mystenmark 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `ExecutionTimeEstimate` mode for congestion control #20994

Add `ExecutionTimeEstimate` mode for congestion control #20994

vercel bot commented Jan 28, 2025 •

edited

Loading