-
Notifications
You must be signed in to change notification settings - Fork 586
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
02-client refactor #1871
02-client refactor #1871
Changes from all commits
a4b5b8f
841d21d
18abb79
a333a73
daa01db
2de5f1d
12f4ed2
ebf40bb
b0fa240
5e9785e
4c8b7c5
bdbaa91
f4480fb
5e3bac0
efbc5a1
b0dd49d
17209f7
fadd9d0
c2602a5
40183b4
a4b3d09
2e2bfab
18f1382
3c7358b
5cf6528
eb48e54
d2be6d5
e2f37b8
8a9978c
c43af66
e249518
e91ee68
e1ec9f4
cf893c2
55b115a
48882a9
8f46821
e1f2103
31b6ead
d120044
e2bdd1f
aab9ca2
4def196
32c4827
1617de7
3987a5b
81a7cae
60e5305
549c181
04df7cd
d8ac28a
d3d91ed
b0a58a8
7237c43
6cdcc63
0d8f408
a7d23fd
f861e0e
607458a
e9a7fac
18eee1a
197c2bd
2da9e65
68e8e79
3d33e8b
163b706
3cb1ba8
81f2d09
311a563
173c1fb
f8debd7
a7407cc
fc4bfaf
9a4ed68
87bae33
88936be
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 |
---|---|---|
@@ -0,0 +1,88 @@ | ||
# ADR 005: UpdateClient Events - ClientState Consensus Heights | ||
|
||
## Changelog | ||
* 25/04/2022: initial draft | ||
|
||
## Status | ||
|
||
Accepted | ||
|
||
## Context | ||
|
||
The `ibc-go` implementation leverages the [Cosmos-SDK's EventManager](https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/docs/core/events.md#EventManager) to provide subscribers a method of reacting to application specific events. | ||
Some IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/ibc-go/blob/v3.0.0/modules/core/02-client/keeper/events.go#L33) attribute emitted as part of `UpdateClient` events in order to run `07-tendermint` misbehaviour detection by cross-checking the details of the *Header* emitted at a given consensus height against those of the *Header* from the originating chain. This includes such details as: | ||
|
||
- The `SignedHeader` containing the commitment root. | ||
- The `ValidatorSet` that signed the *Header*. | ||
- The `TrustedHeight` seen by the client at less than or equal to the height of *Header*. | ||
- The last `TrustedValidatorSet` at the trusted height. | ||
|
||
Following the refactor of the `02-client` submodule and associated `ClientState` interfaces, it will now be possible for | ||
light client implementations to perform such actions as batch updates, inserting `N` number of `ConsensusState`s into the application state tree with a single `UpdateClient` message. This flexibility is provided in `ibc-go` by the usage of the [Protobuf `Any`](https://developers.google.com/protocol-buffers/docs/proto3#any) field contained within the [`UpdateClient`](https://github.com/cosmos/ibc-go/blob/v3.0.0/proto/ibc/core/client/v1/tx.proto#L44) message. | ||
For example, a batched client update message serialized as a Protobuf `Any` type for the `07-tendermint` lightclient implementation could be defined as follows: | ||
|
||
```protobuf | ||
message BatchedHeaders { | ||
repeated Header headers = 1; | ||
} | ||
``` | ||
|
||
To complement this flexibility, the `UpdateClient` handler will now support the submission of [client misbehaviour](https://github.com/cosmos/ibc/tree/master/spec/core/ics-002-client-semantics#misbehaviour) by consolidating the `Header` and `Misbehaviour` interfaces into a single `ClientMessage` interface type: | ||
|
||
```go | ||
// ClientMessage is an interface used to update an IBC client. | ||
// The update may be done by a single header, a batch of headers, misbehaviour, or any type which when verified produces | ||
// a change to state of the IBC client | ||
type ClientMessage interface { | ||
proto.Message | ||
|
||
ClientType() string | ||
ValidateBasic() error | ||
} | ||
``` | ||
|
||
To support this functionality the `GetHeight()` method has been omitted from the new `ClientMessage` interface. | ||
Emission of standardised events from the `02-client` submodule now becomes problematic and is two-fold: | ||
|
||
1. The `02-client` submodule previously depended upon the `GetHeight()` method of `Header` types in order to [retrieve the updated consensus height](https://github.com/cosmos/ibc-go/blob/v3.0.0/modules/core/02-client/keeper/client.go#L90). | ||
2. Emitting a single `consensus_height` event attribute is not sufficient in the case of a batched client update containing multiple *Headers*. | ||
|
||
## Decision | ||
|
||
The following decisions have been made in order to provide flexibility to consumers of `UpdateClient` events in a non-breaking fashion: | ||
|
||
1. Return a list of updated consensus heights `[]exported.Height` from the new `UpdateState` method of the `ClientState` interface. | ||
|
||
```go | ||
// UpdateState updates and stores as necessary any associated information for an IBC client, such as the ClientState and corresponding ConsensusState. | ||
// Upon successful update, a list of consensus heights is returned. It assumes the ClientMessage has already been verified. | ||
UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height | ||
``` | ||
|
||
2. Maintain the `consensus_height` event attribute emitted from the `02-client` update handler, but mark as deprecated for future removal. For example, with tendermint lightclients this will simply be `consensusHeights[0]` following a successful update using a single *Header*. | ||
|
||
3. Add an additional `consensus_heights` event attribute, containing a comma separated list of updated heights. This provides flexibility for emitting a single consensus height or multiple consensus heights in the example use-case of batched header updates. | ||
|
||
## Consequences | ||
|
||
### Positive | ||
|
||
- Subscribers of IBC core events can act upon `UpdateClient` events containing one or more consensus heights. | ||
- Deprecation of the existing `consensus_height` attribute allows consumers to continue to process `UpdateClient` events as normal, with a path to upgrade to using the `consensus_heights` attribute moving forward. | ||
|
||
### Negative | ||
|
||
- Consumers of IBC core `UpdateClient` events are forced to make future code changes. | ||
|
||
### Neutral | ||
|
||
## References | ||
|
||
Discussions: | ||
- [#1208](https://github.com/cosmos/ibc-go/pull/1208#discussion_r839691927) | ||
|
||
Issues: | ||
- [#594](https://github.com/cosmos/ibc-go/issues/594) | ||
|
||
PRs: | ||
- [#1285](https://github.com/cosmos/ibc-go/pull/1285) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,7 @@ packaged inside a payload which is json serialized and passed to `callContract` | |
array of bytes returned by the smart contract. This data is deserialized and passed as return argument. | ||
|
||
```go | ||
func (c *ClientState) CheckProposedHeaderAndUpdateState(context sdk.Context, marshaler codec.BinaryMarshaler, store sdk.KVStore, header exported.Header) (exported.ClientState, exported.ConsensusState, error) { | ||
func (c *ClientState) CheckProposedHeaderAndUpdateState(context sdk.Context, marshaler codec.BinaryMarshaler, store sdk.KVStore, header exported.ClientMessage) (exported.ClientState, exported.ConsensusState, error) { | ||
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. Shouldn't this be split up into two functions or we will allow the ADR authors to update? 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 we should change the status of the ADR to "needs update" |
||
// get consensus state corresponding to client state to check if the client is expired | ||
consensusState, err := GetConsensusState(store, marshaler, c.LatestHeight) | ||
if err != nil { | ||
|
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.
Also in v3.0.0.