From 4b0a00828d5cccb7976b03e63340a1972a94353c Mon Sep 17 00:00:00 2001 From: Andrey Chursin Date: Thu, 8 Sep 2022 13:35:37 -0700 Subject: [PATCH] Increase base consensus timeout and make it a config Sui has a timeout on sending transaction to the nw consensus and automatically adjusts the timeout based on recent latency from nw it has seen in case of success, or linearly increases in case of timeout. In the stress test network we see quite a few of nw timeouts from the sui transaction submission side, and we indeed [see](http://grafana.shared.internal.sui.io:3000/goto/wqnUWbGVz?orgId=1) that control delay is adjusted accordingly to the timeouts and nw is not stuck. However, it seems that current timeout adjustment logic drops timeout pretty quickly which causes timeouts again. While this is not end of the world, it is noise and probably suboptimal. It also can be noticed that current base timeout value is very close to the actual consensus delay, so the timeout has to be frequently adjusted. Therefore, a short-medium term fix (this PR) is just to bump the delay step for timeout calculations so that it is further away from median nw latency. In the future we can improve timeout calculation by using weighted avg over a longer window instead of weighting current and next timing only. --- crates/sui-config/src/builder.rs | 1 + crates/sui-config/src/node.rs | 1 + ...ests__network_config_snapshot_matches.snap | 7 +++++++ crates/sui-core/src/authority_server.rs | 3 ++- crates/sui-core/src/consensus_adapter.rs | 19 +++++++++---------- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/sui-config/src/builder.rs b/crates/sui-config/src/builder.rs index 908edbd40d8b1..37f2eccfa2fc4 100644 --- a/crates/sui-config/src/builder.rs +++ b/crates/sui-config/src/builder.rs @@ -180,6 +180,7 @@ impl ConfigBuilder { let consensus_config = ConsensusConfig { consensus_address, consensus_db_path, + delay_step: Some(15_000), narwhal_config: Default::default(), }; diff --git a/crates/sui-config/src/node.rs b/crates/sui-config/src/node.rs index f14183923328e..3603c6f97eeec 100644 --- a/crates/sui-config/src/node.rs +++ b/crates/sui-config/src/node.rs @@ -152,6 +152,7 @@ impl NodeConfig { pub struct ConsensusConfig { pub consensus_address: Multiaddr, pub consensus_db_path: PathBuf, + pub delay_step: Option, pub narwhal_config: ConsensusParameters, } diff --git a/crates/sui-config/tests/snapshots/snapshot_tests__network_config_snapshot_matches.snap b/crates/sui-config/tests/snapshots/snapshot_tests__network_config_snapshot_matches.snap index d5f2b402c44fe..379ee49810f9a 100644 --- a/crates/sui-config/tests/snapshots/snapshot_tests__network_config_snapshot_matches.snap +++ b/crates/sui-config/tests/snapshots/snapshot_tests__network_config_snapshot_matches.snap @@ -15,6 +15,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -55,6 +56,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -95,6 +97,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -135,6 +138,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -175,6 +179,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -215,6 +220,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms @@ -255,6 +261,7 @@ validator_configs: consensus-config: consensus-address: "" consensus-db-path: /tmp/foo/ + delay-step: 15000 narwhal-config: header_size: 1000 max_header_delay: 100ms diff --git a/crates/sui-core/src/authority_server.rs b/crates/sui-core/src/authority_server.rs index 84d20e1cfe659..0131345c1d7d6 100644 --- a/crates/sui-core/src/authority_server.rs +++ b/crates/sui-core/src/authority_server.rs @@ -213,12 +213,13 @@ impl ValidatorService { let metrics = ConsensusAdapterMetrics::new(&prometheus_registry); + let delay_step = consensus_config.delay_step.unwrap_or(15_000); // The consensus adapter allows the authority to send user certificates through consensus. let consensus_adapter = ConsensusAdapter::new( consensus_config.address().to_owned(), state.clone_committee(), tx_sui_to_consensus.clone(), - /* max_delay */ Duration::from_millis(5_000), + Duration::from_millis(delay_step), metrics.clone(), ); diff --git a/crates/sui-core/src/consensus_adapter.rs b/crates/sui-core/src/consensus_adapter.rs index fed117b40a1d2..fe71941e605ee 100644 --- a/crates/sui-core/src/consensus_adapter.rs +++ b/crates/sui-core/src/consensus_adapter.rs @@ -207,12 +207,11 @@ pub struct ConsensusAdapter { committee: Committee, /// A channel to notify the consensus listener to take action for a transactions. tx_consensus_listener: Sender, - /// The maximum duration to wait from consensus before aborting the transaction. After - /// this delay passed, the client will be notified that its transaction was probably not - /// sequence and it should try to resubmit its transaction. - max_delay: Duration, + /// Additional amount on top of delay_ms to calculate consensus timeout + /// Worst case consensus timeout grows linearly by adding delay_step every timeout + delay_step: Duration, - /// Estimation of the conensus delay, to use to dynamically adjust the delay + /// Estimation of the consensus delay, to use to dynamically adjust the delay /// before we time out, so that we do not spam the consensus adapter with the /// same transaction. delay_ms: AtomicU64, @@ -227,7 +226,7 @@ impl ConsensusAdapter { consensus_address: Multiaddr, committee: Committee, tx_consensus_listener: Sender, - max_delay: Duration, + delay_step: Duration, opt_metrics: OptArcConsensusAdapterMetrics, ) -> Self { let consensus_client = TransactionsClient::new( @@ -238,8 +237,8 @@ impl ConsensusAdapter { consensus_client, committee, tx_consensus_listener, - max_delay, - delay_ms: AtomicU64::new(max_delay.as_millis() as u64), + delay_step, + delay_ms: AtomicU64::new(delay_step.as_millis() as u64), opt_metrics, } } @@ -310,7 +309,7 @@ impl ConsensusAdapter { // client to retry if we timeout without hearing back from consensus (this module does not // handle retries). The best timeout value depends on the consensus protocol. let back_off_delay = - self.max_delay + Duration::from_millis(self.delay_ms.load(Ordering::Relaxed)); + self.delay_step + Duration::from_millis(self.delay_ms.load(Ordering::Relaxed)); let result = match timeout(back_off_delay, waiter.wait_for_result()).await { Ok(_) => { // Increment the attempted certificate sequencing success @@ -345,7 +344,7 @@ impl ConsensusAdapter { // but all we really need here is some max so that we do not wait for ever // in case consensus if dead. let new_delay = - new_delay.min((self.max_delay.as_millis() as u64) * MAX_DELAY_MULTIPLIER); + new_delay.min((self.delay_step.as_millis() as u64) * MAX_DELAY_MULTIPLIER); // Store the latest latency self.opt_metrics.as_ref().map(|metrics| {