light-client: spec assumptions about clock synchronization vs trusting periods

[ebuchman]

https://github.com/informalsystems/tendermint-rs/pull/474 discovered a bug in that we weren't checking if untrusted headers are from the past (up to a clock drift), which they should be if all the clocks are well synced.

This find is also a reminder that there's an underlying assumption in the spec which should perhaps be more explicitly surfaced that the light client's local clock is closely synchronized (within clock_drift) to that of the chain's BFT time (ie. currently the median timestamp from the last commit).

We might want to better motivate this assumption and its relationship with the trusting period and expired headers. Currently, if our trusted header hasn't expired and trusting_period >> clock_drift, this is_header_from_past check ensures the new header is also within the trusting period, even though we don't run is_within_trusting_period(untrusted) directly. But. is_header_from_past(untrusted) is a much stronger condition than is_within_trusting_period(untrusted) and it may have subtle consequences in IBC, so we may want to reconsider it (!).

In IBC, a client's "local" clock is the BFT time of that chain (ie. the median timestamp of the last commit from the validator set ...), so this check would imply that the BFT time ("on chain clock") of any two chains in IBC is within clock_drift of each other, which might be too strong (?!).

Note that BFT time in Tendermint is currently subject to unaccountable control by +1/3 validators - they can arbitrarily fast forward time. This is somewhat counteracted by a punishment mechanism that uses both a hybrid of unbonding period (eg. ~3 weeks) AND minimum number of blocks (eg. ~30,000 blocks) to determine evidence validity, so even if validators fast forward the clock beyond the unbonding period, they can still be punished for some number of blocks.

And of course there are plans to change how BFT time is generated (proposer-based instead of median of timestamps in commits) and that would prevent +1/3 from controlling it, but would assume +2/3 have synchronized clocks on a given chain, and would still assume synchronize clocks between chains.

If validators fast forward the clock, they can make it difficult for light clients to keep up and act on current information (since it will look like the chain is always ahead). This may lead to certain kinds of attacks on IBC channels where validators on one chain can delay the validity of client updates on a chain its connected to (maybe there's an equivalent of Miner Extractable Value attacks on IBC here ...) . Not sure how big a problem this is, and/or how much it can be addressed by just checking is_within_trusting_period(untrusted) instead of is_header_from_past(untrusted) cc @cwgoes @josef-widder ...

_Originally posted by @ebuchman in https://github.com/informalsystems/tendermint-rs/pull/474#discussion_r460289046_

[konnov]

Nice. We can also try to reason about time in tla with apalache. It worked nice with the light client (having one clock)

[cwgoes]

In IBC, a client's "local" clock is the BFT time of that chain (ie. the median timestamp of the last commit from the validator set ...), so this check would imply that the BFT time ("on chain clock") of any two chains in IBC is within clock_drift of each other, which might be too strong (?!).

Yes, I am not sure if this condition will hold, especially for chains with long block times, but presumably we can vary the clock_drift tolerance along with associated updates in the trusting period (shorten it, relative to the unbonding period)?

This may lead to certain kinds of attacks on IBC channels where validators on one chain can delay the validity of client updates on a chain its connected to (maybe there's an equivalent of Miner Extractable Value attacks on IBC here ...).

This is possible, but even if 1/3 of validators can control the timestamp it doesn't seem substantially different from the censorship of IBC packets (outgoing or incoming) which 1/3 of validators could perform anyways. I guess the difference is in censoring packets, while the light clients could continue to be updated, vs. completely preventing light client updates altogether - if the latter attack could be kept up, the light clients which were trying to follow the chain would eventually exceed the trusting period, so I suppose that's a bit concerning. I don't immediately see a way for validators to extract value from that which they couldn't extract just from censoring IBC packets directly, though.

[Shivani912]

Also related: #523 - the code says we adjust for clock drifts but that parameter is not passed in nor used. We should at least update the comment to be correct.