feat: documented the dangers of ordering in message encoding (#225)

there is no cheap fix, so we are adding documentation instead.

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

@@ -174,6 +174,7 @@ impl std::hash::Hash for Signature {
 }
 
 /// Type safety wrapper around a `blst` aggregate signature
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Debug)]
 pub struct AggregateSignature(bls::AggregateSignature);
 

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

@@ -7,6 +7,7 @@ mod test;
 pub mod testonly;
 
 /// Keccak256 hash.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Keccak256(pub(crate) [u8; 32]);
 

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

@@ -5,6 +5,7 @@ use zksync_consensus_crypto::{bls12_381, ByteFmt, Text, TextFmt};
 use zksync_consensus_utils::enum_util::Variant;
 
 /// An aggregate signature from a validator.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Default)]
 pub struct AggregateSignature(pub(crate) bls12_381::AggregateSignature);
 

node/libs/roles/src/validator/messages/block.rs

@@ -39,6 +39,7 @@ impl Payload {
 }
 
 /// Hash of the Payload.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub struct PayloadHash(pub(crate) Keccak256);
 
@@ -62,6 +63,7 @@ impl fmt::Debug for PayloadHash {
 }
 
 /// Sequential number of the block.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct BlockNumber(pub u64);
 
@@ -91,6 +93,7 @@ impl fmt::Display for BlockNumber {
 }
 
 /// A block header.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct BlockHeader {
     /// Number of the block.

node/libs/roles/src/validator/messages/consensus.rs

@@ -92,6 +92,7 @@ impl Variant<Msg> for ReplicaTimeout {
 }
 
 /// View specification.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct View {
     /// Genesis of the chain this view belongs to.
@@ -125,6 +126,7 @@ impl View {
 }
 
 /// A struct that represents a view number.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ViewNumber(pub u64);
 
@@ -148,6 +150,7 @@ impl fmt::Display for ViewNumber {
 
 /// Struct that represents a bit map of validators. We use it to compactly store
 /// which validators signed a given message.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub struct Signers(pub BitVec);
 

node/libs/roles/src/validator/messages/genesis.rs

@@ -33,6 +33,7 @@ impl GenesisRaw {
 }
 
 /// Hash of the genesis specification.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct GenesisHash(pub(crate) Keccak256);
 

node/libs/roles/src/validator/messages/replica_commit.rs

@@ -2,6 +2,7 @@ use super::{BlockHeader, Genesis, Signed, Signers, View};
 use crate::validator;
 
 /// A commit message from a replica.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ReplicaCommit {
     /// View of this message.
@@ -31,6 +32,7 @@ pub enum ReplicaCommitVerifyError {
 
 /// A Commit Quorum Certificate. It is an aggregate of signed ReplicaCommit messages.
 /// The Commit Quorum Certificate is over identical messages, so we only need one message.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub struct CommitQC {
     /// The ReplicaCommit message that the QC is for.

node/libs/roles/src/validator/messages/replica_timeout.rs

@@ -6,6 +6,7 @@ use crate::validator;
 use std::collections::{BTreeMap, HashMap};
 
 /// A timeout message from a replica.
+/// WARNING: any change to this struct may invalidate preexisting signatures. See `TimeoutQC` docs.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub struct ReplicaTimeout {
     /// View of this message.
@@ -53,7 +54,20 @@ pub enum ReplicaTimeoutVerifyError {
 
 /// A quorum certificate of ReplicaTimeout messages. Since not all ReplicaTimeout messages are
 /// identical (they have different high blocks and high QCs), we need to keep the ReplicaTimeout
-///  messages in a map. We can still aggregate the signatures though.
+/// messages in a map. We can still aggregate the signatures though.
+///
+/// WARNING: `TimeoutQC` message contains a map indexed by `ReplicaTimeout` messages.
+/// Therefore, `Ord` implementation of `ReplicaTimeout` affects the unique encoding of the `TimeoutQC` message.
+/// This `Ord` implementation is the derived lexicographic ordering of the fields of
+/// `ReplicaTimeout` (and transitively on types of the fields AS WELL).
+/// As a result ANY change to type of ANY transitive field of ReplicaTimeout struct may invalidate
+/// preexisting signatures of `TimeoutQC` messages (or messages containing `TimeoutQC`).
+///
+/// The proper fix would be to add support for unordered collections on the protobuf level
+/// (for example, by adding a custom option "unordered" for repeated fields, which would make the encoder
+/// sort the encoded elements before encoding the whole message). However the current protobuf
+/// encoding of TimeoutQC keeps the keys and values in separate repeated fields. Until this is
+/// fixed, ANY change to the above mentioned types may be backward incompatible.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct TimeoutQC {
     /// View of this QC.