Notes

[ebuchman]

• Wouldn't this be better if we passed the Header and Commit, rather than this rpc result type? https://github.com/tendermint/light-client/blob/develop/checkpoint.go#L28

• Do simpler checks first. Eg. check height and hash before looping through all votes https://github.com/tendermint/light-client/blob/develop/checkpoint.go#L67

• This comment: https://github.com/tendermint/light-client/blob/develop/proofs.go#L7

• Should Unmarshal really be on the Prover? https://github.com/tendermint/light-client/blob/develop/proofs.go#L10

• Error type for Height mismatch since its so common: https://github.com/tendermint/light-client/blob/develop/proofs/tx.go#L72

• The key for tx proofs is the full transaction. Should we now make it the tx.Hash() ? https://github.com/tendermint/light-client/blob/develop/proofs/tx.go#L30

• This is cool (https://github.com/tendermint/light-client/blob/develop/certifiers/errors.go)! But I'd probably rather the functions be more descriptive, like IsValidatorsChangedErr.

• CacheProvider.GetByHeight is confusing (https://github.com/tendermint/light-client/blob/master/certifiers/provider.go#L118)

• maybe we can have a cert.Update(valSet *types.ValidatorSet) so we don't need to recreate one from the list of vals eg. in https://github.com/tendermint/light-client/blob/master/certifiers/dynamic.go#L68

• https://github.com/tendermint/light-client/blob/master/certifiers/dynamic.go#L139 : No, but maybe we should verify the pubkeys are the same.

• Is this right: https://github.com/tendermint/light-client/blob/master/certifiers/dynamic.go#L149 ? It's saying that we'll accept the commit if +2/3 of the new validators consist of old validators, even if its not +2/3 of the old validators. I believe we need to insist that its signed by +2/3 of the old validators.

[ethanfrey]

The more aesthetic points (the last ones are more logic and will do next).

• Renamed it to CheckpointFromResult. The other usage is just the normal constructor Checkpoint{header, commit} and is more often used like that. Maybe I can remove it entirely?

• Re-ordered the checks

• Added tests for this. I believe this is auto-implemented by the api for AppProof, but I should do this for TxProof as well.

• There can be no global unmarshaller, as there could be many types of proofs. So, when we parse proof, we need a way to determine which one. It seemed the easiest place here, as this "Prover" is responsible for importing proofs, either from the network (via Get or Unmarshall, which is like read from file). Do you have a better idea? This also makes it easy to use the same command code for different types of proofs, just by setting the Prover implementation.

• Good idea, added error for height mismatch and returned that in all locations that check for height

• Good point. Already fixed to use tx hash

• Updated names XXX (error -> bool) to IsXXXErr (error -> bool)

(All fixed as of 657adca)

[ethanfrey]

• Added long comment about why the cache provider works like that. This behavior is tested in TestCacheGetsBestHeight as well.

• Okay, changed to accept ValidatorSet, and wrap all calls with NewValidatorSet as needed. Also stored that way as well.

• Re: dynamic certifiers verification (139), I simplified it to just check PubKey equality the second time, rather than re-evaluating the signature to save cpu cycles.

[ethanfrey]

This last point is super critical and I would like another review of it:

Is this right: https://github.com/tendermint/light-client/blob/master/certifiers/dynamic.go#L149 ? It's saying that we'll accept the commit if +2/3 of the new validators consist of old validators, even if its not +2/3 of the old validators. I believe we need to insist that its signed by +2/3 of the old validators.

My understanding.... it starts with two validator sets (old and current), and it keeps a tally for each. For each signature, if it was valid in the old set, then add the count to the oldVotes. If it was valid in old and current, then add the count to curVotes.

At the end, only accept it if the oldVotes > 2/3 of the total votes in the old set, and the curVotes is > 2/3 of the votes in the current set.

Thus, it must be signed by +2/3 of the old validators. And those signing old validators must also make up +2/3 of the current set. I think this might be too restrictive if anything, as current validators that were not in the old set are not counted for anything. Thus, if all old validators sign, but we added an equal number of votes to new validators (who also all signed), this would be rejected.

@ebuchman Please take another look at this code to make sure it does what I describe here. And whether this restriction on the current (new) validator set is good as it is, or whether it can be relaxed to just have +2/3 of old set and +2/3 of current set.

[ebuchman]

As per the unmarshaller, can we just put it on the proof itself ? is that a bad pattern?

[ebuchman]

As for the logic of the cur set - you're just saying we need to actually verify this Commit, right? I think it would be good if we separated the logic (first authenticate the new validator set, then confirm that the new set committed the block). I also think we're missing a check that the new ValidatorHash is actually in the header!

Also, I think we only need +1/3 of the old validators. Since 1/3+ can cause a fork, they can cause there to be two new validator sets for height H, each with +2/3 signatures.

So I think we should be doing something like the following:

1) Check that the new validator set hash is in the header 2) Check that +1/3 of the old validators signed the header (this authorizes the new validator set) 3) Check that +2/3 of the new validators signed the header (this authorizes the new checkpoint)

I believe that matches the description in https://github.com/tendermint/tendermint/issues/406

[ebuchman]

Actually, do we have another problem where the next validator set isn't getting signed? We could add NextValidatorSet to a header but maybe there's a better way. Thinking aloud ...

The header for block H says validator set is X. block H is committed with +2/3 from X. block H returns some diff in the EndBlock which changes the validator set to Y. so block H+1 has validator set Y. block H+1 is committed with +2/3 from Y. If the max change in a block is -1/3, then we are guaranteed that the +2/3 from Y contains +1/3 from X. Thus we'd have +1/3 from X signing on the new validator set (but not +2/3). I think thats ok.

[ethanfrey]

As per the unmarshaller, can we just put it on the proof itself ? is that a bad pattern?

When I add support for saving proofs to disk and loading them back, then I will actually have code using it. If I can add it to the Proof itself, I will do that. Maybe the same PubKey{PubKeyInner} trick we use elsewhere.

about the logic of validation. let's sit together at the retreat for 30 minutes and make sure this is all kosher.