fix: change verifyClientMessage return type to bool
#862
Conversation
crodriguezvega
requested review from
mpoke,
AdityaSripal,
cwgoes and
colin-axner
as code owners
| @@ -262,7 +262,7 @@ for the given parent `ClientMessage` and the list of network messages. | |||
| The validity predicate is defined as: | |||
|
|
|||
| ```typescript | |||
| type VerifyClientMessage = (ClientMessage) => Void | |||
| type VerifyClientMessage = (ClientMessage) => bool | |||
| ``` | |||
|
|
|||
| `VerifyClientMessage` MUST throw an exception if the provided ClientMessage was not valid. | |||
There was a problem hiding this comment.
The change above is inconsistent with this line.
There was a problem hiding this comment.
I was also not completely sure about this line, but I left it like this because I saw that in line 283 we currently have checkForMisbehaviour MUST throw an exception if the provided proof of misbehaviour was not valid. and checkForMisbehaviour is also a validity predicate... Maybe both statements are inconsistent then?
There was a problem hiding this comment.
@AdityaSripal may have a better understanding of this.
| return false | ||
| } | ||
| // header timestamp must be less than or equal to the trusting period in the future. This should be resolved with an intermediate header. | ||
| if clientState.latestTimeStamp + clientState.trustingPeriod > header.timestamp { |
There was a problem hiding this comment.
Cannot find this check in the implementation.
There was a problem hiding this comment.
Good point. I also cannot find it...
There was a problem hiding this comment.
So the tendermint light client verification function effectively does the following check:
clientState.latestTimestamp + clientState.trustingPeriod <= header.Timestamp + clientState.MaxClockDrift
By the following checks:
Please double check my understanding
There was a problem hiding this comment.
The check
currentTimestamp() >= clientState.latestTimestamp + clientState.trustingPeriod
is done here.
But looks like this check
header.timestamp >= clientState.latestTimeStamp + clientState.trustingPeriod
is not in the implementation.
And the implementation has these two checks, that we don't seem to have in the spec:
https://github.com/tendermint/tendermint/blob/af2981a2f75500bc02412bad590fd26228cf5bc3/light/verifier.go#L170 (which I think would translate to header.timestamp <= clientState.latestTimestamp)
https://github.com/tendermint/tendermint/blob/af2981a2f75500bc02412bad590fd26228cf5bc3/light/verifier.go#L176 (which I think would translate to header.timestamp >= currentTimestamp() + clientState.maxClockDrift)
There was a problem hiding this comment.
https://github.com/tendermint/tendermint/blob/af2981a2f75500bc02412bad590fd26228cf5bc3/light/verifier.go#L170 (which I think would translate to header.timestamp <= clientState.latestTimestamp)
No because the trusted consensus state is not necessarily the latest one.
My point on the two links above.
The first checks the following:
now < trustedConsState.timestamp + trustingPeriod
And the second checks:
now > header.timestamp + maxClockDrift
Putting these together yields:
header.timeStamp + maxClockDrift < trustedConsensusState.timestamp + clientState.trustingPeriod
The highest trusted consensus state is the client state's latest timestamp, so we can replace this in the future update case with:
header.timeStamp + maxClockDrift < clientState.latesttimestamp + clientState.TrustingPeriod
Which would require the check:
header.timestamp >= consensusState.timestamp + clientState.TrustingPeriod - clientState.maxClockDrift
So i would recommend replacing the line with the above conditional check
…ge-should-return-a-boolean
|
I will close this for now. After discussing with @AdityaSripal he thinks that it's better if we don't return a boolean in |
closes: #837