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.

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

Bug in predicate - is_within_trust_period #473

Closed
Shivani912 opened this issue Jul 22, 2020 · 2 comments · Fixed by #474
Closed

Bug in predicate - is_within_trust_period #473

Shivani912 opened this issue Jul 22, 2020 · 2 comments · Fixed by #474
Labels
bug Something isn't working light-client Issues/features which involve the light client

Comments

@Shivani912
Copy link
Contributor

Context: is_within_trust_period predicate is intended to check if the latest trusted header is still within trusting period while we verify a new, untrusted header.

We also need to perform another check for header time that it's "header from future", that means, the untrusted header that we're verifying must not have a time that is beyond our current time (= now + clock_drift)

Problem: Code below for the is_within_trust_period predicate includes the "header from future" check.

This is a bug because the header we have there is the trusted one instead of the untrusted one and so the condition will always evaluate to true.

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 { expires_at, now }
);
Ok(())
}

Fix: Will be clearer if we have a separate predicate for "header from future" check.

@Shivani912 Shivani912 added bug Something isn't working light-client Issues/features which involve the light client labels Jul 22, 2020
@Shivani912
Copy link
Contributor Author

Should I submit a PR for this?

@romac
Copy link
Member

romac commented Jul 22, 2020

Yes please! Great catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working light-client Issues/features which involve the light client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants