Merge remote-tracking branch 'origin/main' into gprusak-dns

node/Cargo.toml

@@ -40,7 +40,7 @@ zksync_protobuf = { path = "libs/protobuf" }
 zksync_protobuf_build = { path = "libs/protobuf_build" }
 
 # Crates from Matter Labs.
-pairing = { package = "pairing_ce", git = "https://github.com/matter-labs/pairing.git", rev = "f55393fd366596eac792d78525d26e9c4d6ed1ca" }
+pairing = { package = "pairing_ce", git = "https://github.com/matter-labs/pairing.git", rev = "d24f2c5871089c4cd4f54c0ca266bb9fef6115eb" }
 vise = { version = "0.1.0", git = "https://github.com/matter-labs/vise.git", rev = "1c9cc500e92cf9ea052b230e114a6f9cce4fb2c1" }
 vise-exporter = { version = "0.1.0", git = "https://github.com/matter-labs/vise.git", rev = "1c9cc500e92cf9ea052b230e114a6f9cce4fb2c1" }
 
@@ -54,7 +54,7 @@ clap = { version = "4.3.3", features = ["derive"] }
 criterion = "0.5.1"
 ed25519-dalek = { version = "2.0.0", features = ["rand_core"] }
 ff_ce = "0.14.3"
-heck = "0.4.1"
+heck = "0.5.0"
 hex = "0.4.3"
 im = "15.1.0"
 once_cell = "1.17.1"
@@ -117,11 +117,9 @@ opt-level = 3
 
 [workspace.lints.rust]
 unsafe_code = "deny"
-noop_method_call = "warn"
 missing_docs = "warn"
 unreachable_pub = "warn"
 unused_qualifications = "warn"
-unused_tuple_struct_fields = "warn"
 
 [workspace.lints.clippy]
 # restriction group

node/actors/bft/src/leader/replica_commit.rs

