-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
migrate tm evidence to proto #7145
Conversation
string client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""]; | ||
string chain_id = 2 [(gogoproto.moretags) = "yaml:\"chain_id\""]; | ||
Header header_1 = 3 [ | ||
(gogoproto.customname) = "Header1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc/ @amaurymartiny . I'm using customname here because otherwise the generated files will output "Header_1". We don't need gRPC calls for evidence because we don't store them in state (we don't even have gRPC calls for this sub-module). Just wanted to check this won't cause issues in relation to #7033 changes. I kept the "Id" naming for other fields in this struct for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not ok I can update to "HeaderOne"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine 👍
@@ -32,9 +32,9 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState( | |||
"client is already frozen at earlier height %d than misbehaviour height %d", cs.FrozenHeight, misbehaviour.GetHeight()) | |||
} | |||
|
|||
tmEvidence, ok := misbehaviour.(Evidence) | |||
tmEvidence, ok := misbehaviour.(*Evidence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to change this in header, will do in the followup with messages
Codecov Report
@@ Coverage Diff @@
## master #7145 +/- ##
=======================================
Coverage 54.74% 54.74%
=======================================
Files 563 563
Lines 38405 38415 +10
=======================================
+ Hits 21024 21031 +7
- Misses 15671 15674 +3
Partials 1710 1710 |
@@ -68,6 +68,26 @@ message ConsensusState { | |||
]; | |||
} | |||
|
|||
// Evidence is a wrapper over tendermint's DuplicateVoteEvidence | |||
// that implements Evidence interface expected by ICS-02 | |||
message Evidence { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you differentiate between different forms of evidence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. we currently only have one form of evidence for tendermint. Whenever we add more we can rename this to be more specific, but that is for another pr. Are we planning to support more than DuplicateVoteEvidence for 1.0? @cwgoes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; we want to support all of the evidence types which Tendermint supports.
Ideally we should just import the proto files from Tendermint directly, if they exist?
cc @cmwaters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are here: https://github.com/tendermint/tendermint/blob/master/proto/tendermint/types/evidence.proto.
I would recommend generating them yourself instead of importing them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, they are located here although they are still undergoing changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened #7146 . @AdityaSripal do you think you could look into doing this? I'm not sure if this would require added logic in misbehaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwgoes I don't see why we would want to support all the evidence types that Tendermint supports in core IBC (we will want to do this later for apps like cross-chain staking). Most are irrelevant for core IBC since they involve individual validators misbehaving.
What is core IBC supposed to do upon receiving Evidence of a single validator misbehaving (suppose a DoubleVote or Amnesia or Lunatic Evidence)?
From my understanding, Nothing. It is incumbent on the chain to punish this validator accordingly upon processing this evidence, but the IBC client should operate as normal as this is entirely within the concerns of the counterparty chain (It is part of the security model assumptions of Tendermint blockchains that individual validators may become malicious without worrying about the lite-client state).
IBC clients only need to do something when the security assumptions of the counterparty chain are violated. I.E. There is a light or full fork of the counterparty chain. This case is already handled in IBC's misbehaviour logic, though we could look into reusing the Evidence type Tendermint uses for this, namely ConflictingHeadersEvidence
Ref further dicussion here: https://github.com/cosmos/cosmos-sdk/issues/7146#issuecomment-679305265
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I was confused by the comment above this message - https://github.com/cosmos/cosmos-sdk/issues/7146#issuecomment-679364939.
sims are failing unrelated to this pr (evidence isn't even in genesis at all) |
@@ -68,6 +68,26 @@ message ConsensusState { | |||
]; | |||
} | |||
|
|||
// Evidence is a wrapper over tendermint's DuplicateVoteEvidence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is NOT a wrapper over DuplicateVoteEvidence
. This is really a reimplementation of ConflictingHeadersEvidence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, will commit the docfix myself
Description
Another step to removing amino. Will do final changes of tendermint messages in a followup, after that we should be pretty close to removing amino (pending localhost revert)
ref: #6254
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes