Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split CheckHeaderAndUpdateState into 4 functions #668

Closed
6 tasks
colin-axner opened this issue Dec 23, 2021 · 0 comments
Closed
6 tasks

Split CheckHeaderAndUpdateState into 4 functions #668

colin-axner opened this issue Dec 23, 2021 · 0 comments

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Dec 23, 2021

Summary

The CheckHeaderAndUpdateState is currently overloaded in functionality. It can be easy for light-client developers to unknowingly allow bugs. We should split the functionality up such that light client developers are explicitly told what they need to account for

Problem Definition

CheckHeaderAndUpdateState requires the light client to:

  • verify the header
  • account for duplicate headers
  • automatically detect misbehaviour
  • return updated client and consensus states

Detecting misbehaviour and accounting for duplicate headers are easy to overlook, but are vital to the security of a light client implementation.

## Proposal

Split CheckHeaderAndUpdateState into:
- VerifyHeader
- CheckHeaderForMisbehaviour
- UpdateStateFromHeader

Updated proposal

ref

Split CheckHeaderAndUpdateState into 4 functions:

  • VerifyHeader
  • CheckForMisbehaviour
  • UpdateStateOnMisbehaviour
  • UpdateState

VerifyHeader: checks header signatures, ensuring everything is constructed correctly and is valid. Should be similar to 07-tendermint checkValidity. Should only return an error

CheckForMisbehaviour: checks to see if a header is evidence of misbehaviour. In 07-tendermint, it'd be these checks. Should return a boolean (foundMisbehaviour)

UpdateStateOnMisbehaviour: freeze the client and updates its state accordingly

UpdateState: Perform a regular update (may do a no-op for duplicate updates)

Splitting PR Recommendation

PR for each light client for each function

Make a PRs for each light client

Replace CheckHeaderAndUpdateState with new functions

Add the 4 new functions to the ClientState interface, remove CheckHeaderAndUpdateState and update 02-client code to use the new functions.

Code:

    if err := clientState.VerifyHeader(header); err != nil {
        return err
    }
    
    foundMisbehaviour := clientState.CheckForMisbehaviour(header)
    if foundMisbehaviour {
        clientState.UpdateStateOnMisbehaviour(header)
        // emit misbehaviour event
        return 
    }
    
    clientState.UpdateState(header) // expects no-op on duplicate header
    // emit update event
    return
}

By splitting the PRs up this way, reviewers don't need to switch context on light client funcitonality or light client vs 02-client.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added the needs discussion Issues that need discussion before they can be worked on label Dec 23, 2021
@colin-axner colin-axner moved this to Todo in ibc-go Feb 3, 2022
@colin-axner colin-axner changed the title Split CheckHeaderAndUpdateState into 3 functions Split CheckHeaderAndUpdateState into 4 functions Feb 7, 2022
@colin-axner colin-axner removed the needs discussion Issues that need discussion before they can be worked on label Feb 7, 2022
@colin-axner colin-axner moved this from Todo to On hold in ibc-go Feb 7, 2022
@colin-axner colin-axner moved this from On hold to Todo in ibc-go Mar 29, 2022
@crodriguezvega crodriguezvega added this to the 02-client refactor milestone Mar 30, 2022
@colin-axner colin-axner moved this from Todo to In review in ibc-go Mar 31, 2022
Repository owner moved this from In review to Done in ibc-go Apr 1, 2022
@crodriguezvega crodriguezvega moved this from Todo to Done in ibc-go Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 🥳
Development

No branches or pull requests

3 participants