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

Update misbehaviour handling to work with IBC 7 #1400

Closed
Tracked by #732
sainoe opened this issue Nov 9, 2023 · 2 comments · Fixed by #1401
Closed
Tracked by #732

Update misbehaviour handling to work with IBC 7 #1400

sainoe opened this issue Nov 9, 2023 · 2 comments · Fixed by #1401
Assignees

Comments

@sainoe
Copy link
Contributor

sainoe commented Nov 9, 2023

Problem

The current implementation of the cryptographic equivocation feature utilizes an IBC-Go method to validate consumer client Misbehaviour. Specifically, in IBC-Go v4, the CheckMisbehaviourAndUpdateState method confirms that a given Misbehaviour satisfies the following three conditions:

  • The headers of the misbehaviour posses different BlockIDs for the same block height.
  • The trusted state in the headers is consistent with valid consensus states.
  • The commit in the headers is signed by at least the TrustLevel of their trusted validator set.

Throughout the migration from IBC-Go v6 to v7, the ClientState interface has undergone some changes, including the split of the CheckMisbehaviourAndUpdateState into four distinct methods ref. IBC-Go doc.

As a result, the upgrade of the cryptographic equivocation feature from IBC-Go v4 to IBC-Go v7 necessitates adaptation to these changes. The adjustment should ensure that the same three conditions mentioned above are enforced for handling Misbehaviours.

Closing criteria

Update the Misbehaviour handling to perform the same verification that in CheckMisbehaviourAndUpdateState.

Problem details

@github-project-automation github-project-automation bot moved this to 🩹 F1: Triage in Cosmos Hub Nov 9, 2023
@sainoe sainoe self-assigned this Nov 9, 2023
@sainoe sainoe moved this from 🩹 F1: Triage to 👀 F3: InReview in Cosmos Hub Nov 9, 2023
@sainoe sainoe changed the title Check how misbehaviour can be verified with [IBC-Go 7 ClientState interface changes](https://ibc.cosmos.network/v7.3.x/migrations/v6-to-v7#clientstate-interface-changes) Update misbehaviour handling to work with IBC 7 Nov 9, 2023
@sainoe sainoe linked a pull request Nov 9, 2023 that will close this issue
21 tasks
@insumity
Copy link
Contributor

What you describe sounds good to me.

The way I see this, is that CheckMisbehaviourAndUpdateState in v4.4.2 consists of the following 2 chunks:

  1. Check if blockIDs are different if the headers contain bogus (i.e., wrong ordering) BFT times:
if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) {
    blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID)
    [...]
    blockID2, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header2.SignedHeader.Commit.BlockID)
    [...]
    if bytes.Equal(blockID1.Hash, blockID2.Hash) {
        return ...
    }
} else {
    if tmMisbehaviour.Header1.SignedHeader.Header.Time.After(tmMisbehaviour.Header2.SignedHeader.Header.Time) {
        return ...
    }
}

In IBC v7.3.1, the same functionality is provided by CheckForMisbehaviour.

  1. Compare against the trusted consensus states and verify signatures using the trusted validator set:
tmConsensusState1, err := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header1.TrustedHeight)
[..]

tmConsensusState2, err := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header2.TrustedHeight)
[...]

if err := checkMisbehaviourHeader(
    &cs, tmConsensusState1, tmMisbehaviour.Header1, ctx.BlockTime(),
); err != nil {
    return ...
}
if err := checkMisbehaviourHeader(
    &cs, tmConsensusState2, tmMisbehaviour.Header2, ctx.BlockTime(),
); err != nil {
    return ...
}

In IBC v7.3.1, the same functionality is provided by VerifyClientMessage when called with a Misbehaviour message because VerifyClientMessage calls verifyMisbehaviour that performs the above.

Therefore, we can replace IBC v4's CheckMisbehaviourAndUpdateState with a call to CheckForMisbehaviour and a call to VerifyClientMessage in IBC v7 as follows:

clientMessage := transform MsgSubmitConsumerMisbehaviour to a clientMessage with a misbehaviour
isMisbehaviour := clientState.CheckForMisbehaviour(ctx, k.cdc, clientStore, clientMessage)
err := clientstate.VerifyClientMessage(ctx, cdc, clientStore, clientMessage)
return isMisbehaviour && err != nil

@sainoe
Copy link
Contributor Author

sainoe commented Nov 14, 2023

Thanks @insumity! Glad to see that we came to the same conclusion for this refactoring to IBC v7.
The #1401 updates the feat/ics-misbehaviour-handling branch accordingly.

@sainoe sainoe closed this as completed Nov 16, 2023
@github-project-automation github-project-automation bot moved this from 👀 F3: InReview to 👍 F4: Assessment in Cosmos Hub Nov 16, 2023
@mpoke mpoke moved this from 👍 F4: Assessment to ✅ Done in Cosmos Hub Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants