ICS07: Add verification method for client update handler #1252
Conversation
…formalsystems/ibc-rs into Check-Header-And-Update-State
…formalsystems/ibc-rs into Check-Header-And-Update-State
adizere
added
A: blocked
|
Update: This is currently blocked on some changes on tm-rs side. The problem is that we’d like to merge this PR without duplicating the code from tendermint-rs. After talking with @thanethomson, the plan is:
Many thanks to @thanethomson and @cezarad for shepherding this! |
adizere
changed the title
| //freeze the client and return the installed consensus state | ||
| return Ok(( | ||
| client_state.with_set_frozen(header.height()), | ||
| consensus_state, | ||
| )); |
There was a problem hiding this comment.
We should freeze the client only if the conflicting header verifies. i.e. we need to run the checks/ validity predicates (from the new tendermint-rs verify function) below. If the header in update doesn't verify we should return an error.
| let latest_consensus_state = | ||
| match ctx.consensus_state(&client_id, client_state.latest_height) { |
There was a problem hiding this comment.
We have to get the consensus state based on the header trusted height which might not be the latest:
| let latest_consensus_state = | |
| match ctx.consensus_state(&client_id, client_state.latest_height) { | |
| let trusted_consensus_state = | |
| match ctx.consensus_state(&client_id, header.trusted_height) { |
| monotonicity_checks(latest_consensus_state, header.clone(), client_state.clone()) | ||
| .map_err(Ics02Error::tendermint_handler_error)?; |
There was a problem hiding this comment.
The monotonicity.. check should also deal with in-the-middle updates where a consensus state with higher height than the header is present. This requires a new ctx api to get the next consensus state, something like this
| monotonicity_checks(latest_consensus_state, header.clone(), client_state.clone()) | |
| .map_err(Ics02Error::tendermint_handler_error)?; | |
| let next_higher_consensus_state: Option<ConsensusState> = ctx.next_consensus_state(&client_id, header.height())?; | |
| monotonicity_checks(trusted_consensus_state, next_higher_consensus_state, header.clone(), client_state.clone()) | |
| .map_err(Ics02Error::tendermint_handler_error)?; |
| let trusted_consensus_state = match ctx.consensus_state(&client_id, header.trusted_height) { | ||
| Some(ts) => downcast!( | ||
| ts => AnyConsensusState::Tendermint | ||
| ) | ||
| .ok_or_else(|| Ics02Error::client_args_type_mismatch(ClientType::Tendermint))?, | ||
| None => { | ||
| return Err(Ics02Error::consensus_state_not_found( | ||
| client_id, | ||
| header.trusted_height, | ||
| )) | ||
| } | ||
| }; |
There was a problem hiding this comment.
this can be removed as we should use this above instead of latest_consensus_state.
|
|
||
| // check that the header's trusted validator set is | ||
| // the next_validator_set of the trusted consensus state | ||
| if Set::hash(&header.validator_set) != trusted_consensus_state.next_validators_hash { |
There was a problem hiding this comment.
This check should be done for both adjacent and non-adjacent. And you need the trusted validator instead:
| if Set::hash(&header.validator_set) != trusted_consensus_state.next_validators_hash { | |
| if Set::hash(&header.trusted_validator_set) != trusted_consensus_state.next_validators_hash { |
This check should be done in the tendermint light client API.
| // check that the validators that sign the commit of the untrusted header | ||
| // have 2/3 of the voting power of the current validator set. | ||
| if let Err(e) = voting_power_in( | ||
| &header.signed_header, | ||
| &header.validator_set, | ||
| TrustThresholdFraction::TWO_THIRDS, | ||
| ) { | ||
| return Err(Error::insufficient_voting_power(e.to_string()).into()); | ||
| } |
There was a problem hiding this comment.
same here, the check should happen for both adjacent and non-adjacent headers. In fact you have this below in the else branch.
| // check that the validators that sign the commit of the untrusted header | ||
| // have 2/3 of the voting power of the current validator set. | ||
| if let Err(e) = voting_power_in( | ||
| &header.signed_header, | ||
| &header.validator_set, | ||
| TrustThresholdFraction::TWO_THIRDS, | ||
| ) { | ||
| return Err(Error::insufficient_voting_power(e.to_string()).into()); | ||
| } | ||
| } else { | ||
| //Non-adjacent | ||
|
|
||
| //check that a subset of the trusted validator set, having 1/3 of the voting power | ||
| //signes the commit of the untrusted header | ||
| if let Err(e) = voting_power_in( | ||
| &header.signed_header, | ||
| &header.trusted_validator_set, | ||
| TrustThresholdFraction::default(), | ||
| ) { | ||
| return Err(Error::not_enough_trusted_vals_signed(e.to_string()).into()); | ||
| }; | ||
|
|
||
| // check that the validators that sign the commit of the untrusted header | ||
| // have 2/3 of the voting power of the current validator set. | ||
| if let Err(e) = voting_power_in( | ||
| &header.signed_header, | ||
| &header.validator_set, | ||
| TrustThresholdFraction::TWO_THIRDS, | ||
| ) { | ||
| return Err(Error::insufficient_voting_power(e.to_string()).into()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
All these checks should happen in the tendermint-rs light client API. It should also include the following checks:
- trusted header is not expired, i.e. it is within trusting period (note that we cannot provide a full trusted header to the API, just what we can get from the consensus state: height, time and next val hash)
- update header timestamp is not beyond
now()plus clock drift
Probably most of the monotonicity check between trusted and update headers should happen in the tendermint light client API. The only one left in this function should be between the update and next higher headers, just check the timestamps are monotonically increased and nothing else.
|
Superseded by #1321. |
Closes: #XXX
Description
This is the former PR check_client_and_update_state:
implements the ICS07 verification of the client update checkValidityAndUpdateState in https://github.com/cosmos/ibc/tree/master/spec/client/ics-007-tendermint-client
modifications in the tendermint testgen to make test pass with this extra check.
added timestamp to Anyclient: On one hand in AnyClientState we have the method
expiredbut in AnyHeader we do not have the timestamp so we cannot check in ICS02 if the update is expired. On the other hand the pseudo code of ICS02 talks only about the height.TODO!
is voting_power_inand the function that is callstotal_power_ofandnon_absent_votefrom ics07_tendermint/header.rsThe return type of
is voting_power_inis a bit simplified but is a modification easily revertible.For contributor use:
unclog.docs/) and code comments.Files changedin the Github PR explorer.