Skip to content
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.

Sign up for GitHub

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

fix predicate bug #474

Merged
merged 3 commits into from
Jul 27, 2020
Merged

fix predicate bug #474

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b6d2ee
fix predicate bug
Shivani912 Jul 22, 2020
d51a025
fix fmt error
Shivani912 Jul 22, 2020
68188bc
changes from ismail's comments + remove header time check
Shivani912 Jul 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions light-client/src/predicates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,28 +95,37 @@ pub trait VerificationPredicates: Send {
Ok(())
}

/// Check that the given header is within the trusting period, adjusting for clock drift.
/// Check that the trusted header is within the trusting period, adjusting for clock drift.
fn is_within_trust_period(
&self,
header: &Header,
trusted_header: &Header,
trusting_period: Duration,
now: Time,
) -> Result<(), VerificationError> {
let expires_at = trusted_header.time + trusting_period;
ensure!(
expires_at > now,
Copy link
Member

@romac romac Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@liamsi liamsi Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

VerificationError::NotWithinTrustPeriod { expires_at, now }
);

Ok(())
}

/// Check that the untrusted header is from past.
fn is_header_from_past(
&self,
untrusted_header: &Header,
clock_drift: Duration,
now: Time,
) -> Result<(), VerificationError> {
ensure!(
header.time < now + clock_drift,
untrusted_header.time < now + clock_drift,
VerificationError::HeaderFromTheFuture {
header_time: header.time,
header_time: untrusted_header.time,
now
}
);
Shivani912 marked this conversation as resolved.
Show resolved Hide resolved

let expires_at = header.time + trusting_period;
ensure!(
expires_at > now,
VerificationError::NotWithinTrustPeriod { expires_at, now }
);

Ok(())
}

Expand Down Expand Up @@ -227,10 +236,12 @@ pub fn verify(
vp.is_within_trust_period(
&trusted.signed_header.header,
options.trusting_period,
options.clock_drift,
now,
)?;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I gave it its own issue: #478


// Ensure the header validator hashes match the given validators
vp.validator_sets_match(&untrusted, &*hasher)?;

Expand Down