From 60b683c22cb2e0a913482c1cdbea77907b2de07e Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 28 Apr 2022 10:39:29 +0200 Subject: [PATCH 1/8] adding context section and draft notes for decision section --- .../adr-005-consensus-height-events.md | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 docs/architecture/adr-005-consensus-height-events.md diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md new file mode 100644 index 00000000000..83c3c3fd8cc --- /dev/null +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -0,0 +1,86 @@ +# ADR 005: ClientState Consensus Height Events + +## 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/main/docs/core/events.md#EventManager) to provide subscribers a method of reacting to application specific events. +IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/events.go#L33) attribute emitted as part of `UpdateClient` events in order to run misbehaviour detection by cross-checking the details of the *Header* emitted at a given consensus height against that of the *Header* from the originating chain. + +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/main/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/main/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 + +> This section explains all of the details of the proposed solution, including implementation details. +It should also describe affects / corollary items that may need to be changed as a part of this. +If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. +(e.g. the optimal split of things to do between separate PR's) + + + +The following decisions have been made in order to provide flexibility to consumers of the consensus height, + +1. Return a list of updated consensus heights `[]exported.Height` from the new UpdateState 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 emitted from 02-client client update handler, but mark as deprecated for future removal. For example, with tendermint lightclients this will simply be heights[0] following a successful update. + +3. Add an additional consensus_heights event, containing a list of updated heights. This provides flexibility for emitting multiple consensus heights from batched header updates. + +## Consequences + +> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. + +### 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 + +### Neutral + +## References + +> Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! + + +* {reference link} From 8554817b1e510c80ebc5d506084951e7d438c8d3 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 29 Apr 2022 11:39:13 +0200 Subject: [PATCH 2/8] updating adr with decisions and consequences sections --- .../adr-005-consensus-height-events.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index 83c3c3fd8cc..2609db89ecc 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -1,4 +1,4 @@ -# ADR 005: ClientState Consensus Height Events +# ADR 005: UpdateClient Events - ClientState Consensus Heights ## Changelog * 25/04/2022: initial draft @@ -49,11 +49,11 @@ It should also describe affects / corollary items that may need to be changed as If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. (e.g. the optimal split of things to do between separate PR's) - +The following decisions have been made in order to provide flexibility to consumers of `UpdateClient` events in a non-breaking fashion: -The following decisions have been made in order to provide flexibility to consumers of the consensus height, +1. 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*. -1. Return a list of updated consensus heights `[]exported.Height` from the new UpdateState interface. +2. 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. @@ -61,14 +61,10 @@ The following decisions have been made in order to provide flexibility to consum UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height ``` -2. Maintain the consensus_height event emitted from 02-client client update handler, but mark as deprecated for future removal. For example, with tendermint lightclients this will simply be heights[0] following a successful update. - -3. Add an additional consensus_heights event, containing a list of updated heights. This provides flexibility for emitting multiple consensus heights from batched header updates. +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 -> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. - ### Positive - Subscribers of IBC core events can act upon `UpdateClient` events containing one or more consensus heights. @@ -76,6 +72,8 @@ UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height ### Negative +- Consumers of IBC core `UpdateClient` events are forced to make future code changes. + ### Neutral ## References From ea4a27abcf640603c09a1c4d73f081c2c130f525 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 29 Apr 2022 11:46:36 +0200 Subject: [PATCH 3/8] adding references to adr 005 --- docs/architecture/adr-005-consensus-height-events.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index 2609db89ecc..c39727aec9c 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -78,7 +78,11 @@ UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height ## References -> Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! +Discussions: +- [#1208](https://github.com/cosmos/ibc-go/pull/1208#discussion_r839691927) - -* {reference link} +Issues: +- [#594](https://github.com/cosmos/ibc-go/issues/594) + +PRs: +- [#1285](https://github.com/cosmos/ibc-go/pull/1285) \ No newline at end of file From 091bee5c9e5fd3c604fd995998a0c0874026189d Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 29 Apr 2022 11:48:06 +0200 Subject: [PATCH 4/8] removing decision templating --- docs/architecture/adr-005-consensus-height-events.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index c39727aec9c..9f37640555f 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -44,11 +44,6 @@ Emission of standardised events from the `02-client` submodule now becomes probl ## Decision -> This section explains all of the details of the proposed solution, including implementation details. -It should also describe affects / corollary items that may need to be changed as a part of this. -If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. -(e.g. the optimal split of things to do between separate PR's) - The following decisions have been made in order to provide flexibility to consumers of `UpdateClient` events in a non-breaking fashion: 1. 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*. From aaf71f5b311b17474f5736f4e7f7d5811ced0cc9 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 2 May 2022 09:52:55 +0200 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Carlos Rodriguez --- docs/architecture/adr-005-consensus-height-events.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index 9f37640555f..7dbf4d9e167 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -10,11 +10,11 @@ Accepted ## Context The `ibc-go` implementation leverages the [Cosmos-SDK's EventManager](https://github.com/cosmos/cosmos-sdk/blob/main/docs/core/events.md#EventManager) to provide subscribers a method of reacting to application specific events. -IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/events.go#L33) attribute emitted as part of `UpdateClient` events in order to run misbehaviour detection by cross-checking the details of the *Header* emitted at a given consensus height against that of the *Header* from the originating chain. +IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/events.go#L33) attribute emitted as part of `UpdateClient` events in order to run 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. 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/main/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: +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/main/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 { From 6a0e1d78503d3d6d5119fd5a58078674c7b2c496 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 2 May 2022 11:22:45 +0200 Subject: [PATCH 6/8] adding versioned links --- docs/architecture/adr-005-consensus-height-events.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index 7dbf4d9e167..b84da7b1751 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -9,11 +9,11 @@ Accepted ## Context -The `ibc-go` implementation leverages the [Cosmos-SDK's EventManager](https://github.com/cosmos/cosmos-sdk/blob/main/docs/core/events.md#EventManager) to provide subscribers a method of reacting to application specific events. -IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/events.go#L33) attribute emitted as part of `UpdateClient` events in order to run 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. +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 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. 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/main/proto/ibc/core/client/v1/tx.proto#L44) message. +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 @@ -39,7 +39,7 @@ type ClientMessage interface { 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/main/modules/core/02-client/keeper/client.go#L90). +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 From 5ec9f0cf0a9720e8ed81d4717e523e2df7b3e97a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 2 May 2022 12:47:32 +0200 Subject: [PATCH 7/8] reorder decision points, add additional info on header cross-checking --- .../architecture/adr-005-consensus-height-events.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index b84da7b1751..58df16af774 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -10,7 +10,12 @@ 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 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. +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 as 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. @@ -46,9 +51,7 @@ Emission of standardised events from the `02-client` submodule now becomes probl The following decisions have been made in order to provide flexibility to consumers of `UpdateClient` events in a non-breaking fashion: -1. 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*. - -2. Return a list of updated consensus heights `[]exported.Height` from the new `UpdateState` method of the `ClientState` interface. +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. @@ -56,6 +59,8 @@ The following decisions have been made in order to provide flexibility to consum 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 From ca1d008dd32bf6a5c78878c2e14ecd4dd1c61d53 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 10 May 2022 13:54:18 +0200 Subject: [PATCH 8/8] typo --- docs/architecture/adr-005-consensus-height-events.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index 58df16af774..430d7064a1e 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -14,7 +14,7 @@ Some IBC relayers depend on the [`consensus_height`](https://github.com/cosmos/i - The `SignedHeader` containing the commitment root. - The `ValidatorSet` that signed the *Header*. -- The `TrustedHeight` seen by the client as less than or equal to the height of *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 @@ -85,4 +85,4 @@ Issues: - [#594](https://github.com/cosmos/ibc-go/issues/594) PRs: -- [#1285](https://github.com/cosmos/ibc-go/pull/1285) \ No newline at end of file +- [#1285](https://github.com/cosmos/ibc-go/pull/1285)