lite: light client MVP #3989
lite: light client MVP #3989
Conversation
make trustLevel an option
Codecov Report
@@ Coverage Diff @@
## master #3989 +/- ##
==========================================
- Coverage 67.23% 66.71% -0.53%
==========================================
Files 223 223
Lines 18964 19159 +195
==========================================
+ Hits 12751 12782 +31
- Misses 5255 5391 +136
- Partials 958 986 +28
|
|
I feel it'll be great if we can have error types everywhere instead of error strings. That will help standardize error handling and so we can use the same for Tendermint in Rust. Let me know :) |
lite2/client.go
Outdated
| alternatives []provider.Provider | ||
|
|
||
| // Where trusted headers are stored. | ||
| trustedStore store.Store |
There was a problem hiding this comment.
Could you comment on some properties of the store? e.g. stores all headers that have been verified. At the time of verification these headers were within the trusting period. I believe headers are not removed from the store as they become "old", i.e. get out of the trusting period.
Maybe some comments on why and how they are useful after becoming old.
There was a problem hiding this comment.
I believe headers are not removed from the store as they become "old", i.e. get out of the trusting period.
This is a TODO. We need to only store the amount of headers necessary for fork-accountability evidence. I don't know the exact number
There was a problem hiding this comment.
I will add a comment once the requirements are known and implementation is adjusted accordingly
There was a problem hiding this comment.
we will need to store all headers that are within trusted period
| return err | ||
| } | ||
|
|
||
| // NOTE: Verify func will check if it's expired or not. |
There was a problem hiding this comment.
True but maybe Client initialization should fail (? not sure myself)
| func (c *Client) VerifyHeaderAtHeight(height int64, now time.Time) error { | ||
| if c.trustedHeader.Height >= height { | ||
| return errors.Errorf("height #%d is already trusted (last: #%d)", height, c.trustedHeader.Height) | ||
| } |
There was a problem hiding this comment.
Is it true that it is "already trusted"? Maybe this is an older header that was never verified., (skipping case). Should we lookup the store instead?
Let's say we start with height A (trusted) and we successfully verify for height Z. So we store headers A and Z.
Now we get a request for height Q with A < Q < Z.
Is this a valid request? Or is there a requirement that VerifyHeaderAtHeight() must be called with increasing heights for both sequential and skipping case?
If it is a valid request then we should not trust Q automatically just because we trusted Z. It is possible that a verification of Q against A will fail (e.g. less than 1/3rds common validators between A and Q signed even though there were more than 1/3rds common between A and Z -> Note*)
Also, a proper solution for this "backward" case should check that A is still within trusting period and if not perform backwards-sequential verification (i.e. h, h-1, h-2, etc.) or return error if backwards-sequential is not supported.
So to summarize, if we do allow for random (not necessarily increasing in height) client header verification for skipping case then some changes are needed. If we do not, then maybe clarify this someplace.
Note* - if validators are required to change address when re-bonding then probably this is not an issue.
There was a problem hiding this comment.
Even in bisection I don't think we should ever end up looking up a header that is older than one we've already decided to trust. We should never start bisection from a 'start header' that's before one we've already decided to trust, so presumably this won't happen (but good question & merits a comment).
There was a problem hiding this comment.
Ok. We do have (potentially) many trusted headers in the store. Any new bisection call for H could start with the highest trusted header <= H. I think the code can support this with minimal changes if we ever need it.
But at least, the error message we currently have height #%d is already trusted.. should be changed.
There was a problem hiding this comment.
We do have (potentially) many trusted headers in the store. Any new bisection call for H could start with the highest trusted header <= H.
I think it only makes sense with a different provider. Consider this:
a) light client performs bisection and decides to trust H#100
b) later it receives an evidence of attach (lunatic) and learns that H#100 is fabricated
c) light client exits (we should probably remove all "trusted" headers from DB at this point)
c) operator resets it with new primary source and new trusted header
- Block no longer contains BlockMeta - Validators now accept two additional params: page and perPage
There was a problem hiding this comment.
Updated / responded to comments. I think the VerifyXYZ logic is solid, less sure about the Tendermint-side state management. Do you anticipate merging this soon @melekes or do you prefer to wait for more reviews of the state management? Either is fine, in the latter case I suppose we can target the IBC code on this branch (though a bit annoying).
| type Option func(*Client) | ||
|
|
||
| // SequentialVerification option configures the light client to sequentially | ||
| // check the headers. Note this is much slower than SkippingVerification, |
There was a problem hiding this comment.
Yes (every header, in ascending height order).
| // } | ||
|
|
||
| func (c *Client) initializeWithTrustOptions(options TrustOptions) error { | ||
| h, err := c.primary.SignedHeader(options.Height) |
There was a problem hiding this comment.
Is initializeWithTrustOptions intended to be called multiple times?
| func (c *Client) VerifyHeaderAtHeight(height int64, now time.Time) error { | ||
| if c.trustedHeader.Height >= height { | ||
| return errors.Errorf("height #%d is already trusted (last: #%d)", height, c.trustedHeader.Height) | ||
| } |
There was a problem hiding this comment.
Even in bisection I don't think we should ever end up looking up a header that is older than one we've already decided to trust. We should never start bisection from a 'start header' that's before one we've already decided to trust, so presumably this won't happen (but good question & merits a comment).
in bisection func + replace TODO with a comment #3989 (comment)
because that way we avoid DB call + add godoc comments + check if there are no headers yet in AutoClient #3989 (review)
* rename adjusted to adjacent Refs #3989 (comment) * rename ErrTooMuchChange to ErrNotEnoughVotingPowerSigned Refs #3989 (comment) * verify commit is properly signed * remove no longer trusted headers * restore trustedHeader and trustedNextVals * check trustedHeader using options Refs #4209 (comment) * use correct var when checking if headers are adjacent in bisection func + replace TODO with a comment #3989 (comment) * return header in VerifyHeaderAtHeight because that way we avoid DB call + add godoc comments + check if there are no headers yet in AutoClient #3989 (review) * TestVerifyAdjacentHeaders: add 2 more test-cases + add TestVerifyReturnsErrorIfTrustLevelIsInvalid * lite: avoid overflow when parsing key in db store! * lite: rename AutoClient#Err to Errs * lite: add a test for AutoClient * lite: fix keyPattern and call itr.Next in db store * lite: add two tests for db store * lite: add TestClientRemovesNoLongerTrustedHeaders * lite: test Client#Cleanup * lite: test restoring trustedHeader #4209 (comment) * lite: comment out unused code in test_helpers * fix TestVerifyReturnsErrorIfTrustLevelIsInvalid after merge * change defaultRemoveNoLongerTrustedHeadersPeriod and add docs * write more doc * lite: uncomment testable examples * use stdlog.Fatal to stop AutoClient tests * make lll linter happy * separate errors for 2 cases - the validator set of a skipped header cannot be trusted, i.e. <1/3rd of h1 validator set has signed (new error, something like ErrNewValSetCantBeTrusted) - the validator set is trusted but < 2/3rds has signed (ErrNewHeaderCantBeTrusted) #4209 (comment) * remove all headers (even the last one) that are outside of the trusting period. By doing this, we avoid checking the trustedHeader's hash in checkTrustedHeaderUsingOptions (case #1). #4209 (comment) * explain restoreTrustedHeaderAndNextVals better #4209 (comment) * add ConfirmationFunction option for optionally prompting for user input Y/n before removing headers Refs #4209 (comment) * make cleaning optional #4209 (comment) * return error when user refused to remove headers * check for double votes in VerifyCommitTrusting * leave only ErrNewValSetCantBeTrusted error to differenciate between h2Vals.VerifyCommit and h1NextVals.VerifyCommitTrusting * fix example tests * remove unnecessary if condition #4209 (comment) It will be handled by the above switch. * verifyCommitBasic does not depend on vals Co-authored-by: Marko <marbar3778@yahoo.com>
Refs #1771
ADR: https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-044-lite-client-with-weak-subjectivity.md (incomplete - #2133)