-
Notifications
You must be signed in to change notification settings - Fork 398
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
ICS 7: Identify exact fields required for Tendermint light client state #540
Comments
We will definitely need However, this is not only relevant for client state but also for type LightBlock struct {
Header Header
Commit Commit
Validators ValidatorSet
NextValidators ValidatorSet
Provider PeerID
} |
Per @AdityaSripal we can just add I suspect this will also allow us to remove |
I am not sure about the consequences of removing |
Yes; we can just store the hash of the validator set and/or next validator set, once verified. |
I guess we don't look into the validator set anymore; but I have to think about this again in the context of fork detection. However, the |
If we need the full validator set, presumably it can be provided in the transaction & we can check against the hash. |
Yeah, that's true. I was thinking about checking what evidence to submit. As this will rarely happen, if at all, we don't need store the validators explicitly, most likely, and provide additional information only when needed. |
So when we verify the next header, we usually would use https://github.com/tendermint/tendermint/blob/v0.33.4/lite2/verifier.go#L140 This is because the This will make the bisecting verification slightly less efficient from a typical light client, in the sense that a light client that uses However, there is no correctness concern with just using |
Note: We already need need the current |
I am not sure why there is no correctness concern: If the validator set and the nextvalidator set of height h do not intersect, then the lightclient is not able to verify the correct header of height h+1 by using the validator set of height h. |
We don't use the validator set of height https://github.com/tendermint/tendermint/blob/v0.33.4/lite2/verifier.go#L122 |
So you use Regarding ICS07, this means that we have to add NextValidators to the specification, either as hash or as set (at several places). Right now, it is not there at all. I suggest we add NextValidators also in the consensus state. Then you could use h.NextValidators also for skipping, which you do not use right now, as it is not there. |
Ref cosmos/cosmos-sdk#7176, small discrepancy in consensus state, should be easy to fix. |
Right now the fields in interface ClientState {
chainID: string
trustLevel: Rational
trustingPeriod: uint64
unbondingPeriod: uint64
latestHeight: Height
frozenHeight: Maybe<uint64>
upgradePath: []string
maxClockDrift: uint64
proofSpecs: []ProofSpec
} are the same as the ones used in ibc-go: type ClientState struct {
ChainId string
TrustLevel Fraction
// duration of the period since the LastestTimestamp during which the
// submitted headers are valid for upgrade
TrustingPeriod time.Duration
// duration of the staking unbonding period
UnbondingPeriod time.Duration
// defines how much new (untrusted) header's Time can drift into the future.
MaxClockDrift time.Duration
// Block height when the client was frozen due to a misbehaviour
FrozenHeight types.Height
// Latest height the client was updated to
LatestHeight types.Height
// Proof specifications used in verifying counterparty state
ProofSpecs []*_go.ProofSpec
// Path at which next upgraded client will be committed.
// Each element corresponds to the key for a single CommitmentProof in the
// chained proof. NOTE: ClientState must stored under
// `{upgradePath}/{upgradeHeight}/clientState` ConsensusState must be stored
// under `{upgradepath}/{upgradeHeight}/consensusState` For SDK chains using
// the default upgrade module, upgrade_path should be []string{"upgrade",
// "upgradedIBCState"}`
UpgradePath []string
// allow_update_after_expiry is deprecated
AllowUpdateAfterExpiry bool // Deprecated: Do not use.
// allow_update_after_misbehaviour is deprecated
AllowUpdateAfterMisbehaviour bool // Deprecated: Do not use.
} except for So I think this issue can now be closed. |
Ref cosmos/cosmos-sdk#6736
At present, the specification and implementation do not agree on what fields a Tendermint light client state should store.
The specification (ICS 7) has the following fields:
The Cosmos SDK implementation, however, has the following fields:
Differences:
upgradeCommitmentPrefix
andupgradeKey
have yet to be added to the implementation. This will be done soon (I believe @AdityaSripal is working on the upgrade-related features at the moment).Header
, which includes chain ID, validator set, height, timestamp, and other metadata (defined here):tmtypes.SignedHeader
(defined here) includes:Commit
(defined here) includes a lot of data we definitely don't need to keep in the client state, and some data which is just duplicated in theHeader
:As does
Header
(defined here):ICS 7 is already set up to store the
AppHash
elsewhere (under a height-specific consensus state key). The client state should store only the absolute minimum information required for verification of subsequent headers, so I think the best option would be to figure out if any fields other than what the ICS 7 specification currently specifies are actually required for light client verification, add them if necessary, and simplify the light client verification API to take only the fields required. This is relevant not only for IBC but also for other light clients which may wish to avoid storing superfluous metadata.The text was updated successfully, but these errors were encountered: