From 41a4e1a79d75eb58c0b293391a47b5a56ec0c71d Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 26 Oct 2023 17:16:08 +0200 Subject: [PATCH] moved protocol_version to ConsensusMessage --- .../consensus/src/leader/replica_commit.rs | 5 ++- .../consensus/src/leader/replica_prepare.rs | 1 + node/actors/consensus/src/leader/tests.rs | 1 + node/actors/consensus/src/lib.rs | 8 +++++ .../consensus/src/replica/leader_commit.rs | 13 -------- .../consensus/src/replica/leader_prepare.rs | 14 +------- node/actors/consensus/src/replica/new_view.rs | 1 + node/actors/consensus/src/replica/tests.rs | 1 + node/actors/consensus/src/testonly/fuzz.rs | 20 ++++++++---- node/actors/consensus/src/testonly/make.rs | 1 + node/actors/sync_blocks/src/tests/mod.rs | 1 + node/libs/roles/src/validator/conv.rs | 10 ++++-- .../roles/src/validator/messages/block.rs | 20 ------------ .../roles/src/validator/messages/consensus.rs | 32 +++++++++++++++++++ node/libs/roles/src/validator/testonly.rs | 8 +++-- node/libs/schema/proto/roles/validator.proto | 8 ++--- 16 files changed, 82 insertions(+), 62 deletions(-) diff --git a/node/actors/consensus/src/leader/replica_commit.rs b/node/actors/consensus/src/leader/replica_commit.rs index 499b2a77..82ab32b2 100644 --- a/node/actors/consensus/src/leader/replica_commit.rs +++ b/node/actors/consensus/src/leader/replica_commit.rs @@ -133,7 +133,10 @@ impl StateMachine { message: consensus .secret_key .sign_msg(validator::ConsensusMsg::LeaderCommit( - validator::LeaderCommit { justification }, + validator::LeaderCommit { + protocol_version: validator::CURRENT_VERSION, + justification, + }, )), recipient: Target::Broadcast, }; diff --git a/node/actors/consensus/src/leader/replica_prepare.rs b/node/actors/consensus/src/leader/replica_prepare.rs index b872d859..800bd331 100644 --- a/node/actors/consensus/src/leader/replica_prepare.rs +++ b/node/actors/consensus/src/leader/replica_prepare.rs @@ -197,6 +197,7 @@ impl StateMachine { .secret_key .sign_msg(validator::ConsensusMsg::LeaderPrepare( validator::LeaderPrepare { + protocol_version: validator::CURRENT_VERSION, view: self.view, proposal, proposal_payload: payload, diff --git a/node/actors/consensus/src/leader/tests.rs b/node/actors/consensus/src/leader/tests.rs index a4e00750..2010b08f 100644 --- a/node/actors/consensus/src/leader/tests.rs +++ b/node/actors/consensus/src/leader/tests.rs @@ -22,6 +22,7 @@ async fn replica_commit() { .secret_key .sign_msg(validator::ConsensusMsg::ReplicaCommit( validator::ReplicaCommit { + protocol_version: validator::CURRENT_VERSION, view: consensus.leader.view, proposal: rng.gen(), }, diff --git a/node/actors/consensus/src/lib.rs b/node/actors/consensus/src/lib.rs index 0857233e..6d278e05 100644 --- a/node/actors/consensus/src/lib.rs +++ b/node/actors/consensus/src/lib.rs @@ -99,6 +99,14 @@ impl Consensus { match input { Some(InputMessage::Network(req)) => { + if req.msg.msg.protocol_version() != validator::CURRENT_VERSION { + tracing::warn!( + "bad protocol version (expected: {:?}, received: {:?})", + validator::CURRENT_VERSION, + req.msg.msg.protocol_version() + ); + continue; + } match &req.msg.msg { validator::ConsensusMsg::ReplicaPrepare(_) | validator::ConsensusMsg::ReplicaCommit(_) => { diff --git a/node/actors/consensus/src/replica/leader_commit.rs b/node/actors/consensus/src/replica/leader_commit.rs index 7f1614ae..32cf70ce 100644 --- a/node/actors/consensus/src/replica/leader_commit.rs +++ b/node/actors/consensus/src/replica/leader_commit.rs @@ -8,11 +8,6 @@ use tracing::instrument; #[derive(thiserror::Error, Debug)] #[allow(clippy::missing_docs_in_private_items)] pub(crate) enum Error { - #[error("bad protocol version (expected: {expected:?}, received: {received:?})")] - BadProtocolVersion { - expected: validator::ProtocolVersion, - received: validator::ProtocolVersion, - }, #[error("invalid leader (correct leader: {correct_leader:?}, received leader: {received_leader:?})]")] InvalidLeader { correct_leader: validator::PublicKey, @@ -48,14 +43,6 @@ impl StateMachine { let author = &signed_message.key; let view = message.justification.message.view; - // Check protocol version. - if message.justification.message.proposal.protocol_version != validator::CURRENT_VERSION { - return Err(Error::BadProtocolVersion { - expected: validator::CURRENT_VERSION, - received: message.justification.message.proposal.protocol_version, - }); - } - // Check that it comes from the correct leader. if author != &consensus.view_leader(view) { return Err(Error::InvalidLeader { diff --git a/node/actors/consensus/src/replica/leader_prepare.rs b/node/actors/consensus/src/replica/leader_prepare.rs index fa22defd..7eee6ac9 100644 --- a/node/actors/consensus/src/replica/leader_prepare.rs +++ b/node/actors/consensus/src/replica/leader_prepare.rs @@ -10,11 +10,6 @@ use tracing::instrument; #[derive(thiserror::Error, Debug)] #[allow(clippy::missing_docs_in_private_items)] pub(crate) enum Error { - #[error("bad protocol version (expected: {expected:?}, received: {received:?})")] - BadProtocolVersion { - expected: validator::ProtocolVersion, - received: validator::ProtocolVersion, - }, #[error("invalid leader (correct leader: {correct_leader:?}, received leader: {received_leader:?}])")] InvalidLeader { correct_leader: validator::PublicKey, @@ -87,14 +82,6 @@ impl StateMachine { let author = &signed_message.key; let view = message.view; - // Check protocol version. - if message.proposal.protocol_version != validator::CURRENT_VERSION { - return Err(Error::BadProtocolVersion { - expected: validator::CURRENT_VERSION, - received: message.proposal.protocol_version, - }); - } - // Check that it comes from the correct leader. if author != &consensus.view_leader(view) { return Err(Error::InvalidLeader { @@ -227,6 +214,7 @@ impl StateMachine { // Create our commit vote. let commit_vote = validator::ReplicaCommit { + protocol_version: validator::CURRENT_VERSION, view, proposal: message.proposal, }; diff --git a/node/actors/consensus/src/replica/new_view.rs b/node/actors/consensus/src/replica/new_view.rs index e0548105..078fe488 100644 --- a/node/actors/consensus/src/replica/new_view.rs +++ b/node/actors/consensus/src/replica/new_view.rs @@ -35,6 +35,7 @@ impl StateMachine { .secret_key .sign_msg(validator::ConsensusMsg::ReplicaPrepare( validator::ReplicaPrepare { + protocol_version: validator::CURRENT_VERSION, view: next_view, high_vote: self.high_vote, high_qc: self.high_qc.clone(), diff --git a/node/actors/consensus/src/replica/tests.rs b/node/actors/consensus/src/replica/tests.rs index 9149d57e..775929e8 100644 --- a/node/actors/consensus/src/replica/tests.rs +++ b/node/actors/consensus/src/replica/tests.rs @@ -38,6 +38,7 @@ async fn start_new_view_not_leader() { .secret_key .sign_msg(validator::ConsensusMsg::ReplicaPrepare( validator::ReplicaPrepare { + protocol_version: validator::CURRENT_VERSION, view: consensus.replica.view, high_vote: consensus.replica.high_vote, high_qc: consensus.replica.high_qc.clone(), diff --git a/node/actors/consensus/src/testonly/fuzz.rs b/node/actors/consensus/src/testonly/fuzz.rs index a7dd8ca3..e4b95c78 100644 --- a/node/actors/consensus/src/testonly/fuzz.rs +++ b/node/actors/consensus/src/testonly/fuzz.rs @@ -35,10 +35,11 @@ impl Fuzz for validator::ConsensusMsg { impl Fuzz for validator::ReplicaPrepare { fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { + match rng.gen_range(0..4) { 0 => self.view = rng.gen(), 1 => self.high_vote.mutate(rng), 2 => self.high_qc.mutate(rng), + 3 => self.protocol_version = rng.gen(), _ => unreachable!(), } } @@ -46,9 +47,10 @@ impl Fuzz for validator::ReplicaPrepare { impl Fuzz for validator::ReplicaCommit { fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { + match rng.gen_range(0..3) { 0 => self.view = rng.gen(), 1 => self.proposal.mutate(rng), + 2 => self.protocol_version = rng.gen(), _ => unreachable!(), } } @@ -56,9 +58,10 @@ impl Fuzz for validator::ReplicaCommit { impl Fuzz for validator::LeaderPrepare { fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { + match rng.gen_range(0..3) { 0 => self.proposal.mutate(rng), 1 => self.justification.mutate(rng), + 2 => self.protocol_version = rng.gen(), _ => unreachable!(), } } @@ -66,7 +69,11 @@ impl Fuzz for validator::LeaderPrepare { impl Fuzz for validator::LeaderCommit { fn mutate(&mut self, rng: &mut impl Rng) { - self.justification.mutate(rng); + match rng.gen_range(0..2) { + 0 => self.justification.mutate(rng), + 1 => self.protocol_version = rng.gen(), + _ => unreachable!(), + } } } @@ -157,11 +164,10 @@ impl Fuzz for validator::Payload { impl Fuzz for validator::BlockHeader { fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..4) { + match rng.gen_range(0..3) { 0 => self.parent = rng.gen(), 1 => self.number = rng.gen(), - 2 => self.protocol_version = rng.gen(), - 3 => self.payload = rng.gen(), + 2 => self.payload = rng.gen(), _ => unreachable!(), } } diff --git a/node/actors/consensus/src/testonly/make.rs b/node/actors/consensus/src/testonly/make.rs index 2beb15f9..9af2b6d4 100644 --- a/node/actors/consensus/src/testonly/make.rs +++ b/node/actors/consensus/src/testonly/make.rs @@ -53,6 +53,7 @@ pub fn make_genesis( .iter() .map(|sk| { sk.sign_msg(validator::ReplicaCommit { + protocol_version: validator::CURRENT_VERSION, view: validator::ViewNumber(0), proposal: header, }) diff --git a/node/actors/sync_blocks/src/tests/mod.rs b/node/actors/sync_blocks/src/tests/mod.rs index 9ddd6804..dd6619dd 100644 --- a/node/actors/sync_blocks/src/tests/mod.rs +++ b/node/actors/sync_blocks/src/tests/mod.rs @@ -69,6 +69,7 @@ impl TestValidators { fn certify_block(&self, proposal: &BlockHeader) -> CommitQC { let message_to_sign = validator::ReplicaCommit { + protocol_version: validator::CURRENT_VERSION, view: validator::ViewNumber(proposal.number.0), proposal: *proposal, }; diff --git a/node/libs/roles/src/validator/conv.rs b/node/libs/roles/src/validator/conv.rs index d9be555b..0b65e63d 100644 --- a/node/libs/roles/src/validator/conv.rs +++ b/node/libs/roles/src/validator/conv.rs @@ -40,7 +40,6 @@ impl ProtoFmt for BlockHeader { type Proto = proto::BlockHeader; fn read(r: &Self::Proto) -> anyhow::Result { Ok(Self { - protocol_version: ProtocolVersion(r.protocol_version.context("protocol_version")?), parent: read_required(&r.parent).context("parent")?, number: BlockNumber(r.number.context("number")?), payload: read_required(&r.payload).context("payload")?, @@ -48,7 +47,6 @@ impl ProtoFmt for BlockHeader { } fn build(&self) -> Self::Proto { Self::Proto { - protocol_version: Some(self.protocol_version.0), parent: Some(self.parent.build()), number: Some(self.number.0), payload: Some(self.payload.build()), @@ -109,6 +107,7 @@ impl ProtoFmt for ReplicaPrepare { fn read(r: &Self::Proto) -> anyhow::Result { Ok(Self { + protocol_version: ProtocolVersion(r.protocol_version.context("protocol_version")?), view: ViewNumber(*required(&r.view).context("view")?), high_vote: read_required(&r.high_vote).context("high_vote")?, high_qc: read_required(&r.high_qc).context("high_qc")?, @@ -117,6 +116,7 @@ impl ProtoFmt for ReplicaPrepare { fn build(&self) -> Self::Proto { Self::Proto { + protocol_version: Some(self.protocol_version.0), view: Some(self.view.0), high_vote: Some(self.high_vote.build()), high_qc: Some(self.high_qc.build()), @@ -129,6 +129,7 @@ impl ProtoFmt for ReplicaCommit { fn read(r: &Self::Proto) -> anyhow::Result { Ok(Self { + protocol_version: ProtocolVersion(r.protocol_version.context("protocol_version")?), view: ViewNumber(*required(&r.view).context("view")?), proposal: read_required(&r.proposal).context("proposal")?, }) @@ -136,6 +137,7 @@ impl ProtoFmt for ReplicaCommit { fn build(&self) -> Self::Proto { Self::Proto { + protocol_version: Some(self.protocol_version.0), view: Some(self.view.0), proposal: Some(self.proposal.build()), } @@ -147,6 +149,7 @@ impl ProtoFmt for LeaderPrepare { fn read(r: &Self::Proto) -> anyhow::Result { Ok(Self { + protocol_version: ProtocolVersion(r.protocol_version.context("protocol_version")?), view: ViewNumber(*required(&r.view).context("view")?), proposal: read_required(&r.proposal).context("proposal")?, proposal_payload: r.proposal_payload.as_ref().map(|p| Payload(p.clone())), @@ -156,6 +159,7 @@ impl ProtoFmt for LeaderPrepare { fn build(&self) -> Self::Proto { Self::Proto { + protocol_version: Some(self.protocol_version.0), view: Some(self.view.0), proposal: Some(self.proposal.build()), proposal_payload: self.proposal_payload.as_ref().map(|p| p.0.clone()), @@ -169,12 +173,14 @@ impl ProtoFmt for LeaderCommit { fn read(r: &Self::Proto) -> anyhow::Result { Ok(Self { + protocol_version: ProtocolVersion(r.protocol_version.context("protocol_version")?), justification: read_required(&r.justification).context("justification")?, }) } fn build(&self) -> Self::Proto { Self::Proto { + protocol_version: Some(self.protocol_version.0), justification: Some(self.justification.build()), } } diff --git a/node/libs/roles/src/validator/messages/block.rs b/node/libs/roles/src/validator/messages/block.rs index 2fcdcb4a..770361d3 100644 --- a/node/libs/roles/src/validator/messages/block.rs +++ b/node/libs/roles/src/validator/messages/block.rs @@ -4,22 +4,6 @@ use super::CommitQC; use crypto::{sha256, ByteFmt, Text, TextFmt}; use std::fmt; -/// Version of the consensus algorithm used by the leader in a given view. -/// Currently it is stored in the BlockHeader (TODO: consider whether it should be present in every -/// consensus message). -/// Currently it also determines the format and semantics of the block payload -/// (TODO: consider whether the payload should be versioned separately). -#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct ProtocolVersion(pub(crate) u32); - -/// We use a hardcoded protocol version for now. -/// Eventually validators should determine which version to use for which block by observing the relevant L1 contract. -/// -/// The validator binary has to support the current and next protocol version whenever -/// a protocol version update is needed (so that it can dynamically switch from producing -/// blocks for version X to version X+1). -pub const CURRENT_VERSION: ProtocolVersion = ProtocolVersion(0); - /// Payload of the block. Consensus algorithm does not interpret the payload /// (except for imposing a size limit for the payload). Proposing a payload /// for a new block and interpreting the payload of the finalized blocks @@ -102,8 +86,6 @@ impl fmt::Debug for BlockHeaderHash { /// A block header. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct BlockHeader { - /// Protocol version according to which this block should be interpreted. - pub protocol_version: ProtocolVersion, /// Hash of the parent block. pub parent: BlockHeaderHash, /// Number of the block. @@ -121,7 +103,6 @@ impl BlockHeader { /// Creates a genesis block. pub fn genesis(payload: PayloadHash) -> Self { Self { - protocol_version: CURRENT_VERSION, parent: BlockHeaderHash(sha256::Sha256::default()), number: BlockNumber(0), payload, @@ -131,7 +112,6 @@ impl BlockHeader { /// Creates a child block for the given parent. pub fn new(parent: &BlockHeader, payload: PayloadHash) -> Self { Self { - protocol_version: CURRENT_VERSION, parent: parent.hash(), number: parent.number.next(), payload, diff --git a/node/libs/roles/src/validator/messages/consensus.rs b/node/libs/roles/src/validator/messages/consensus.rs index d1390d37..1af7147f 100644 --- a/node/libs/roles/src/validator/messages/consensus.rs +++ b/node/libs/roles/src/validator/messages/consensus.rs @@ -7,6 +7,20 @@ use bit_vec::BitVec; use std::collections::{BTreeMap, BTreeSet, HashMap}; use utils::enum_util::{ErrBadVariant, Variant}; +/// Version of the consensus algorithm that the validator is using. +/// It allows to prevent misinterpretation of messages signed by validators +/// using different versions of the binaries. +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ProtocolVersion(pub(crate) u32); + +/// We use a hardcoded protocol version for now. +/// Eventually validators should determine which version to use for which block by observing the relevant L1 contract. +/// +/// The validator binary has to support the current and next protocol version whenever +/// a protocol version update is needed (so that it can dynamically switch from producing +/// blocks for version X to version X+1). +pub const CURRENT_VERSION: ProtocolVersion = ProtocolVersion(0); + /// Consensus messages. #[allow(missing_docs)] #[derive(Clone, Debug, PartialEq, Eq)] @@ -27,6 +41,16 @@ impl ConsensusMsg { Self::LeaderCommit(_) => "LeaderCommit", } } + + /// Protocol version of this message. + pub fn protocol_version(&self) -> ProtocolVersion { + match self { + Self::ReplicaPrepare(m) => m.protocol_version, + Self::ReplicaCommit(m) => m.protocol_version, + Self::LeaderPrepare(m) => m.protocol_version, + Self::LeaderCommit(m) => m.protocol_version, + } + } } impl Variant for ReplicaPrepare { @@ -80,6 +104,8 @@ impl Variant for LeaderCommit { /// A Prepare message from a replica. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub struct ReplicaPrepare { + /// Protocol version. + pub protocol_version: ProtocolVersion, /// The number of the current view. pub view: ViewNumber, /// The highest block that the replica has committed to. @@ -91,6 +117,8 @@ pub struct ReplicaPrepare { /// A Commit message from a replica. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ReplicaCommit { + /// Protocol version. + pub protocol_version: ProtocolVersion, /// The number of the current view. pub view: ViewNumber, /// The header of the block that the replica is committing to. @@ -100,6 +128,8 @@ pub struct ReplicaCommit { /// A Prepare message from a leader. #[derive(Clone, Debug, PartialEq, Eq)] pub struct LeaderPrepare { + /// Protocol version. + pub protocol_version: ProtocolVersion, /// The number of the current view. pub view: ViewNumber, /// The header of the block that the leader is proposing. @@ -114,6 +144,8 @@ pub struct LeaderPrepare { /// A Commit message from a leader. #[derive(Clone, Debug, PartialEq, Eq)] pub struct LeaderCommit { + /// Protocol version. + pub protocol_version: ProtocolVersion, /// The CommitQC that justifies the message from the leader. pub justification: CommitQC, } diff --git a/node/libs/roles/src/validator/testonly.rs b/node/libs/roles/src/validator/testonly.rs index a0dd8efa..dad8d8e8 100644 --- a/node/libs/roles/src/validator/testonly.rs +++ b/node/libs/roles/src/validator/testonly.rs @@ -3,7 +3,7 @@ use super::{ AggregateSignature, BlockHeader, BlockHeaderHash, BlockNumber, CommitQC, ConsensusMsg, FinalBlock, LeaderCommit, LeaderPrepare, Msg, MsgHash, NetAddress, Payload, PayloadHash, Phase, PrepareQC, ProtocolVersion, PublicKey, ReplicaCommit, ReplicaPrepare, SecretKey, Signature, - Signed, Signers, ValidatorSet, ViewNumber, + Signed, Signers, ValidatorSet, ViewNumber, CURRENT_VERSION, }; use bit_vec::BitVec; use concurrency::time; @@ -19,6 +19,7 @@ use utils::enum_util::Variant; pub fn make_justification(rng: &mut R, header: &BlockHeader) -> CommitQC { CommitQC { message: ReplicaCommit { + protocol_version: CURRENT_VERSION, view: ViewNumber(header.number.0), proposal: *header, }, @@ -104,7 +105,6 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> BlockHeader { BlockHeader { - protocol_version: ProtocolVersion(rng.gen()), parent: rng.gen(), number: rng.gen(), payload: rng.gen(), @@ -132,6 +132,7 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> ReplicaPrepare { ReplicaPrepare { + protocol_version: rng.gen(), view: rng.gen(), high_vote: rng.gen(), high_qc: rng.gen(), @@ -142,6 +143,7 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> ReplicaCommit { ReplicaCommit { + protocol_version: rng.gen(), view: rng.gen(), proposal: rng.gen(), } @@ -151,6 +153,7 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> LeaderPrepare { LeaderPrepare { + protocol_version: rng.gen(), view: rng.gen(), proposal: rng.gen(), proposal_payload: rng.gen(), @@ -162,6 +165,7 @@ impl Distribution for Standard { impl Distribution for Standard { fn sample(&self, rng: &mut R) -> LeaderCommit { LeaderCommit { + protocol_version: rng.gen(), justification: rng.gen(), } } diff --git a/node/libs/schema/proto/roles/validator.proto b/node/libs/schema/proto/roles/validator.proto index 8b307391..8d02e1a7 100644 --- a/node/libs/schema/proto/roles/validator.proto +++ b/node/libs/schema/proto/roles/validator.proto @@ -13,10 +13,6 @@ message BlockHeaderHash { } message BlockHeader { - // Protocol version determines the algorithm - // according to which this block has been constructed. - // In particular it defines how payload should be interpreted. - optional uint32 protocol_version = 1; // required // Hash of the parent Block. optional BlockHeaderHash parent = 2; // required // Sequential number of the block = parent.number + 1. @@ -41,17 +37,20 @@ message ConsensusMsg { } message ReplicaPrepare { + optional uint32 protocol_version = 4; // required optional uint64 view = 1; // required optional ReplicaCommit high_vote = 2; // required optional CommitQC high_qc = 3; // required } message ReplicaCommit { + optional uint32 protocol_version = 3; // required optional uint64 view = 1; // required optional BlockHeader proposal = 2; // required } message LeaderPrepare { + optional uint32 protocol_version = 5; // required optional uint64 view = 1; // required optional BlockHeader proposal = 2; // required optional bytes proposal_payload = 3; // optional (depending on justification) @@ -59,6 +58,7 @@ message LeaderPrepare { } message LeaderCommit { + optional uint32 protocol_version = 2; // required optional CommitQC justification = 1; // required }