Added single-step-skipping JSON tests #143

Shivani912 · 2020-02-03T20:39:54Z

Purpose: To add the remaining single step skipping JSON test cases

Changes:

Modified the test case structure -> test_name is changed to batch_name
Diretory structure -> Categorized and split the json files into single_step_sequential and single_step_skipping
Added tests for single_step_skipping cases

liamsi · 2020-02-03T20:50:16Z

@Shivani912 Can you point me to the exact case in the JSON where the tests pass despite the wrong header hash?

tendermint/tests/support/lite/single_step_sequential/commit_tests.json

liamsi · 2020-02-03T21:57:06Z

tendermint/tests/support/lite/single_step_sequential/commit_tests.json

+ 			"expected_output": "error"
+ 		},
+ 		{
+ 			"description": "Case: one lite block, wrong PartsHeader.Total, with error",


That's the problematic test!

liamsi · 2020-02-03T22:29:37Z

0a30022 solves the problem with the "Case: one lite block, wrong PartsHeader.Total, with error", next one in that file is "Case: one lite block, less than one-third vals don't sign, no error"

liamsi · 2020-02-04T17:31:33Z

tendermint/tests/support/lite/single_step_sequential/commit_tests.json

+ 									"type": "tendermint/PubKeyEd25519",
+ 									"value": "b6hwk3pjiOTJfLVCcLDA3I3lO71zWJ0VSded5LUl9T0="
+ 								},
+ 								"voting_power": "50",


That's the next test that is failing. Quick question: Should we rename this to: "Case: one lite block, more than two-third vals did sign, no error". I was first confused why there is no error if less than 1/3 signed :-D The

This one seems to fail on the newly introduced check in regarding the parts.hash btw:

i: 0, Case: one lite block, less than one-third vals don't sign, no error parts.hash: Hash::Sha256(F7338645FBBEE68EC2F2A9F4AE6DB46DDC720A75760E08A5323FE8BF2CBEFBE2) self.header_hash(): Hash::Sha256(EEE5DE7802C5519394F4A4986AAB9B9D9E1F2C736289182104E8C342559753EF)

I don't fully understand why this doesn't err on the go side too: https://github.com/tendermint/tendermint/blob/df3eee455c9d2a4a9698a35aa0dfe6d5d2efd53d/types/block.go#L778-L784

Oh it's "less than one-third don't sign" which is same as "more than two-thirds did sign". That's why it does not error on the go side too.

