ICS-06: Solomachine Refactor #821
Conversation
AdityaSripal
requested review from
mpoke,
cwgoes and
colin-axner
as code owners
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
| clientState.consensusState.diversifier = header.newDiversifier | ||
| clientState.consensusState.timestamp = header.timestamp | ||
| clientState.consensusState.sequence++ | ||
| function verifyClientMessage(clientMsg: ClientMessage) { |
There was a problem hiding this comment.
In this PR (assuming it gets merged) we are changing the signature of 07-tendermint's verifyClientMessage to return a boolean. Should we do a similar update here then?
There was a problem hiding this comment.
I actually prefer the error at least in implementations, since it bubbles up more information to user
Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
| ### Validity predicate | ||
|
|
||
| The solo machine client `checkValidityAndUpdateState` function checks that the currently registered public key has signed over the new public key with the correct sequence. | ||
| The solo machine client `verifyClientMessage` function checks that the currently registered public key and diversifier signed over the client message at the expected sequence. If the client message is an update, then it must be the current sequence. If the client message is misbehaviour then it must be the sequence of the misbehaviour. |
There was a problem hiding this comment.
nit on the wording here, what about something like:
The solo machine client
verifyClientMessagefunction checks that the private key associated with the current public key has signed over the client message using the current diversifier and expected sequence.
The diversifier is part of the client message sign bytes which we verify using the pub key, right?
I think it should also be clear that the private key does the actual signing
There was a problem hiding this comment.
I think the wording with public key is consistent with other documentation regarding public-private key cryptography. Reworded to clarify the diversifier part
| function updateState(clientMessage: ClientMessage) { | ||
| clientState.consensusState.publicKey = header.newPubKey | ||
| clientState.consensusState.diversifier = header.newDiversifier | ||
| clientState.consensusState.timestamp = header.timestamp | ||
| clientState.consensusState.sequence++ | ||
| clientState.consensusState.timestamp = proof.timestamp | ||
| set("clients/{identifier}/clientState", clientState) | ||
| } |
There was a problem hiding this comment.
I wonder do we need: abortTransactionUnless(clientMessage instanceof header)
Similarly we could add below for UpdateStateOnMisbehaviour: abortTransactionUnless(clientMessage instanceof misbehaviour)
There was a problem hiding this comment.
These are called earlier by VerifyClientMessage which does a switch statement on type. They should not be called by any other caller.
There was a problem hiding this comment.
ah yes! you're right, nice!
|
|
||
| All solo machine client state verification functions simply check a signature, which must be provided by the solo machine. | ||
|
|
||
| Note that value concatenation should be implemented in a state-machine-specific escaped fashion. |
There was a problem hiding this comment.
what is meant by this exactly?
There was a problem hiding this comment.
Sorry this was from before. Now that we standardized how signbytes should look it is no longer relevant
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
…nto aditya/solomachine-refactor
| Path: s2.path, | ||
| Data: s2.data | ||
| ) | ||
| // either the path or data must be different in order for the misbehaviour to be valid |
There was a problem hiding this comment.
should we note that it is not misbehaviour if only the timestamp is different?
Closes: #811
Closes: #812