-
Notifications
You must be signed in to change notification settings - Fork 224
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
Light client: trait impls #36
Changes from 14 commits
4fef6a1
7a10257
d9455af
1a232e5
d3ce237
4b44d6a
360a37a
3217eae
4f53c44
123adcb
dba1303
b7e9e46
a0588a0
cc66c2b
37f9353
372cb4d
4140bbb
9131c4e
15f8b02
e686c45
c941c95
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 |
---|---|---|
@@ -0,0 +1,21 @@ | ||
use crate::block::*; | ||
|
||
#[derive(Clone, Message)] | ||
pub struct ConsensusVersion { | ||
/// Block version | ||
#[prost(uint64, tag = "1")] | ||
pub block: u64, | ||
|
||
/// App version | ||
#[prost(uint64, tag = "2")] | ||
pub app: u64, | ||
} | ||
|
||
impl From<&header::Version> for ConsensusVersion { | ||
fn from(version: &header::Version) -> Self { | ||
ConsensusVersion { | ||
block: version.block, | ||
app: version.app, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,12 @@ use super::{ | |
validate::{ConsensusMessage, ValidationError, ValidationErrorKind::*}, | ||
SignedMsgType, | ||
}; | ||
use crate::amino_types::PartsSetHeader; | ||
use crate::{ | ||
block::{self, ParseId}, | ||
chain, consensus, | ||
error::Error, | ||
vote, Hash, | ||
}; | ||
use bytes::BufMut; | ||
use prost::{error::EncodeError, Message}; | ||
|
@@ -49,6 +51,30 @@ impl Vote { | |
} | ||
} | ||
|
||
impl From<&vote::Vote> for Vote { | ||
fn from(vote: &vote::Vote) -> Self { | ||
Vote { | ||
vote_type: vote.vote_type.to_u32(), | ||
height: vote.height.value() as i64, // TODO potential overflow :-/ | ||
block_id: Some(BlockId { | ||
hash: match vote.block_id.hash { | ||
Hash::Sha256(h) => h.to_vec(), | ||
_ => vec![], | ||
}, | ||
parts_header: match &vote.block_id.parts { | ||
Some(parts) => Some(PartsSetHeader::from(parts)), | ||
None => None, | ||
}, | ||
}), | ||
round: vote.round as i64, | ||
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. Some reason this doesn't follow the same order as the fields in the struct, ie. the round before the block_id? 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. Probably not. Thanks, fixed! |
||
timestamp: Some(TimeMsg::from(vote.timestamp)), | ||
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. Same re why this is an 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. See above. Time is a bit different because we want to encode a particular time if None was passed in ("1970-01-01 00:00:00 +0000 UTC"). |
||
validator_address: vote.validator_address.as_bytes().to_vec(), | ||
validator_index: vote.validator_index as i64, // TODO potential overflow :-/ | ||
signature: vote.signature.as_bytes().to_vec(), | ||
} | ||
} | ||
} | ||
|
||
impl block::ParseHeight for Vote { | ||
fn parse_block_height(&self) -> Result<block::Height, Error> { | ||
block::Height::try_from_i64(self.height) | ||
|
@@ -102,7 +128,7 @@ impl block::ParseHeight for CanonicalVote { | |
} | ||
|
||
impl CanonicalVote { | ||
fn new(vote: Vote, chain_id: &str) -> CanonicalVote { | ||
pub fn new(vote: Vote, chain_id: &str) -> CanonicalVote { | ||
CanonicalVote { | ||
vote_type: vote.vote_type, | ||
chain_id: chain_id.to_string(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
//! Block headers | ||
use crate::{account, block, chain, lite, Hash, Time}; | ||
use crate::merkle::simple_hash_from_byte_slices; | ||
use crate::{account, amino_types, block, chain, lite, Hash, Time}; | ||
use prost::Message; | ||
use { | ||
crate::serializers, | ||
serde::{Deserialize, Serialize}, | ||
|
@@ -83,11 +85,89 @@ impl lite::Header for Header { | |
} | ||
|
||
fn next_validators_hash(&self) -> Hash { | ||
unimplemented!() | ||
self.next_validators_hash | ||
} | ||
|
||
fn hash(&self) -> Hash { | ||
unimplemented!() | ||
let mut version_enc = vec![]; | ||
// TODO: if there is an encoding problem this will | ||
// panic (as the golang code would): | ||
// https://github.com/tendermint/tendermint/blob/134fe2896275bb926b49743c1e25493f6b24cc31/types/block.go#L393 | ||
// https://github.com/tendermint/tendermint/blob/134fe2896275bb926b49743c1e25493f6b24cc31/types/encoding_helper.go#L9:6 | ||
// Instead, handle errors gracefully 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. Why would there ever be an encoding error and what would it mean to handle it gracefully? Seems like the kind of thing where a panic is actually waranted? 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 can't think of a scenario where there could be and encoding error here (while encoding for hashing). If there is any case, it still feels wrong to me to panic instead of returning an error and let the caller decide if it is correct to panic (or handle the error in another way). |
||
amino_types::ConsensusVersion::from(&self.version) | ||
.encode(&mut version_enc) | ||
.unwrap(); | ||
let mut height_enc = vec![]; | ||
prost_amino::encoding::encode_varint(self.height.value(), &mut height_enc); | ||
let mut time_enc = vec![]; | ||
amino_types::TimeMsg::from(self.time) | ||
.encode(&mut time_enc) | ||
.unwrap(); | ||
let chain_id_bytes = self.chain_id.as_bytes(); | ||
let chain_id_enc = encode_bytes(&chain_id_bytes); | ||
let mut num_tx_enc = vec![]; | ||
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 we should use a function for this repeated pattern of encoding a varint |
||
prost_amino::encoding::encode_varint(self.num_txs, &mut num_tx_enc); | ||
let mut total_tx_enc = vec![]; | ||
prost_amino::encoding::encode_varint(self.total_txs, &mut total_tx_enc); | ||
let mut last_block_id_enc = vec![]; | ||
amino_types::BlockId::from(&self.last_block_id) | ||
.encode(&mut last_block_id_enc) | ||
.unwrap(); | ||
let mut last_commit_hash_enc = vec![]; | ||
if let Some(last_commit_hash_bytes) = self.last_commit_hash.as_bytes() { | ||
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. Can we dedup this pattern too? 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. Done (there is a helper encode_hash now). |
||
last_commit_hash_enc = encode_bytes(last_commit_hash_bytes); | ||
} | ||
let mut data_hash_enc = vec![]; | ||
if let Some(data_hash_bytes) = self.data_hash.as_bytes() { | ||
data_hash_enc = encode_bytes(data_hash_bytes); | ||
} | ||
let mut validator_hash_enc = vec![]; | ||
if let Some(validator_hash_bytes) = self.validators_hash.as_bytes() { | ||
validator_hash_enc = encode_bytes(validator_hash_bytes); | ||
} | ||
let mut next_validator_hash_enc = vec![]; | ||
if let Some(next_validator_hash_bytes) = self.next_validators_hash.as_bytes() { | ||
next_validator_hash_enc = encode_bytes(next_validator_hash_bytes); | ||
} | ||
let mut consensus_hash_enc = vec![]; | ||
if let Some(consensus_hash_bytes) = self.consensus_hash.as_bytes() { | ||
consensus_hash_enc = encode_bytes(consensus_hash_bytes); | ||
} | ||
let mut app_hash_enc = vec![]; | ||
if let Some(app_hash_bytes) = self.app_hash.as_bytes() { | ||
app_hash_enc = encode_bytes(app_hash_bytes); | ||
} | ||
let mut last_result_hash_enc = vec![]; | ||
if let Some(last_result_hash_bytes) = self.last_results_hash.as_bytes() { | ||
last_result_hash_enc = encode_bytes(last_result_hash_bytes); | ||
} | ||
let mut evidence_hash_enc = vec![]; | ||
if let Some(evidence_hash_bytes) = self.evidence_hash.as_bytes() { | ||
evidence_hash_enc = encode_bytes(evidence_hash_bytes); | ||
} | ||
let proposer_address_bytes = self.proposer_address.as_bytes(); | ||
let proposer_address_enc = encode_bytes(&proposer_address_bytes); | ||
|
||
let mut byteslices: Vec<&[u8]> = vec![]; | ||
byteslices.push(version_enc.as_slice()); | ||
byteslices.push(chain_id_enc.as_slice()); | ||
byteslices.push(height_enc.as_slice()); | ||
byteslices.push(time_enc.as_slice()); | ||
byteslices.push(num_tx_enc.as_slice()); | ||
byteslices.push(total_tx_enc.as_slice()); | ||
byteslices.push(last_block_id_enc.as_slice()); | ||
byteslices.push(last_commit_hash_enc.as_slice()); | ||
byteslices.push(data_hash_enc.as_slice()); | ||
byteslices.push(validator_hash_enc.as_slice()); | ||
byteslices.push(next_validator_hash_enc.as_slice()); | ||
byteslices.push(consensus_hash_enc.as_slice()); | ||
byteslices.push(app_hash_enc.as_slice()); | ||
byteslices.push(last_result_hash_enc.as_slice()); | ||
byteslices.push(evidence_hash_enc.as_slice()); | ||
byteslices.push(proposer_address_enc.as_slice()); | ||
|
||
Hash::Sha256(simple_hash_from_byte_slices(byteslices.as_slice())) | ||
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 looks a bit too exhaustive but aims to do exactly the same as: https://github.com/tendermint/tendermint/blob/134fe2896275bb926b49743c1e25493f6b24cc31/types/block.go#L393 This could probably be simplified substantially. Maybe, 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. Definitely seems excessive. Good idea re a write based API. Though I don't think you usually see that for Merkle trees? At least the Merkle tree version needs to track the boundaries of each write, where as a normal hasher just concatenates everything together. 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. Note to myself: add back the test that checks the hash 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. done in e686c45 |
||
} | ||
} | ||
|
||
|
@@ -111,3 +191,10 @@ pub struct Version { | |
)] | ||
pub app: u64, | ||
} | ||
|
||
fn encode_bytes(bytes: &[u8]) -> Vec<u8> { | ||
let mut chain_id_enc = vec![]; | ||
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. Should be called 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. All other encoding methods are named |
||
prost_amino::encode_length_delimiter(bytes.len(), &mut chain_id_enc).unwrap(); | ||
chain_id_enc.append(&mut bytes.to_vec()); | ||
chain_id_enc | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
pub use self::types::*; | ||
pub mod types; | ||
pub mod verifier; | ||
|
||
pub use self::types::*; | ||
pub use self::verifier::*; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ use crate::block::Height; | |
use crate::Hash; | ||
#[allow(clippy::all)] | ||
use crate::Time; | ||
use bytes::BufMut; | ||
|
||
/// TrustedState stores the latest state trusted by a lite client, | ||
/// including the last header and the validator set to use to verify | ||
|
@@ -81,6 +82,8 @@ pub trait Commit { | |
/// we ignore absent votes and votes for nil here. | ||
/// NOTE: we may want to check signatures for nil votes, | ||
/// and thus use an ternary enum here instead of the binary Option. | ||
// TODO figure out if we want/can do an iter() method here that returns a | ||
// VoteIterator instead of returning a vec | ||
ebuchman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn into_vec(&self) -> Vec<Option<Self::Vote>>; | ||
} | ||
|
||
|
@@ -96,7 +99,9 @@ pub trait Commit { | |
/// is only necessary to avoid slashing in the multi chain context. | ||
pub trait Vote { | ||
fn validator_id(&self) -> Id; | ||
fn sign_bytes(&self) -> &[u8]; | ||
fn sign_bytes<B>(&self, sign_bytes: &mut B) | ||
where | ||
B: BufMut; | ||
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. So why did this need to take a buf mut instead of returning something? 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 I did run into issues regarding the lifetime. I'll try to see if I can simply return 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. Actually, the problem is that the slice we want return here is created from a temp value inside that method (instead of sth borrowed from parameters of that method, in this case only fn sign_bytes(&self) -> &[u8] {
let mut sign_bytes = vec![];
CanonicalVote::new(AminoVote::from(self), "TODO")
.encode(&mut sign_bytes)
.unwrap();
sign_bytes.as_slice()
} On simple way to returning something would be to return 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. I think it'd make sense to return 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. Thanks @tarcieri. Changed to |
||
fn signature(&self) -> &[u8]; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,7 +127,9 @@ where | |
}; | ||
|
||
// check vote is valid from validator | ||
if !val.verify_signature(vote.sign_bytes(), vote.signature()) { | ||
let mut sign_bytes = vec![]; | ||
vote.sign_bytes(&mut sign_bytes); | ||
if !val.verify_signature(&sign_bytes, vote.signature()) { | ||
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. Lines 121-133 are duplicated in verify_commit_trusting() and here. Maybe a function that takes a validator and a vote and returns error be useful to both? I know in verify_commit_trusting() we do an extra check but that could be done before or after. 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. Good catch. There is a subtle difference in those lines though: in one case the validator is looked up in the set in the other it part of the looped elements (also some of the lines are part of the control flow via 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 saw that but was thinking that once there is a validator and a vote the checks should be the same (most the loop body). But I looked closer and it's not much left in common to justify a separate function. Another question, for 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. Also, for 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 only check is that length matches. Also AFAIR the commits / validators are sorted. That means if the the len matches there is the desired correspondence. But I'll double check and follow up in #63. It seems this is more confusing than helpful maybe we should deduplicate and do the additional lookups instead. |
||
return Err(Error::InvalidSignature); | ||
} | ||
signed_power += val.power(); | ||
|
@@ -176,7 +178,9 @@ where | |
}; | ||
|
||
// check vote is valid from validator | ||
if !val.verify_signature(vote.sign_bytes(), vote.signature()) { | ||
let mut sign_bytes = vec![]; | ||
vote.sign_bytes(&mut sign_bytes); | ||
if !val.verify_signature(&sign_bytes, vote.signature()) { | ||
return Err(Error::InvalidSignature); | ||
} | ||
signed_power += val.power(); | ||
|
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.
Why is block_id an Option here if we're always going to populate it with Some ? Is there a case elsewhere we use a None?
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.
Hmm, I don't remember tbh. These amino types are quite old. Let me check if the encoding matches if remove the
Option
. I think it was sth related toprost
and the#[prost(message)]
attribute.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.
So, it looks like one can simply add the
#[prost(message)]
for a nested struct if it is anOption
. Otherwise, it seems a little more effort is necessary. e.g. I locally changed the field's type toBlockId
instead ofOption<BlockId>
. Then the compiler (due to prost-amino) will complain:Click here for Rust chatty compiler output
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.
OK, it looks like this can be fixed by adding in a
required
attribute: https://github.com/danburkert/prost/blob/9551f2852e414df72339634087c46ce5a08e85b2/prost-derive/src/field/message.rs#L14-L56I'm looking into that.
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.
Adding the
required
attribute, e.g.:makes the compiler happy 😄but breaks some encoding test-vectors 🤔
Hmm, this feels somehow familiar. Will investigate further.
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.
OK, here is the catch:
In golang one can simply not initalize a field. Golang won't complain and implicitly init everything with default values. Then, Amino will treat this as sth. that can be skipped (and hence is optional in the protobuf2 sense which is the default in proto3).
Now on the rust side you have to init a field (unless you make it an option). If you make it an Option, the encoding behaves like amino (None will be not encoded at all). If you have a regular field (e.g. BlockId), and add a
required
attribute, prost(-amino) will treat asrequired
(in the protobuf2 sense) and signal, we got fiel blah of type blubb but nothing in there (for blockId this is fieldnumber 4 and hence4 << 3 | 2 = 34
followed by a0x00
for indicating that this was empty; which is exactly the two bytes difference in above test-vector).Amino actually does the same - enc. defaults as required as described above - but rolls back if it found out there is nothing (via that zero 0x00) unless told otherwise via
writeEmpty
(at least it seems to work now):https://github.com/tendermint/go-amino/blob/fbf776258498dbb3aa6637ca82c7fe5c7255fd2f/binary-encode.go#L496-L524