CommitSig refactor

CHANGES.md

@@ -1,5 +1,10 @@
 ## Pending
 
+CommitSig:
+- Refactored CommitSig into a more Rust-friendly enum. ([#247](https://github.com/informalsystems/tendermint-rs/issues/247))
+- Added CommitSig compatibility code to Absent vote [#260](https://github.com/informalsystems/tendermint-rs/issues/260)
+- Added CommitSig timestamp zero-check compatibility code [#259](https://github.com/informalsystems/tendermint-rs/issues/259)
+
 Testing:
 - Updated abci_info test to 0.17.0 ([#249](https://github.com/informalsystems/tendermint-rs/issues/249))
 

tendermint/Cargo.toml

@@ -45,6 +45,7 @@ prost-amino-derive = "0.5"
 serde = { version = "1", features = ["derive"] }
 serde_json = { version = "1" }
 serde_bytes = "0.11"
+serde_repr = "0.1"
 sha2 = { version = "0.8", default-features = false }
 signatory = { version = "0.19", features = ["ed25519", "ecdsa"] }
 signatory-dalek = "0.19"

tendermint/src/block/commit_sig.rs

@@ -1,78 +1,119 @@
 //! CommitSig within Commit
 
-use crate::serializers;
+use crate::serializers::BlockIDFlag;
+use crate::serializers::RawCommitSig;
 use crate::{account, Signature, Time};
-use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
-
-/// BlockIDFlag is used to indicate the validator has voted either for nil, a particular BlockID or was absent.
-#[derive(Copy, Clone, Debug, PartialEq)]
-pub enum BlockIDFlag {
-    /// BlockIDFlagAbsent - no vote was received from a validator.
-    BlockIDFlagAbsent = 1,
-    /// BlockIDFlagCommit - voted for the Commit.BlockID.
-    BlockIDFlagCommit = 2,
-    /// BlockIDFlagNil - voted for nil.
-    BlockIDFlagNil = 3,
-}
-
-impl BlockIDFlag {
-    /// Deserialize this type from a byte
-    pub fn from_u8(byte: u8) -> Option<BlockIDFlag> {
-        match byte {
-            1 => Some(BlockIDFlag::BlockIDFlagAbsent),
-            2 => Some(BlockIDFlag::BlockIDFlagCommit),
-            3 => Some(BlockIDFlag::BlockIDFlagNil),
-            _ => None,
-        }
-    }
-
-    /// Serialize this type as a byte
-    pub fn to_u8(self) -> u8 {
-        self as u8
-    }
-
-    /// Serialize this type as a 32-bit unsigned integer
-    pub fn to_u32(self) -> u32 {
-        self as u32
-    }
-}
-
-impl Serialize for BlockIDFlag {
-    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
-        self.to_u8().serialize(serializer)
-    }
-}
-
-impl<'de> Deserialize<'de> for BlockIDFlag {
-    fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
-        let byte = u8::deserialize(deserializer)?;
-        BlockIDFlag::from_u8(byte)
-            .ok_or_else(|| D::Error::custom(format!("invalid block ID flag: {}", byte)))
-    }
-}
+use serde::{Deserialize, Serialize};
+use std::convert::TryFrom;
 
 /// CommitSig represents a signature of a validator.
 /// It's a part of the Commit and can be used to reconstruct the vote set given the validator set.
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
-pub struct CommitSig {
-    /// Block ID FLag
-    pub block_id_flag: BlockIDFlag,
-
-    /// Validator address
-    #[serde(deserialize_with = "serializers::parse_non_empty_id")]
-    pub validator_address: Option<account::Id>,
-
-    /// Timestamp
-    pub timestamp: Time,
-
-    /// Signature
-    #[serde(deserialize_with = "serializers::parse_non_empty_signature")]
-    pub signature: Option<Signature>,
+#[serde(try_from = "RawCommitSig")]
+pub enum CommitSig {
+    /// no vote was received from a validator.
+    // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/260
+    BlockIDFlagAbsent,
+    // === Real code after compatibility issue is resolved
+    /*
+    BlockIDFlagAbsent {
+        /// Validator address
+        validator_address: account::Id,
+    },
+    */
+    // >>> end of real code
+    /// voted for the Commit.BlockID.
+    BlockIDFlagCommit {
+        /// Validator address
+        validator_address: account::Id,
+        /// Timestamp of vote
+        timestamp: Time,
+        /// Signature of vote
+        signature: Signature,
+    },
+    /// voted for nil.
+    BlockIDFlagNil {
+        /// Validator address
+        validator_address: account::Id,
+        /// Timestamp of vote
+        timestamp: Time,
+        /// Signature of vote
+        signature: Signature,
+    },
 }
 
-impl CommitSig {
-    /// Checks if a validator's vote is absent
-    pub fn is_absent(&self) -> bool {
-        self.block_id_flag == BlockIDFlag::BlockIDFlagAbsent
+impl TryFrom<RawCommitSig> for CommitSig {
+    type Error = &'static str;
+
+    // Todo: @melekes asked: where's validator_address validation?
+    fn try_from(value: RawCommitSig) -> Result<Self, Self::Error> {
+        // Validate CommitSig (strict)
+        match value.block_id_flag {
+            BlockIDFlag::BlockIDFlagAbsent => {
+                if value.timestamp.is_some() {
+                    // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/259
+                    if value.timestamp.unwrap()
+                        != Time::parse_from_rfc3339("0001-01-01T00:00:00Z").unwrap()
+                    {
+                        return Err("timestamp is present for BlockIDFlagAbsent CommitSig");
+                    }
+                    // === Real code after compatibility issue is resolved
+                    /*
+                    return Err("timestamp is present for BlockIDFlagAbsent CommitSig");
+                    */
+                    // >>> end of real code
+                }
+                if value.signature.is_some() {
+                    return Err("signature is present for BlockIDFlagAbsent CommitSig");
+                }
+                // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/260
+                Ok(CommitSig::BlockIDFlagAbsent)
+                // === Real code after compatibility issue is resolved
+                /*
+                Ok(CommitSig::BlockIDFlagAbsent {
+                    validator_address: value.validator_address,
+                })
+                */
+                // >>> end of real code
+            }
+            BlockIDFlag::BlockIDFlagCommit => {
+                if value.timestamp.is_none() {
+                    Err("timestamp is null for BlockIDFlagCommit CommitSig")
+                } else if value.signature.is_none() {
+                    Err("signature is null for BlockIDFlagCommit CommitSig")
+                } else {
+                    Ok(CommitSig::BlockIDFlagCommit {
+                        // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/260
+                        validator_address: value.validator_address.unwrap(),
+                        // === Real code after compatibility issue is resolved
+                        /*
+                        validator_address: value.validator_address,
+                        */
+                        // >>> end of real code
+                        timestamp: value.timestamp.unwrap(),
+                        signature: value.signature.unwrap(),
+                    })
+                }
+            }
+            BlockIDFlag::BlockIDFlagNil => {
+                if value.timestamp.is_none() {
+                    Err("timestamp is null for BlockIDFlagNil CommitSig")
+                } else if value.signature.is_none() {
+                    Err("signature is null for BlockIDFlagNil CommitSig")
+                } else {
+                    Ok(CommitSig::BlockIDFlagNil {
+                        // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/260
+                        validator_address: value.validator_address.unwrap(),
+                        // === Real code after compatibility issue is resolved
+                        /*
+                        validator_address: value.validator_address,
+                        */
+                        // >>> end of real code
+                        timestamp: value.timestamp.unwrap(),
+                        signature: value.signature.unwrap(),
+                    })
+                }
+            }
+        }
     }
 }

tendermint/src/lite_impl/signed_header.rs

@@ -1,9 +1,10 @@
 //! [`lite::SignedHeader`] implementation for [`block::signed_header::SignedHeader`].
 
+use crate::block::CommitSig;
 use crate::lite::error::{Error, Kind};
 use crate::lite::ValidatorSet;
 use crate::validator::Set;
-use crate::{block, block::BlockIDFlag, hash, lite, vote};
+use crate::{block, hash, lite, vote};
 use anomaly::fail;
 use std::convert::TryFrom;
 
@@ -59,8 +60,37 @@ impl lite::Commit for block::signed_header::SignedHeader {
             );
         }
 
+        // TODO: this last check is only necessary if we do full verification (2/3)
+        // Todo: @melekes asked why the above is a TODO, find out and resolve it or open an issue
+        // returns ImplementationSpecific error if it detects a signer
+        // that is not present in the validator set:
         for commit_sig in self.commit.signatures.iter() {
-            commit_sig.validate(vals)?;
+            let extracted_validator_address;
+            match commit_sig {
+                // <<< Compatibility code for https://github.com/informalsystems/tendermint-rs/issues/260
+                CommitSig::BlockIDFlagAbsent => continue,
+                // === Real code after compatibility issue is resolved
+                /*
+                CommitSig::BlockIDFlagAbsent => {
+                    validator_address, ..
+                } => extracted_validator_address = validator_address,
+                */
+                // >>> end of real code
+                CommitSig::BlockIDFlagCommit {
+                    validator_address, ..
+                } => extracted_validator_address = validator_address,
+                CommitSig::BlockIDFlagNil {
+                    validator_address, ..
+                } => extracted_validator_address = validator_address,
+            }
+            if vals.validator(*extracted_validator_address) == None {
+                fail!(
+                    Kind::ImplementationSpecific,
+                    "Found a faulty signer ({}) not present in the validator set ({})",
+                    extracted_validator_address,
+                    vals.hash()
+                );
+            }
         }
 
         Ok(())
@@ -72,87 +102,43 @@ impl lite::Commit for block::signed_header::SignedHeader {
 fn non_absent_votes(commit: &block::Commit) -> Vec<vote::Vote> {
     let mut votes: Vec<vote::Vote> = Default::default();
     for (i, commit_sig) in commit.signatures.iter().enumerate() {
-        if commit_sig.is_absent() {
-            continue;
-        }
-
-        if let Some(val_addr) = commit_sig.validator_address {
-            if let Some(sig) = commit_sig.signature.clone() {
-                let vote = vote::Vote {
-                    vote_type: vote::Type::Precommit,
-                    height: commit.height,
-                    round: commit.round,
-                    block_id: Option::from(commit.block_id.clone()),
-                    timestamp: commit_sig.timestamp,
-                    validator_address: val_addr,
-                    validator_index: u64::try_from(i)
-                        .expect("usize to u64 conversion failed for validator index"),
-                    signature: sig,
-                };
-                votes.push(vote);
-            }
-        }
-    }
-    votes
-}
-
-// TODO: consider moving this into commit_sig.rs instead and making it pub
-impl block::commit_sig::CommitSig {
-    fn validate(&self, vals: &Set) -> Result<(), Error> {
-        match self.block_id_flag {
-            BlockIDFlag::BlockIDFlagAbsent => {
-                if self.validator_address.is_some() {
-                    fail!(
-                        Kind::ImplementationSpecific,
-                        "validator address is present for absent CommitSig {:#?}",
-                        self
-                    );
-                }
-                if self.signature.is_some() {
-                    fail!(
-                        Kind::ImplementationSpecific,
-                        "signature is present for absent CommitSig {:#?}",
-                        self
-                    );
-                }
-                // TODO: deal with Time
-                // see https://github.com/informalsystems/tendermint-rs/pull/196#discussion_r401027989
+        let extracted_validator_address;
+        let extracted_timestamp;
+        let extracted_signature;
+        match commit_sig {
+            CommitSig::BlockIDFlagAbsent { .. } => continue,
+            CommitSig::BlockIDFlagCommit {
+                validator_address,
+                timestamp,
+                signature,
+            } => {
+                extracted_validator_address = validator_address;
+                extracted_timestamp = timestamp;
+                extracted_signature = signature;
             }
-            BlockIDFlag::BlockIDFlagCommit | BlockIDFlag::BlockIDFlagNil => {
-                if self.validator_address.is_none() {
-                    fail!(
-                        Kind::ImplementationSpecific,
-                        "missing validator address for non-absent CommitSig {:#?}",
-                        self
-                    );
-                }
-                if self.signature.is_none() {
-                    fail!(
-                        Kind::ImplementationSpecific,
-                        "missing signature for non-absent CommitSig {:#?}",
-                        self
-                    );
-                }
-                // TODO: this last check is only necessary if we do full verification (2/3) but the
-                // above checks should actually happen always (even if we skip forward)
-                //
-                // returns ImplementationSpecific error if it detects a signer
-                // that is not present in the validator set:
-                if let Some(val_addr) = self.validator_address {
-                    if vals.validator(val_addr) == None {
-                        fail!(
-                            Kind::ImplementationSpecific,
-                            "Found a faulty signer ({}) not present in the validator set ({})",
-                            val_addr,
-                            vals.hash()
-                        );
-                    }
-                }
+            CommitSig::BlockIDFlagNil {
+                validator_address,
+                timestamp,
+                signature,
+            } => {
+                extracted_validator_address = validator_address;
+                extracted_timestamp = timestamp;
+                extracted_signature = signature;
             }
         }
-
-        Ok(())
+        votes.push(vote::Vote {
+            vote_type: vote::Type::Precommit,
+            height: commit.height,
+            round: commit.round,
+            block_id: Option::from(commit.block_id.clone()),
+            timestamp: *extracted_timestamp,
+            validator_address: *extracted_validator_address,
+            validator_index: u64::try_from(i)
+                .expect("usize to u64 conversion failed for validator index"),
+            signature: extracted_signature.clone(),
+        })
     }
+    votes
 }
 
 impl block::signed_header::SignedHeader {

tendermint/src/serializers.rs

@@ -40,11 +40,13 @@ pub(crate) mod bytes;
 pub(crate) mod from_str;
 pub(crate) mod time_duration;
 
+mod raw_commit_sig;
+pub(crate) use raw_commit_sig::BlockIDFlag;
+pub(crate) use raw_commit_sig::RawCommitSig;
+
 #[cfg(test)]
 mod tests;
 
 mod custom;
 pub(crate) use custom::parse_non_empty_block_id;
 pub(crate) use custom::parse_non_empty_hash;
-pub(crate) use custom::parse_non_empty_id;
-pub(crate) use custom::parse_non_empty_signature;