From 300e605c73816810300b091578846a730c4fa9a6 Mon Sep 17 00:00:00 2001 From: pompon0 Date: Fri, 8 Dec 2023 16:09:25 +0100 Subject: [PATCH] Removed protocol_version from config (#49) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What ❔ Removed protocol_version from config. Instead, I've added a constant in bft crate indicating the version of the protocol implementation. The payload format should be versioned separately. Payload format version should be included in the payload itself (set via PayloadSource::propose, verified via WriteBlockStore::verify_payload). ## Why ❔ What is being versioned is actually the implementation of the consensus, so it doesn't make sense to configure it via config. If it was configurable the implementation wouldn't be able to reason about the protocol version and compatibility of the peers. --- node/actors/bft/src/inner.rs | 2 - node/actors/bft/src/leader/replica_commit.rs | 11 ++--- node/actors/bft/src/leader/replica_prepare.rs | 11 ++--- node/actors/bft/src/lib.rs | 5 ++- node/actors/bft/src/replica/leader_commit.rs | 9 ++-- node/actors/bft/src/replica/leader_prepare.rs | 11 ++--- node/actors/bft/src/replica/new_view.rs | 4 +- node/actors/bft/src/testonly/make.rs | 4 +- node/actors/bft/src/testonly/run.rs | 9 +--- node/actors/bft/src/testonly/ut_harness.rs | 12 ++---- node/actors/executor/src/config/mod.rs | 7 ---- .../executor/src/config/proto/mod.proto | 2 - node/actors/executor/src/config/tests.rs | 1 - node/actors/executor/src/lib.rs | 1 - node/actors/executor/src/testonly.rs | 13 +----- node/actors/executor/src/tests.rs | 42 +++++-------------- node/tools/src/bin/localnet_config.rs | 3 -- 17 files changed, 39 insertions(+), 108 deletions(-) diff --git a/node/actors/bft/src/inner.rs b/node/actors/bft/src/inner.rs index bd8a1044..631e90d2 100644 --- a/node/actors/bft/src/inner.rs +++ b/node/actors/bft/src/inner.rs @@ -18,8 +18,6 @@ pub(crate) struct ConsensusInner { pub(crate) secret_key: validator::SecretKey, /// A vector of public keys for all the validators in the network. pub(crate) validator_set: validator::ValidatorSet, - /// Current protocol version for the consensus messages. - pub(crate) protocol_version: validator::ProtocolVersion, } impl ConsensusInner { diff --git a/node/actors/bft/src/leader/replica_commit.rs b/node/actors/bft/src/leader/replica_commit.rs index f82e598a..b0af302f 100644 --- a/node/actors/bft/src/leader/replica_commit.rs +++ b/node/actors/bft/src/leader/replica_commit.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::{inner::ConsensusInner, metrics}; +use crate::{inner::ConsensusInner, metrics, Consensus}; use tracing::instrument; use zksync_concurrency::{ctx, metrics::LatencyHistogramExt as _}; use zksync_consensus_network::io::{ConsensusInputMessage, Target}; @@ -73,13 +73,10 @@ impl StateMachine { let author = &signed_message.key; // Check protocol version compatibility. - if !consensus - .protocol_version - .compatible(&message.protocol_version) - { + if !Consensus::PROTOCOL_VERSION.compatible(&message.protocol_version) { return Err(Error::IncompatibleProtocolVersion { message_version: message.protocol_version, - local_version: consensus.protocol_version, + local_version: Consensus::PROTOCOL_VERSION, }); } @@ -181,7 +178,7 @@ impl StateMachine { .secret_key .sign_msg(validator::ConsensusMsg::LeaderCommit( validator::LeaderCommit { - protocol_version: consensus.protocol_version, + protocol_version: Consensus::PROTOCOL_VERSION, justification, }, )), diff --git a/node/actors/bft/src/leader/replica_prepare.rs b/node/actors/bft/src/leader/replica_prepare.rs index 9982aee6..278ed033 100644 --- a/node/actors/bft/src/leader/replica_prepare.rs +++ b/node/actors/bft/src/leader/replica_prepare.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::{inner::ConsensusInner, metrics}; +use crate::{inner::ConsensusInner, metrics, Consensus}; use std::collections::HashMap; use tracing::instrument; use zksync_concurrency::{ctx, error::Wrap}; @@ -99,13 +99,10 @@ impl StateMachine { let author = &signed_message.key; // Check protocol version compatibility. - if !consensus - .protocol_version - .compatible(&message.protocol_version) - { + if !Consensus::PROTOCOL_VERSION.compatible(&message.protocol_version) { return Err(Error::IncompatibleProtocolVersion { message_version: message.protocol_version, - local_version: consensus.protocol_version, + local_version: Consensus::PROTOCOL_VERSION, }); } @@ -260,7 +257,7 @@ impl StateMachine { .secret_key .sign_msg(validator::ConsensusMsg::LeaderPrepare( validator::LeaderPrepare { - protocol_version: consensus.protocol_version, + protocol_version: Consensus::PROTOCOL_VERSION, view: self.view, proposal, proposal_payload: payload, diff --git a/node/actors/bft/src/lib.rs b/node/actors/bft/src/lib.rs index 840f905a..26029303 100644 --- a/node/actors/bft/src/lib.rs +++ b/node/actors/bft/src/lib.rs @@ -56,12 +56,14 @@ pub struct Consensus { } impl Consensus { + /// Protocol version of this BFT implementation. + pub const PROTOCOL_VERSION: validator::ProtocolVersion = validator::ProtocolVersion::EARLIEST; + /// Creates a new Consensus struct. #[instrument(level = "trace", skip(payload_source))] pub async fn new( ctx: &ctx::Ctx, pipe: ActorPipe, - protocol_version: validator::ProtocolVersion, secret_key: validator::SecretKey, validator_set: validator::ValidatorSet, storage: ReplicaStore, @@ -72,7 +74,6 @@ impl Consensus { pipe, secret_key, validator_set, - protocol_version, }, replica: replica::StateMachine::new(ctx, storage).await?, leader: leader::StateMachine::new(ctx, payload_source), diff --git a/node/actors/bft/src/replica/leader_commit.rs b/node/actors/bft/src/replica/leader_commit.rs index ebd4c282..370727fb 100644 --- a/node/actors/bft/src/replica/leader_commit.rs +++ b/node/actors/bft/src/replica/leader_commit.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::inner::ConsensusInner; +use crate::{inner::ConsensusInner, Consensus}; use tracing::instrument; use zksync_concurrency::{ctx, error::Wrap}; use zksync_consensus_roles::validator::{self, ProtocolVersion}; @@ -74,13 +74,10 @@ impl StateMachine { let view = message.justification.message.view; // Check protocol version compatibility. - if !consensus - .protocol_version - .compatible(&message.protocol_version) - { + if !Consensus::PROTOCOL_VERSION.compatible(&message.protocol_version) { return Err(Error::IncompatibleProtocolVersion { message_version: message.protocol_version, - local_version: consensus.protocol_version, + local_version: Consensus::PROTOCOL_VERSION, }); } diff --git a/node/actors/bft/src/replica/leader_prepare.rs b/node/actors/bft/src/replica/leader_prepare.rs index 82d273cf..cdb02ed8 100644 --- a/node/actors/bft/src/replica/leader_prepare.rs +++ b/node/actors/bft/src/replica/leader_prepare.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::inner::ConsensusInner; +use crate::{inner::ConsensusInner, Consensus}; use std::collections::HashMap; use tracing::instrument; use zksync_concurrency::{ctx, error::Wrap}; @@ -144,13 +144,10 @@ impl StateMachine { let view = message.view; // Check protocol version compatibility. - if !consensus - .protocol_version - .compatible(&message.protocol_version) - { + if !Consensus::PROTOCOL_VERSION.compatible(&message.protocol_version) { return Err(Error::IncompatibleProtocolVersion { message_version: message.protocol_version, - local_version: consensus.protocol_version, + local_version: Consensus::PROTOCOL_VERSION, }); } @@ -300,7 +297,7 @@ impl StateMachine { // Create our commit vote. let commit_vote = validator::ReplicaCommit { - protocol_version: consensus.protocol_version, + protocol_version: Consensus::PROTOCOL_VERSION, view, proposal: message.proposal, }; diff --git a/node/actors/bft/src/replica/new_view.rs b/node/actors/bft/src/replica/new_view.rs index d8f98347..a5f117f6 100644 --- a/node/actors/bft/src/replica/new_view.rs +++ b/node/actors/bft/src/replica/new_view.rs @@ -1,5 +1,5 @@ use super::StateMachine; -use crate::ConsensusInner; +use crate::{Consensus, ConsensusInner}; use tracing::instrument; use zksync_concurrency::{ctx, error::Wrap as _}; use zksync_consensus_network::io::{ConsensusInputMessage, Target}; @@ -34,7 +34,7 @@ impl StateMachine { .secret_key .sign_msg(validator::ConsensusMsg::ReplicaPrepare( validator::ReplicaPrepare { - protocol_version: consensus.protocol_version, + protocol_version: Consensus::PROTOCOL_VERSION, view: next_view, high_vote: self.high_vote, high_qc: self.high_qc.clone(), diff --git a/node/actors/bft/src/testonly/make.rs b/node/actors/bft/src/testonly/make.rs index c87fbd78..18734e3c 100644 --- a/node/actors/bft/src/testonly/make.rs +++ b/node/actors/bft/src/testonly/make.rs @@ -41,7 +41,6 @@ pub async fn make_consensus( let consensus = Consensus::new( ctx, consensus_pipe, - genesis_block.justification.message.protocol_version, key.clone(), validator_set.clone(), ReplicaStore::from_store(Arc::new(storage)), @@ -57,7 +56,6 @@ pub async fn make_consensus( /// and a validator set for the chain. pub fn make_genesis( keys: &[validator::SecretKey], - protocol_version: validator::ProtocolVersion, payload: validator::Payload, block_number: validator::BlockNumber, ) -> (validator::FinalBlock, validator::ValidatorSet) { @@ -67,7 +65,7 @@ pub fn make_genesis( .iter() .map(|sk| { sk.sign_msg(validator::ReplicaCommit { - protocol_version, + protocol_version: validator::ProtocolVersion::EARLIEST, view: validator::ViewNumber(0), proposal: header, }) diff --git a/node/actors/bft/src/testonly/run.rs b/node/actors/bft/src/testonly/run.rs index 63384362..4cf0133c 100644 --- a/node/actors/bft/src/testonly/run.rs +++ b/node/actors/bft/src/testonly/run.rs @@ -32,12 +32,8 @@ impl Test { .iter() .map(|node| node.consensus_config().key.clone()) .collect(); - let (genesis_block, _) = testonly::make_genesis( - &keys, - validator::ProtocolVersion::EARLIEST, - validator::Payload(vec![]), - validator::BlockNumber(0), - ); + let (genesis_block, _) = + testonly::make_genesis(&keys, validator::Payload(vec![]), validator::BlockNumber(0)); let nodes: Vec<_> = nodes .into_iter() .enumerate() @@ -107,7 +103,6 @@ async fn run_nodes(ctx: &ctx::Ctx, network: Network, nodes: &[Node]) -> anyhow:: let consensus = Consensus::new( ctx, consensus_actor_pipe, - validator::ProtocolVersion::EARLIEST, node.net.consensus_config().key.clone(), validator_set, storage, diff --git a/node/actors/bft/src/testonly/ut_harness.rs b/node/actors/bft/src/testonly/ut_harness.rs index 741b3088..300feeea 100644 --- a/node/actors/bft/src/testonly/ut_harness.rs +++ b/node/actors/bft/src/testonly/ut_harness.rs @@ -32,12 +32,8 @@ impl UTHarness { pub(crate) async fn new(ctx: &ctx::Ctx, num_validators: usize) -> UTHarness { let mut rng = ctx.rng(); let keys: Vec<_> = (0..num_validators).map(|_| rng.gen()).collect(); - let (genesis, val_set) = crate::testonly::make_genesis( - &keys, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - validator::BlockNumber(0), - ); + let (genesis, val_set) = + crate::testonly::make_genesis(&keys, Payload(vec![]), validator::BlockNumber(0)); let (mut consensus, pipe) = crate::testonly::make_consensus(ctx, &keys[0], &val_set, &genesis).await; @@ -62,7 +58,7 @@ impl UTHarness { /// recovers after a timeout. pub(crate) async fn produce_block_after_timeout(&mut self, ctx: &ctx::Ctx) { let want = ReplicaPrepare { - protocol_version: self.consensus.inner.protocol_version, + protocol_version: self.protocol_version(), view: self.consensus.replica.view.next(), high_qc: self.consensus.replica.high_qc.clone(), high_vote: self.consensus.replica.high_vote, @@ -81,7 +77,7 @@ impl UTHarness { } pub(crate) fn protocol_version(&self) -> validator::ProtocolVersion { - self.consensus.inner.protocol_version + Consensus::PROTOCOL_VERSION } pub(crate) fn incompatible_protocol_version(&self) -> validator::ProtocolVersion { diff --git a/node/actors/executor/src/config/mod.rs b/node/actors/executor/src/config/mod.rs index abf1fda9..600d8c5d 100644 --- a/node/actors/executor/src/config/mod.rs +++ b/node/actors/executor/src/config/mod.rs @@ -23,8 +23,6 @@ pub struct ConsensusConfig { /// Public TCP address that other validators are expected to connect to. /// It is announced over gossip network. pub public_addr: net::SocketAddr, - /// Currently used protocol version for consensus messages. - pub protocol_version: validator::ProtocolVersion, } impl ProtoFmt for ConsensusConfig { @@ -34,10 +32,6 @@ impl ProtoFmt for ConsensusConfig { Ok(Self { key: read_required_text(&proto.key).context("key")?, public_addr: read_required_text(&proto.public_addr).context("public_addr")?, - protocol_version: required(&proto.protocol_version) - .copied() - .and_then(validator::ProtocolVersion::try_from) - .context("protocol_version")?, }) } @@ -45,7 +39,6 @@ impl ProtoFmt for ConsensusConfig { Self::Proto { key: Some(self.key.encode()), public_addr: Some(self.public_addr.encode()), - protocol_version: Some(self.protocol_version.as_u32()), } } } diff --git a/node/actors/executor/src/config/proto/mod.proto b/node/actors/executor/src/config/proto/mod.proto index 71d75d57..8cfce427 100644 --- a/node/actors/executor/src/config/proto/mod.proto +++ b/node/actors/executor/src/config/proto/mod.proto @@ -48,8 +48,6 @@ message NodeAddr { message ConsensusConfig { optional string key = 1; // [required] ValidatorPublicKey optional string public_addr = 2; // [required] IpAddr - // Currently used protocol version for consensus messages. - optional uint32 protocol_version = 3; // [required] } // Config of the gossip network. diff --git a/node/actors/executor/src/config/tests.rs b/node/actors/executor/src/config/tests.rs index 2c6d0262..3c18f072 100644 --- a/node/actors/executor/src/config/tests.rs +++ b/node/actors/executor/src/config/tests.rs @@ -16,7 +16,6 @@ impl Distribution for Standard { ConsensusConfig { key: rng.gen::().public(), public_addr: make_addr(rng), - protocol_version: validator::ProtocolVersion::EARLIEST, } } } diff --git a/node/actors/executor/src/lib.rs b/node/actors/executor/src/lib.rs index a65857d6..01f3712e 100644 --- a/node/actors/executor/src/lib.rs +++ b/node/actors/executor/src/lib.rs @@ -181,7 +181,6 @@ impl Executor { let consensus = Consensus::new( ctx, consensus_actor_pipe, - validator.config.protocol_version, validator.key.clone(), validator_set.clone(), consensus_storage, diff --git a/node/actors/executor/src/testonly.rs b/node/actors/executor/src/testonly.rs index d88264cd..a57393fb 100644 --- a/node/actors/executor/src/testonly.rs +++ b/node/actors/executor/src/testonly.rs @@ -8,14 +8,10 @@ use zksync_consensus_network::{consensus, testonly::Instance}; use zksync_consensus_roles::{node, validator}; impl ConsensusConfig { - fn from_network_config( - src: consensus::Config, - protocol_version: validator::ProtocolVersion, - ) -> Self { + fn from_network_config(src: consensus::Config) -> Self { Self { key: src.key.public(), public_addr: src.public_addr, - protocol_version, } } } @@ -36,11 +32,8 @@ pub struct FullValidatorConfig { impl FullValidatorConfig { /// Generates a validator config for a network with a single validator. - /// - /// `protocol_version` is used both for the genesis block and as the current protocol version. pub fn for_single_validator( rng: &mut impl Rng, - protocol_version: validator::ProtocolVersion, genesis_block_payload: validator::Payload, genesis_block_number: validator::BlockNumber, ) -> Self { @@ -49,12 +42,10 @@ impl FullValidatorConfig { let net_config = net_configs.pop().unwrap(); let consensus_config = net_config.consensus.unwrap(); let validator_key = consensus_config.key.clone(); - let consensus_config = - ConsensusConfig::from_network_config(consensus_config, protocol_version); + let consensus_config = ConsensusConfig::from_network_config(consensus_config); let (genesis_block, validators) = make_genesis( &[validator_key.clone()], - protocol_version, genesis_block_payload, genesis_block_number, ); diff --git a/node/actors/executor/src/tests.rs b/node/actors/executor/src/tests.rs index 0e035355..94e87747 100644 --- a/node/actors/executor/src/tests.rs +++ b/node/actors/executor/src/tests.rs @@ -6,7 +6,7 @@ use rand::{thread_rng, Rng}; use std::iter; use test_casing::test_casing; use zksync_concurrency::{sync, testonly::abort_on_panic, time}; -use zksync_consensus_bft::testonly::RandomPayloadSource; +use zksync_consensus_bft::{testonly::RandomPayloadSource, Consensus}; use zksync_consensus_roles::validator::{BlockNumber, FinalBlock, Payload}; use zksync_consensus_storage::{BlockStore, InMemoryStorage}; @@ -22,7 +22,7 @@ impl FullValidatorConfig { payload: payload.hash(), }; let commit = self.validator_key.sign_msg(validator::ReplicaCommit { - protocol_version: validator::ProtocolVersion::EARLIEST, + protocol_version: Consensus::PROTOCOL_VERSION, view: validator::ViewNumber(header.number.0), proposal: header, }); @@ -73,12 +73,8 @@ async fn executor_misconfiguration(name: &str, mutation: fn(&mut FinalBlock)) { let ctx = &ctx::root(); let rng = &mut ctx.rng(); - let mut validator = FullValidatorConfig::for_single_validator( - rng, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - BlockNumber(0), - ); + let mut validator = + FullValidatorConfig::for_single_validator(rng, Payload(vec![]), BlockNumber(0)); let genesis_block = &mut validator.node_config.genesis_block; mutation(genesis_block); let storage = Arc::new(InMemoryStorage::new(genesis_block.clone())); @@ -95,12 +91,7 @@ async fn genesis_block_mismatch() { let ctx = &ctx::root(); let rng = &mut ctx.rng(); - let validator = FullValidatorConfig::for_single_validator( - rng, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - BlockNumber(0), - ); + let validator = FullValidatorConfig::for_single_validator(rng, Payload(vec![]), BlockNumber(0)); let mut genesis_block = validator.node_config.genesis_block.clone(); genesis_block.header.number = BlockNumber(1); let storage = Arc::new(InMemoryStorage::new(genesis_block.clone())); @@ -117,12 +108,7 @@ async fn executing_single_validator() { let ctx = &ctx::root(); let rng = &mut ctx.rng(); - let validator = FullValidatorConfig::for_single_validator( - rng, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - BlockNumber(0), - ); + let validator = FullValidatorConfig::for_single_validator(rng, Payload(vec![]), BlockNumber(0)); let genesis_block = &validator.node_config.genesis_block; let storage = InMemoryStorage::new(genesis_block.clone()); let storage = Arc::new(storage); @@ -147,12 +133,8 @@ async fn executing_validator_and_full_node() { let ctx = &ctx::test_root(&ctx::AffineClock::new(20.0)); let rng = &mut ctx.rng(); - let mut validator = FullValidatorConfig::for_single_validator( - rng, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - BlockNumber(0), - ); + let mut validator = + FullValidatorConfig::for_single_validator(rng, Payload(vec![]), BlockNumber(0)); let full_node = validator.connect_full_node(rng); let genesis_block = &validator.node_config.genesis_block; @@ -194,12 +176,8 @@ async fn syncing_full_node_from_snapshot(delay_block_storage: bool) { let ctx = &ctx::test_root(&ctx::AffineClock::new(20.0)); let rng = &mut ctx.rng(); - let mut validator = FullValidatorConfig::for_single_validator( - rng, - validator::ProtocolVersion::EARLIEST, - Payload(vec![]), - BlockNumber(0), - ); + let mut validator = + FullValidatorConfig::for_single_validator(rng, Payload(vec![]), BlockNumber(0)); let mut full_node = validator.connect_full_node(rng); let genesis_block = &validator.node_config.genesis_block; diff --git a/node/tools/src/bin/localnet_config.rs b/node/tools/src/bin/localnet_config.rs index bbeffebf..81b80c86 100644 --- a/node/tools/src/bin/localnet_config.rs +++ b/node/tools/src/bin/localnet_config.rs @@ -65,11 +65,9 @@ fn main() -> anyhow::Result<()> { let node_keys: Vec = (0..addrs.len()).map(|_| rng.gen()).collect(); // Generate the genesis block. - let protocol_version = validator::ProtocolVersion::EARLIEST; // TODO: generating genesis block shouldn't require knowing the private keys. let (genesis, validator_set) = testonly::make_genesis( &validator_keys, - protocol_version, validator::Payload(vec![]), validator::BlockNumber(0), ); @@ -114,7 +112,6 @@ fn main() -> anyhow::Result<()> { consensus: Some(ConsensusConfig { key: validator_keys[i].public(), public_addr: addrs[i], - protocol_version, }), };