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: Validate genesis block in executor config #40

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions node/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions node/actors/executor/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::{
collections::{HashMap, HashSet},
net,
};
use zksync_consensus_bft::misc::consensus_threshold;
use zksync_consensus_crypto::{read_required_text, Text, TextFmt};
use zksync_consensus_network::{consensus, gossip};
use zksync_consensus_roles::{node, validator};
Expand Down Expand Up @@ -139,6 +140,16 @@ pub struct ExecutorConfig {
pub validators: validator::ValidatorSet,
}

impl ExecutorConfig {
/// Validates internal consistency of this config.
pub(crate) fn validate(&self) -> anyhow::Result<()> {
let consensus_threshold = consensus_threshold(self.validators.len());
self.genesis_block
.validate(&self.validators, consensus_threshold)?;
Ok(())
}
}

impl ProtoFmt for ExecutorConfig {
type Proto = proto::ExecutorConfig;

Expand Down
18 changes: 15 additions & 3 deletions node/actors/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

use crate::io::Dispatcher;
use anyhow::Context as _;
use std::sync::Arc;
use std::{any, sync::Arc};
use zksync_concurrency::{ctx, net, scope};
use zksync_consensus_bft::{misc::consensus_threshold, Consensus};
use zksync_consensus_network as network;
use zksync_consensus_roles::{node, validator};
use zksync_consensus_storage::{ReplicaStateStore, ReplicaStore, WriteBlockStore};
use zksync_consensus_storage::{BlockStore, ReplicaStateStore, ReplicaStore, WriteBlockStore};
use zksync_consensus_sync_blocks::SyncBlocks;
use zksync_consensus_utils::pipe;

Expand Down Expand Up @@ -56,18 +56,30 @@ pub struct Executor<S> {

impl<S: WriteBlockStore + 'static> Executor<S> {
/// Creates a new executor with the specified parameters.
pub fn new(
pub async fn new(
ctx: &ctx::Ctx,
node_config: ExecutorConfig,
node_key: node::SecretKey,
storage: Arc<S>,
) -> anyhow::Result<Self> {
node_config.validate()?;
anyhow::ensure!(
node_config.gossip.key == node_key.public(),
"config.gossip.key = {:?} doesn't match the secret key {:?}",
node_config.gossip.key,
node_key
);

// While justifications may differ among nodes for an arbitrary block, we assume that
// the genesis block has a hardcoded justification.
let first_block = storage.first_block(ctx).await.context("first_block")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure whether it's valid for a storage to contain blocks before the genesis block. IMO, it makes reasoning about the storage more difficult, so the current check disallows it.

anyhow::ensure!(
first_block == node_config.genesis_block,
"First stored block {first_block:?} in `{}` is not equal to the configured genesis block {:?}",
any::type_name::<S>(),
node_config.genesis_block
);

Ok(Self {
executor_config: node_config,
node_key,
Expand Down
74 changes: 68 additions & 6 deletions node/actors/executor/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use super::*;
use crate::testonly::FullValidatorConfig;
use rand::Rng;
use rand::{thread_rng, Rng};
use std::iter;
use test_casing::test_casing;
use zksync_concurrency::{sync, testonly::abort_on_panic, time};
Expand Down Expand Up @@ -31,15 +31,68 @@ impl FullValidatorConfig {
blocks.skip(1).take(count).collect()
}

fn into_executor(self, storage: Arc<InMemoryStorage>) -> Executor<InMemoryStorage> {
let mut executor = Executor::new(self.node_config, self.node_key, storage.clone()).unwrap();
async fn into_executor(
self,
ctx: &ctx::Ctx,
storage: Arc<InMemoryStorage>,
) -> Executor<InMemoryStorage> {
let mut executor = Executor::new(ctx, self.node_config, self.node_key, storage.clone())
.await
.unwrap();
executor
.set_validator(self.consensus_config, self.validator_key, storage)
.unwrap();
executor
}
}

type BlockMutation = (&'static str, fn(&mut FinalBlock));
const BLOCK_MUTATIONS: [BlockMutation; 3] = [
("number", |block| {
block.header.number = BlockNumber(1);
}),
("payload", |block| {
block.payload = Payload(b"test".to_vec());
}),
("justification", |block| {
block.justification = thread_rng().gen();
}),
];

#[test_casing(3, BLOCK_MUTATIONS)]
#[tokio::test]
async fn executor_misconfiguration(name: &str, mutation: fn(&mut FinalBlock)) {
abort_on_panic();
let _span = tracing::info_span!("executor_misconfiguration", name).entered();
let ctx = &ctx::root();
let rng = &mut ctx.rng();

let mut validator = FullValidatorConfig::for_single_validator(rng, Payload(vec![]));
let genesis_block = &mut validator.node_config.genesis_block;
mutation(genesis_block);
let storage = Arc::new(InMemoryStorage::new(genesis_block.clone()));
let err = Executor::new(ctx, validator.node_config, validator.node_key, storage)
.await
.unwrap_err();
tracing::info!(%err, "received expected validation error");
}

#[tokio::test]
async fn genesis_block_mismatch() {
abort_on_panic();
let ctx = &ctx::root();
let rng = &mut ctx.rng();

let validator = FullValidatorConfig::for_single_validator(rng, Payload(vec![]));
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()));
let err = Executor::new(ctx, validator.node_config, validator.node_key, storage)
.await
.unwrap_err();
tracing::info!(%err, "received expected validation error");
}

#[tokio::test]
async fn executing_single_validator() {
abort_on_panic();
Expand All @@ -50,7 +103,7 @@ async fn executing_single_validator() {
let genesis_block = &validator.node_config.genesis_block;
let storage = InMemoryStorage::new(genesis_block.clone());
let storage = Arc::new(storage);
let executor = validator.into_executor(storage.clone());
let executor = validator.into_executor(ctx, storage.clone()).await;

scope::run!(ctx, |ctx, s| async {
s.spawn_bg(executor.run(ctx));
Expand Down Expand Up @@ -81,12 +134,16 @@ async fn executing_validator_and_full_node() {
let full_node_storage = Arc::new(full_node_storage);
let mut full_node_subscriber = full_node_storage.subscribe_to_block_writes();

let validator = validator.into_executor(validator_storage.clone());
let validator = validator
.into_executor(ctx, validator_storage.clone())
.await;
let full_node = Executor::new(
ctx,
full_node.node_config,
full_node.node_key,
full_node_storage.clone(),
)
.await
.unwrap();

scope::run!(ctx, |ctx, s| async {
Expand All @@ -110,7 +167,7 @@ async fn syncing_full_node_from_snapshot(delay_block_storage: bool) {
let rng = &mut ctx.rng();

let mut validator = FullValidatorConfig::for_single_validator(rng, Payload(vec![]));
let full_node = validator.connect_full_node(rng);
let mut full_node = validator.connect_full_node(rng);

let genesis_block = &validator.node_config.genesis_block;
let blocks = validator.gen_blocks(rng, 10);
Expand All @@ -123,22 +180,27 @@ async fn syncing_full_node_from_snapshot(delay_block_storage: bool) {
}
}
let validator = Executor::new(
ctx,
validator.node_config,
validator.node_key,
validator_storage.clone(),
)
.await
.unwrap();

// Start a full node from a snapshot.
full_node.node_config.genesis_block = blocks[3].clone();
let full_node_storage = InMemoryStorage::new(blocks[3].clone());
let full_node_storage = Arc::new(full_node_storage);
let mut full_node_subscriber = full_node_storage.subscribe_to_block_writes();

let full_node = Executor::new(
ctx,
full_node.node_config,
full_node.node_key,
full_node_storage.clone(),
)
.await
.unwrap();

scope::run!(ctx, |ctx, s| async {
Expand Down
58 changes: 7 additions & 51 deletions node/actors/sync_blocks/src/peers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use zksync_concurrency::{
use zksync_consensus_network::io::{SyncBlocksInputMessage, SyncState};
use zksync_consensus_roles::{
node,
validator::{BlockHeader, BlockNumber, FinalBlock, PayloadHash},
validator::{BlockNumber, BlockValidationError, FinalBlock},
};
use zksync_consensus_storage::{StorageResult, WriteBlockStore};

Expand Down Expand Up @@ -444,29 +444,13 @@ impl PeerStates {
block: &FinalBlock,
) -> Result<(), BlockValidationError> {
if block.header.number != block_number {
return Err(BlockValidationError::NumberMismatch {
requested: block_number,
got: block.header.number,
});
}
let payload_hash = block.payload.hash();
if payload_hash != block.header.payload {
return Err(BlockValidationError::HashMismatch {
header_hash: block.header.payload,
payload_hash,
});
}
if block.header != block.justification.message.proposal {
return Err(BlockValidationError::ProposalMismatch {
block_header: Box::new(block.header),
qc_header: Box::new(block.justification.message.proposal),
});
let err = anyhow::anyhow!(
"block does not have requested number (requested: {block_number}, got: {})",
block.header.number
);
return Err(BlockValidationError::Other(err));
}

block
.justification
.verify(&self.config.validator_set, self.config.consensus_threshold)
.map_err(BlockValidationError::Justification)
block.validate(&self.config.validator_set, self.config.consensus_threshold)
}

#[instrument(level = "trace", skip(self, ctx))]
Expand All @@ -485,31 +469,3 @@ impl PeerStates {
Ok(())
}
}

/// Errors that can occur validating a `FinalBlock` received from a node.
#[derive(Debug, thiserror::Error)]
enum BlockValidationError {
#[error("block does not have requested number (requested: {requested}, got: {got})")]
NumberMismatch {
requested: BlockNumber,
got: BlockNumber,
},
#[error(
"block payload doesn't match the block header (hash in header: {header_hash:?}, \
payload hash: {payload_hash:?})"
)]
HashMismatch {
header_hash: PayloadHash,
payload_hash: PayloadHash,
},
#[error(
"quorum certificate proposal doesn't match the block header (block header: {block_header:?}, \
header in QC: {qc_header:?})"
)]
ProposalMismatch {
block_header: Box<BlockHeader>,
qc_header: Box<BlockHeader>,
},
#[error("failed verifying quorum certificate: {0:#?}")]
Justification(#[source] anyhow::Error),
}
8 changes: 1 addition & 7 deletions node/actors/sync_blocks/src/peers/tests/fakes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,7 @@ async fn processing_invalid_blocks() {
let err = peer_states
.validate_block(BlockNumber(1), invalid_block)
.unwrap_err();
assert_matches!(
err,
BlockValidationError::NumberMismatch {
requested: BlockNumber(1),
got: BlockNumber(0),
}
);
assert_matches!(err, BlockValidationError::Other(_));

let mut invalid_block = test_validators.final_blocks[1].clone();
invalid_block.justification = test_validators.final_blocks[0].justification.clone();
Expand Down
1 change: 1 addition & 0 deletions node/libs/roles/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ hex.workspace = true
prost.workspace = true
rand.workspace = true
serde.workspace = true
thiserror.workspace = true
tracing.workspace = true

[build-dependencies]
Expand Down
24 changes: 13 additions & 11 deletions node/libs/roles/src/validator/keys/aggregate_signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub struct AggregateSignature(pub(crate) bn254::AggregateSignature);
impl AggregateSignature {
/// Generate a new aggregate signature from a list of signatures.
pub fn aggregate<'a>(sigs: impl IntoIterator<Item = &'a Signature>) -> Self {
AggregateSignature(bn254::AggregateSignature::aggregate(
Self(bn254::AggregateSignature::aggregate(
sigs.into_iter().map(|sig| &sig.0).collect::<Vec<_>>(),
))
}
Expand Down Expand Up @@ -42,30 +42,32 @@ impl AggregateSignature {
}

impl ByteFmt for AggregateSignature {
fn encode(&self) -> Vec<u8> {
ByteFmt::encode(&self.0)
}
fn decode(bytes: &[u8]) -> anyhow::Result<Self> {
ByteFmt::decode(bytes).map(Self)
}

fn encode(&self) -> Vec<u8> {
ByteFmt::encode(&self.0)
}
}

impl TextFmt for AggregateSignature {
fn decode(text: Text) -> anyhow::Result<Self> {
text.strip("validator:aggregate_signature:bn254:")?
.decode_hex()
.map(Self)
}

fn encode(&self) -> String {
format!(
"validator:aggregate_signature:bn254:{}",
hex::encode(ByteFmt::encode(&self.0))
)
}
fn decode(text: Text) -> anyhow::Result<Self> {
text.strip("validator:aggregate_signature:bn254:")?
.decode_hex()
.map(Self)
}
}

impl fmt::Debug for AggregateSignature {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
writeln!(fmt, "Signature: {}", hex::encode(ByteFmt::encode(&self.0)))
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
formatter.write_str(&TextFmt::encode(self))
}
}
Loading