Fix `VotingPowerCalculator::voting_power_in` #306

romac · 2020-06-08T18:12:06Z

Fix #282
Fix #335

Provide two helper methods for checking validators overlap and if there is enough trust w.r.t. the trusted state
Only count Commit signatures towards the voting power (BlockIdFlagNil votes counted when verifying a commit #282)
Do not rely on signed_votes method to get the votes
Only count the voting power of a validator once (Don't add voting power of the same validator multiple times in voting_power_in #335)

Notes

Needs to throw a proper error in some places
Trust threshold is currently hardcoded and should be rather passed in
The method should likely be renamed or its signature should be changed to reflect the new behavior
~~Most of logic and comments are taken from the Go code, so perhaps some attribution should be added as well~~
~~Tests are failing because the signatures currently fail to verify, likely because of mismatch with their validator~~

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

light-client/src/operations/voting_power.rs

melekes

lgtm

melekes · 2020-06-11T05:35:23Z

light-client/src/operations/voting_power.rs

+    commit: &Commit,
+) -> Option<Vote> {
+    let (validator_address, timestamp, signature, block_id) = match commit_sig {
+        CommitSig::BlockIDFlagAbsent { .. } => return None,


shouldn't we panic here because method assumes non_absent signature? then we won't need the Option<>

This function is actually the one doing the filtering for non absent signatures, hence the need for the Option. I guess the name isn't great, I am open to suggestions :)

light-client/tests/light_client.rs

- Provide two helper methods for checking validators overlap and if there is enough trust w.r.t. the trusted state. - Only count `Commit` signatures towards the voting power. - Do not rely on `signed_votes` method to get the votes.

romac · 2020-06-18T11:47:06Z

I pushed a new version which should now be doing the right thing w.r.t. checking trust vs validators signers, but still relies on validator addresses. I guess we can address the latter in a future PR once we are convinced that this version is correct.

light-client/src/operations/voting_power.rs

romac · 2020-06-18T15:29:38Z

It would be great if someone could run the tests of the light-client crate and inspect the output to ensure that errors that are raised are indeed the ones we expect from the JSON file. I did so myself

$ cd light-client/
$ cargo test -- --nocapture --test-threads 1

I already did so myself, and I am not too sure about this test:

Running light client against bisection test-file: ./tests/support/bisection/single_peer/not_enough_commits.json
  - Case: Trusted height=1, fails at height 6 because more than one-third (trust level) vals didn't sign
[light-client/tests/light_client.rs:247] e = Error(
    Context {
        kind: InvalidLightBlock(
            InvalidNextValidatorSet {
                header_next_validators_hash: Hash::Sha256(75B9F27F25F3515EF7CA61DD42C272CCC280C537E6A9ED493710EB361CB90B0D),
                next_validators_hash: Hash::Sha256(26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564),
            },
        ),
...

This test is passing because we expect an error, but I don't think that that is the right one, except if there's perhaps more than one error encoded in the JSON test. In particular, I would have expected the test to result in either NotEnoughTrust or InsufficientValidatorsOverlap but we are getting InvalidNextValidatorSet instead.

@Shivani912 Is that something you could look into?

brapse · 2020-06-18T15:30:18Z

light-client/src/operations/voting_power.rs

+        let mut tallied_voting_power = 0_u64;
+        let mut seen_validators = HashSet::new();
+
+        for (idx, signature) in signatures.into_iter().enumerate() {


Does it make sense to reference the test vector which exercises this particular edge case?

What edge case are you referring to here?

Oops perhaps I did not comment the correct line and actually there is an informative comment further down Ensure we only count a validator's power once

Unfortunately, we currently do not have a test vector which covers this edge case.

cc @Shivani912

Edit: typo

brapse · 2020-06-18T15:30:55Z

light-client/src/operations/voting_power.rs

+        let mut seen_validators = HashSet::new();
+
+        for (idx, signature) in signatures.into_iter().enumerate() {
+            let vote = vote_from_non_absent_signature(signature, idx as u64, &signed_header.commit);


Similar comment as above, IIRC this was a bug in the past.

You mean counting absent signatures? I don't think that's possible since a CommitSig::BlockIDFlagAbsent does not contain any of the data required to craft a Vote (ie. validator_address, timestamp and signature).

light-client/src/operations/voting_power.rs

Shivani912 · 2020-06-18T18:24:26Z

It would be great if someone could run the tests of the light-client crate and inspect the output to ensure that errors that are raised are indeed the ones we expect from the JSON file. I did so myself
$ cd light-client/
$ cargo test -- --nocapture --test-threads 1
I already did so myself, and I am not too sure about this test:
Running light client against bisection test-file: ./tests/support/bisection/single_peer/not_enough_commits.json
  - Case: Trusted height=1, fails at height 6 because more than one-third (trust level) vals didn't sign
[light-client/tests/light_client.rs:247] e = Error(
    Context {
        kind: InvalidLightBlock(
            InvalidNextValidatorSet {
                header_next_validators_hash: Hash::Sha256(75B9F27F25F3515EF7CA61DD42C272CCC280C537E6A9ED493710EB361CB90B0D),
                next_validators_hash: Hash::Sha256(26952B5D784A1564D167DF98D2D37376B5E77771928256D25E6FF9AE3AD11564),
            },
        ),
...
This test is passing because we expect an error, but I don't think that that is the right one, except if there's perhaps more than one error encoded in the JSON test. In particular, I would have expected the test to result in either NotEnoughTrust or InsufficientValidatorsOverlap but we are getting InvalidNextValidatorSet instead.

@Shivani912 Is that something you could look into?

@romac There's a bug in this one (issue #290) I'm working on it and will do a PR once I fix it!

I feel we really need to have checks (in the test itself) on errors even if the tests pass. I tried to do this at the end of Q1/beginning of Q2 but it seemed it was a little too early to be able to do so. Think this should be on priority for next quarter?

OStevan

@romac thanks for asking me to review. I wrote what I noticed about differences between the go implementation and the one here. Also, I wrote some comments mostly around VotingPower, however I might be missing some context so those could just be wrong.

OStevan · 2020-06-19T10:06:23Z

light-client/src/operations/voting_power.rs

+        let voting_power =
+            self.voting_power_of(untrusted_header, untrusted_validators, trust_threshold)?;
+
+        if trust_threshold.is_enough_power(voting_power.tallied, voting_power.total) {


Mentioned in one of the comments above, I would probably make more sense to just check this in the voting_power_of and instead of a value just return a boolean value or something else to tell us if everything is fine or there is some error.

OStevan · 2020-06-19T10:08:26Z

light-client/src/operations/voting_power.rs


 pub trait VotingPowerCalculator: Send {
    fn total_power_of(&self, validators: &ValidatorSet) -> u64;
-    fn voting_power_in(
+
+    fn check_enough_trust(


The name and the result of this function were for me a bit misleading as the name would suggest a boolean value (or in this case Ok() or Error(...)) and it is more or less used as such since the result value in the Ok case is just ignored.

OStevan · 2020-06-19T10:12:03Z

light-client/src/operations/voting_power.rs

+use tendermint::vote::{SignedVote, Vote};
+
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub struct VotingPower {


For me this is a bit misleading as the struct has more information than what I would expect by reading just the name. Also, I think it would be beneficial to implement the TODO about the breaking out of the loop as than this would go probably go away.

Agreed that the name is perhaps not the best, but we still need this information to be able to yield a meaningful error in case of lack of trust/signers overlap.

OStevan · 2020-06-19T10:22:13Z

light-client/src/operations/voting_power.rs

+            if seen_validators.contains(&vote.validator_address) {
+                continue;
+            } else {
+                seen_validators.insert(vote.validator_address);
+            }


One interesting difference between go code and the implementation here is this if. (go) In case of 2/3 threshold this check is done by comparing the size of the signers validator set and the expected validator set, if both are the same length everything is fine and we don't even do these checks. However in the case where we are interested in trust between two non adjacent headers, then branch should raise an exception.

@melekes What do you think?

I guess raising an error is probably the proper behavior there. Will already change that, and we can always revert it back if that was wrong.

romac · 2020-06-19T10:38:13Z

@romac There's a bug in this one (issue #290) I'm working on it and will do a PR once I fix it!

Awesome, thanks :)

I feel we really need to have checks (in the test itself) on errors even if the tests pass. I tried to do this at the end of Q1/beginning of Q2 but it seemed it was a little too early to be able to do so. Think this should be on priority for next quarter?

Yes, that would definitely increase our confidence in the tests and thus the correctness of implementation :)

brapse

I think this is good to go with follow up unit tests coming when we have the fixtures lined up.

ebuchman

Looks good! I think we still want to break the voting_power_in into two functions, one for check_signers and one for check_enough_trust, and neither should depend on vote.validator_address, but I'll follow up in another issue about that

ebuchman · 2020-06-24T19:28:56Z

light-client/src/operations/voting_power.rs

+    fn check_enough_trust(
+        &self,
+        untrusted_header: &SignedHeader,
+        untrusted_validators: &ValidatorSet,


These are trusted validators

romac · 2020-06-25T10:13:52Z

@ebuchman

I think we still want to break the voting_power_in into two functions, one for check_signers and one for check_enough_trust

It would be great if you could describe the differences between the two versions somewhere.

and neither should depend on vote.validator_address

I initially tried to fix that as well but eventually chose to focus on fixing the computation itself, and improve it later.

ebuchman · 2020-06-25T22:34:59Z

Done in #377

romac mentioned this pull request Jun 8, 2020

BlockIdFlagNil votes counted when verifying a commit #282

Closed

liamsi reviewed Jun 8, 2020

View reviewed changes

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

Base automatically changed from romain/light-client-followup to master June 9, 2020 10:29

melekes reviewed Jun 11, 2020

View reviewed changes

romac force-pushed the romain/voting-power-in branch from 74786f4 to e973dd9 Compare June 15, 2020 11:40

Fix VotingPowerCalculator::voting_power_in.

208cf97

- Provide two helper methods for checking validators overlap and if there is enough trust w.r.t. the trusted state. - Only count `Commit` signatures towards the voting power. - Do not rely on `signed_votes` method to get the votes.

romac force-pushed the romain/voting-power-in branch from e973dd9 to 208cf97 Compare June 18, 2020 11:41

romac changed the title ~~Rewrite VotingPowerCalculator::voting_power_in~~ Fix VotingPowerCalculator::voting_power_in Jun 18, 2020

Don't add voting power of the same validator multiple times

024a73d

romac mentioned this pull request Jun 18, 2020

Don't add voting power of the same validator multiple times in voting_power_in #335

Closed

romac marked this pull request as ready for review June 18, 2020 12:02

Cleanup and comments

c828155

brapse reviewed Jun 18, 2020

View reviewed changes

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

brapse reviewed Jun 18, 2020

View reviewed changes

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

brapse reviewed Jun 18, 2020

View reviewed changes

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

brapse reviewed Jun 18, 2020

View reviewed changes

romac added 2 commits June 18, 2020 17:33

Remove debug statements

319c9f2

Add a constant for a trust threshold of 2/3

e9c51a0

brapse reviewed Jun 18, 2020

View reviewed changes

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

romac added 3 commits June 18, 2020 17:39

Fix clippy warnings

90c32ba

Mention #235 in TODO comment

8c0c5e6

Small refactor

7e3b537

Fix clippy warnings and a typo

ef3660c

OStevan reviewed Jun 19, 2020

View reviewed changes

Address Stevan's comments

30d680f

romac added 2 commits June 19, 2020 13:28

Remove debug condition

2f45b73

Raise error when seeing duplicate validator in voting power computation

fc5e89f

brapse approved these changes Jun 19, 2020

View reviewed changes

brapse merged commit 8ab1989 into master Jun 19, 2020

brapse deleted the romain/voting-power-in branch June 19, 2020 15:16

ebuchman reviewed Jun 24, 2020

View reviewed changes

romac mentioned this pull request Jun 25, 2020

Rename untrusted_validators param to trusted_validators in check_enough_trust #374

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `VotingPowerCalculator::voting_power_in` #306

Fix `VotingPowerCalculator::voting_power_in` #306

romac commented Jun 8, 2020 •

edited

Loading

melekes left a comment

melekes Jun 11, 2020

romac Jun 18, 2020

romac commented Jun 18, 2020

romac commented Jun 18, 2020

brapse Jun 18, 2020

romac Jun 19, 2020

brapse Jun 19, 2020

romac Jun 19, 2020 •

edited

Loading

brapse Jun 18, 2020

romac Jun 19, 2020

brapse Jun 19, 2020

Shivani912 commented Jun 18, 2020

OStevan left a comment

OStevan Jun 19, 2020

OStevan Jun 19, 2020

OStevan Jun 19, 2020

romac Jun 19, 2020

OStevan Jun 19, 2020

romac Jun 19, 2020

romac Jun 19, 2020

romac commented Jun 19, 2020

brapse left a comment

ebuchman left a comment

ebuchman Jun 24, 2020

romac commented Jun 25, 2020

ebuchman commented Jun 25, 2020

Fix VotingPowerCalculator::voting_power_in #306

Fix VotingPowerCalculator::voting_power_in #306

Conversation

romac commented Jun 8, 2020 • edited Loading

Notes

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac commented Jun 18, 2020

romac commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac Jun 19, 2020 • 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

Shivani912 commented Jun 18, 2020

OStevan 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

Choose a reason for hiding this comment

romac commented Jun 19, 2020

brapse left a 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

romac commented Jun 25, 2020

ebuchman commented Jun 25, 2020

Fix `VotingPowerCalculator::voting_power_in` #306

Fix `VotingPowerCalculator::voting_power_in` #306

romac commented Jun 8, 2020 •

edited

Loading

romac Jun 19, 2020 •

edited

Loading