feat!: Cryptographic verification of equivocation #1287

mpoke · 2023-09-12T11:04:20Z

Description

Closes: #732

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

* define msg to submit misbehaviour to provider implement msg handling logic e2e test msg handling logic * wip: get byzantine validators in misbehavioiur handling * add tx handler * format HandleConsumerMisbehaviour * add tx handler * add debugging stuff * Add misbehaviour handler * create message for consumer double voting evidence * add DRAFT double vote handler * Add cli cmd for submit consumer double voting * Add double-vote handler * add last update * fix jailing * pass first jailing integration test * format tests * doc * save * update e2e tests' * fix typo and improve docs * remove unwanted tm evidence protofile * fix typos * update submit-consumer-misbehaviour cli description * check that header1 and header2 have the same TrustedValidators * feat: add e2e tests for ICS misbehaviour (#1118) * remove unwanted changes * fix hermes config with assigned key * revert unwanted changes * revert local setup * remove log file * typo * update doc * update ICS misbehaviour test * update ICS misbehaviour test * revert mixed commits * add doc * lint * update to handle only equivocations * improve doc * update doc * update E2E tests comment * optimize signatures check * doc * update e2e tests * linter * remove todo * Feat: avoid race condition in ICS misbehaviour handling (#1148) * remove unwanted changes * fix hermes config with assigned key * revert unwanted changes * revert local setup * remove log file * typo * update doc * update ICS misbehaviour test * update ICS misbehaviour test * revert mixed commits * update ICS misbehaviour test * update ICS misbehaviour test * Add test for MsgSubmitConsumerMisbehaviour parsing * fix linter * save progress * add CheckMisbehaviourAndUpdateState * update integration tests * typo * remove e2e tests from another PRs * cleaning' * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * update integration tests * save * save * nits * remove todo * lint * Update x/ccv/provider/keeper/misbehaviour.go --------- Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> Co-authored-by: Marius Poke <marius.poke@posteo.de> * Update x/ccv/provider/client/cli/tx.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Update x/ccv/provider/client/cli/tx.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * add attributes to EventTypeSubmitConsumerMisbehaviour * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * apply review suggestions * fix docstring * Update x/ccv/provider/keeper/misbehaviour.go Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * fix link * apply review suggestions * update docstring --------- Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> Co-authored-by: Marius Poke <marius.poke@posteo.de>

* update e2e tests * update the chain halt assertion

) * remove interface * improve comment * update godoc * address last comments

* create new endpoint for consumer double voting * add first draft handling logic * first iteration of double voting * draft first mem test * error handling * refactor * add unit test of double voting verification * remove evidence age checks * document * doc * protogen * reformat double voting handling * logger nit * nits * check evidence age duration * move verify double voting evidence to ut * fix nit * nits * fix e2e tests * improve double vote testing coverage * remove TODO * lint * add UT for JailAndTombstoneValidator * nits * nits * remove tombstoning and evidence age check * lint * typo * improve godoc

* fix double voting cli * fix bug double signing handler * godoc * nits * revert wrong push of lasts commits

…1254) * fix double voting cli * fix bug double signing handler * godoc * nits * lint * nit

…bleVoting` msg (#1264) * verify dv evidence using malicious validator pubkey in infraction block header * nits * nits

) * save changes * fix hermes config * fist successful run * nit * nits * nits * doc and nits * lint

* fix double voting cli * add double-signing e2e test * refortmat e2e double voting test * godoc, revert unwanted changes * nit * verify dv evidence using malicious validator pubkey in infraction block header * save changes * fix hermes config * fist successful run * nit * nits * nits * doc and nits * lint * refactor * typo * change hermes docker image * nits * Update tests/e2e/steps.go Co-authored-by: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> * address PR comments * nits --------- Co-authored-by: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com>

sainoe · 2023-09-13T09:18:40Z

proto/interchain_security/ccv/provider/v1/tx.proto

+// observed on a consumer chain
+// Note that the misbheaviour' headers must contain the same trusted states
+message MsgSubmitConsumerMisbehaviour {


explain it's about light client attacks

sainoe · 2023-09-13T09:19:19Z

proto/interchain_security/ccv/provider/v1/tx.proto

+// MsgSubmitConsumerDoubleVoting defines a message that reports an equivocation
+// observed on a consumer chain
+message MsgSubmitConsumerDoubleVoting {


emphasize it's about double signing attacks

sainoe · 2023-09-13T09:21:57Z

tests/e2e/actions.go

@@ -133,7 +133,7 @@ func (tr TestRun) startChain(
 	cmd := exec.Command("docker", "exec", tr.containerConfig.instanceName, "/bin/bash",
 		"/testnet-scripts/start-chain.sh", chainConfig.binaryName, string(vals),
 		string(chainConfig.chainId), chainConfig.ipPrefix, genesisChanges,
-		fmt.Sprint(action.skipGentx),


check that the sovereign tests are not broken @MSalopek

sainoe · 2023-09-13T09:24:20Z

tests/e2e/actions.go

+
+	saveMnemonicCommand := fmt.Sprintf(`echo '%s' > %s`, mnemonic, "/root/.hermes/mnemonic.txt")
+	fmt.Println("Add to hermes", action.validator)
+	fmt.Println(mnemonic)


remove it or log it

sainoe · 2023-09-13T09:29:01Z

tests/e2e/actions.go

+// which detects evidences committed to the blocks of a consumer chain.
+// Each infraction detected is reported to the provider chain using
+// either a SubmitConsumerDoubleVoting or a SubmitConsumerMisbehaviour message.
+type detectConsumerEvidenceAction struct {


rename to startConsumerEvidenceDetectorAction

sainoe · 2023-09-13T09:29:56Z

tests/e2e/actions.go

+) {
+	chainConfig := tr.chainConfigs[action.chain]
+	//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments.
+	bz, err := exec.Command("docker", "exec", "-d", tr.containerConfig.instanceName,


add a comment that it will keep running on the background

p-offtermatt

Thanks for the review session, helped a lot to get context. Some more comments. I will approve because the core logic we went over looks good and we want this to be merged asap

p-offtermatt · 2023-09-13T10:24:21Z

Dockerfile

@@ -28,7 +28,7 @@ RUN go mod tidy
 RUN make install

 # Get Hermes build
-FROM ghcr.io/informalsystems/hermes:1.4.1 AS hermes-builder
+FROM otacrew/hermes-ics:latest AS hermes-builder


maybe change this from latest to a specific tag

p-offtermatt · 2023-09-13T10:25:06Z

tests/e2e/actions.go

@@ -521,7 +524,7 @@ func (tr TestRun) voteGovProposal(
 	}

 	wg.Wait()
-	time.Sleep(time.Duration(tr.chainConfigs[action.chain].votingWaitTime) * time.Second)
+	time.Sleep((time.Duration(tr.chainConfigs[action.chain].votingWaitTime)) * time.Second)


nit: extra brackets

p-offtermatt · 2023-09-13T10:26:34Z

tests/e2e/actions_consumer_misbehaviour.go

+	"time"
+)
+
+type forkConsumerChainAction struct {


Could you comment somewhere what we can expect when calling this?

p-offtermatt · 2023-09-13T10:34:03Z

x/ccv/provider/client/cli/tx.go

+
+func NewSubmitConsumerDoubleVotingCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "submit-consumer-double-voting [evidence] [infraction_header]",


Please add a long version here, like for the misbehaviour

p-offtermatt · 2023-09-13T10:36:35Z

x/ccv/provider/types/codec.go

@@ -36,6 +36,11 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
 		&EquivocationProposal{},
 	)

+	registry.RegisterImplementations(
+		(*sdk.Msg)(nil),


Pattern matching: Should the MsgSubmitConsumerDoubleVoting also have something like this?

insumity · 2023-09-13T10:42:22Z

x/ccv/provider/keeper/double_vote.go

+	// Note that since we're only jailing validators for double voting on a consumer chain,
+	// the age of the evidence is irrelevant and therefore isn't checked.
+
+	// TODO: check the age of the evidence once we slash


Remove this TODO (?)
Even the current approach we intend for following on slashing won't check the age.

insumity · 2023-09-13T10:43:20Z

x/ccv/provider/keeper/double_vote.go

+			evidence.VoteB.Height, evidence.VoteB.Round, evidence.VoteB.Type)
+	}
+
+	// Address must be the same


nit: s/Address/addresses

insumity · 2023-09-13T10:51:49Z

x/ccv/provider/keeper/misbehaviour.go

+)
+
+// HandleConsumerMisbehaviour checks if the given IBC misbehaviour corresponds to an equivocation light client attack,
+// and in this case, jails and tombstones the Byzantine validators


remove "tombstones"

insumity · 2023-09-13T10:52:25Z

x/ccv/provider/keeper/misbehaviour.go

+
+	// Since the misbehaviour packet was received within the trusting period
+	// w.r.t to the trusted consensus states the infraction age
+	// isn't too old. see ibc-go/modules/light-clients/07-tendermint/types/misbehaviour_handle.go


nit: do we want to refer to a specific ibc-go version here?
At a later point in misbehaviour.go, you have a full link // see https://github.com/cosmos/ibc-go/blob/v4.2.0/modules/light-clients/07-tendermint/types/misbehaviour_handle.go#L120 instead of just the suffix. We can remove the https://github.com/ below to be consistent.

insumity · 2023-09-13T10:52:37Z

x/ccv/provider/keeper/misbehaviour.go

+
+	provAddrs := make([]types.ProviderConsAddress, len(byzantineValidators))
+
+	// jail and tombstone the Byzantine validators


remove "tombstone"

insumity · 2023-09-13T11:06:51Z

x/ccv/provider/keeper/misbehaviour.go

+	}
+
+	// CheckMisbehaviourAndUpdateState verifies the misbehaviour against the trusted consensus states
+	// but does NOT update the light client state.


From what I see here the client state might be updated in the sense of getting frozen.
Is that not the case?

Why do we care that the light client is not having its state updated?

Line cs.FrozenHeight = FrozenHeight updates the cs variable in memory which isn't persisted to the store in CheckMisbehaviourAndUpdateState.

We care because the IBC misbehaviour handler is already freezing the client.
Please check #1147.

insumity · 2023-09-13T11:08:35Z

x/ccv/provider/keeper/msg_server.go

+func (k msgServer) SubmitConsumerMisbehaviour(goCtx context.Context, msg *types.MsgSubmitConsumerMisbehaviour) (*types.MsgSubmitConsumerMisbehaviourResponse, error) {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+	if err := k.Keeper.HandleConsumerMisbehaviour(ctx, *msg.Misbehaviour); err != nil {
+		return &types.MsgSubmitConsumerMisbehaviourResponse{}, err


nit: return nil, err to be consistent with what we do in SubmitConsumerDoubleVoting

insumity · 2023-09-13T11:36:57Z

x/ccv/provider/types/msg.go

-var _ sdk.Msg = &MsgAssignConsumerKey{}
+var (
+	_ sdk.Msg = &MsgAssignConsumerKey{}
+	_ sdk.Msg = &MsgSubmitConsumerMisbehaviour{}


similar to Philip's comment above ... do we need MsgSubmitConsumerDoubleVoting here?

insumity · 2023-09-13T11:38:55Z

x/ccv/provider/types/msg.go

+		return fmt.Errorf("signed header in infraction block header cannot be nil")
+	}
+
+	if msg.InfractionBlockHeader.SignedHeader.Header == nil {


Do we need to check InfractionBlockHeader.ValidatorSet here as well?

insumity · 2023-09-13T11:40:09Z

x/ccv/provider/keeper/msg_server.go

+
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			ccvtypes.EventTypeSubmitConsumerMisbehaviour,


Do we need a different type for double voting events?

insumity · 2023-09-13T11:43:07Z

x/ccv/types/errors.go

@@ -25,4 +25,5 @@ var (
 	ErrClientNotFound           = errorsmod.Register(ModuleName, 18, "client not found")
 	ErrDuplicateConsumerChain   = errorsmod.Register(ModuleName, 19, "consumer chain already exists")
 	ErrConsumerChainNotFound    = errorsmod.Register(ModuleName, 20, "consumer chain not found")
+	ErrInvalidEvidence          = errorsmod.Register(ModuleName, 21, "invalid consumer double voting evidence")


Would it make sense to call this ErrInvalidDoubleVotingEvidence?

From what I understand, we do not have an error type for misbehaviour because you use IBC error types such as ibcclienttypes.ErrInvalidMisbehaviour already?

insumity

Left comments.
Didn't go thoroughly through the E2E/integration tests of this PR itself but I looked more into the actual code. Looks good to me for merging due to its time sensitivity and we could do any updates later on (?)

…ics-misbehaviour-handling

sainoe and others added 9 commits August 14, 2023 23:24

feat: improve ICS misbehaviour E2E testing coverage (#1225)

21e3d83

* update e2e tests * update the chain halt assertion

refactor: address comments of ICS Misbehaviour PRs #826 and #1148 (#1223

292ad75

) * remove interface * improve comment * update godoc * address last comments

fix: tiny bug in NewSubmitConsumerDoubleVotingCmd (#1247)

f12a5c0

* fix double voting cli * fix bug double signing handler * godoc * nits * revert wrong push of lasts commits

fix: make HandleConsumerDoubleVoting works with provider pubkeys (#…

2501e83

…1254) * fix double voting cli * fix bug double signing handler * godoc * nits * lint * nit

fix: verify equivocation using validator pubkey in `SubmitConsumerDou…

