Light client: trait impls #36

liamsi · 2019-09-18T21:54:07Z

This PR will have implementations of the traits created in #31 and a few tests.

https://rust-lang.github.io/rust-clippy/master/#try_err

This is just the simplest way to move forward implementing the traits of the lite package. There are alternatives: We do not want a create a circular dependency between lite and tendermint (which does not even compile). To avoid this we could: 1) make lite a module of tendermint, or, 2) replicate a lot of the types of the tendermint crate in the lite package (e.g. Time, Ids etc), or 3) have a dependency from tendermint to the lite package only (but then the trait impls do need to live in the lite crate instead with the types in the tendermint crate directly).

respective amino types, and, directly encode some

2DC46AD76277039F1B65FE3C7F2064788B1C12FE701CFE7EC93F751586A48781) will add a test after #42 gets merged

liamsi · 2019-09-26T20:41:46Z

tendermint/src/block/header.rs

+        byteslices.push(evidence_hash_enc.as_slice());
+        byteslices.push(proposer_address_enc.as_slice());
+
+        Hash::Sha256(simple_hash_from_byte_slices(byteslices.as_slice()))


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
(It actually does will add a few tests after #41 is merged)

This could probably be simplified substantially. Maybe, simple_hash_from_byte_slices had a similar logic as a Hasher in golang (Write the slices and then Sum to create the tree once and return the hash)?

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.

Note to myself: add back the test that checks the hash here:
https://github.com/interchainio/tendermint-rs/blob/262e252a04cb7d7b660a829f3240e7196ad955b9/tendermint/tests/rpc.rs#L167-L168

done in e686c45

ebuchman

Great start! I merged some things to master and merged them back into bucky/lite, so we should merge bucky/lite back into here, but then we also need to deal with the change from into_vec() into iter() in the traits ...

tendermint/src/validator.rs

ebuchman · 2019-10-03T03:21:41Z

tendermint/src/block/header.rs

+        byteslices.push(evidence_hash_enc.as_slice());
+        byteslices.push(proposer_address_enc.as_slice());
+
+        Hash::Sha256(simple_hash_from_byte_slices(byteslices.as_slice()))


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.

tendermint/src/lite/types.rs

ebuchman · 2019-11-01T21:50:47Z

tendermint/src/amino_types/vote.rs

+                    None => None,
+                },
+            }),
+            round: vote.round as i64,


Some reason this doesn't follow the same order as the fields in the struct, ie. the round before the block_id?

Probably not. Thanks, fixed!

ebuchman · 2019-11-01T21:51:32Z

tendermint/src/amino_types/vote.rs

