Skip to content

Commit 44541b8

Browse files
colin-axnercolin-axner
authored
add ADR 006 (cosmos#1846)
1 parent 73fdde9 commit 44541b8

File tree

1 file changed

+196
-0
lines changed

1 file changed

+196
-0
lines changed

‎docs/architecture/adr-006-02-client-refactor.md

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
# ADR 006: ICS-02 client refactor
2+
3+
## Changelog
4+
* 2022-08-01: Initial Draft
5+
6+
## Status
7+
8+
Accepted and applied in v6 of ibc-go
9+
10+
## Context
11+
12+
During the initial development of the 02-client submodule, each light client supported (06-solomachine, 07-tendermint, 09-localhost) was referenced through hardcoding.
13+
Here is an example of the [code](https://github.com/cosmos/cosmos-sdk/commit/b93300288e3a04faef9c0774b75c13b24450ba1c#diff-c5f6b956947375f28d611f18d0e670cf28f8f305300a89c5a9b239b0eeec5064R83) that existed in the 02-client submodule:
14+
```go
15+
func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.Header) (exported.ClientState, error) {
16+
17+
...
18+
19+
switch clientType {
20+
case exported.Tendermint:
21+
clientState, consensusState, err = tendermint.CheckValidityAndUpdateState(
22+
clientState, header, ctx.BlockTime(),
23+
)
24+
case exported.Localhost:
25+
// override client state and update the block height
26+
clientState = localhosttypes.NewClientState(
27+
ctx.ChainID(), // use the chain ID from context since the client is from the running chain (i.e self).
28+
ctx.BlockHeight(),
29+
)
30+
default:
31+
err = types.ErrInvalidClientType
32+
}
33+
```
34+
35+
To add additional light clients, code would need to be added directly to the 02-client submodule.
36+
Evidently, this would likely become problematic as IBC scaled to many chains using consensus mechanisms beyond the initial supported light clients.
37+
Issue [#6064](https://github.com/cosmos/cosmos-sdk/issues/6064) on the SDK addressed this problem by creating a more modular 02-client submodule.
38+
The 02-client submodule would now interact with each light client via an interface.
39+
While, this change was positive in development, increasing the flexibility and adoptability of IBC, it also opened the door to new problems.
40+
41+
The difficulty of generalizing light clients became apparent once changes to those light clients were required.
42+
Each light client represents a different consensus algorithm which may contain a host of complexity and nuances.
43+
Here are some examples of issues which arose for light clients that are not applicable to all the light clients supported (06-solomachine, 07-tendermint, 09-localhost):
44+
45+
### Tendermint non-zero height upgrades
46+
47+
Before the launch of IBC, it was determined that the golang implementation of [tendermint](https://github.com/tendermint/tendermint) would not be capable of supporting non-zero height upgrades.
48+
This implies that any upgrade would require changing of the chain ID and resetting the height to 0.
49+
A chain is uniquely identified by its chain-id and validator set.
50+
Two different chain ID's can be viewed as different chains and thus a normal update produced by a validator set cannot change the chain ID.
51+
To work around the lack of support for non-zero height upgrades, an abstract height type was created along with an upgrade mechanism.
52+
This type would indicate the revision number (the number of times the chain ID has been changed) and revision height (the current height of the blockchain).
53+
54+
Refs:
55+
- Issue [#439](https://github.com/cosmos/ibc/issues/439) on IBC specification repository.
56+
- Specification changes in [#447](https://github.com/cosmos/ibc/pull/447)
57+
- Implementation changes for the abstract height type, [SDK#7211](https://github.com/cosmos/cosmos-sdk/pull/7211)
58+
59+
### Tendermint requires misbehaviour detection during updates
60+
61+
The initial release of the IBC module and the 07-tendermint light client implementation did not support misbehaviour detection during update nor did it prevent overwriting of previous updates.
62+
Despite the fact that we designed the `ClientState` interface and developed the 07-tendermint client, we failed to detect even a duplicate update that constituted misbehaviour and thus should freeze the client.
63+
This was fixed in PR [#141](https://github.com/cosmos/ibc-go/pull/141) which required light client implementations to be aware that they must handle duplicate updates and misbehaviour detection.
64+
Misbehaviour detection during updates is not applicable to the solomachine nor localhost.
65+
It is also not obvious that `CheckHeaderAndUpdateState` should be performing this functionality.
66+
67+
### Localhost requires access to the entire client store
68+
69+
The localhost has been broken since the initial version of the IBC module.
70+
The localhost tried to be developed underneath the 02-client interfaces without special exception, but this proved to be impossible.
71+
The issues were outlined in [#27](https://github.com/cosmos/ibc-go/issues/27) and further discussed in the attempted ADR in [#75](https://github.com/cosmos/ibc-go/pull/75).
72+
Unlike all other clients, the localhost requires access to the entire IBC store and not just the prefixed client store.
73+
74+
### Solomachine doesn't set consensus states
75+
76+
The 06-solomachine does not set the consensus states within the prefixed client store.
77+
It has a single consensus state that is stored within the client state.
78+
This causes setting of the consensus state at the 02-client level to use unnecessary storage.
79+
It also causes timeouts to fail with solo machines.
80+
Previously, the timeout logic within IBC would obtain the consensus state at the height a timeout is being proved.
81+
This is problematic for the solo machine as no consensus state is set.
82+
See issue [#562](https://github.com/cosmos/ibc/issues/562) on the IBC specification repo.
83+
84+
### New clients may want to do batch updates
85+
86+
New light clients may not function in a similar fashion to 06-solomachine and 07-tendermint.
87+
They may require setting many consensus states in a single update.
88+
As @seunlanlege [states](https://github.com/cosmos/ibc-go/issues/284#issuecomment-1005583679):
89+
90+
> I'm in support of these changes for 2 reasons:
91+
>
92+
> - This would allow light clients handle batch header updates in CheckHeaderAndUpdateState, for the special case of 11-beefy proving the finality for a batch of headers is much more space and time efficient than the space/time complexity of proving each individual headers in that batch, combined.
93+
>
94+
> - This also allows for a single light client instance of 11-beefy be used to prove finality for every parachain connected to the relay chain (Polkadot/Kusama). We achieve this by setting the appropriate ConsensusState for individual parachain headers in CheckHeaderAndUpdateState
95+
96+
## Decision
97+
98+
### Require light clients to set client and consensus states
99+
The IBC specification states:
100+
101+
> If the provided header was valid, the client MUST also mutate internal state to store now-finalised consensus roots and update any necessary signature authority tracking (e.g. changes to the validator set) for future calls to the validity predicate.
102+
103+
The initial version of the IBC go SDK based module did not fulfill this requirement.
104+
Instead, the 02-client submodule required each light client to return the client and consensus state which should be updated in the client prefixed store.
105+
This decision lead to the issues "Solomachine doesn't set consensus states" and "New clients may want to do batch updates".
106+
107+
Each light client should be required to set its own client and consensus states on any update necessary.
108+
The go implementation should be changed to match the specification requirements.
109+
This will allow more flexibility for light clients to manage their own internal storage and do batch updates.
110+
111+
### Merge `Header`/`Misbehaviour` interface and rename to `ClientMessage`
112+
113+
Remove `GetHeight()` from the header interface (as light clients now set the client/consensus states).
114+
This results in the `Header`/`Misbehaviour` interfaces being the same.
115+
To reduce complexity of the codebase, the `Header`/`Misbehaviour` interfaces should be merged into `ClientMessage`.
116+
`ClientMessage` will provide the client with some authenticated information which may result in regular updates, misbehaviour detection, batch updates, or other custom functionality a light client requires.
117+
118+
119+
### Split `CheckHeaderAndUpdateState` into 4 functions
120+
121+
See [#668](https://github.com/cosmos/ibc-go/issues/668).
122+
123+
Split `CheckHeaderAndUpdateState` into 4 functions:
124+
- `VerifyClientMessage`
125+
- `CheckForMisbehaviour`
126+
- `UpdateStateOnMisbehaviour`
127+
- `UpdateState`
128+
129+
`VerifyClientMessage` checks the that the structure of a `ClientMessage` is correct and that all authentication data provided is valid.
130+
131+
`CheckForMisbehaviour` checks to see if a `ClientMessage` is evidence of misbehaviour.
132+
133+
`UpdateStateOnMisbehaviour` freezes the client and updates its state accordingly.
134+
135+
`UpdateState` performs a regular update or a no-op on duplicate updates.
136+
137+
The code roughly looks like:
138+
```go
139+
func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.Header) error {
140+
141+
...
142+
143+
if err := clientState.VerifyClientMessage(clientMessage); err != nil {
144+
return err
145+
}
146+
147+
foundMisbehaviour := clientState.CheckForMisbehaviour(clientMessage)
148+
if foundMisbehaviour {
149+
clientState.UpdateStateOnMisbehaviour(header)
150+
// emit misbehaviour event
151+
return
152+
}
153+
154+
clientState.UpdateState(clientMessage) // expects no-op on duplicate header
155+
// emit update event
156+
return
157+
}
158+
```
159+
160+
### Add `GetTimestampAtHeight` to the client state interface
161+
162+
By adding `GetTimestampAtHeight` to the ClientState interface, we allow light clients which do non-traditional consensus state/timestamp storage to process timeouts correctly.
163+
This fixes the issues outlined for the solo machine client.
164+
165+
### Add generic verification functions
166+
167+
As the complexity and the functionality grows, new verification functions will be required for additional paths.
168+
This was explained in [#684](https://github.com/cosmos/ibc/issues/684) on the specification repo.
169+
These generic verification functions would be immediately useful for the new paths added in connection/channel upgradability as well as for custom paths defined by IBC applications such as Interchain Queries.
170+
The old verification functions (`VerifyClientState`, `VerifyConnection`, etc) should be removed in favor of the generic verification functions.
171+
172+
## Consequences
173+
174+
### Positive
175+
- Flexibility for light client implementations
176+
- Well defined interfaces and their required functionality
177+
- Generic verification functions
178+
- Applies changes necessary for future client/connection/channel upgrabability features
179+
- Timeout processing for solo machines
180+
- Reduced code complexity
181+
182+
### Negative
183+
- The refactor touches on sensitive areas of the ibc-go codebase
184+
- Changing of established naming (`Header`/`Misbehaviour` to `ClientMessage`)
185+
186+
### Neutral
187+
No notable consequences
188+
189+
## References
190+
191+
Issues:
192+
- [#284](https://github.com/cosmos/ibc-go/issues/284)
193+
194+
PRs:
195+
- [#1871](https://github.com/cosmos/ibc-go/pull/1871)
196+

0 commit comments

Comments
 (0)