Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added view timeout duration as a config parameter #222

Merged
merged 6 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
pompon0 marked this conversation as resolved.
Show resolved Hide resolved
}
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: usize,
brunoffranca marked this conversation as resolved.
Show resolved Hide resolved
/// 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: time::Duration::milliseconds(self.config.view_timeout as i64),
}
.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: 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
1 change: 1 addition & 0 deletions node/tools/src/bin/deployer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,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: 2000,
validator_key: Some(validator_keys[i].clone()),
attester_key: Some(attester_keys[i].clone()),
node_key: node_keys[i].clone(),
Expand Down
1 change: 1 addition & 0 deletions node/tools/src/bin/localnet_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ fn main() -> anyhow::Result<()> {
.map(|port| SocketAddr::new(Ipv4Addr::UNSPECIFIED.into(), port)),
genesis: setup.genesis.clone(),
max_payload_size: 1000000,
view_timeout: 2000,
node_key: node_keys[i].clone(),
validator_key: validator_keys.get(i).cloned(),
attester_key: attester_keys.get(i).cloned(),
Expand Down
8 changes: 8 additions & 0 deletions node/tools/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ pub struct App {

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

Expand Down Expand Up @@ -167,6 +168,10 @@ impl ProtoFmt for App {
.and_then(|x| Ok((*x).try_into()?))
.context("max_payload_size")?;

let view_timeout = required(&r.view_timeout)
.and_then(|x| Ok((*x).try_into()?))
.context("view_timeout")?;

Ok(Self {
server_addr: read_required_text(&r.server_addr).context("server_addr")?,
public_addr: net::Host(required(&r.public_addr).context("public_addr")?.clone()),
Expand All @@ -176,6 +181,7 @@ impl ProtoFmt for App {

genesis: read_required(&r.genesis).context("genesis")?,
max_payload_size,
view_timeout,
// TODO: read secret.
validator_key: read_optional_secret_text(&r.validator_secret_key)
.context("validator_secret_key")?,
Expand All @@ -201,6 +207,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.try_into().unwrap()),
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 +277,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
3 changes: 3 additions & 0 deletions node/tools/src/proto/mod.proto
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ message AppConfig {
// Maximal size of the block payload.
optional uint64 max_payload_size = 5; // required; bytes

// The duration of the view timeout.
optional uint64 view_timeout = 19; // required; milliseconds
brunoffranca marked this conversation as resolved.
Show resolved Hide resolved

// 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: rng.gen(),
validator_key: self.sample_opt(|| rng.gen()),
attester_key: self.sample_opt(|| rng.gen()),

Expand Down
Loading