-
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
Changes from 2 commits
c8bce5a
8e7e5fa
a18020f
45d2786
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,26 @@ message ConsensusState { | |
]; | ||
} | ||
|
||
// Evidence is a wrapper over tendermint's DuplicateVoteEvidence | ||
AdityaSripal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// that implements Evidence interface expected by ICS-02 | ||
message Evidence { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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)? 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 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 commentThe 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. |
||
option (gogoproto.goproto_getters) = false; | ||
option (gogoproto.goproto_stringer) = false; | ||
|
||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine 👍 |
||
(gogoproto.nullable) = false, | ||
(gogoproto.moretags) = "yaml:\"header_1\"" | ||
]; | ||
Header header_2 = 4 [ | ||
(gogoproto.customname) = "Header2", | ||
(gogoproto.nullable) = false, | ||
(gogoproto.moretags) = "yaml:\"header_2\"" | ||
]; | ||
} | ||
|
||
// Header defines the Tendermint client consensus Header. | ||
// It encapsulates all the information necessary to update from a trusted | ||
// Tendermint ConsensusState. The inclusion of TrustedHeight and | ||
|
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 ofConflictingHeadersEvidence