Skip to content

Commit

Permalink
feat: Added view timeout duration as a config parameter (#222)
Browse files Browse the repository at this point in the history
## What ❔

The hardcoded 2s view timeout was causing problems in Era testnet
because the block time there is longer (>5s). View timeout needs to be a
configurable parameter since different chains will want different block
times.
  • Loading branch information
brunoffranca authored Dec 13, 2024
1 parent f1522f8 commit f07fcfa
Show file tree
Hide file tree
Showing 14 changed files with 38 additions and 12 deletions.
5 changes: 1 addition & 4 deletions node/components/bft/src/chonky_bft/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ pub(crate) mod testonly;
mod tests;
mod timeout;

/// The duration of the view timeout.
pub(crate) const VIEW_TIMEOUT_DURATION: time::Duration = time::Duration::milliseconds(2000);

/// The StateMachine struct contains the state of the replica and implements all the
/// logic of ChonkyBFT.
#[derive(Debug)]
Expand Down Expand Up @@ -90,6 +87,7 @@ impl StateMachine {
}

let this = Self {
view_timeout: time::Deadline::Finite(ctx.now() + config.view_timeout),
config,
outbound_channel,
inbound_channel,
Expand All @@ -104,7 +102,6 @@ impl StateMachine {
commit_qcs_cache: BTreeMap::new(),
timeout_views_cache: BTreeMap::new(),
timeout_qcs_cache: BTreeMap::new(),
view_timeout: time::Deadline::Finite(ctx.now() + VIEW_TIMEOUT_DURATION),
view_start: ctx.now(),
};

Expand Down
4 changes: 2 additions & 2 deletions node/components/bft/src/chonky_bft/new_view.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::StateMachine;
use crate::{chonky_bft::VIEW_TIMEOUT_DURATION, metrics};
use crate::metrics;
use zksync_concurrency::{ctx, error::Wrap, metrics::LatencyHistogramExt as _, time};
use zksync_consensus_network::io::ConsensusInputMessage;
use zksync_consensus_roles::validator;
Expand Down Expand Up @@ -155,7 +155,7 @@ impl StateMachine {
self.view_start = now;

// Reset the timeout.
self.view_timeout = time::Deadline::Finite(ctx.now() + VIEW_TIMEOUT_DURATION);
self.view_timeout = time::Deadline::Finite(ctx.now() + self.config.view_timeout);

Ok(())
}
Expand Down
5 changes: 2 additions & 3 deletions node/components/bft/src/chonky_bft/proposer.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use super::VIEW_TIMEOUT_DURATION;
use crate::{metrics, Config, ToNetworkMessage};
use std::sync::Arc;
use zksync_concurrency::{ctx, error::Wrap as _, sync};
Expand Down Expand Up @@ -27,15 +26,15 @@ pub(crate) async fn run_proposer(

// Create a proposal for the given justification, within the timeout.
let proposal = match create_proposal(
&ctx.with_timeout(VIEW_TIMEOUT_DURATION),
&ctx.with_timeout(cfg.view_timeout),
cfg.clone(),
justification,
)
.await
{
Ok(proposal) => proposal,
Err(ctx::Error::Canceled(_)) => {
tracing::error!("run_proposer(): timed out while creating a proposal");
tracing::warn!("run_proposer(): timed out while creating a proposal");
continue;
}
Err(ctx::Error::Internal(err)) => {
Expand Down
7 changes: 6 additions & 1 deletion node/components/bft/src/chonky_bft/testonly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ use crate::{
};
use assert_matches::assert_matches;
use std::sync::Arc;
use zksync_concurrency::{ctx, sync, sync::prunable_mpsc};
use zksync_concurrency::{
ctx,
sync::{self, prunable_mpsc},
time,
};
use zksync_consensus_roles::validator;
use zksync_consensus_storage::{
testonly::{in_memory, TestMemoryStorage},
Expand Down Expand Up @@ -70,6 +74,7 @@ impl UTHarness {
replica_store: Box::new(in_memory::ReplicaStore::default()),
payload_manager,
max_payload_size: MAX_PAYLOAD_SIZE,
view_timeout: time::Duration::milliseconds(2000),
});
let replica = StateMachine::start(
ctx,
Expand Down
3 changes: 3 additions & 0 deletions node/components/bft/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! The inner data of the consensus state machine. This is shared between the different roles.
use crate::PayloadManager;
use std::sync::Arc;
use zksync_concurrency::time;
use zksync_consensus_roles::validator;
use zksync_consensus_storage as storage;

Expand All @@ -12,6 +13,8 @@ pub struct Config {
/// The maximum size of the payload of a block, in bytes. We will
/// reject blocks with payloads larger than this.
pub max_payload_size: usize,
/// The duration of the view timeout.
pub view_timeout: time::Duration,
/// Block store.
pub block_store: Arc<storage::BlockStore>,
/// Replica store.
Expand Down
2 changes: 2 additions & 0 deletions node/components/bft/src/testonly/node.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{testonly, FromNetworkMessage, PayloadManager, ToNetworkMessage};
use anyhow::Context as _;
use std::sync::Arc;
use zksync_concurrency::time;
use zksync_concurrency::{ctx, ctx::channel, scope, sync};
use zksync_consensus_network as network;
use zksync_consensus_storage as storage;
Expand Down Expand Up @@ -58,6 +59,7 @@ impl Node {
replica_store: Box::new(in_memory::ReplicaStore::default()),
payload_manager: self.behavior.payload_manager(),
max_payload_size: MAX_PAYLOAD_SIZE,
view_timeout: time::Duration::milliseconds(2000),
}
.run(ctx, net_send, consensus_receiver)
.await
Expand Down
3 changes: 3 additions & 0 deletions node/components/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ pub struct Config {
pub public_addr: net::Host,
/// Maximal size of the block payload.
pub max_payload_size: usize,
/// The duration of the view timeout, in milliseconds.
pub view_timeout: time::Duration,
/// Key of this node. It uniquely identifies the node.
/// It should match the secret key provided in the `node_key` file.
pub node_key: node::SecretKey,
Expand Down Expand Up @@ -157,6 +159,7 @@ impl Executor {
replica_store: validator.replica_store,
payload_manager: validator.payload_manager,
max_payload_size: self.config.max_payload_size,
view_timeout: self.config.view_timeout,
}
.run(ctx, network_send, consensus_recv)
.await
Expand Down
1 change: 1 addition & 0 deletions node/components/executor/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ fn config(cfg: &network::Config) -> Config {
server_addr: *cfg.server_addr,
public_addr: cfg.public_addr.clone(),
max_payload_size: usize::MAX,
view_timeout: time::Duration::milliseconds(1000),
node_key: cfg.gossip.key.clone(),
gossip_dynamic_inbound_limit: cfg.gossip.dynamic_inbound_limit,
gossip_static_inbound: cfg.gossip.static_inbound.clone(),
Expand Down
5 changes: 4 additions & 1 deletion node/tools/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ fn main() {
zksync_protobuf_build::Config {
input_root: "src/proto".into(),
proto_root: "zksync/tools".into(),
dependencies: vec!["::zksync_consensus_roles::proto".parse().unwrap()],
dependencies: vec![
"::zksync_protobuf::proto".parse().unwrap(),
"::zksync_consensus_roles::proto".parse().unwrap(),
],
protobuf_crate: "::zksync_protobuf".parse().unwrap(),
is_public: false,
}
Expand Down
2 changes: 2 additions & 0 deletions node/tools/src/bin/deployer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::{
collections::HashMap,
net::{Ipv4Addr, SocketAddr},
};
use zksync_concurrency::time;
use zksync_consensus_roles::{node::SecretKey, validator};
use zksync_consensus_tools::{config, k8s, k8s::ConsensusNode};

Expand Down Expand Up @@ -48,6 +49,7 @@ fn generate_consensus_nodes(nodes: usize, seed_nodes_amount: Option<usize>) -> V
metrics_server_addr: None,
genesis: setup.genesis.clone(),
max_payload_size: 1000000,
view_timeout: time::Duration::milliseconds(2000),
validator_key: Some(validator_keys[i].clone()),
attester_key: Some(attester_keys[i].clone()),
node_key: node_keys[i].clone(),
Expand Down
2 changes: 2 additions & 0 deletions node/tools/src/bin/localnet_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::{
os::unix::fs::PermissionsExt,
path::PathBuf,
};
use zksync_concurrency::time;
use zksync_consensus_roles::{node, validator};
use zksync_consensus_tools::config;
use zksync_protobuf::serde::Serialize;
Expand Down Expand Up @@ -76,6 +77,7 @@ fn main() -> anyhow::Result<()> {
.map(|port| SocketAddr::new(Ipv4Addr::UNSPECIFIED.into(), port)),
genesis: setup.genesis.clone(),
max_payload_size: 1000000,
view_timeout: time::Duration::milliseconds(2000),
node_key: node_keys[i].clone(),
validator_key: validator_keys.get(i).cloned(),
attester_key: attester_keys.get(i).cloned(),
Expand Down
6 changes: 5 additions & 1 deletion node/tools/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::{
sync::Arc,
};
use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer};
use zksync_concurrency::{ctx, net};
use zksync_concurrency::{ctx, net, time};
use zksync_consensus_bft as bft;
use zksync_consensus_crypto::{read_optional_text, read_required_text, Text, TextFmt};
use zksync_consensus_executor::{self as executor, attestation};
Expand Down Expand Up @@ -76,6 +76,7 @@ pub struct App {

pub genesis: validator::Genesis,
pub max_payload_size: usize,
pub view_timeout: time::Duration,
pub validator_key: Option<validator::SecretKey>,
pub attester_key: Option<attester::SecretKey>,

Expand Down Expand Up @@ -176,6 +177,7 @@ impl ProtoFmt for App {

genesis: read_required(&r.genesis).context("genesis")?,
max_payload_size,
view_timeout: read_required(&r.view_timeout).context("view_timeout")?,
// TODO: read secret.
validator_key: read_optional_secret_text(&r.validator_secret_key)
.context("validator_secret_key")?,
Expand All @@ -201,6 +203,7 @@ impl ProtoFmt for App {

genesis: Some(self.genesis.build()),
max_payload_size: Some(self.max_payload_size.try_into().unwrap()),
view_timeout: Some(self.view_timeout.build()),
validator_secret_key: self.validator_key.as_ref().map(TextFmt::encode),
attester_secret_key: self.attester_key.as_ref().map(TextFmt::encode),

Expand Down Expand Up @@ -270,6 +273,7 @@ impl Configs {
gossip_static_inbound: self.app.gossip_static_inbound.clone(),
gossip_static_outbound: self.app.gossip_static_outbound.clone(),
max_payload_size: self.app.max_payload_size,
view_timeout: self.app.view_timeout,
rpc: executor::RpcConfig::default(),
debug_page: self
.app
Expand Down
4 changes: 4 additions & 0 deletions node/tools/src/proto/mod.proto
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ syntax = "proto3";
package zksync.tools;

import "zksync/roles/validator.proto";
import "zksync/std.proto";

// (public key, ip address) of a gossip network node.
message NodeAddr {
Expand Down Expand Up @@ -98,6 +99,9 @@ message AppConfig {
// Maximal size of the block payload.
optional uint64 max_payload_size = 5; // required; bytes

// The duration of the view timeout.
optional std.Duration view_timeout = 19; // required; milliseconds

// Validator secret key.
optional string validator_secret_key = 10; // optional; ValidatorSecretKey

Expand Down
1 change: 1 addition & 0 deletions node/tools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ impl Distribution<config::App> for EncodeDist {

genesis: rng.gen(),
max_payload_size: rng.gen(),
view_timeout: self.sample(rng),
validator_key: self.sample_opt(|| rng.gen()),
attester_key: self.sample_opt(|| rng.gen()),

Expand Down

0 comments on commit f07fcfa

Please sign in to comment.