-
Notifications
You must be signed in to change notification settings - Fork 216
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
Fix VotingPowerCalculator::voting_power_in
#306
Changes from 3 commits
208cf97
024a73d
c828155
319c9f2
e9c51a0
90c32ba
8c0c5e6
7e3b537
ef3660c
30d680f
2f45b73
fc5e89f
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 |
---|---|---|
@@ -1,19 +1,78 @@ | ||
use crate::{ | ||
bail, | ||
predicates::errors::VerificationError, | ||
types::{SignedHeader, ValidatorSet}, | ||
types::{Commit, SignedHeader, TrustThreshold, ValidatorSet}, | ||
}; | ||
|
||
use anomaly::BoxError; | ||
use serde::{Deserialize, Serialize}; | ||
use std::collections::HashSet; | ||
use std::fmt; | ||
|
||
use tendermint::block::CommitSig; | ||
use tendermint::lite::types::TrustThreshold as _; | ||
use tendermint::lite::types::ValidatorSet as _; | ||
use tendermint::vote::{SignedVote, Vote}; | ||
|
||
#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)] | ||
pub struct VotingPower { | ||
pub total: u64, | ||
pub tallied: u64, | ||
pub trust_threshold: TrustThreshold, | ||
} | ||
|
||
impl fmt::Display for VotingPower { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!( | ||
f, | ||
"VotingPower(total={} tallied={} trust_threshold={})", | ||
self.total, self.tallied, self.trust_threshold | ||
) | ||
} | ||
} | ||
|
||
pub trait VotingPowerCalculator: Send { | ||
fn total_power_of(&self, validators: &ValidatorSet) -> u64; | ||
fn voting_power_in( | ||
|
||
fn check_enough_trust( | ||
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. The name and the result of this function were for me a bit misleading as the name would suggest a boolean value (or in this case |
||
&self, | ||
untrusted_header: &SignedHeader, | ||
untrusted_validators: &ValidatorSet, | ||
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. These are trusted validators |
||
trust_threshold: TrustThreshold, | ||
) -> Result<VotingPower, VerificationError> { | ||
println!("check_validators_overlap"); | ||
romac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let voting_power = | ||
self.voting_power_of(untrusted_header, untrusted_validators, trust_threshold)?; | ||
|
||
if trust_threshold.is_enough_power(voting_power.tallied, voting_power.total) { | ||
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. Mentioned in one of the comments above, I would probably make more sense to just check this in the |
||
Ok(voting_power) | ||
} else { | ||
Err(VerificationError::NotEnoughTrust(voting_power)) | ||
} | ||
} | ||
|
||
fn check_signers_overlap( | ||
&self, | ||
untrusted_header: &SignedHeader, | ||
untrusted_validators: &ValidatorSet, | ||
) -> Result<VotingPower, VerificationError> { | ||
println!("check_signers_overlap"); | ||
romac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let two_thirds = TrustThreshold::new(2, 3).unwrap(); | ||
romac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let voting_power = | ||
self.voting_power_of(untrusted_header, untrusted_validators, two_thirds)?; | ||
|
||
if two_thirds.is_enough_power(voting_power.tallied, voting_power.total) { | ||
Ok(voting_power) | ||
} else { | ||
Err(VerificationError::InsufficientSignersOverlap(voting_power)) | ||
} | ||
} | ||
|
||
fn voting_power_of( | ||
&self, | ||
signed_header: &SignedHeader, | ||
validators: &ValidatorSet, | ||
) -> Result<u64, BoxError>; | ||
validator_set: &ValidatorSet, | ||
trust_threshold: TrustThreshold, | ||
) -> Result<VotingPower, VerificationError>; | ||
} | ||
|
||
#[derive(Copy, Clone, Debug)] | ||
|
@@ -24,39 +83,111 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator { | |
validators.total_power() | ||
} | ||
|
||
fn voting_power_in( | ||
fn voting_power_of( | ||
&self, | ||
signed_header: &SignedHeader, | ||
validators: &ValidatorSet, | ||
) -> Result<u64, BoxError> { | ||
// 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 = 0_u64; | ||
|
||
for vote in &signed_header.signed_votes() { | ||
romac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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, | ||
None => continue, | ||
validator_set: &ValidatorSet, | ||
trust_threshold: TrustThreshold, | ||
) -> Result<VotingPower, VerificationError> { | ||
let signatures = &signed_header.commit.signatures; | ||
|
||
let mut tallied_voting_power = 0_u64; | ||
let mut seen_validators = HashSet::new(); | ||
|
||
for (idx, signature) in signatures.into_iter().enumerate() { | ||
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. Does it make sense to reference the test vector which exercises this particular edge case? 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. What edge case are you referring to here? 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. Oops perhaps I did not comment the correct line and actually there is an informative comment further down 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. Unfortunately, we currently do not have a test vector which covers this edge case. cc @Shivani912 Edit: typo |
||
let vote = vote_from_non_absent_signature(signature, idx as u64, &signed_header.commit); | ||
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. Similar comment as above, IIRC this was a bug in the past. 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. You mean counting absent signatures? I don't think that's possible since a 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. True! |
||
let vote = match vote { | ||
Some(vote) => vote, | ||
None => continue, // Ok, some signatures can be absent | ||
}; | ||
|
||
// Ensure we only count a validator's power once | ||
if seen_validators.contains(&vote.validator_address) { | ||
continue; | ||
} else { | ||
seen_validators.insert(vote.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. One interesting difference between go code and the implementation here is this 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. @melekes What do you think? 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. I guess raising an error is probably the proper behavior there. Will already change that, and we can always revert it back if that was wrong. |
||
|
||
let validator = match validator_set.validator(vote.validator_address) { | ||
Some(validator) => validator, | ||
None => continue, // Cannot find matching validator, so we skip the vote | ||
}; | ||
|
||
// check vote is valid from validator | ||
let sign_bytes = vote.sign_bytes(); | ||
let signed_vote = SignedVote::new( | ||
(&vote).into(), | ||
signed_header.header.chain_id.as_str(), | ||
vote.validator_address, | ||
vote.signature, | ||
); | ||
|
||
if !val.verify_signature(&sign_bytes, vote.signature()) { | ||
bail!(VerificationError::ImplementationSpecific(format!( | ||
"Couldn't verify signature {:?} with validator {:?} on sign_bytes {:?}", | ||
vote.signature(), | ||
val, | ||
// Check vote is valid | ||
let sign_bytes = signed_vote.sign_bytes(); | ||
if !validator.verify_signature(&sign_bytes, signed_vote.signature()) { | ||
bail!(VerificationError::InvalidSignature { | ||
signature: signed_vote.signature().to_vec(), | ||
validator, | ||
sign_bytes, | ||
))); | ||
}); | ||
} | ||
|
||
// If the vote is neither absent nor nil, tally its power | ||
if signature.is_commit() { | ||
tallied_voting_power += validator.power(); | ||
} else { | ||
// It's OK. We include stray signatures (~votes for nil) | ||
// to measure validator availability. | ||
} | ||
|
||
signed_power += val.power(); | ||
// TODO: Break out when we have enough voting power | ||
romac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
Ok(signed_power) | ||
let voting_power = VotingPower { | ||
total: self.total_power_of(validator_set), | ||
tallied: tallied_voting_power, | ||
trust_threshold, | ||
}; | ||
|
||
Ok(voting_power) | ||
} | ||
} | ||
|
||
fn vote_from_non_absent_signature( | ||
commit_sig: &CommitSig, | ||
validator_index: u64, | ||
commit: &Commit, | ||
) -> Option<Vote> { | ||
let (validator_address, timestamp, signature, block_id) = match commit_sig { | ||
CommitSig::BlockIDFlagAbsent { .. } => return None, | ||
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. shouldn't we panic here because method assumes non_absent signature? then we won't need the Option<> 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. This function is actually the one doing the filtering for non absent signatures, hence the need for the Option. I guess the name isn't great, I am open to suggestions :) |
||
CommitSig::BlockIDFlagCommit { | ||
validator_address, | ||
timestamp, | ||
signature, | ||
} => ( | ||
validator_address.clone(), | ||
timestamp.clone(), | ||
signature.clone(), | ||
Some(commit.block_id.clone()), | ||
), | ||
CommitSig::BlockIDFlagNil { | ||
validator_address, | ||
timestamp, | ||
signature, | ||
} => ( | ||
validator_address.clone(), | ||
timestamp.clone(), | ||
signature.clone(), | ||
None, | ||
), | ||
}; | ||
|
||
Some(Vote { | ||
vote_type: tendermint::vote::Type::Precommit, | ||
height: commit.height, | ||
round: commit.round, | ||
block_id, | ||
timestamp, | ||
validator_address, | ||
validator_index, | ||
signature, | ||
}) | ||
} |
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.
For me this is a bit misleading as the struct has more information than what I would expect by reading just the name. Also, I think it would be beneficial to implement the TODO about the breaking out of the loop as than this would go probably go away.
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.
Agreed that the name is perhaps not the best, but we still need this information to be able to yield a meaningful error in case of lack of trust/signers overlap.