fix(consensus): payload encoding protected by protocol_version (#3168)

Changing payload encoding without protocol version change would
invalidate consensus signatures.

.github/workflows/ci-core-reusable.yml

@@ -8,6 +8,10 @@ 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
+  PASSED_ENV_VARS: RUST_BACKTRACE
+
 jobs:
   lint:
     name: lint

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<Self::Type> {
+        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,32 @@ 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")?
+                .unwrap_or_default(),
+        };
+        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 {
+        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()),
@@ -188,16 +220,11 @@ 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: if self.pubdata_params == PubdataParams::default() {
+                None
+            } else {
+                Some(ProtoRepr::build(&self.pubdata_params))
+            },
         };
         match self.protocol_version {
             v if v >= ProtocolVersionId::Version25 => {

core/lib/dal/src/consensus/mod.rs

@@ -48,7 +48,7 @@ pub struct Payload {
     pub operator_address: Address,
     pub transactions: Vec<Transaction>,
     pub last_in_batch: bool,
-    pub pubdata_params: Option<PubdataParams>,
+    pub pubdata_params: PubdataParams,
 }
 
 impl Payload {

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() {
+            PubdataParams::default()
+        } else {
+            PubdataParams {
+                pubdata_type: match rng.gen_range(0..2) {
+                    0 => L1BatchCommitmentMode::Rollup,
+                    _ => L1BatchCommitmentMode::Validium,
+                },
+                l2_da_validator_address: rng.gen(),
+            }
+        },
     }
 }
 
 /// 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::<FmtConv<AttestationStatus>>(rng);
@@ -78,10 +83,15 @@ fn test_encoding() {
     encode_decode::<proto::Transaction, ComparableTransaction>(
         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<P, C>(msg: P::Type)

core/lib/dal/src/models/storage_sync.rs

@@ -139,7 +139,7 @@ impl SyncBlock {
             operator_address: self.fee_account_address,
             transactions,
             last_in_batch: self.last_in_batch,
-            pubdata_params: Some(self.pubdata_params),
+            pubdata_params: self.pubdata_params,
         }
     }
 }

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

zkstack_cli/rust-toolchain

@@ -1 +1 @@
-1.81.0
+nightly-2024-08-01