Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

light-client: Attack detector and evidence reporting #1292

Merged
merged 62 commits into from
Apr 28, 2023

Conversation

romac
Copy link
Member

@romac romac commented Apr 4, 2023

Closes: #1291
Closes: #1219
Closes: #415

  • 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/

light-client/cli/src/main.rs Outdated Show resolved Hide resolved
light-client/src/detector/provider.rs Outdated Show resolved Hide resolved
proto/src/prost/v0_34/tendermint.types.rs Show resolved Hide resolved
tools/proto-compiler/src/constants.rs Outdated Show resolved Hide resolved
tools/proto-compiler/src/main.rs Outdated Show resolved Hide resolved
light-client/src/builder/light_client.rs Show resolved Hide resolved
@romac romac marked this pull request as ready for review April 4, 2023 16:57
@romac romac force-pushed the romac/new-misbehavior-detector branch from 51e2a3f to 6ba0f6e Compare April 20, 2023 13:46
@ancazamfir
Copy link
Contributor

ancazamfir commented Apr 24, 2023

Trying to read the evidence with a hermes version using this branch and I see:

$ hermes --config config.toml equivocation --chain ibc-1
2023-04-24T04:46:55.627984Z  INFO ThreadId(01) running Hermes v1.4.0+17ba1b5e3-dirty
2023-04-24T04:46:55.648529Z DEBUG ThreadId(18) event_monitor{chain=ibc-1}: starting event monitor
2023-04-24T04:46:55.660791Z DEBUG ThreadId(01) trying to check for evidence at height 1-106
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: serde parse error: mismatch between raw voting (Power(0)) and computed one (Power(100000)) at line 1 column 4149', crates/relayer-cli/src/commands/equivocation.rs:107:75
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Also tried to use the block here (which is the same as the one read by hermes above, coming from Go light client evidence after a forked chain) in one of the gaia fixtures and seeing similar error:

---- incoming_fixtures stdout ----
thread 'incoming_fixtures' panicked at 'called `Result::unwrap()` on an `Err` value: serde parse error

Caused by:
    mismatch between raw voting (Power(0)) and computed one (Power(100000))

Edit:
The error comes from

// Ensure that the raw voting power matches the computed one
let raw_voting_power = value.total_voting_power.try_into()?;
if raw_voting_power != validator_set.total_voting_power() {
return Err(Error::raw_voting_power_mismatch(
raw_voting_power,
validator_set.total_voting_power(),
));
}
.
The total_voting_power is missing in the ValidatorSet of the ConflictingBlock (of type LigthBlock). Not sure if it was ever sent by the Go light client. I will also try with the Rust light client.
But iirc, we had this issue also in the past with the IBC light client updates. If the field is missing we shouldn't do the check, not sure how easy we can determine this though.

@ancazamfir
Copy link
Contributor

I will also try with the Rust light client.

Same error

@romac
Copy link
Member Author

romac commented Apr 24, 2023

If the field is missing we shouldn't do the check, not sure how easy we can determine this though.

Unfortunately there is no way to distinguish between the field being missing or its value being 0. I believe the best we can do is to drop the check if the value is 0. What do you think?

@ancazamfir
Copy link
Contributor

If the field is missing we shouldn't do the check, not sure how easy we can determine this though.

Unfortunately there is no way to distinguish between the field being missing or its value being 0. I believe the best we can do is to drop the check if the value is 0. What do you think?

Yes, let's drop the check. There is this unexported comment in validator set totalVotingPower in Comet:
https://github.com/cometbft/cometbft/blob/b8187b0f538195eae07ef4c79c2eb3c927bc181c/types/validator_set.go#L51-L58

Maybe it specifically means that it will not be included in RPC responses that have validator set. Maybe we should clarify with the Comet team.

There are also some comments in ValidatorSetFromProto and ToProto() that suggest that for protos also this field is set to 0 on encoding and computed when decoding.

@romac romac force-pushed the romac/new-misbehavior-detector branch from 98188b6 to 61a0f6b Compare April 28, 2023 13:28
@romac
Copy link
Member Author

romac commented Apr 28, 2023

I removed the changes related to the addition of light client CLI and the removal of the supervisor into their own PR, to be merged after this one: #1308

@romac romac marked this pull request as ready for review April 28, 2023 13:38
@romac romac requested a review from ancazamfir April 28, 2023 14:00
Copy link
Contributor

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @romac! Thanks also for the help @mzabaluev

@romac romac merged commit de10198 into main Apr 28, 2023
24 checks passed
@romac romac deleted the romac/new-misbehavior-detector branch April 28, 2023 15:02
@romac romac mentioned this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants