-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commit followup #199
commit followup #199
Changes from all commits
c529e69
7191d32
d6be3e0
deecf80
4a1ff69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,9 @@ | |
use crate::lite::error::{Error, Kind}; | ||
use crate::lite::ValidatorSet; | ||
use crate::validator::Set; | ||
use crate::{block, hash, lite, vote}; | ||
use crate::{block, block::BlockIDFlag, hash, lite, vote}; | ||
use anomaly::fail; | ||
use std::convert::TryFrom; | ||
|
||
impl lite::Commit for block::signed_header::SignedHeader { | ||
type ValidatorSet = Set; | ||
|
@@ -16,13 +17,9 @@ impl lite::Commit for block::signed_header::SignedHeader { | |
// NOTE we don't know the validators that committed this block, | ||
// so we have to check for each vote if its validator is already known. | ||
let mut signed_power = 0u64; | ||
for vote in &self.iter().unwrap() { | ||
// skip absent and nil votes | ||
// NOTE: do we want to check the validity of votes | ||
// for nil ? | ||
// TODO: clarify this! | ||
|
||
// check if this vote is from a known validator | ||
for vote in &self.signed_votes() { | ||
// Only count if this vote is from a known validator. | ||
// TODO: we still need to check that we didn't see a vote from this validator twice ... | ||
let val_id = vote.validator_id(); | ||
let val = match validators.validator(val_id) { | ||
Some(v) => v, | ||
|
@@ -48,6 +45,11 @@ impl lite::Commit for block::signed_header::SignedHeader { | |
} | ||
|
||
fn validate(&self, vals: &Self::ValidatorSet) -> Result<(), Error> { | ||
// TODO: self.commit.block_id cannot be zero in the same way as in go | ||
// clarify if this another encoding related issue | ||
if self.commit.signatures.len() == 0 { | ||
fail!(Kind::ImplementationSpecific, "no signatures for commit"); | ||
} | ||
if self.commit.signatures.len() != vals.validators().len() { | ||
fail!( | ||
Kind::ImplementationSpecific, | ||
|
@@ -58,69 +60,108 @@ impl lite::Commit for block::signed_header::SignedHeader { | |
} | ||
|
||
for commit_sig in self.commit.signatures.iter() { | ||
// returns FaultyFullNode error if it detects a signer that is not present in the validator set | ||
if let Some(val_addr) = commit_sig.validator_address { | ||
if vals.validator(val_addr) == None { | ||
fail!( | ||
Kind::FaultyFullNode, | ||
"Found a faulty signer ({}) not present in the validator set ({})", | ||
val_addr, | ||
vals.hash() | ||
); | ||
} | ||
} | ||
commit_sig.validate(vals)?; | ||
} | ||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
fn commit_to_votes(commit: block::Commit) -> Result<Vec<vote::Vote>, Error> { | ||
// this private helper function does *not* do any validation but extracts | ||
// all non-BlockIDFlagAbsent votes from the commit: | ||
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; | ||
} | ||
|
||
match commit_sig.validator_address { | ||
Some(val_addr) => { | ||
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: i as u64, | ||
signature: sig, | ||
}; | ||
votes.push(vote); | ||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I think this should be moved to commit_sig.rs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do in a followup PR. |
||
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 | ||
liamsi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
None => { | ||
fail!( | ||
Kind::ImplementationSpecific, | ||
"validator address missing in commit_sig {:#?}", | ||
commit_sig | ||
); | ||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe move this check into a separate function which is only called in case of full verification? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do in a followup PR. |
||
if vals.validator(val_addr) == None { | ||
fail!( | ||
Kind::ImplementationSpecific, | ||
"Found a faulty signer ({}) not present in the validator set ({})", | ||
val_addr, | ||
vals.hash() | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
||
Ok(()) | ||
} | ||
Ok(votes) | ||
} | ||
|
||
impl block::signed_header::SignedHeader { | ||
/// This is a private helper method to iterate over the underlying | ||
/// votes to compute the voting power (see `voting_power_in` below). | ||
fn iter(&self) -> Result<Vec<vote::SignedVote>, Error> { | ||
fn signed_votes(&self) -> Vec<vote::SignedVote> { | ||
let chain_id = self.header.chain_id.to_string(); | ||
// if let Ok(mut votes) = commit_to_votes(self.commit.clone()) | ||
let mut votes = match commit_to_votes(self.commit.clone()) { | ||
Ok(votes_vec) => votes_vec, | ||
Err(e) => return Err(e), | ||
}; | ||
Ok(votes | ||
let mut votes = non_absent_votes(&self.commit); | ||
votes | ||
.drain(..) | ||
.map(|vote| { | ||
vote::SignedVote::new( | ||
|
@@ -130,7 +171,7 @@ impl block::signed_header::SignedHeader { | |
vote.signature, | ||
) | ||
}) | ||
.collect()) | ||
.collect() | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, in which case do we find
self.commit.block_id
to be zero in Go?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I meant, we validate if it is zero the same way as in golang
https://github.com/tendermint/tendermint/blob/ef56e6661121a7f8d054868689707cd817f27b24/types/block.go#L689-L691
because in the Struct here it is not an
Option
.