+        Vote {
+            vote_type: vote.vote_type.to_u32(),
+            height: vote.height.value() as i64, // TODO potential overflow :-/
+            block_id: Some(BlockId {


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?

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 to prost and the #[prost(message)] attribute.

So, it looks like one can simply add the #[prost(message)] for a nested struct if it is an Option. Otherwise, it seems a little more effort is necessary. e.g. I locally changed the field's type to BlockId instead of Option<BlockId>. Then the compiler (due to prost-amino) will complain:

Click here for Rust chatty compiler output

error[E0599]: no method named `as_ref` found for type `amino_types::block_id::BlockId` in the current scope --> tendermint/src/amino_types/vote.rs:22:28 | 22 | #[derive(Clone, PartialEq, Message)] | ^^^^^^^ method not found in `amino_types::block_id::BlockId` | ::: tendermint/src/amino_types/block_id.rs:11:1 | 11 | pub struct BlockId { | ------------------ method `as_ref` not found for this | = help: items from traits can only be used if the trait is implemented and in scope = note: the following trait defines an item `as_ref`, perhaps you need to implement it: candidate #1: `std::convert::AsRef` error[E0308]: mismatched types --> tendermint/src/amino_types/vote.rs:22:28 | 22 | #[derive(Clone, PartialEq, Message)] | ^^^^^^^ | | | expected struct `amino_types::block_id::BlockId`, found enum `std::option::Option` | this match expression has type `amino_types::block_id::BlockId` | = note: expected type `amino_types::block_id::BlockId` found type `std::option::Option<_>` error[E0599]: no method named `get_or_insert_with` found for type `amino_types::block_id::BlockId` in the current scope --> tendermint/src/amino_types/vote.rs:22:28 | 22 | #[derive(Clone, PartialEq, Message)] | ^^^^^^^ method not found in `amino_types::block_id::BlockId` | ::: tendermint/src/amino_types/block_id.rs:11:1 | 11 | pub struct BlockId { | ------------------ method `get_or_insert_with` not found for this error[E0308]: mismatched types --> tendermint/src/amino_types/vote.rs:22:28 | 22 | #[derive(Clone, PartialEq, Message)] | ^^^^^^^ expected struct `amino_types::block_id::BlockId`, found enum `std::option::Option` | = note: expected type `amino_types::block_id::BlockId` found type `std::option::Option<_>` error: aborting due to 4 previous errors Some errors have detailed explanations: E0308, E0599. For more information about an error, try `rustc --explain E0308`. error: Could not compile `tendermint`. ´´´ </details>

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-L56

I'm looking into that.

Adding the required attribute, e.g.:

#[prost(message, required, tag = "4")] pub block_id: BlockId,

makes the compiler happy 😄but breaks some encoding test-vectors 🤔

thread 'amino_types::vote::tests::test_sign_bytes_compatibility' panicked at 'assertion failed: `(left == right)` left: `[15, 34, 0, 42, 11, 8, 128, 146, 184, 195, 152, 254, 255, 255, 255, 1]`, right: `[13, 42, 11, 8, 128, 146, 184, 195, 152, 254, 255, 255, 255, 1]`', tendermint/src/amino_types/vote.rs:317:9

Hmm, this feels somehow familiar. Will investigate further.

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 as required (in the protobuf2 sense) and signal, we got fiel blah of type blubb but nothing in there (for blockId this is fieldnumber 4 and hence 4 << 3 | 2 = 34 followed by a 0x00 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

ebuchman · 2019-11-01T21:51:59Z

tendermint/src/amino_types/vote.rs

+                },
+            }),
+            round: vote.round as i64,
+            timestamp: Some(TimeMsg::from(vote.timestamp)),


Same re why this is an Option

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").

ebuchman · 2019-11-01T21:53:02Z

tendermint/src/block/header.rs

+        // 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.


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?

Why would there ever be an encoding error and what would it mean to handle it gracefully?

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).
If we can not hash the header, this probably means a bug in the encoding library (or another bigger problem) and the caller should probably decide to panic anyways. I just feel it isn't the job of the hash method to decide.

ebuchman · 2019-11-01T21:54:32Z

tendermint/src/block/header.rs

@@ -111,3 +191,10 @@ pub struct Version {
    )]
    pub app: u64,
 }
