Skip to content
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

Specify proposer when constructing validator set in light client #706

Merged
merged 3 commits into from
Dec 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
key/value pairs decoded from base64, where previously tag key/value pairs
were Base64-encoded ([#717])
- `[tendermint]` (Since v0.17.0-rc3) Bech32 encoding fix ([#690])
- `[tendermint, light-client]` Specify the proposer in the validator set of fetched light blocks ([#705])

[#425]: https://github.com/informalsystems/tendermint-rs/issues/425
[#646]: https://github.com/informalsystems/tendermint-rs/pull/646
[#690]: https://github.com/informalsystems/tendermint-rs/issues/690
[#701]: https://github.com/informalsystems/tendermint-rs/pull/701
[#717]: https://github.com/informalsystems/tendermint-rs/issues/717
[#705]: https://github.com/informalsystems/tendermint-rs/issues/705

## v0.17.0-rc3

Expand Down
32 changes: 24 additions & 8 deletions light-client/src/components/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ pub enum IoError {
#[error("invalid height: {0}")]
InvalidHeight(String),

/// Fetched validator set is invalid
#[error("fetched validator set is invalid: {0}")]
InvalidValidatorSet(String),

/// Task timed out.
#[error("task timed out after {} ms", .0.as_millis())]
Timeout(Duration),
Expand Down Expand Up @@ -84,6 +88,7 @@ mod prod {
use crate::types::PeerId;
use crate::utils::block_on;

use tendermint::account::Id as TMAccountId;
use tendermint::block::signed_header::SignedHeader as TMSignedHeader;
use tendermint::validator::Set as TMValidatorSet;

Expand All @@ -100,9 +105,10 @@ mod prod {
fn fetch_light_block(&self, height: AtHeight) -> Result<LightBlock, IoError> {
let signed_header = self.fetch_signed_header(height)?;
let height = signed_header.header.height;
let proposer_address = signed_header.header.proposer_address;

let validator_set = self.fetch_validator_set(height.into())?;
let next_validator_set = self.fetch_validator_set(height.increment().into())?;
let validator_set = self.fetch_validator_set(height.into(), Some(proposer_address))?;
let next_validator_set = self.fetch_validator_set(height.increment().into(), None)?;

let light_block = LightBlock::new(
signed_header,
Expand Down Expand Up @@ -147,7 +153,11 @@ mod prod {
}
}

fn fetch_validator_set(&self, height: AtHeight) -> Result<TMValidatorSet, IoError> {
fn fetch_validator_set(
&self,
height: AtHeight,
proposer_address: Option<TMAccountId>,
melekes marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<TMValidatorSet, IoError> {
let height = match height {
AtHeight::Highest => bail!(IoError::InvalidHeight(
"given height must be greater than 0".to_string()
Expand All @@ -156,12 +166,18 @@ mod prod {
};

let client = self.rpc_client.clone();
let res = block_on(self.timeout, async move { client.validators(height).await })?;
let response = block_on(self.timeout, async move { client.validators(height).await })?
.map_err(IoError::RpcError)?;

match res {
Ok(response) => Ok(TMValidatorSet::new_simple(response.validators)),
Err(err) => Err(IoError::RpcError(err)),
}
let validator_set = match proposer_address {
Some(proposer_address) => {
TMValidatorSet::with_proposer(response.validators, proposer_address)
.map_err(|e| IoError::InvalidValidatorSet(e.to_string()))?
}
None => TMValidatorSet::without_proposer(response.validators),
};

Ok(validator_set)
}
}
}
4 changes: 2 additions & 2 deletions light-client/src/predicates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ mod tests {
.generate()
.expect("Failed to generate validator"),
);
let val_set_with_faulty_signer = Set::new_simple(bad_vals);
let val_set_with_faulty_signer = Set::without_proposer(bad_vals);

// reset signatures
signed_header.commit.signatures = signatures;
Expand Down Expand Up @@ -675,7 +675,7 @@ mod tests {
.generate()
.unwrap(),
);
let bad_valset = Set::new_simple(vals);
let bad_valset = Set::without_proposer(vals);

trust_threshold = TrustThreshold::new(2, 3).expect("Cannot make trust threshold");

Expand Down
9 changes: 5 additions & 4 deletions light-client/tests/model_based.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ impl SingleStepTestFuzzer for HeaderValHashFuzzer {
Validator::new("2"),
Validator::new("3"),
];
let valset = ValidatorSet::new_simple(generate_validators(&vals).unwrap());
let valset = ValidatorSet::without_proposer(generate_validators(&vals).unwrap());

input.block.validators = valset;
(String::from("header validators_hash"), LiteVerdict::Invalid)
Expand All @@ -260,7 +260,7 @@ impl SingleStepTestFuzzer for HeaderNextValHashFuzzer {
Validator::new("2"),
Validator::new("3"),
];
let valset = ValidatorSet::new_simple(generate_validators(&vals).unwrap());
let valset = ValidatorSet::without_proposer(generate_validators(&vals).unwrap());

input.block.next_validators = valset;
(
Expand Down Expand Up @@ -540,10 +540,11 @@ fn single_step_test(
// Below is a temporary work around to get rid of bug-gy validator sorting
// which was making all the tests fail
let current_vals = input.block.validators.clone();
let current_resorted = Set::new_simple(current_vals.validators().to_vec());
let current_resorted = Set::without_proposer(current_vals.validators().to_vec());

let current_next_vals = input.block.next_validators.clone();
let current_next_resorted = Set::new_simple(current_next_vals.validators().to_vec());
let current_next_resorted =
Set::without_proposer(current_next_vals.validators().to_vec());

let mut mutated_block = input.block.clone();
mutated_block.validators = current_resorted;
Expand Down
6 changes: 6 additions & 0 deletions tendermint/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
use anomaly::{BoxError, Context};
use thiserror::Error;

use crate::account;

/// Error type
pub type Error = BoxError;

Expand Down Expand Up @@ -176,6 +178,10 @@ pub enum Kind {
/// Missing max_age_duration in evidence parameters
#[error("missing max_age_duration")]
MissingMaxAgeDuration,

/// Proposer not found in validator set
#[error("proposer with address '{}' not found in validator set", _0)]
ProposerNotFound(account::Id),
}

impl Kind {
Expand Down
55 changes: 37 additions & 18 deletions tendermint/src/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ use tendermint_proto::Protobuf;
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
pub struct Set {
validators: Vec<Info>,

proposer: Option<Info>,

total_voting_power: vote::Power,
}

Expand All @@ -35,7 +33,7 @@ impl TryFrom<RawValidatorSet> for Set {
Ok(Self::new(
unsorted_validators_result?,
value.proposer.map(TryInto::try_into).transpose()?,
value.total_voting_power.try_into()?,
Some(value.total_voting_power.try_into()?),
))
}
}
Expand All @@ -51,14 +49,25 @@ impl From<Set> for RawValidatorSet {
}

impl Set {
/// constructor
/// Constructor
pub fn new(
validators: Vec<Info>,
proposer: Option<Info>,
total_voting_power: vote::Power,
total_voting_power: Option<vote::Power>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would someone need to pass total_voting_power if it can be computed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wonder about this as well. Maybe @ebuchman knows why it was done like this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they do not I've been campaigning against total voting power and proposer fields in this type for a few weeks now! I suspect it was done without really thinking about it ...

) -> Set {
let mut validators = validators;
Self::sort_validators(&mut validators);

// Compute the total voting power
let total_voting_power = total_voting_power.unwrap_or_else(|| {
validators
.iter()
.map(|v| v.voting_power.value())
.sum::<u64>()
.try_into()
.unwrap()
});

Set {
validators,
proposer,
Expand All @@ -67,8 +76,25 @@ impl Set {
}

/// Convenience constructor for cases where there is no proposer
pub fn new_simple(validators: Vec<Info>) -> Set {
Self::new(validators, None, vote::Power::default())
pub fn without_proposer(validators: Vec<Info>) -> Set {
Self::new(validators, None, None)
}

/// Convenience constructor for cases where there is a proposer
pub fn with_proposer(
validators: Vec<Info>,
proposer_address: account::Id,
) -> Result<Self, Error> {
// Get the proposer.
let proposer = validators
.iter()
.find(|v| v.address == proposer_address)
.cloned()
.ok_or(Kind::ProposerNotFound(proposer_address))?;

// Create the validator set with the given proposer.
// This is required by IBC on-chain validation.
Ok(Self::new(validators, Some(proposer), None))
}

/// Get Info of the underlying validators.
Expand All @@ -82,8 +108,8 @@ impl Set {
}

/// Get total voting power
pub fn total_voting_power(&self) -> &vote::Power {
&self.total_voting_power
pub fn total_voting_power(&self) -> vote::Power {
self.total_voting_power
}

/// Sort the validators according to the current Tendermint requirements
Expand All @@ -110,13 +136,6 @@ impl Set {

Hash::Sha256(merkle::simple_hash_from_byte_vectors(validator_bytes))
}

/// Compute the total voting power within this validator set
pub fn total_power(&self) -> u64 {
self.validators().iter().fold(0u64, |total, val_info| {
total + val_info.voting_power.value()
})
}
}

/// Validator information
Expand Down Expand Up @@ -382,7 +401,7 @@ mod tests {
22, 57, 84, 71, 122, 200, 169, 192, 252, 41, 148, 223, 180,
];

let val_set = Set::new_simple(vec![v1, v2, v3]);
let val_set = Set::without_proposer(vec![v1, v2, v3]);
let hash = val_set.hash();
assert_eq!(hash_expect, hash.as_bytes().to_vec());

Expand All @@ -399,7 +418,7 @@ mod tests {
assert_eq!(val_set.validator(v3.address).unwrap(), v3);
assert_eq!(val_set.validator(not_in_set.address), None);
assert_eq!(
val_set.total_power(),
val_set.total_voting_power().value(),
148_151_478_422_287_875 + 158_095_448_483_785_107 + 770_561_664_770_006_272
);
}
Expand Down
4 changes: 2 additions & 2 deletions testgen/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ impl Generator<block::Header> for Header {
} else {
Validator::new("a").generate().unwrap().address
};
let valset = validator::Set::new_simple(vals);
let valset = validator::Set::without_proposer(vals);
let next_valset = match &self.next_validators {
Some(next_vals) => validator::Set::new_simple(generate_validators(next_vals)?),
Some(next_vals) => validator::Set::without_proposer(generate_validators(next_vals)?),
None => valset.clone(),
};
let chain_id = match chain::Id::from_str(
Expand Down
6 changes: 3 additions & 3 deletions testgen/src/light_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,17 @@ impl Generator<TMLightBlock> for LightBlock {
generate_signed_header(header, commit).expect("Could not generate signed header");

let validators = match &self.validators {
None => validator::Set::new_simple(generate_validators(
None => validator::Set::without_proposer(generate_validators(
header
.validators
.as_ref()
.expect("missing validators in header"),
)?),
Some(vals) => validator::Set::new_simple(generate_validators(vals)?),
Some(vals) => validator::Set::without_proposer(generate_validators(vals)?),
};

let next_validators = match &self.next_validators {
Some(next_vals) => validator::Set::new_simple(generate_validators(next_vals)?),
Some(next_vals) => validator::Set::without_proposer(generate_validators(next_vals)?),
None => validators.clone(),
};

Expand Down
6 changes: 3 additions & 3 deletions testgen/src/validator_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Generator<validator::Set> for ValidatorSet {

fn generate(&self) -> Result<validator::Set, SimpleError> {
let vals = generate_validators(&self.validators.as_ref().unwrap())?;
Ok(validator::Set::new_simple(vals))
Ok(validator::Set::without_proposer(vals))
}
}

Expand All @@ -65,7 +65,7 @@ mod tests {
Validator::new("b"),
Validator::new("c"),
];
let valset2 = validator::Set::new_simple(generate_validators(&vals1).unwrap());
let valset2 = validator::Set::without_proposer(generate_validators(&vals1).unwrap());

assert_eq!(valset1.hash(), valset2.hash());

Expand All @@ -82,7 +82,7 @@ mod tests {
Validator::new("b"),
Validator::new("c"),
];
let valset5 = validator::Set::new_simple(generate_validators(&vals2).unwrap());
let valset5 = validator::Set::without_proposer(generate_validators(&vals2).unwrap());
assert_ne!(valset2.hash(), valset5.hash());
}
}