Shivani/detect faulty vals #163
Conversation
Codecov Report
@@ Coverage Diff @@
## master #163 +/- ##
==========================================
+ Coverage 39.68% 39.78% +0.09%
==========================================
Files 91 91
Lines 3276 3283 +7
Branches 490 491 +1
==========================================
+ Hits 1300 1306 +6
- Misses 1690 1691 +1
Partials 286 286
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
We need to clarify if it makes sense to have a new error kind in the light module to signal that we detected a vote from a validator not in the val set (should be aligned with the spec).
A small test that tests (and thereby documents) the new (and correct) behaviour would also be great.
Shivani912
force-pushed
the
shivani/detect-faulty-vals
branch
from
7c0e619
to
f95c01d
Compare
tendermint/src/lite/error.rs
Outdated
|
|
||
| /// This is returned when a faulty i.e misbehaving full node is found | ||
| #[error("Faulty full node: {reason}")] | ||
| FaultyFullNode { reason: String }, |
There was a problem hiding this comment.
As discussed with @Shivani912 this error needs to be mentioned in the spec too.
|
@Shivani912 can you briefly summarize the changes to the JSON file? And if it was regenerated reference the used commit in your fork (if it was a different one)? |
| @@ -119,6 +119,8 @@ impl MockRequester { | |||
| } | |||
| } | |||
|
|
|||
| // Link to the commit that generated below JSON test files: | |||
| // https://github.com/Shivani912/tendermint/commit/f7d16ab59b55a4f1a5cdbfa6b0c24467aa88fdb2 | |||
There was a problem hiding this comment.
@liamsi Here is the link to the commit that generated the new val_set_tests.json
There was a problem hiding this comment.
I added this test that fails with FaultyFullNode error:
https://github.com/interchainio/tendermint-rs/blob/b28b3da6049cb4b0868e3c234f447c22daa2d289/tendermint/tests/support/lite/single_step_sequential/val_set_tests.json#L13078
Here, the input contains a validator set of 3 vals (v1, v2, v3) for header at height 2 with precommits from 3 vals (v1, v2, v4). This makes it pass all other validation tests but fails because there is a vote from val v4 which is not in the val set and thus there is a chance that the full node who sent this data is misbehaving.
There was a problem hiding this comment.
Awesome work! Thanks @Shivani912 👍
Although a bit redundant, I think it would be good to add a (rust only) unit test for the (implementation specific) validate method. This and adding the error to the spec can happen in separate PRs.
Thanks @liamsi ! I actually pinged @milosevic about this and we're not currently sure on what level of specification this should appear. We probably need some discussions over this. Do we have a spec where these error types are defined? For rust only test, we need a way to generate votes in the code. I was looking into it, but couldn't find how to produce signatures for votes. |
IMHO wherever we specify detecting (and handling) cases of faulty nodes, we need to mention these errors (prob here).
Not yet AFAIK. The other error kinds also live in the spec though: (e.g see |
I see. It might not be too difficult to generate a signature for votes (with the same libs / deps we are using for verification) but it looks definitely like more work than I though. Thanks for looking into this 👍 |
| "validator({}) voted for header {}, but current header is {}", | ||
| precommit.validator_address, | ||
| header_hash, | ||
| self.header_hash() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // returns FaultyFullNode error if it detects a signer isn't present in the validator set |
There was a problem hiding this comment.
I think this is actually an ImplementationSpecific error, since it depends on Commits containing the validator set addresses. So we don't need a new error type in the light client.
There was a problem hiding this comment.
Makes sense @ebuchman. Will address that :)
closes #140