eb6a079

…bleVoting` msg (#1264) * verify dv evidence using malicious validator pubkey in infraction block header * nits * nits

refactor: update the E2E tests to work with Hermes relayer v1.6.0 (#1278

98af9c0

) * save changes * fix hermes config * fist successful run * nit * nits * nits * doc and nits * lint

mpoke requested a review from a team as a code owner September 12, 2023 11:04

sainoe reviewed Sep 13, 2023

View reviewed changes

p-offtermatt approved these changes Sep 13, 2023

View reviewed changes

insumity reviewed Sep 13, 2023

View reviewed changes

insumity approved these changes Sep 13, 2023

View reviewed changes

save

a71f1fe

sainoe added 2 commits September 14, 2023 09:28

fix nits

3be76ad

Merge remote-tracking branch 'upstream/release/v2.1.x-lsm' into feat/…

691d206

…ics-misbehaviour-handling

sainoe linked an issue Sep 14, 2023 that may be closed by this pull request

EPIC: Cryptographic verification of equivocation #732

Closed

update changelog and fix nits

88e0717

sainoe merged commit b1a6f31 into release/v2.1.x-lsm Sep 14, 2023
10 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Cryptographic verification of equivocation #1287

feat!: Cryptographic verification of equivocation #1287

mpoke commented Sep 12, 2023

sainoe Sep 13, 2023

sainoe Sep 13, 2023

sainoe Sep 13, 2023

sainoe Sep 13, 2023

sainoe Sep 13, 2023 •

edited

Loading

sainoe Sep 13, 2023

p-offtermatt left a comment

p-offtermatt Sep 13, 2023

p-offtermatt Sep 13, 2023

p-offtermatt Sep 13, 2023

p-offtermatt Sep 13, 2023

p-offtermatt Sep 13, 2023

sainoe Sep 13, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023 •

edited

Loading

insumity Sep 13, 2023

insumity Sep 13, 2023

sainoe Sep 14, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023

sainoe Sep 14, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023

insumity Sep 13, 2023

insumity left a comment


		provAddrs := make([]types.ProviderConsAddress, len(byzantineValidators))

		// jail and tombstone the Byzantine validators

feat!: Cryptographic verification of equivocation #1287

feat!: Cryptographic verification of equivocation #1287

Conversation

mpoke commented Sep 12, 2023

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainoe Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-offtermatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insumity left a comment

Choose a reason for hiding this comment

sainoe Sep 13, 2023 •

edited

Loading

insumity Sep 13, 2023 •

edited

Loading