@@ -37,7 +37,7 @@ pub(crate) enum Error {
     NotLeaderInView,
     /// Invalid message.
     #[error("invalid message: {0:#}")]
-    InvalidMessage(anyhow::Error),
+    InvalidMessage(#[source] anyhow::Error),
     /// Duplicate message from a replica.
     #[error("duplicate message from a replica (existing message: {existing_message:?}")]
     DuplicateMessage {
@@ -46,7 +46,7 @@ pub(crate) enum Error {
     },
     /// Invalid message signature.
     #[error("invalid signature: {0:#}")]
-    InvalidSignature(#[source] validator::Error),
+    InvalidSignature(#[source] anyhow::Error),
 }
 
 impl StateMachine {

node/actors/bft/src/leader/replica_prepare.rs

@@ -40,7 +40,7 @@ pub(crate) enum Error {
     },
     /// Invalid message signature.
     #[error("invalid signature: {0:#}")]
-    InvalidSignature(#[source] validator::Error),
+    InvalidSignature(#[source] anyhow::Error),
     /// Invalid message.
     #[error(transparent)]
     InvalidMessage(validator::ReplicaPrepareVerifyError),

node/actors/bft/src/replica/leader_commit.rs

@@ -33,7 +33,7 @@ pub(crate) enum Error {
     },
     /// Invalid message signature.
     #[error("invalid signature: {0:#}")]
-    InvalidSignature(validator::Error),
+    InvalidSignature(#[source] anyhow::Error),
     /// Invalid message.
     #[error("invalid message: {0:#}")]
     InvalidMessage(validator::CommitQCVerifyError),

node/actors/bft/src/replica/leader_prepare.rs

@@ -38,7 +38,7 @@ pub(crate) enum Error {
     },
     /// Invalid message signature.
     #[error("invalid signature: {0:#}")]
-    InvalidSignature(#[source] validator::Error),
+    InvalidSignature(#[source] anyhow::Error),
     /// Invalid message.
     #[error("invalid message: {0:#}")]
     InvalidMessage(#[source] validator::LeaderPrepareVerifyError),

node/actors/network/src/consensus/handshake/mod.rs

@@ -50,7 +50,7 @@ pub(super) enum Error {
     #[error("unexpected peer")]
     PeerMismatch,
     #[error("validator signature {0}")]
-    Signature(#[from] validator::Error),
+    Signature(#[from] anyhow::Error),
     #[error("stream {0}")]
     Stream(#[source] anyhow::Error),
 }

node/actors/sync_blocks/src/peers/events.rs

@@ -4,7 +4,7 @@ use zksync_consensus_roles::{node, validator::BlockNumber};
 
 /// Events emitted by `PeerStates` actor. Only used for tests so far.
 #[derive(Debug)]
-#[allow(dead_code, unused_tuple_struct_fields)] // Variant fields are only read in tests
+#[allow(dead_code)] // Variant fields are only read in tests
 pub(super) enum PeerStateEvent {
     /// Node has successfully downloaded the specified block.
     GotBlock(BlockNumber),

node/deny.toml

@@ -59,6 +59,7 @@ multiple-versions = "deny"
 skip = [
     # Old versions required by tempfile and prost-build.
     { name = "bitflags", version = "1.3.2" },
+    { name = "heck", version = "0.4.1" },
 
     # Old version required by tracing-subscriber.
     { name = "regex-automata", version = "0.1.10" },

node/libs/concurrency/src/testonly.rs

@@ -37,7 +37,7 @@ pub fn abort_on_panic() {
 
 /// Guard which has to be dropped before timeout is reached.
 /// Otherwise the test will panic.
-#[allow(unused_tuple_struct_fields)]
+#[allow(dead_code)]
 #[must_use]
 pub struct TimeoutGuard(std::sync::mpsc::Sender<()>);
 

node/libs/crypto/src/bn254/mod.rs

@@ -4,7 +4,6 @@
 //! hence it does not protect the secret key from side-channel attacks.
 
 use crate::ByteFmt;
-pub use error::Error;
 use ff_ce::Field as _;
 use pairing::{
     bn256::{Bn256, Fq12, Fr, FrRepr, G1Affine, G1Compressed, G2Affine, G2Compressed, G1, G2},
@@ -18,9 +17,6 @@ use std::{
     io::Cursor,
 };
 
-#[doc(hidden)]
-pub mod error;
-
 #[cfg(test)]
 mod tests;
 
@@ -33,13 +29,25 @@ pub struct SecretKey(Fr);
 impl SecretKey {
     /// Generates a secret key from a cryptographically-secure entropy source.
     pub fn generate() -> Self {
-        Self(rand04::Rand::rand(&mut rand04::OsRng::new().unwrap()))
+        loop {
+            let fr: Fr = rand04::Rand::rand(&mut rand04::OsRng::new().unwrap());
+
+            if !fr.is_zero() {
+                return Self(fr);
+            }
+        }
     }
 
     /// Gets the corresponding [`PublicKey`] for this [`SecretKey`]
     pub fn public(&self) -> PublicKey {
         let p = G2Affine::one().mul(self.0);
-        PublicKey(p)
+        let pk = PublicKey(p);
+
+        // Verify public key is valid. Since we already check the validity of a
+        // secret key when constructing it, this should never fail (in theory).
+        pk.verify().unwrap();
+
+        pk
     }
 
     /// Produces a signature using this [`SecretKey`]
@@ -55,6 +63,9 @@ impl ByteFmt for SecretKey {
         let mut fr_repr = FrRepr::default();
         fr_repr.read_be(Cursor::new(bytes))?;
         let fr = Fr::from_repr(fr_repr)?;
+
+        anyhow::ensure!(!fr.is_zero(), "Secret key can't be zero");
+
         Ok(SecretKey(fr))
     }
 
@@ -81,9 +92,21 @@ impl PartialEq for SecretKey {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct PublicKey(G2);
 
-impl Default for PublicKey {
-    fn default() -> Self {
-        PublicKey(G2::zero())
+impl PublicKey {
+    /// Checks if the public key is not the identity element and is in the correct subgroup. Verifying signatures
+    /// against public keys that are not valid is insecure.
+    fn verify(&self) -> anyhow::Result<()> {
+        // Check that the point is not the identity element.
+        anyhow::ensure!(!self.0.is_zero(), "Public key can't be zero");
+
+        // We multiply the point by the order and check if the result is the identity element.
+        // If it is, then the point is on the correct subgroup.
+        let order = Fr::char();
+        let mut p = self.0;
+        p.mul_assign(order);
+        anyhow::ensure!(p.is_zero(), "Public key must be in the correct subgroup");
+
+        Ok(())
     }
 }
 
@@ -96,10 +119,12 @@ impl Hash for PublicKey {
 impl ByteFmt for PublicKey {
     fn decode(bytes: &[u8]) -> anyhow::Result<Self> {
         let arr: [u8; 64] = bytes.try_into()?;
-        let p = G2Compressed::from_fixed_bytes(arr)
-            .into_affine()?
-            .into_projective();
-        Ok(PublicKey(p))
+        let p = G2Compressed::from_fixed_bytes(arr).into_affine()?;
+        let pk = PublicKey(p.into());
+
+        pk.verify()?;
+
+        Ok(pk)
     }
 
     fn encode(&self) -> Vec<u8> {
@@ -129,19 +154,21 @@ impl Signature {
     /// This function is intentionally non-generic and disallow inlining to ensure that compilation optimizations can be effectively applied.
     /// This optimization is needed for ensuring that tests can run within a reasonable time frame.
     #[inline(never)]
-    pub fn verify(&self, msg: &[u8], pk: &PublicKey) -> Result<(), Error> {
+    pub fn verify(&self, msg: &[u8], pk: &PublicKey) -> anyhow::Result<()> {
+        // Verify public key is valid. Since we already check the validity of a
+        // public key when constructing it, this should never fail (in theory).
+        pk.verify().unwrap();
+
         let hash_point = hash::hash_to_g1(msg);
 
         // First pair: e(H(m): G1, pk: G2)
         let a = Bn256::pairing(hash_point, pk.0);
         // Second pair: e(sig: G1, generator: G2)
         let b = Bn256::pairing(self.0, G2Affine::one());
 
-        if a == b {
-            Ok(())
-        } else {
-            Err(Error::SignatureVerificationFailure)
-        }
+        anyhow::ensure!(a == b, "Signature verification failure");
+
+        Ok(())
     }
 }
 
@@ -190,13 +217,22 @@ impl AggregateSignature {
     /// This function is intentionally non-generic and disallow inlining to ensure that compilation optimizations can be effectively applied.
     /// This optimization is needed for ensuring that tests can run within a reasonable time frame.
     #[inline(never)]
-    fn verify_raw(&self, msgs_and_pks: &[(&[u8], &PublicKey)]) -> Result<(), Error> {
+    fn verify_raw(&self, msgs_and_pks: &[(&[u8], &PublicKey)]) -> anyhow::Result<()> {
         // Aggregate public keys if they are signing the same hash. Each public key aggregated
         // is one fewer pairing to calculate.
         let mut pairs: HashMap<&[u8], PublicKey> = HashMap::new();
+
         for (msg, pk) in msgs_and_pks {
-            pairs.entry(msg).or_default().0.add_assign(&pk.0);
+            // Verify public key is valid. Since we already check the validity of a
+            // public key when constructing it, this should never fail (in theory).
+            pk.verify().unwrap();
+
+            pairs
+                .entry(msg)
+                .and_modify(|agg_pk| agg_pk.0.add_assign(&pk.0))
+                .or_insert((*pk).clone());
         }
+
         // First pair: e(sig: G1, generator: G2)
         let a = Bn256::pairing(self.0, G2::one());
 
@@ -206,11 +242,9 @@ impl AggregateSignature {
             b.mul_assign(&Bn256::pairing(hash::hash_to_g1(msg), pk.0))
         }
 
-        if a == b {
-            Ok(())
-        } else {
-            Err(Error::AggregateSignatureVerificationFailure)
-        }
+        anyhow::ensure!(a == b, "Aggregate signature verification failure");
+
+        Ok(())
     }
 
     /// Verifies an aggregated signature for multiple messages against the provided list of public keys.
@@ -219,7 +253,7 @@ impl AggregateSignature {
     pub fn verify<'a>(
         &self,
         msgs_and_pks: impl Iterator<Item = (&'a [u8], &'a PublicKey)>,
-    ) -> Result<(), Error> {
+    ) -> anyhow::Result<()> {
         self.verify_raw(&msgs_and_pks.collect::<Vec<_>>()[..])
     }
 }

node/libs/crypto/src/bn254/tests.rs

@@ -2,6 +2,11 @@ use crate::{
     bn254::{AggregateSignature, PublicKey, SecretKey, Signature},
     ByteFmt,
 };
+use ff_ce::PrimeField;
+use pairing::{
+    bn256::{Fr, G2Affine, G2},
+    CurveAffine, CurveProjective,
+};
 use rand::{rngs::StdRng, Rng, SeedableRng};
 use std::iter::repeat_with;
 
@@ -113,3 +118,34 @@ fn byte_fmt_correctness() {
     let agg_decoded = AggregateSignature::decode(&bytes).unwrap();
     assert_eq!(agg, agg_decoded);
 }
+
+#[test]
+fn pk_is_valid_correctness() {
+    // Check that the null point is invalid.
+    let mut pk = PublicKey(G2::zero());
+    assert!(pk.verify().is_err());
+
+    // Check that a point in the wrong subgroup is invalid.
+    let mut rng = rand04::OsRng::new().unwrap();
+
+    loop {
+        let x = rand04::Rand::rand(&mut rng);
+        let greatest = rand04::Rand::rand(&mut rng);
+
+        if let Some(p) = G2Affine::get_point_from_x(x, greatest) {
+            if !p.is_zero() && p.is_on_curve() {
+                // Check that it's not on the subgroup.
+                let order = Fr::char();
+                let mut p = p.into_projective();
+                p.mul_assign(order);
+
+                if !p.is_zero() {
+                    pk = PublicKey(p);
+                    break;
+                }
+            }
+        }
+    }
+
+    assert!(pk.verify().is_err());
+}

node/libs/roles/src/validator/keys/aggregate_signature.rs

@@ -1,4 +1,4 @@
-use super::{Error, PublicKey, Signature};
+use super::{PublicKey, Signature};
 use crate::validator::messages::{Msg, MsgHash};
 use std::fmt;
 use zksync_consensus_crypto::{bn254, ByteFmt, Text, TextFmt};
@@ -18,7 +18,7 @@ impl AggregateSignature {
     pub(crate) fn verify_messages<'a, V: Variant<Msg>>(
         &self,
         messages_and_keys: impl Iterator<Item = (V, &'a PublicKey)>,
-    ) -> Result<(), Error> {
+    ) -> anyhow::Result<()> {
         let hashes_and_keys =
             messages_and_keys.map(|(message, key)| (message.insert().hash(), key));
         self.verify_hash(hashes_and_keys)
@@ -28,7 +28,7 @@ impl AggregateSignature {
     pub(crate) fn verify_hash<'a>(
         &self,
         hashes_and_keys: impl Iterator<Item = (MsgHash, &'a PublicKey)>,
-    ) -> Result<(), Error> {
+    ) -> anyhow::Result<()> {
         let bytes_and_pks: Vec<_> = hashes_and_keys
             .map(|(hash, pk)| (hash.0.as_bytes().to_owned(), &pk.0))
             .collect();

node/libs/roles/src/validator/keys/mod.rs

@@ -9,6 +9,3 @@ pub use aggregate_signature::AggregateSignature;
 pub use public_key::PublicKey;
 pub use secret_key::SecretKey;
 pub use signature::Signature;
-
-/// Error type returned by validator key operations.
-pub type Error = zksync_consensus_crypto::bn254::Error;

node/libs/roles/src/validator/keys/signature.rs

@@ -1,4 +1,4 @@
-use super::{Error, PublicKey};
+use super::PublicKey;
 use crate::validator::messages::{Msg, MsgHash};
 use std::fmt;
 use zksync_consensus_crypto::{bn254, ByteFmt, Text, TextFmt};
@@ -9,12 +9,12 @@ pub struct Signature(pub(crate) bn254::Signature);
 
 impl Signature {
     /// Verify a message against a public key.
-    pub fn verify_msg(&self, msg: &Msg, pk: &PublicKey) -> Result<(), Error> {
+    pub fn verify_msg(&self, msg: &Msg, pk: &PublicKey) -> anyhow::Result<()> {
         self.verify_hash(&msg.hash(), pk)
     }
 
     /// Verify a message hash against a public key.
-    pub fn verify_hash(&self, msg_hash: &MsgHash, pk: &PublicKey) -> Result<(), Error> {
+    pub fn verify_hash(&self, msg_hash: &MsgHash, pk: &PublicKey) -> anyhow::Result<()> {
         self.0.verify(&ByteFmt::encode(msg_hash), &pk.0)
     }
 }