refactor(lightclient): removed the need for next stateInfo for valiadition

[mtsitrin]

DRY out the validation methods into 2:
ValidateHeaderAgainstStateInfo - validates LC updated against existing state info
ValidateStateInfoAgainstConsensusStates - validates State Info against existing consensus states

main changes:

• on HandleMsgUpdateClient

  • refactor validation to use single state info, instead of checking state info for H+1

• on SetCanonicalClient

  • changed iteration key: instead of iterating all the consensus states, we iterate over state infos
  • each state info is validated

[danwt]

The cleanups are great, but the bit about iterating descending doesn't make sense.

dymension/x/lightclient/keeper/canonical_client.go

Lines 97 to 125 in 1deed9f

	IterateConsensusStateDescending(csStore, func(h exported.Height) bool {
	// skip future heights
	if h.GetRevisionHeight() >= maxHeight {
	return false
	}
	// iterate until we pass the fraud height
	if h.GetRevisionHeight() < minHeight {
	return true // break
	}
	consensusState, ok := k.ibcClientKeeper.GetClientConsensusState(ctx, clientID, h)
	if !ok {
	return false
	}
	tmConsensusState, ok := consensusState.(*ibctm.ConsensusState)
	if !ok {
	return false
	}
	err = k.ValidateUpdatePessimistically(ctx, sInfo, tmConsensusState, h.GetRevisionHeight())
	if err != nil {
	err = errorsmod.Wrapf(err, "validate pessimistic h: %d", h.GetRevisionHeight())
	return true // break
	}
	atLeastOneMatch = true
	return true // break
	})

You're only checking the consensus states with heights in the state info, which isn't right. I'm sorry I should've thought about it more when you initially asked me on slack.

Now it's not secure; why? Because there can be invalid consensus states below the ones you check, which are opening a security hole by having a wrong nextValidatorsHash.

We need to iterate through all the consensus states and check that they're valid. In fact, we just need to check that there is a valid prefix . The reason it was implemented the way it was to begin with was because it's just simpler: if we check all up to and including the ones in the state info, then (in the previous design where a mismatch in state update would automatically trigger fraud handler) then we would be guaranteed to catch a wrong one.

I would really suggest to just keep it as it was before. You can still change it to be an iterator though (I think why Spoorthi made it fetch all before was because iterator was not publicly exposed).

The rest of the PR is good

[codecov]

Codecov Report

Attention: Patch coverage is 91.75258% with 8 lines in your changes missing coverage. Please review.

Project coverage is 22.69%. Comparing base (a308fd0) to head (4d50d9c).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
x/lightclient/keeper/canonical_client.go	90.69%	3 Missing and 1 partial ⚠️
x/lightclient/keeper/hook_listener.go	92.59%	1 Missing and 1 partial ⚠️
x/lightclient/ante/ibc_msg_update_client.go	90.00%	1 Missing ⚠️
x/rollapp/types/state_info.go	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1467      +/-   ##
==========================================
- Coverage   22.69%   22.69%   -0.01%     
==========================================
  Files         575      575              
  Lines      124579   124516      -63     
==========================================
- Hits        28273    28257      -16     
+ Misses      92528    92494      -34     
+ Partials     3778     3765      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.