+
+fn encode_bytes(bytes: &[u8]) -> Vec<u8> {
+    let mut chain_id_enc = vec![];


Should be called bytes_enc I guess

All other encoding methods are named encode_*. But it is a private helper function. Happy to rename it.

ebuchman · 2019-11-01T21:58:50Z

tendermint/src/validator.rs

+    type Validator = Info;
+
+    fn hash(&self) -> Hash {
+        // TODO almost the same as above's pub fn hash(self) -> merkle::Hash


So we should be able to dedup?

Yes, we can actually just delete the pub fn hash(self) -> merkle::Hash from impl Set. As far as I can see, it isn't used anywhere.

Added in: 23042bb

ebuchman · 2019-11-01T21:59:27Z

tendermint/src/validator.rs

+            if let Ok(sig) = ed25519::Signature::from_bytes(signature) {
+                return verifier.verify(sign_bytes, &sig).is_ok();
+            }
+            return false;


ebuchman · 2019-11-01T22:00:14Z

tendermint/src/vote.rs

+    where
+        B: BufMut,
+    {
+        // TODO: 1) everytime we encode sth. an error can occur. Change the trait to return a result


Why should we ever have an error here? Assuming we control what's being encoded. And how would we handle it gracefully? Again, seems there might not be a choice but to panic.

Same reason as above. Yes, probably we should panic if can't encode this but I would prefer to leave it to the caller to decide (especially as this aims to be a generic trait). But maybe I'm overthinking this.

ebuchman · 2019-11-01T22:00:38Z

tendermint/src/vote.rs

+    {
+        // TODO: 1) everytime we encode sth. an error can occur. Change the trait to return a result
+        // instead to enable proper error handling.
+        // 2) Figure out where the chain_id should come from (if sign_bytes remains on Vote, the


Good point - may have been thinking we'd use something else here, eg. the CanonicalVote, which contains the ChainID

So, if you are suggesting to implement the trait methods directly on CanonicalVote: indeed, this would solve that issue with the chain_id but the CanonicalVote does not contain the validator address, nor does it contain the signature. Both are needed to implement the current trait:

pub trait Vote { fn validator_id(&self) -> Id; fn sign_bytes(&self) -> Vec<u8>; fn signature(&self) -> &[u8]; }

Can we wrap the CanonicalVote in something that also contains the ID and the signature?

I think that makes sense. Will create a separate struct tomorrow. Feel free to merge this (if you want), I won't forget this change.

Done in c941c95. Name of the new wrapping type SignedVote.

ebuchman · 2019-11-01T22:01:07Z

tendermint/src/vote.rs

@@ -92,6 +121,11 @@ impl Type {
    pub fn to_u8(self) -> u8 {
        self as u8
    }
+
+    /// Serialize this type as a 32-bit unsigned integer
+    pub fn to_u32(self) -> u32 {


Added this because clippy didn't like to_u8() as u32

- Consistency: Rename encode_bytes to bytes_enc - remove redundant return statement

and the signature

liamsi · 2019-11-13T10:56:25Z

Note that @yihuang added and impl for lite::Commit (#67) which I've merged (and then slightly modified) in #63 (via 3e987fe and e1a7560).
I can "backport" this impl into this branch and we can deal with the changes of #63 separately.

After that the still to-be implemented traits are:

ValidatorSetLookup (impl lite::ValidatorSetLookup #68)
~~TrustedState~~ (not a trait)

liamsi · 2019-11-19T10:07:55Z

@ebuchman Is there anything left to do before we can merge this?

ebuchman · 2019-11-26T18:51:19Z

@liamsi I'm unfortunately out of commission this week so will leave it to you. Thanks @yihuang for your contributions, much appreciated!!

liamsi · 2019-11-27T22:05:35Z

OK, thanks. I'll merge in a bunch of (reviewed) changes from #63 into lite_impl. Will add more tests and if everything looks OK, merge all this into your branch bucky/lite. Think we should still do another review on bucky/lite -> master. Then we have a proper (skipping) light client to use.

liamsi · 2019-11-28T14:10:02Z

Actually, no. Will merge your intial traits PR #31 to master and base the follow-up PRs (this one #36 and #63) to master too.

ancazamfir

Looks good! A couple minor comments.

ancazamfir · 2019-12-02T13:00:19Z

tendermint/src/lite/verifier.rs

@@ -127,7 +127,8 @@ where
        };

        // check vote is valid from validator
-        if !val.verify_signature(vote.sign_bytes(), vote.signature()) {
+        let sign_bytes = vote.sign_bytes();
+        if !val.verify_signature(&sign_bytes, vote.signature()) {


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.

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 None => continue,). This could be deduplicated (completely) if we do not zip the iterators and do a lookup in both cases. I'll leave this as is for now.

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 None => continue,).

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 verify_commit_full() we do assume: The vals and commit have a 1-to-1 correspondence; is this checked elsewhere? I was looking for a check that verifies that val.address == vote.validator_address. Or val.verify_signature() will take care of it?

Also, for vals_iter.zip(commit_iter), I believe this works as we want as long as there is no nil validator in the set (I think this is the case but I have a hard time navigating the Rust code :( ) ...otherwise according to the doc: If the first iterator returns None, zip will short-circuit and next will not be called on the second iterator.

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.

ancazamfir · 2019-12-02T13:01:02Z

tendermint/src/vote.rs

@@ -67,6 +69,47 @@ impl Vote {
    }
 }

+/// SignedVote is the union of a canoncialized vote, the signature on


Suggested change

/// SignedVote is the union of a canoncialized vote, the signature on

/// SignedVote is the union of a canonicalized vote, the signature on

brapse

There is a notable lack of tests but this still represents good progress! In the name of keeping things moving along I would recommend merging this for now and following up with tests in the next PR 👍

liamsi · 2019-12-10T10:29:25Z

Thanks for the review @brapse and @ancazamfir! I would also like to merge this as is. A few tests are in a follow-up PR #63 which I didn't merge in here as this PR would grow quite large.

…p), and fmt: - #77 introduced an fmt issue - #36 was not up-to date regarding changes which were merged to master (renaming of simple_merkle) - new upstream release of made build of master fail (solution: explicitly add it ed25519-dalek (1.0.0-pre.3) with rand feature fixes that

…p), and fmt: (#83) - #77 introduced an fmt issue - #36 was not up-to date regarding changes which were merged to master (renaming of simple_merkle) - new upstream release of made build of master fail (solution: explicitly add it ed25519-dalek (1.0.0-pre.3) with rand feature fixes that

liamsi added 3 commits September 18, 2019 17:48

Use concrete basic types for time, hash, bytes, validator id

4fef6a1

cargo fmt

7a10257

into_iter unnecessarily moves the Vec (clippy)

d9455af

liamsi mentioned this pull request Sep 22, 2019

RPC incompatibility: SignedHeader does not contain Commit #38

Closed

liamsi added 2 commits September 22, 2019 10:37

Resolve a bunch of clippy warnings about:

1a232e5

https://rust-lang.github.io/rust-clippy/master/#try_err

liamsi force-pushed the lite_impl branch from 45eea29 to d3ce237 Compare September 25, 2019 08:41

add amino type for header Version

4b44d6a

liamsi force-pushed the lite_impl branch from 0862130 to 4b44d6a Compare September 25, 2019 17:55

liamsi added 2 commits September 25, 2019 21:40

working towards hashing the header: conversion between fields and their

360a37a

respective amino types, and, directly encode some

Header::hash works now (tested against JSON fixture only and yields:

3217eae

2DC46AD76277039F1B65FE3C7F2064788B1C12FE701CFE7EC93F751586A48781) will add a test after #42 gets merged

liamsi force-pushed the lite_impl branch from 3e614a5 to 3217eae Compare September 25, 2019 20:55

liamsi added 4 commits September 25, 2019 22:57

remove todo

4f53c44

implement lite::Validator trait

123adcb

implement lite::ValidatorSet trait

dba1303

implement lite::Vote trait

b7e9e46

liamsi force-pushed the lite_impl branch from 3d26958 to b7e9e46 Compare September 26, 2019 16:26

liamsi commented Sep 26, 2019

View reviewed changes

ebuchman reviewed Oct 3, 2019

View reviewed changes

liamsi mentioned this pull request Oct 29, 2019

Use concrete basic types for time, hash, bytes, validator id #51

Merged

Merge branch 'bucky/lite' into lite_impl

a0588a0

ebuchman mentioned this pull request Nov 1, 2019

MVP Generic Sequential & Skipping Lite Client Verifiers #31

Merged

bring back BufMut; fix clippy warnings

cc66c2b

ebuchman reviewed Nov 1, 2019

View reviewed changes

liamsi added 5 commits November 5, 2019 11:54

Address some review comments:

37f9353

- Consistency: Rename encode_bytes to bytes_enc - remove redundant return statement

Consistency: From<&vote::Vote> same order as the fields in the struct

372cb4d

deduplicate encoding a hash: add a helper

4140bbb

deduplicate encoding hashes and varints: add a helper for each

9131c4e

simplify sign_bytes to return Vec<u8> instead of passing in a BufMut

15f8b02

liamsi marked this pull request as ready for review November 5, 2019 19:28

liamsi added 2 commits November 6, 2019 17:31

Add test for lite::Header.hash() in rpc tests

e686c45

wrap the CanonicalVote in a struct that contains the validator ID

c941c95

and the signature

ebuchman mentioned this pull request Nov 6, 2019

mvp commit validation #26

Closed

liamsi mentioned this pull request Nov 11, 2019

Light client: further trait impls, improvements, and tests #63

Closed

liamsi mentioned this pull request Nov 16, 2019

Remove secret_connection and ring dependency #60

Merged

liamsi requested review from brapse and tarcieri November 19, 2019 10:21

liamsi changed the title ~~Trait impls~~ Light client: trait impls Nov 27, 2019

liamsi changed the base branch from bucky/lite to master November 28, 2019 19:11

liamsi requested review from adizere, ancazamfir, andynog, josef-widder, milosevic and thanethomson November 28, 2019 23:08

ancazamfir reviewed Dec 2, 2019

View reviewed changes

brapse approved these changes Dec 10, 2019

View reviewed changes

ancazamfir approved these changes Dec 10, 2019

View reviewed changes

liamsi merged commit b30991f into master Dec 10, 2019

liamsi mentioned this pull request Dec 10, 2019

Fix build on master #83

Merged

liamsi deleted the lite_impl branch December 10, 2019 21:37

liamsi mentioned this pull request Dec 10, 2019

Light client: further trait impls, improvements, and tests #84

Merged

	/// SignedVote is the union of a canoncialized vote, the signature on
	/// SignedVote is the union of a canonicalized vote, the signature on

Light client: trait impls #36

Light client: trait impls #36

Conversation

liamsi commented Sep 18, 2019

liamsi Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebuchman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Nov 13, 2019 • edited Loading

liamsi commented Nov 19, 2019 • edited Loading

ebuchman commented Nov 26, 2019

liamsi commented Nov 27, 2019

liamsi commented Nov 28, 2019

ancazamfir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brapse left a comment

Choose a reason for hiding this comment

liamsi commented Dec 10, 2019

liamsi Sep 26, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi Nov 5, 2019 •

edited

Loading

liamsi commented Nov 13, 2019 •

edited

Loading

liamsi commented Nov 19, 2019 •

edited

Loading