Me wondering was more about the parts hash. I understand the equiv. of both and suggested to rename the description to the latter "more than two-thirds did sign" as it is less confusing (but maybe it's just me).

Ah okay. I can do that

Shivani912 · 2020-02-04T22:26:34Z

tendermint/src/lite_impl/signed_header.rs

@@ -47,6 +47,12 @@ impl lite::Commit for block::signed_header::SignedHeader {
        if self.commit.precommits.len() != vals.validators().len() {
            return Err(lite::Error::InvalidCommitSignatures);
        }
+        if let Some(parts) = &self.commit.block_id.parts {
+            if parts.hash != self.header_hash() {


@liamsi parts.hash is the merkleRoot(partsHeader) which is different from the header_hash. That's why this check is making many other tests to fail too. We want to check that the commit.BlockID is equal to each of the precommit.BlockID. In reality, there can be precommits for other blocks and that's totally fine. But then, those precommits should not be considered for this block. This means that when we calculate the total voting power i.e. signed_power, we do not count voting power of validators who signed these precommits. Maybe this check should go under the voting_power_in method(?)

I see. Thanks for the clarification! To move forward, I'd suggest to merge this, and open an issue referencing this comment and the failing test.

Oh, is this about the bug I raised here: #145 (comment) ? We should be testing (either here or in voting_power_in) that the votes being tallied are for the right block ID

tendermint/src/lite_impl/signed_header.rs

codecov-io · 2020-02-05T18:47:30Z

Codecov Report

Merging #143 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #143   +/-   ##
=======================================
  Coverage   36.08%   36.08%           
=======================================
  Files          90       90           
  Lines        3140     3140           
  Branches      490      490           
=======================================
  Hits         1133     1133           
+ Misses       1751     1750    -1     
- Partials      256      257    +1

Impacted Files	Coverage Δ
tendermint/src/lite_impl/signed_header.rs	`84.44% <ø> (+2.22%)`	⬆️
tendermint/src/merkle.rs	`75.67% <0%> (-1.36%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ca3e8c...413b780. Read the comment docs.

liamsi

It seems now that some of the existing test files can be deleted? Even the descriptions are the same. I think it's fine to review and remove those unless they contain further test cases which aren't in the new ones. The rest LGTM! 💪

clean up

Shivani912 · 2020-02-05T19:26:27Z

It seems now that some of the existing test files can be deleted? Even the descriptions are the same. I think it's fine to review and remove those unless they contain further test cases which aren't in the new ones. The rest LGTM! 💪

Done!

liamsi

Awesome! Thanks @Shivani912!

One last ask: Can you link to the version of your go-code with which you've generated the JSON files? E.g. just here in the comments; could be via linking a commit (after that do not force push in your branch please). Then someone could reproduce this :-)

liamsi · 2020-02-06T01:11:49Z

Congrats to your 1st contribution to tendermint-rs 🎉 👍

Shivani912 · 2020-02-06T01:21:50Z

Awesome! Thanks @Shivani912!

One last ask: Can you link to the version of your go-code with which you've generated the JSON files? E.g. just here in the comments; could be via linking a commit (after that do not force push in your branch please). Then someone could reproduce this :-)

Here: https://github.com/Shivani912/tendermint/tree/shivani-refactor/types/lite-client
I'll be pushing some more code soon!

Added single-step-skipping JSON tests

f9d872a

Shivani912 commented Feb 3, 2020

View reviewed changes

tendermint/tests/support/lite/single_step_sequential/commit_tests.json Show resolved Hide resolved

liamsi reviewed Feb 3, 2020

View reviewed changes

Add a check for the parts.hash in commit.validate impl

0a30022

liamsi reviewed Feb 4, 2020

View reviewed changes

Shivani912 commented Feb 4, 2020

View reviewed changes

All tests passing

bbb07cf

liamsi reviewed Feb 5, 2020

View reviewed changes

tendermint/src/lite_impl/signed_header.rs Outdated Show resolved Hide resolved

cargo fmt and delete comment

413b780

liamsi marked this pull request as ready for review February 5, 2020 18:49

liamsi mentioned this pull request Feb 5, 2020

Light client voting_power_in can't detect invalid signers #140

Closed

liamsi reviewed Feb 5, 2020

View reviewed changes

Shivani912 added 2 commits February 5, 2020 14:17

clean up

46a3c9d

Merge branch 'master' of github.com:Shivani912/tendermint-rs

d009ab3

clean up

Shivani912 requested a review from ebuchman February 5, 2020 19:27

liamsi approved these changes Feb 6, 2020

View reviewed changes

liamsi merged commit 994b457 into informalsystems:master Feb 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added single-step-skipping JSON tests #143

Added single-step-skipping JSON tests #143

Shivani912 commented Feb 3, 2020

liamsi commented Feb 3, 2020 •

edited

Loading

liamsi Feb 3, 2020

liamsi commented Feb 3, 2020

liamsi Feb 4, 2020 •

edited

Loading

Shivani912 Feb 5, 2020

liamsi Feb 5, 2020

Shivani912 Feb 5, 2020

Shivani912 Feb 4, 2020

liamsi Feb 5, 2020

ebuchman Feb 9, 2020

codecov-io commented Feb 5, 2020

liamsi left a comment

Shivani912 commented Feb 5, 2020

liamsi left a comment •

edited

Loading

liamsi commented Feb 6, 2020

Shivani912 commented Feb 6, 2020

Added single-step-skipping JSON tests #143

Added single-step-skipping JSON tests #143

Conversation

Shivani912 commented Feb 3, 2020

liamsi commented Feb 3, 2020 • edited Loading

liamsi Feb 3, 2020

Choose a reason for hiding this comment

liamsi commented Feb 3, 2020

liamsi Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Shivani912 Feb 5, 2020

Choose a reason for hiding this comment

liamsi Feb 5, 2020

Choose a reason for hiding this comment

Shivani912 Feb 5, 2020

Choose a reason for hiding this comment

Shivani912 Feb 4, 2020

Choose a reason for hiding this comment

liamsi Feb 5, 2020

Choose a reason for hiding this comment

ebuchman Feb 9, 2020

Choose a reason for hiding this comment

codecov-io commented Feb 5, 2020

Codecov Report

liamsi left a comment

Choose a reason for hiding this comment

Shivani912 commented Feb 5, 2020

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

liamsi commented Feb 6, 2020

Shivani912 commented Feb 6, 2020

liamsi commented Feb 3, 2020 •

edited

Loading

liamsi Feb 4, 2020 •

edited

Loading

liamsi left a comment •

edited

Loading