Light client time followup

[ebuchman]

As per #110 (comment):

• ensure that time in headers is monotonic, ie. untrusted_header.bft_time() > trusted_header.bft_time() in verify_single (see Valid TrustThresholdFraction and further simplify tests code #142)
• check that the blockchain is not ahead of our local clock, ie. untrusted_header.bft_time() < now + X for every header we fetch

In theory, if we check that time is monotonic, we only need to check that the highest height we're trying to sync to is not ahead of our local clock (ie. we don't need to check this for every header, since all the other headers are less than the highest one). However, if we only check for monotonic time after we've trusted a pivot header, its possible we would trust a pivot header who's time is too far ahead of our local clock, and hence might be outside the trusting period.

[liamsi]

Note to myself: the spec calls x clockdrift and it is passed in as a parameter: https://github.com/tendermint/spec/blob/master/spec/consensus/light-client/verification.md#functions-1

[liamsi]

Also, isn't there a weird edge case, where this method passes:

func isWithinTrustedPeriod(header Header,
                           trustingPeriod Duration,
                           now Time) bool {

    return header.Time + trustedPeriod > now
}

but only because the trusted header has header.Time > now? This shouldn't happen but shouldn't this still be covered by the spec too?
Note there is a check in the current Rust code:
https://github.com/interchainio/tendermint-rs/blob/a6f7c64e8214d5fbf32adee5ee3b6cbac09ee9da/tendermint/src/lite/verifier.rs#L43-L49
The name DurationOutOfRange stems from a time where we where using the duration_since method from SystemTime.

[ebuchman]

Yes but that's why we need to ensure the header is not ahead of our local clock! ie untrusted_header.bft_time() < now + X

[liamsi]

Ah, I think I see how this works: we make sure untrusted_header.bft_time() < now + X but also ensure trusted_header.bft_time() < untrusted_header.bft_time() < now + X. So we also have made sure trusted_header.bft_time() < now (or more precisely now+X) and the case I mentioned would have been caught by this without an extra check that trusted_header.bft_time() <= now.

[brapse]

This has been addressed and landed in master:

tendermint-rs/light-client/src/predicates.rs

Lines 79 to 120 in 740cb66

	fn is_within_trust_period(
	&self,
	header: &Header,
	trusting_period: Duration,
	clock_drift: Duration,
	now: Time,
	) -> Result<(), VerificationError> {
	ensure!(
	header.time < now + clock_drift,
	VerificationError::HeaderFromTheFuture {
	header_time: header.time,
	now
	}
	);
	let expires_at = header.time + trusting_period;
	ensure!(
	expires_at > now,
	VerificationError::NotWithinTrustPeriod {
	at: expires_at,
	now,
	}
	);
	Ok(())
	}
	fn is_monotonic_bft_time(
	&self,
	untrusted_header: &Header,
	trusted_header: &Header,
	) -> Result<(), VerificationError> {
	ensure!(
	untrusted_header.time > trusted_header.time,
	VerificationError::NonMonotonicBftTime {
	header_bft_time: untrusted_header.time,
	trusted_header_bft_time: trusted_header.time,
	}
	);
	Ok(())
	}