From 793d17096d9291b201ab51bc71b6e855e9da213c Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 24 Oct 2024 13:42:30 +0200 Subject: [PATCH 1/5] payload encoding protected by protocol_version --- core/lib/dal/src/consensus/conv.rs | 64 ++++++++++++++----------- core/lib/dal/src/consensus/tests.rs | 34 ++++++++----- core/lib/dal/src/models/storage_sync.rs | 6 ++- 3 files changed, 63 insertions(+), 41 deletions(-) diff --git a/core/lib/dal/src/consensus/conv.rs b/core/lib/dal/src/consensus/conv.rs index 2b8488dd0c2a..a9d30056f877 100644 --- a/core/lib/dal/src/consensus/conv.rs +++ b/core/lib/dal/src/consensus/conv.rs @@ -2,7 +2,7 @@ use anyhow::{anyhow, Context as _}; use zksync_concurrency::net; use zksync_consensus_roles::{attester, node}; -use zksync_protobuf::{read_required, required, ProtoFmt, ProtoRepr}; +use zksync_protobuf::{read_optional_repr, read_required, required, ProtoFmt, ProtoRepr}; use zksync_types::{ abi, commitment::{L1BatchCommitmentMode, PubdataParams}, @@ -104,6 +104,31 @@ impl ProtoFmt for AttestationStatus { } } +impl ProtoRepr for proto::PubdataParams { + type Type = PubdataParams; + + fn read(&self) -> anyhow::Result { + Ok(Self::Type { + l2_da_validator_address: required(&self.l2_da_validator_address) + .and_then(|a| parse_h160(a)) + .context("l2_da_validator_address")?, + pubdata_type: required(&self.pubdata_type) + .and_then(|x| Ok(proto::L1BatchCommitDataGeneratorMode::try_from(*x)?)) + .context("pubdata_type")? + .parse(), + }) + } + + fn build(this: &Self::Type) -> Self { + Self { + l2_da_validator_address: Some(this.l2_da_validator_address.as_bytes().into()), + pubdata_type: Some( + proto::L1BatchCommitDataGeneratorMode::new(&this.pubdata_type) as i32, + ), + } + } +} + impl ProtoFmt for Payload { type Proto = proto::Payload; @@ -137,21 +162,7 @@ impl ProtoFmt for Payload { } } - let pubdata_params = if let Some(pubdata_params) = &r.pubdata_params { - Some(PubdataParams { - l2_da_validator_address: required(&pubdata_params.l2_da_validator_address) - .and_then(|a| parse_h160(a)) - .context("l2_da_validator_address")?, - pubdata_type: required(&pubdata_params.pubdata_type) - .and_then(|x| Ok(proto::L1BatchCommitDataGeneratorMode::try_from(*x)?)) - .context("pubdata_type")? - .parse(), - }) - } else { - None - }; - - Ok(Self { + let this = Self { protocol_version, hash: required(&r.hash) .and_then(|h| parse_h256(h)) @@ -169,11 +180,17 @@ impl ProtoFmt for Payload { .context("operator_address")?, transactions, last_in_batch: *required(&r.last_in_batch).context("last_in_batch")?, - pubdata_params, - }) + pubdata_params: read_optional_repr(&r.pubdata_params).context("pubdata_params")?, + }; + anyhow::ensure!(this.pubdata_params.is_none() == this.protocol_version.is_pre_gateway()); + Ok(this) } fn build(&self) -> Self::Proto { + assert!( + self.protocol_version.is_pre_gateway() == self.pubdata_params.is_none(), + "pubdata_params should be None iff protocol_version is pre-gateway" + ); let mut x = Self::Proto { protocol_version: Some((self.protocol_version as u16).into()), hash: Some(self.hash.as_bytes().into()), @@ -188,16 +205,7 @@ impl ProtoFmt for Payload { transactions: vec![], transactions_v25: vec![], last_in_batch: Some(self.last_in_batch), - pubdata_params: self - .pubdata_params - .map(|pubdata_params| proto::PubdataParams { - l2_da_validator_address: Some( - pubdata_params.l2_da_validator_address.as_bytes().into(), - ), - pubdata_type: Some(proto::L1BatchCommitDataGeneratorMode::new( - &pubdata_params.pubdata_type, - ) as i32), - }), + pubdata_params: self.pubdata_params.as_ref().map(ProtoRepr::build), }; match self.protocol_version { v if v >= ProtocolVersionId::Version25 => { diff --git a/core/lib/dal/src/consensus/tests.rs b/core/lib/dal/src/consensus/tests.rs index c9fd91748b2b..dc34f0c1f596 100644 --- a/core/lib/dal/src/consensus/tests.rs +++ b/core/lib/dal/src/consensus/tests.rs @@ -1,7 +1,7 @@ use std::fmt::Debug; use rand::Rng; -use zksync_concurrency::ctx; +use zksync_concurrency::{ctx, testonly::abort_on_panic}; use zksync_protobuf::{ repr::{decode, encode}, testonly::{test_encode, test_encode_all_formats, FmtConv}, @@ -53,19 +53,24 @@ fn payload(rng: &mut impl Rng, protocol_version: ProtocolVersionId) -> Payload { }) .collect(), last_in_batch: rng.gen(), - pubdata_params: Some(PubdataParams { - pubdata_type: match rng.gen_range(0..2) { - 0 => L1BatchCommitmentMode::Rollup, - _ => L1BatchCommitmentMode::Validium, - }, - l2_da_validator_address: rng.gen(), - }), + pubdata_params: if !protocol_version.is_pre_gateway() { + Some(PubdataParams { + pubdata_type: match rng.gen_range(0..2) { + 0 => L1BatchCommitmentMode::Rollup, + _ => L1BatchCommitmentMode::Validium, + }, + l2_da_validator_address: rng.gen(), + }) + } else { + None + }, } } /// Tests struct <-> proto struct conversions. #[test] fn test_encoding() { + abort_on_panic(); let ctx = &ctx::test_root(&ctx::RealClock); let rng = &mut ctx.rng(); test_encode_all_formats::>(rng); @@ -78,10 +83,15 @@ fn test_encoding() { encode_decode::( mock_protocol_upgrade_transaction().into(), ); - let p = payload(rng, ProtocolVersionId::Version24); - test_encode(rng, &p); - let p = payload(rng, ProtocolVersionId::Version25); - test_encode(rng, &p); + // Test encoding in the current and all the future versions. + for v in ProtocolVersionId::latest() as u16.. { + let Ok(v) = ProtocolVersionId::try_from(v) else { + break; + }; + tracing::info!("version {v}"); + let p = payload(rng, v); + test_encode(rng, &p); + } } fn encode_decode(msg: P::Type) diff --git a/core/lib/dal/src/models/storage_sync.rs b/core/lib/dal/src/models/storage_sync.rs index 0eb65a606d1f..726ff38e5e8f 100644 --- a/core/lib/dal/src/models/storage_sync.rs +++ b/core/lib/dal/src/models/storage_sync.rs @@ -139,7 +139,11 @@ impl SyncBlock { operator_address: self.fee_account_address, transactions, last_in_batch: self.last_in_batch, - pubdata_params: Some(self.pubdata_params), + pubdata_params: if self.protocol_version.is_pre_gateway() { + Some(self.pubdata_params) + } else { + None + }, } } } From 3562bc90eb37f444feeabbdf988d0d40968072ce Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 24 Oct 2024 14:06:05 +0200 Subject: [PATCH 2/5] rust backtrace --- .github/workflows/ci-core-reusable.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/ci-core-reusable.yml b/.github/workflows/ci-core-reusable.yml index 9aaa476d740d..2a560380d61a 100644 --- a/.github/workflows/ci-core-reusable.yml +++ b/.github/workflows/ci-core-reusable.yml @@ -8,6 +8,9 @@ on: required: false default: '[{ "zksolc": ["1.3.14", "1.3.16", "1.3.17", "1.3.1", "1.3.7", "1.3.18", "1.3.19", "1.3.21"] } , { "zkvyper": ["1.3.13"] }]' +env: + RUST_BACKTRACE: 1 + jobs: lint: name: lint From fc3312e3bb451f40270dc329ae2c5beaf84e11e8 Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 24 Oct 2024 14:10:25 +0200 Subject: [PATCH 3/5] nightly for zkstack_cli --- zkstack_cli/rust-toolchain | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zkstack_cli/rust-toolchain b/zkstack_cli/rust-toolchain index dbd41264aa9f..03c040b91f1f 100644 --- a/zkstack_cli/rust-toolchain +++ b/zkstack_cli/rust-toolchain @@ -1 +1 @@ -1.81.0 +nightly-2024-08-01 From 3a4aeb55bd7fa5df847bb404bcb3de27fa3804d2 Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 24 Oct 2024 14:28:28 +0200 Subject: [PATCH 4/5] a type safe solution --- .github/workflows/ci-core-reusable.yml | 1 + core/lib/dal/src/consensus/conv.rs | 27 ++++++++++++++++++------ core/lib/dal/src/consensus/mod.rs | 2 +- core/lib/dal/src/consensus/tests.rs | 10 ++++----- core/lib/dal/src/models/storage_sync.rs | 6 +----- core/node/consensus/src/storage/store.rs | 9 +------- 6 files changed, 29 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci-core-reusable.yml b/.github/workflows/ci-core-reusable.yml index 2a560380d61a..c79e34315763 100644 --- a/.github/workflows/ci-core-reusable.yml +++ b/.github/workflows/ci-core-reusable.yml @@ -10,6 +10,7 @@ on: env: RUST_BACKTRACE: 1 + PASSED_ENV_VARS: RUST_BACKTRACE jobs: lint: diff --git a/core/lib/dal/src/consensus/conv.rs b/core/lib/dal/src/consensus/conv.rs index a9d30056f877..161519290c3e 100644 --- a/core/lib/dal/src/consensus/conv.rs +++ b/core/lib/dal/src/consensus/conv.rs @@ -180,17 +180,26 @@ impl ProtoFmt for Payload { .context("operator_address")?, transactions, last_in_batch: *required(&r.last_in_batch).context("last_in_batch")?, - pubdata_params: read_optional_repr(&r.pubdata_params).context("pubdata_params")?, + pubdata_params: read_optional_repr(&r.pubdata_params) + .context("pubdata_params")? + .unwrap_or_default(), }; - anyhow::ensure!(this.pubdata_params.is_none() == this.protocol_version.is_pre_gateway()); + if this.protocol_version.is_pre_gateway() { + anyhow::ensure!( + this.pubdata_params == PubdataParams::default(), + "pubdata_params should have the default value in pre-gateway protocol_version" + ); + } + if this.pubdata_params == PubdataParams::default() { + anyhow::ensure!( + r.pubdata_params.is_none(), + "default pubdata_params should be encoded as None" + ); + } Ok(this) } fn build(&self) -> Self::Proto { - assert!( - self.protocol_version.is_pre_gateway() == self.pubdata_params.is_none(), - "pubdata_params should be None iff protocol_version is pre-gateway" - ); let mut x = Self::Proto { protocol_version: Some((self.protocol_version as u16).into()), hash: Some(self.hash.as_bytes().into()), @@ -205,7 +214,11 @@ impl ProtoFmt for Payload { transactions: vec![], transactions_v25: vec![], last_in_batch: Some(self.last_in_batch), - pubdata_params: self.pubdata_params.as_ref().map(ProtoRepr::build), + pubdata_params: if self.pubdata_params == PubdataParams::default() { + None + } else { + Some(ProtoRepr::build(&self.pubdata_params)) + }, }; match self.protocol_version { v if v >= ProtocolVersionId::Version25 => { diff --git a/core/lib/dal/src/consensus/mod.rs b/core/lib/dal/src/consensus/mod.rs index c7e46b2cf1b7..96efc6348350 100644 --- a/core/lib/dal/src/consensus/mod.rs +++ b/core/lib/dal/src/consensus/mod.rs @@ -48,7 +48,7 @@ pub struct Payload { pub operator_address: Address, pub transactions: Vec, pub last_in_batch: bool, - pub pubdata_params: Option, + pub pubdata_params: PubdataParams, } impl Payload { diff --git a/core/lib/dal/src/consensus/tests.rs b/core/lib/dal/src/consensus/tests.rs index dc34f0c1f596..df6ee24bfa94 100644 --- a/core/lib/dal/src/consensus/tests.rs +++ b/core/lib/dal/src/consensus/tests.rs @@ -53,16 +53,16 @@ fn payload(rng: &mut impl Rng, protocol_version: ProtocolVersionId) -> Payload { }) .collect(), last_in_batch: rng.gen(), - pubdata_params: if !protocol_version.is_pre_gateway() { - Some(PubdataParams { + pubdata_params: if protocol_version.is_pre_gateway() { + PubdataParams::default() + } else { + PubdataParams { pubdata_type: match rng.gen_range(0..2) { 0 => L1BatchCommitmentMode::Rollup, _ => L1BatchCommitmentMode::Validium, }, l2_da_validator_address: rng.gen(), - }) - } else { - None + } }, } } diff --git a/core/lib/dal/src/models/storage_sync.rs b/core/lib/dal/src/models/storage_sync.rs index 726ff38e5e8f..3f80f52c56eb 100644 --- a/core/lib/dal/src/models/storage_sync.rs +++ b/core/lib/dal/src/models/storage_sync.rs @@ -139,11 +139,7 @@ impl SyncBlock { operator_address: self.fee_account_address, transactions, last_in_batch: self.last_in_batch, - pubdata_params: if self.protocol_version.is_pre_gateway() { - Some(self.pubdata_params) - } else { - None - }, + pubdata_params: self.pubdata_params, } } } diff --git a/core/node/consensus/src/storage/store.rs b/core/node/consensus/src/storage/store.rs index 53be2fc63c75..4dce9041a106 100644 --- a/core/node/consensus/src/storage/store.rs +++ b/core/node/consensus/src/storage/store.rs @@ -28,13 +28,6 @@ fn to_fetched_block( .context("Integer overflow converting block number")?, ); let payload = Payload::decode(payload).context("Payload::decode()")?; - let pubdata_params = if payload.protocol_version.is_pre_gateway() { - payload.pubdata_params.unwrap_or_default() - } else { - payload - .pubdata_params - .context("Missing `pubdata_params` for post-gateway payload")? - }; Ok(FetchedBlock { number, l1_batch_number: payload.l1_batch_number, @@ -45,7 +38,7 @@ fn to_fetched_block( l1_gas_price: payload.l1_gas_price, l2_fair_gas_price: payload.l2_fair_gas_price, fair_pubdata_price: payload.fair_pubdata_price, - pubdata_params, + pubdata_params: payload.pubdata_params, virtual_blocks: payload.virtual_blocks, operator_address: payload.operator_address, transactions: payload From 7bd28baf2cb79f4d0c17ee19cda0ece7bef53be6 Mon Sep 17 00:00:00 2001 From: Grzegorz Prusak Date: Thu, 24 Oct 2024 14:44:38 +0200 Subject: [PATCH 5/5] added back the assert --- core/lib/dal/src/consensus/conv.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/lib/dal/src/consensus/conv.rs b/core/lib/dal/src/consensus/conv.rs index 161519290c3e..f0948adfd1da 100644 --- a/core/lib/dal/src/consensus/conv.rs +++ b/core/lib/dal/src/consensus/conv.rs @@ -200,6 +200,12 @@ impl ProtoFmt for Payload { } fn build(&self) -> Self::Proto { + if self.protocol_version.is_pre_gateway() { + assert_eq!( + self.pubdata_params, PubdataParams::default(), + "BUG DETECTED: pubdata_params should have the default value in pre-gateway protocol_version" + ); + } let mut x = Self::Proto { protocol_version: Some((self.protocol_version as u16).into()), hash: Some(self.hash.as_bytes().into()),