fix predicate bug #474

Shivani912 · 2020-07-22T17:10:10Z

closes #473

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

Shivani912 · 2020-07-22T17:23:31Z

Failing test-nightly-coverage issue #471

light-client/src/predicates.rs

liamsi · 2020-07-23T08:33:51Z

light-client/src/predicates.rs

@@ -231,6 +249,9 @@ pub fn verify(
        now,
    )?;

+    // Ensure the header isn't from a future time
+    vp.is_header_from_past(&untrusted.signed_header.header, options.clock_drift, now)?;


Great find! 👍

This is a great find. It's 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 ...

Ok I gave it its own issue: #478

liamsi

Left some minor (optional) renamings that could clarify wether the functions are expecting a trusted/vs untrusted header (maybe we can use the type system for that on the long-run instead). Can be addressed here or in followups.

Thanks @Shivani912 🙏 Great work!

Shivani912 · 2020-07-23T09:07:11Z

Left some minor (optional) renamings that could clarify wether the functions are expecting a trusted/vs untrusted header (maybe we can use the type system for that on the long-run instead). Can be addressed here or in followups.

Thanks @Shivani912 🙏 Great work!

Thanks for reviewing @liamsi ! I think doing that makes a lot of sense. @romac would be best person to comment on this as he has a better view of it than I do.

romac · 2020-07-23T10:52:26Z

light-client/src/predicates.rs

+    ) -> Result<(), VerificationError> {
+        let expires_at = trusted_header.time + trusting_period;
+        ensure!(
+            expires_at > now,


I wonder if we need to account for clock drift here as well?

As far as I remember the spec applies the clockdrift only to untrusted headers. If we change that here we should feed that back into the spec too:
https://github.com/informalsystems/tendermint-rs/blob/master/docs/spec/lightclient/verification/verification.md#lcv-func-valid1

Found this gem in the english spec 🤣

If trusted.Header.Time > now - trustingPeriod the blabla

who knows if "the blabla" isn't the clockDrift though...

cc @milosevic

A benefit to not adding the clock_drift is that we have some extra time to account for the time taken by the next verification steps. Also this could mean that if the header has already expired like a second ago but because we add the clock_drift there, it could satisfy the condition and consider it valid.

I think we shouldn't need to worry about clock drift here since trusting_period is orders of magnitude greater than clock_drift but I'm not sure how best to surface that reasoning in the code (maybe just a comment). Also cc @josef-widder

romac · 2020-07-23T10:55:28Z

Great stuff @Shivani912, I just have one question about clock drift inlined above.

liamsi

LGTM (regarding the clock drift: https://github.com/informalsystems/tendermint-rs/pull/474/files#r459496060 it seems that it is safer here to not account for clockdrift ...)

ebuchman · 2020-07-24T21:12:16Z

Great catch @Shivani912 - how'd you find this?

I think this is fine to merge but it does open a can of worms (which already existed but we may not have been paying attention to): #478

Shivani912 · 2020-07-27T10:28:51Z

Thanks for looking into this @ebuchman !

Great catch @Shivani912 - how'd you find this?

I wrote a test specifically for the case where verification must fail because of this: header_time > now + clock_drift. And upon investigating why the test failed, it wasn't too difficult to find this!

Shivani912 · 2020-07-27T10:32:06Z

Thanks everyone for your input here! Merging this now and clock synchronization issue can be followed at #478

This reverts commit db54050.

Shivani912 added 2 commits July 22, 2020 22:35

fix predicate bug

0b6d2ee

fix fmt error

d51a025

Shivani912 requested a review from romac July 22, 2020 17:23

liamsi reviewed Jul 23, 2020

View reviewed changes

light-client/src/predicates.rs Outdated Show resolved Hide resolved

liamsi reviewed Jul 23, 2020

View reviewed changes

light-client/src/predicates.rs Outdated Show resolved Hide resolved

liamsi reviewed Jul 23, 2020

View reviewed changes

light-client/src/predicates.rs Show resolved Hide resolved

liamsi reviewed Jul 23, 2020

View reviewed changes

liamsi previously approved these changes Jul 23, 2020

View reviewed changes

changes from ismail's comments + remove header time check

68188bc

Shivani912 dismissed liamsi’s stale review via 68188bc July 23, 2020 09:15

romac reviewed Jul 23, 2020

View reviewed changes

romac mentioned this pull request Jul 23, 2020

Adds more verification tests #475

Merged

5 tasks

liamsi approved these changes Jul 24, 2020

View reviewed changes

ebuchman mentioned this pull request Jul 24, 2020

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

Open

Shivani912 merged commit db54050 into master Jul 27, 2020

Shivani912 deleted the shivani/fix-predicate branch July 27, 2020 10:32

Shivani912 added a commit that referenced this pull request Jul 27, 2020

Revert "fix predicate bug (#474)"

7c737fe

This reverts commit db54050.

OStevan mentioned this pull request Aug 13, 2020

light-client: clock drift for trusting period #523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix predicate bug #474

fix predicate bug #474

Shivani912 commented Jul 22, 2020

Shivani912 commented Jul 22, 2020 •

edited

Loading

liamsi Jul 23, 2020

ebuchman Jul 24, 2020

ebuchman Jul 24, 2020

liamsi left a comment

Shivani912 commented Jul 23, 2020

romac Jul 23, 2020 •

edited

Loading

liamsi Jul 23, 2020 •

edited

Loading

Shivani912 Jul 23, 2020

ebuchman Jul 24, 2020

romac commented Jul 23, 2020

liamsi left a comment •

edited

Loading

ebuchman commented Jul 24, 2020

Shivani912 commented Jul 27, 2020

Shivani912 commented Jul 27, 2020

fix predicate bug #474

fix predicate bug #474

Conversation

Shivani912 commented Jul 22, 2020

Shivani912 commented Jul 22, 2020 • edited Loading

liamsi Jul 23, 2020

Choose a reason for hiding this comment

ebuchman Jul 24, 2020

Choose a reason for hiding this comment

ebuchman Jul 24, 2020

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Shivani912 commented Jul 23, 2020

romac Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

liamsi Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Shivani912 Jul 23, 2020

Choose a reason for hiding this comment

ebuchman Jul 24, 2020

Choose a reason for hiding this comment

romac commented Jul 23, 2020

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

ebuchman commented Jul 24, 2020

Shivani912 commented Jul 27, 2020

Shivani912 commented Jul 27, 2020

Shivani912 commented Jul 22, 2020 •

edited

Loading

romac Jul 23, 2020 •

edited

Loading

liamsi Jul 23, 2020 •

edited

Loading

liamsi left a comment •

edited

Loading