Implement verify_light() and verify_light_trusting() #1226

hu55a1n1 · 2022-11-10T16:10:53Z

Closes: #1222

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

mzabaluev

The changes look good so far. It would be nice if the names matched so that the code compiles.
We may also need some more tests for the newly added functionality.

light-client-verifier/src/operations/voting_power.rs

light-client-verifier/src/predicates.rs

codecov-commenter · 2022-11-15T18:13:38Z

Codecov Report

Merging #1226 (90bdd30) into main (d102af4) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##            main   #1226    +/-   ##
======================================
  Coverage   64.2%   64.3%            
======================================
  Files        244     245     +1     
  Lines      21364   21485   +121     
======================================
+ Hits       13734   13821    +87     
- Misses      7630    7664    +34

Impacted Files	Coverage Δ
...ght-client-verifier/src/operations/voting_power.rs	`98.0% <ø> (ø)`
light-client/src/tests.rs	`93.5% <ø> (-0.1%)`	⬇️
light-client-verifier/src/verifier.rs	`91.0% <100.0%> (+3.1%)`	⬆️
abci/src/codec.rs	`88.8% <0.0%> (-1.3%)`	⬇️
testgen/src/commit.rs	`90.6% <0.0%> (-0.7%)`	⬇️
testgen/src/header.rs	`83.5% <0.0%> (-0.6%)`	⬇️
tendermint/src/hash.rs	`63.0% <0.0%> (-0.6%)`	⬇️
abci/src/server.rs	`8.8% <0.0%> (-0.4%)`	⬇️
rpc/src/endpoint/tx.rs	`25.0% <0.0%> (ø)`
tendermint/src/error.rs	`0.0% <0.0%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

thanethomson

This seems good to me, thanks @hu55a1n1! Could you please add a changelog entry for this?

mzabaluev

Looks good!

mzabaluev · 2022-11-17T10:54:26Z

light-client/src/tests.rs

+    trait IntoResult<T, E> {
+        fn into_result(self, ok: T) -> Result<T, E>;
+    }
+
+    impl<T> IntoResult<T, Verdict> for Verdict {
+        fn into_result(self, ok: T) -> Result<T, Verdict> {
+            match self {
+                Verdict::Success => Ok(ok),
+                error => Err(error),
+            }
+        }
+    }


Nit: the purpose of this could look more obvious if this were just a helper function. Then again, method chaining is neat.

As an aside, not related to this PR: why did there have to be a non-idiomatic Verdict, rather than a Result<(), VerificationError> where VerificationError would be an enum or otherwise easily matchable to that variant?
Basically, what the existing impl From<Result<(), VerificationError>> for Verdict does could be just how the Result returned value is supposed to be used if you care about the NotEnoughTrust case. Cc @romac

Agreed, let's do that.

Agreed, let's do that.

I didn't mean this to be done in scope for this PR, which so far has not introduced breaking changes, but if you think it would be good and easy thing to do in the next release, I'm all for it.

Indeed, let's target this for 0.27.

hu55a1n1 · 2022-11-18T19:47:25Z

Thanks, @mzabaluev, @thanethomson and @romac for reviewing this. @ancazamfir pointed out that the misbehaviour handling in IBC used the same logic as the update handling (i.e. ProdVerifier::verify), but it was done in a way that required us to separate the expensive checks from the inexpensive ones and call them alternatively for each header in the misbehaviour evidence/msg. I ended up extracting 4 functions from the verify() method, making them publicly visible and implementing verify() itself using these component methods.

ancazamfir

Thanks @hu55a1n1! Left a few comments but in general it looks good. Could we see cosmos/ibc-rs#215 updates before we merge this?

light-client-verifier/src/verifier.rs

Co-authored-by: Anca Zamfir <[email protected]>

light-client-verifier/src/verifier.rs

Co-authored-by: Anca Zamfir <[email protected]>

light-client-verifier/src/verifier.rs

ancazamfir

I will approve as it looks great, thanks @hu55a1n1
If you can make the API name in this discussion it's great. I also added two very minor comments.

Co-authored-by: Anca Zamfir <[email protected]>

…hub.com:informalsystems/tendermint-rs into hu55a1n1/1222-impl-verify-commit-light-trusting

hu55a1n1 · 2022-11-24T15:03:51Z

If you can make the API name cosmos/ibc-rs#215 (comment) it's great.

Done -> c0f2469

Implement verify_light() and verify_light_trusting()

a8211b7

mzabaluev reviewed Nov 11, 2022

View reviewed changes

light-client-verifier/src/operations/voting_power.rs Outdated Show resolved Hide resolved

light-client-verifier/src/predicates.rs Outdated Show resolved Hide resolved

hu55a1n1 added 3 commits November 15, 2022 20:13

Rename check_signers_overlap_level() -> check_enough_signers_overlap

01c5112

Add unit test for ProdPredicates::has_specified_signers_overlap()

bd89492

Add MBT test for light verifier methods

1f4d6dc

Clippy fix

22690ed

hu55a1n1 marked this pull request as ready for review November 15, 2022 18:49

thanethomson reviewed Nov 16, 2022

View reviewed changes

mzabaluev reviewed Nov 17, 2022

View reviewed changes

mzabaluev mentioned this pull request Nov 17, 2022

light-client: Change Verdict return type to idiomatic Result #1236

Open

hu55a1n1 added 3 commits November 19, 2022 00:23

Split verify into 4 component methods

5fccfb6

Rewrite ProdVerifier::verify() using component methods

1fd3407

Remove unused code

56d05e1

hu55a1n1 marked this pull request as draft November 18, 2022 19:29

hu55a1n1 requested a review from ancazamfir November 18, 2022 19:47

Add changelog entry

58b46b6

hu55a1n1 marked this pull request as ready for review November 18, 2022 19:54

ancazamfir requested changes Nov 19, 2022

View reviewed changes

hu55a1n1 and others added 2 commits November 21, 2022 14:14

Apply suggestions from code review

841ab0b

Co-authored-by: Anca Zamfir <[email protected]>

Improve naming

4555afe

ancazamfir reviewed Nov 22, 2022

View reviewed changes

light-client-verifier/src/verifier.rs Outdated Show resolved Hide resolved

hu55a1n1 and others added 3 commits November 22, 2022 20:34

Apply suggestion to remove todo comment

7df64ac

Co-authored-by: Anca Zamfir <[email protected]>

Return Verdict from all PredicateVerifier verification methods

c1b5c34

Update comment

90bdd30

ancazamfir reviewed Nov 24, 2022

View reviewed changes

light-client-verifier/src/verifier.rs Outdated Show resolved Hide resolved

ancazamfir reviewed Nov 24, 2022

View reviewed changes

light-client-verifier/src/verifier.rs Outdated Show resolved Hide resolved

ancazamfir approved these changes Nov 24, 2022

View reviewed changes

Apply suggestions from code review

8274a4d

Co-authored-by: Anca Zamfir <[email protected]>

hu55a1n1 added 2 commits November 24, 2022 20:32

Rename validate() -> verify_validator_sets()

c0f2469

Merge branch 'hu55a1n1/1222-impl-verify-commit-light-trusting' of git…

0cc27e3

…hub.com:informalsystems/tendermint-rs into hu55a1n1/1222-impl-verify-commit-light-trusting

hu55a1n1 mentioned this pull request Nov 24, 2022

Misbehaviour handling implementation cosmos/ibc-rs#215

Merged

7 tasks

thanethomson approved these changes Nov 24, 2022

View reviewed changes

thanethomson merged commit e1468e2 into main Nov 24, 2022

thanethomson deleted the hu55a1n1/1222-impl-verify-commit-light-trusting branch November 24, 2022 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement verify_light() and verify_light_trusting() #1226

Implement verify_light() and verify_light_trusting() #1226

hu55a1n1 commented Nov 10, 2022 •

edited

Loading

mzabaluev left a comment •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

thanethomson left a comment

mzabaluev left a comment

mzabaluev Nov 17, 2022

mzabaluev Nov 17, 2022

romac Nov 17, 2022

mzabaluev Nov 17, 2022

romac Nov 17, 2022

hu55a1n1 commented Nov 18, 2022

ancazamfir left a comment

ancazamfir left a comment

hu55a1n1 commented Nov 24, 2022 •

edited

Loading

Implement verify_light() and verify_light_trusting() #1226

Implement verify_light() and verify_light_trusting() #1226

Conversation

hu55a1n1 commented Nov 10, 2022 • edited Loading

mzabaluev left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 15, 2022 • edited Loading

Codecov Report

thanethomson left a comment

Choose a reason for hiding this comment

mzabaluev left a comment

Choose a reason for hiding this comment

mzabaluev Nov 17, 2022

Choose a reason for hiding this comment

mzabaluev Nov 17, 2022

Choose a reason for hiding this comment

romac Nov 17, 2022

Choose a reason for hiding this comment

mzabaluev Nov 17, 2022

Choose a reason for hiding this comment

romac Nov 17, 2022

Choose a reason for hiding this comment

hu55a1n1 commented Nov 18, 2022

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

hu55a1n1 commented Nov 24, 2022 • edited Loading

hu55a1n1 commented Nov 10, 2022 •

edited

Loading

mzabaluev left a comment •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

hu55a1n1 commented Nov 24, 2022 •

edited

Loading