diff --git a/CHANGES.md b/CHANGES.md index 5a8eba9da..ef6ea4636 100644 --- a/CHANGES.md +++ b/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)) diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml index 24fc4a12f..2bacac08b 100644 --- a/tendermint/Cargo.toml +++ b/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" diff --git a/tendermint/src/block/commit_sig.rs b/tendermint/src/block/commit_sig.rs index efcf3df57..6fead48ca 100644 --- a/tendermint/src/block/commit_sig.rs +++ b/tendermint/src/block/commit_sig.rs @@ -1,79 +1,88 @@ //! 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 { - 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(&self, serializer: S) -> Result { - self.to_u8().serialize(serializer) - } -} - -impl<'de> Deserialize<'de> for BlockIDFlag { - fn deserialize>(deserializer: D) -> Result { - 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, - - /// Timestamp - pub timestamp: Time, - - /// Signature - #[serde(deserialize_with = "serializers::parse_non_empty_signature")] - pub signature: Option, +#[serde(try_from = "RawCommitSig")] +pub enum CommitSig { + /// no vote was received from a validator. + // Todo: https://github.com/informalsystems/tendermint-rs/issues/260 - CommitSig validator address missing in Absent vote + BlockIDFlagAbsent, + /// 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 +// Todo: https://github.com/informalsystems/tendermint-rs/issues/259 - CommitSig Timestamp can be zero time +// Todo: https://github.com/informalsystems/tendermint-rs/issues/260 - CommitSig validator address missing in Absent vote +impl TryFrom for CommitSig { + type Error = &'static str; + + fn try_from(value: RawCommitSig) -> Result { + match value.block_id_flag { + BlockIDFlag::BlockIDFlagAbsent => { + if value.timestamp.is_some() + && value.timestamp.unwrap() + != Time::parse_from_rfc3339("0001-01-01T00:00:00Z").unwrap() + { + return Err("timestamp is present for BlockIDFlagAbsent CommitSig"); + } + if value.signature.is_some() { + return Err("signature is present for BlockIDFlagAbsent CommitSig"); + } + Ok(CommitSig::BlockIDFlagAbsent) + } + BlockIDFlag::BlockIDFlagCommit => { + if value.timestamp.is_none() { + Err("timestamp is missing for BlockIDFlagCommit CommitSig") + } else if value.signature.is_none() { + Err("signature is missing for BlockIDFlagCommit CommitSig") + } else if value.validator_address.is_none() { + Err("validator_address is missing for BlockIDFlagCommit CommitSig") + } else { + Ok(CommitSig::BlockIDFlagCommit { + validator_address: value.validator_address.unwrap(), + timestamp: value.timestamp.unwrap(), + signature: value.signature.unwrap(), + }) + } + } + BlockIDFlag::BlockIDFlagNil => { + if value.timestamp.is_none() { + Err("timestamp is missing for BlockIDFlagNil CommitSig") + } else if value.signature.is_none() { + Err("signature is missing for BlockIDFlagNil CommitSig") + } else if value.validator_address.is_none() { + Err("validator_address is missing for BlockIDFlagNil CommitSig") + } else { + Ok(CommitSig::BlockIDFlagNil { + validator_address: value.validator_address.unwrap(), + timestamp: value.timestamp.unwrap(), + signature: value.signature.unwrap(), + }) + } + } + } } } diff --git a/tendermint/src/lite_impl/signed_header.rs b/tendermint/src/lite_impl/signed_header.rs index 33e94ddd3..1c4a8781a 100644 --- a/tendermint/src/lite_impl/signed_header.rs +++ b/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,30 @@ impl lite::Commit for block::signed_header::SignedHeader { ); } + // TODO: this last check is only necessary if we do full verification (2/3) + // https://github.com/informalsystems/tendermint-rs/issues/281 + // 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 { + // Todo: https://github.com/informalsystems/tendermint-rs/issues/260 - CommitSig validator address missing in Absent vote + CommitSig::BlockIDFlagAbsent => continue, + 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 +95,43 @@ impl lite::Commit for block::signed_header::SignedHeader { fn non_absent_votes(commit: &block::Commit) -> Vec { let mut votes: Vec = 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 { diff --git a/tendermint/src/serializers.rs b/tendermint/src/serializers.rs index e40ae3afe..1e20e6ecf 100644 --- a/tendermint/src/serializers.rs +++ b/tendermint/src/serializers.rs @@ -42,11 +42,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; diff --git a/tendermint/src/serializers/custom.rs b/tendermint/src/serializers/custom.rs index d8417495c..07453728d 100644 --- a/tendermint/src/serializers/custom.rs +++ b/tendermint/src/serializers/custom.rs @@ -1,7 +1,6 @@ //! Custom, legacy serializers -use crate::account::{Id, LENGTH}; -use crate::{block, Hash, Signature}; +use crate::{block, Hash}; use serde::{de::Error as _, Deserialize, Deserializer}; use std::str::FromStr; @@ -59,36 +58,3 @@ where })) } } - -/// Option deserialization -pub(crate) fn parse_non_empty_id<'de, D>(deserializer: D) -> Result, D::Error> -where - D: Deserializer<'de>, -{ - let s = String::deserialize(deserializer)?; - if s.is_empty() { - Ok(None) - } else { - // TODO: how can we avoid rewriting code here? - match Id::from_str(&s).map_err(|_| { - D::Error::custom(format!( - "expected {}-character hex string, got {:?}", - LENGTH * 2, - s - )) - }) { - Ok(id) => Ok(Option::from(id)), - Err(_) => Ok(None), - } - } -} - -/// Option deserialization -pub(crate) fn parse_non_empty_signature<'de, D>( - deserializer: D, -) -> Result, D::Error> -where - D: Deserializer<'de>, -{ - Deserialize::deserialize(deserializer).map(|x: Option<_>| x.unwrap_or(None)) -} diff --git a/tendermint/src/serializers/raw_commit_sig.rs b/tendermint/src/serializers/raw_commit_sig.rs new file mode 100644 index 000000000..4f411f0d6 --- /dev/null +++ b/tendermint/src/serializers/raw_commit_sig.rs @@ -0,0 +1,59 @@ +//! RawCommitSig type for deserialization +use crate::{account, Signature, Time}; +use serde::de::Error; +use serde::{Deserialize, Deserializer}; +use serde_repr::{Deserialize_repr, Serialize_repr}; +use std::str::FromStr; + +// Implements decision: https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-025-commit.md#decision + +/// indicate which BlockID the signature is for +#[derive(Serialize_repr, Deserialize_repr, PartialEq, Debug)] +#[repr(u8)] +pub enum BlockIDFlag { + /// vote is not included in the Commit.Precommits + BlockIDFlagAbsent = 1, + /// voted for the Commit.BlockID + BlockIDFlagCommit = 2, + /// voted for nil + BlockIDFlagNil = 3, +} + +/// RawCommitSig struct for interim deserialization of JSON object +#[derive(Deserialize)] +pub struct RawCommitSig { + /// indicate which BlockID the signature is for + pub block_id_flag: BlockIDFlag, + /// Validator Address + // Todo: https://github.com/informalsystems/tendermint-rs/issues/260 - CommitSig validator address missing in Absent vote + #[serde(default, deserialize_with = "emptystring_or_accountid")] + pub validator_address: Option, + /// Timestamp + #[serde(default)] + pub timestamp: Option