From 52936f05aadf8cbf746351d5dec5a70d2278b6bd Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 25 Jun 2024 15:40:38 +0100 Subject: [PATCH] BFT-470: Remove the Byzantine and Random behaviours along with the Mock network (#131) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What ❔ * [x] Remove `Behavior::Byzantine` * [x] Remove `Behavior::Random` * [x] Remove `Network::Mock` * [x] Remove the `fuzz` module ## Why ❔ The randomization behaviours were deemed to be too trivial to be useful in these tests, not revealing anything interesting about the consensus algorithm itself. The mock network was nice in that it made it relatively easy to see how the actor and the network pipes fit together, but its functionality is a subset of what the Twins network does (the mock network delivered messages unconditionally and immediately), so it seemed that if the Twins variants work, then the mock one will trivially pass. --------- Co-authored-by: Bruno França --- node/actors/bft/src/testonly/fuzz.rs | 182 --------------------------- node/actors/bft/src/testonly/mod.rs | 4 - node/actors/bft/src/testonly/node.rs | 21 +--- node/actors/bft/src/testonly/run.rs | 54 -------- node/actors/bft/src/tests.rs | 30 ----- 5 files changed, 1 insertion(+), 290 deletions(-) delete mode 100644 node/actors/bft/src/testonly/fuzz.rs diff --git a/node/actors/bft/src/testonly/fuzz.rs b/node/actors/bft/src/testonly/fuzz.rs deleted file mode 100644 index fd0cdd4a..00000000 --- a/node/actors/bft/src/testonly/fuzz.rs +++ /dev/null @@ -1,182 +0,0 @@ -use crate::testonly::node::MAX_PAYLOAD_SIZE; -use rand::{ - distributions::{Distribution, Standard}, - seq::SliceRandom, - Rng, -}; -use zksync_consensus_roles::validator; - -/// Trait that allows us to mutate types. It's an approach to fuzzing that instead of starting with completely random inputs -/// (which will basically always fail on the first check), starts from a real valid message and modifies a single value to -/// make it invalid. The idea is to try to hit as many checks as possible, and create more realistic malicious messages. -pub(crate) trait Fuzz { - /// Mutates a message. It will take a message (ideally a valid message, but we don't check this here) and change a single - /// value to make it invalid (but ideally pretty close to being valid). - fn mutate(&mut self, rng: &mut impl Rng); -} - -impl Fuzz for Option -where - Standard: Distribution, -{ - fn mutate(&mut self, rng: &mut impl Rng) { - if let Some(v) = self.as_mut() { - v.mutate(rng); - } else { - *self = Some(rng.gen()); - } - } -} - -impl Fuzz for validator::Signed { - fn mutate(&mut self, rng: &mut impl Rng) { - // We give them different weights because we want to mutate the message more often. - match rng.gen_range(0..20) { - 0..=17 => self.msg.mutate(rng), - 18 => self.key = rng.gen(), - 19 => self.sig = rng.gen(), - _ => unreachable!(), - } - } -} - -impl Fuzz for validator::ConsensusMsg { - fn mutate(&mut self, rng: &mut impl Rng) { - match self { - validator::ConsensusMsg::LeaderPrepare(msg) => msg.mutate(rng), - validator::ConsensusMsg::LeaderCommit(msg) => msg.mutate(rng), - validator::ConsensusMsg::ReplicaPrepare(msg) => msg.mutate(rng), - validator::ConsensusMsg::ReplicaCommit(msg) => msg.mutate(rng), - } - } -} - -impl Fuzz for validator::ReplicaPrepare { - fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..3) { - 0 => self.view = rng.gen(), - 1 => self.high_vote.mutate(rng), - _ => self.high_qc.mutate(rng), - } - } -} - -impl Fuzz for validator::ReplicaCommit { - fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { - 0 => self.view = rng.gen(), - _ => self.proposal.mutate(rng), - } - } -} - -// TODO: why payload is not fuzzed? -impl Fuzz for validator::LeaderPrepare { - fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { - 0 => self.proposal.mutate(rng), - 1 => self.justification.mutate(rng), - _ => unreachable!(), - } - } -} - -impl Fuzz for validator::LeaderCommit { - fn mutate(&mut self, rng: &mut impl Rng) { - self.justification.mutate(rng); - } -} - -impl Fuzz for validator::PrepareQC { - fn mutate(&mut self, rng: &mut impl Rng) { - // We give them different weights because we want to mutate the signature less often. - match rng.gen_range(0..10) { - 0..=8 => { - // Get keys from the aggregate QC map - let keys = self.map.keys().cloned().collect::>(); - - // select random element from the vector - let mut key = keys.choose(rng).unwrap().clone(); - - // get value correspondent to the key - let mut value = self.map.get(&key).unwrap().clone(); - - // either mutate the key or the value. - if rng.gen() { - self.map.remove(&key); - key.mutate(rng); - } else { - value.mutate(rng); - } - - self.map.insert(key, value); - } - 9 => self.signature = rng.gen(), - _ => unreachable!(), - } - } -} - -impl Fuzz for validator::CommitQC { - fn mutate(&mut self, rng: &mut impl Rng) { - // We give them different weights because we want to mutate the message more often. - match rng.gen_range(0..10) { - 0..=6 => self.message.mutate(rng), - 7 | 8 => self.signers.mutate(rng), - 9 => self.signature = rng.gen(), - _ => unreachable!(), - } - } -} - -impl Fuzz for validator::Signers { - fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..5) { - // Flips a random number of bits. - 0 => { - for _ in 1..rng.gen_range(0..self.0.len()) { - self.0.set(rng.gen_range(0..self.0.len()), rng.gen()); - } - } - // Flips one random bit. - 1 => { - let pos = rng.gen_range(0..self.0.len()); - let bit = self.0.get(pos).unwrap(); - self.0.set(pos, !bit); - } - // Sets all bits to false. - 2 => { - self.0.set_all(); - self.0.negate(); - } - // Sets all bits to true. - 3 => { - self.0.set_all(); - } - // Delete one bit. - 4 => { - self.0.pop(); - } - _ => unreachable!(), - } - } -} - -impl Fuzz for validator::Payload { - fn mutate(&mut self, rng: &mut impl Rng) { - // Push bytes into the payload until it exceeds the limit. - let num_bytes = MAX_PAYLOAD_SIZE - self.0.len() + 1; - let bytes: Vec = (0..num_bytes).map(|_| rng.gen()).collect(); - self.0.extend_from_slice(&bytes); - assert!(self.0.len() > MAX_PAYLOAD_SIZE); - } -} - -impl Fuzz for validator::BlockHeader { - fn mutate(&mut self, rng: &mut impl Rng) { - match rng.gen_range(0..2) { - 0 => self.number = rng.gen(), - _ => self.payload = rng.gen(), - } - } -} diff --git a/node/actors/bft/src/testonly/mod.rs b/node/actors/bft/src/testonly/mod.rs index 2c38382a..504bb149 100644 --- a/node/actors/bft/src/testonly/mod.rs +++ b/node/actors/bft/src/testonly/mod.rs @@ -5,8 +5,6 @@ use rand::{distributions::Standard, prelude::Distribution, Rng}; use zksync_concurrency::oneshot; use zksync_consensus_network::io::ConsensusReq; -#[cfg(test)] -mod fuzz; mod make; #[cfg(test)] mod node; @@ -15,8 +13,6 @@ mod run; #[cfg(test)] pub(crate) mod ut_harness; -#[cfg(test)] -pub(crate) use fuzz::*; pub use make::*; #[cfg(test)] pub(crate) use node::*; diff --git a/node/actors/bft/src/testonly/node.rs b/node/actors/bft/src/testonly/node.rs index 344e43a6..2ed792eb 100644 --- a/node/actors/bft/src/testonly/node.rs +++ b/node/actors/bft/src/testonly/node.rs @@ -1,11 +1,8 @@ -use super::Fuzz; use crate::{io, testonly, PayloadManager}; use anyhow::Context as _; -use rand::Rng; use std::sync::Arc; use zksync_concurrency::{ctx, scope}; use zksync_consensus_network as network; -use zksync_consensus_network::io::ConsensusInputMessage; use zksync_consensus_storage as storage; use zksync_consensus_storage::testonly::in_memory; use zksync_consensus_utils::pipe; @@ -21,12 +18,6 @@ pub(crate) enum Behavior { HonestNotProposing, /// A replica that is always offline and does not produce any messages. Offline, - /// A replica that is always online and behaves randomly. It will produce - /// completely random messages. - Random, - /// A replica that is always online and behaves maliciously. It will produce - /// realistic but wrong messages. - Byzantine, } impl Behavior { @@ -56,7 +47,6 @@ impl Node { network::io::OutputMessage, >, ) -> anyhow::Result<()> { - let rng = &mut ctx.rng(); let net_recv = &mut network_pipe.recv; let net_send = &mut network_pipe.send; let (consensus_actor_pipe, consensus_pipe) = pipe::new(); @@ -94,19 +84,10 @@ impl Node { // Get the next message from the channel. Our response depends on what type of replica we are. while let Ok(msg) = con_recv.recv(ctx).await { match msg { - io::OutputMessage::Network(mut message) => { + io::OutputMessage::Network(message) => { let message_to_send = match self.behavior { Behavior::Offline => continue, Behavior::Honest | Behavior::HonestNotProposing => message, - // Create a random consensus message and broadcast. - Behavior::Random => ConsensusInputMessage { - message: rng.gen(), - recipient: network::io::Target::Broadcast, - }, - Behavior::Byzantine => { - message.message.mutate(rng); - message - } }; net_send.send(message_to_send.into()); } diff --git a/node/actors/bft/src/testonly/run.rs b/node/actors/bft/src/testonly/run.rs index 0f27362f..9f907f14 100644 --- a/node/actors/bft/src/testonly/run.rs +++ b/node/actors/bft/src/testonly/run.rs @@ -21,7 +21,6 @@ use zksync_consensus_utils::pipe; pub(crate) enum Network { Real, - Mock, Twins(PortRouter), } @@ -179,7 +178,6 @@ impl Test { async fn run_nodes(ctx: &ctx::Ctx, network: &Network, specs: &[Node]) -> anyhow::Result<()> { match network { Network::Real => run_nodes_real(ctx, specs).await, - Network::Mock => run_nodes_mock(ctx, specs).await, Network::Twins(router) => run_nodes_twins(ctx, specs, router).await, } } @@ -213,58 +211,6 @@ async fn run_nodes_real(ctx: &ctx::Ctx, specs: &[Node]) -> anyhow::Result<()> { .await } -/// Run a set of nodes with a mock network. -async fn run_nodes_mock(ctx: &ctx::Ctx, specs: &[Node]) -> anyhow::Result<()> { - scope::run!(ctx, |ctx, s| async { - // Actor inputs, ie. where the test can send messages to the consensus. - let mut sends = HashMap::new(); - // Actor outputs, ie. the messages the actor wants to send to the others. - let mut recvs = vec![]; - for (i, spec) in specs.iter().enumerate() { - let (actor_pipe, dispatcher_pipe) = pipe::new(); - let key = spec.net.validator_key.as_ref().unwrap().public(); - sends.insert(key, actor_pipe.send); - recvs.push(actor_pipe.recv); - // Run consensus; the dispatcher pipe is its network connection, which means we can use the actor pipe to: - // * send Output messages from other actors to this consensus instance - // * receive Input messages sent by this consensus to the other actors - s.spawn( - async { - let mut network_pipe = dispatcher_pipe; - spec.run(ctx, &mut network_pipe).await - } - .instrument(tracing::info_span!("node", i)), - ); - } - // Run mock networks by receiving the output-turned-input from all consensus - // instances and forwarding them to the others. - scope::run!(ctx, |ctx, s| async { - for recv in recvs { - s.spawn(async { - use zksync_consensus_network::io; - let mut recv = recv; - while let Ok(io::InputMessage::Consensus(message)) = recv.recv(ctx).await { - let msg = || { - io::OutputMessage::Consensus(io::ConsensusReq { - msg: message.message.clone(), - ack: oneshot::channel().0, - }) - }; - match message.recipient { - io::Target::Validator(v) => sends.get(&v).unwrap().send(msg()), - io::Target::Broadcast => sends.values().for_each(|s| s.send(msg())), - } - } - Ok(()) - }); - } - anyhow::Ok(()) - }) - .await - }) - .await -} - /// Run a set of nodes with a Twins network configuration. async fn run_nodes_twins( ctx: &ctx::Ctx, diff --git a/node/actors/bft/src/tests.rs b/node/actors/bft/src/tests.rs index eda7f594..d498f424 100644 --- a/node/actors/bft/src/tests.rs +++ b/node/actors/bft/src/tests.rs @@ -38,46 +38,16 @@ async fn run_test(behavior: Behavior, network: Network) { .unwrap() } -#[tokio::test] -async fn honest_mock_network() { - run_test(Behavior::Honest, Network::Mock).await -} - #[tokio::test] async fn honest_real_network() { run_test(Behavior::Honest, Network::Real).await } -#[tokio::test] -async fn offline_mock_network() { - run_test(Behavior::Offline, Network::Mock).await -} - #[tokio::test] async fn offline_real_network() { run_test(Behavior::Offline, Network::Real).await } -#[tokio::test] -async fn random_mock_network() { - run_test(Behavior::Random, Network::Mock).await -} - -#[tokio::test] -async fn random_real_network() { - run_test(Behavior::Random, Network::Real).await -} - -#[tokio::test] -async fn byzantine_mock_network() { - run_test(Behavior::Byzantine, Network::Mock).await -} - -#[tokio::test] -async fn byzantine_real_network() { - run_test(Behavior::Byzantine, Network::Real).await -} - /// Testing liveness after the network becomes idle with leader having no cached prepare messages for the current view. #[tokio::test] async fn timeout_leader_no_prepares() {