feat: update ICS misbehaviour to work with all IBC client implementations #1128

sainoe · 2023-07-11T09:48:37Z

Description

Closes: #1125

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

implement msg handling logic e2e test msg handling logic

ancazamfir · 2023-07-23T11:08:12Z

x/ccv/provider/keeper/misbehaviour.go

+	// Assign the Tendermint client misbehaviour concrete type
+	tmMisbehaviour := misbehaviour.(*ibctmtypes.Misbehaviour)


Maybe we should have an explicit check here for non-tendermint misbehaviour. I know it's already done above but it is hidden.

ancazamfir · 2023-07-23T11:10:51Z

x/ccv/provider/keeper/misbehaviour.go

+// HandleConsumerMisbehaviour checks whether the given IBC misbehaviour is valid and, if they are, the misbehaving
+// CheckConsumerMisbehaviour check that the given IBC misbehaviour headers forms a valid light client attack evidence.
+// proceed to the jailing and tombstoning of the bzyantine validators.
+func (k Keeper) HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour exported.Misbehaviour) error {


If we want to make this generic, this should be moved to a new tendermint/misbehaviour.go since this is tendermint specific. Also a few other functions in this file.

p-offtermatt

Thanks for the work! I left a few comments.
I think none are really pressing, so I'm approving.

For the failing checks:
The failure in the markdown check seems unrelated to the content of the PR as far as I can tell.
For the code quality failure, the uncovered code is boilerplate and client/cli which is exhibited by the e2e tests, so it seems unproblematic.

Thanks again, great work!

p-offtermatt · 2023-08-14T07:34:02Z

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


Could you add a comment here why we need ICS from that repo instead of from the informal systems one?

p-offtermatt · 2023-08-14T07:58:32Z

tests/e2e/steps_consumer_misbehaviour.go

+
+// stepsCauseConsumerMisbehaviour forks a consumer chain in order to cause a ICS misbehaviour.
+// The malicious validator behind the attack gets jailed and the consumer client freezed on the provider.
+func stepsCauseConsumerMisbehaviour(consumerName string) []Step {


Would it make sense to call this something like stepsForkConsumerChain instead?
stepsCauseConsumerMisbehaviour seems more broad and less informative, since there could be other types of consumer misbehaviour than a fork, right?

p-offtermatt · 2023-08-14T08:13:17Z

tests/e2e/steps_consumer_misbehaviour.go

+		},
+		{
+			// start a consumer chain using a single big validator knowing that it holds more than 2/3 of the voting power
+			// and that the other validators hold less than 5% so they won't get jailed thanks to the sof opt-out mechanism.


Suggested change

// and that the other validators hold less than 5% so they won't get jailed thanks to the sof opt-out mechanism.

// and that the other validators hold less than 5% so they won't get jailed thanks to the soft opt-out mechanism.

p-offtermatt · 2023-08-14T08:14:28Z

tests/e2e/steps_consumer_misbehaviour.go

+				chainA:      chainID(consumerName),
+				chainB:      chainID("provi"),
+				connectionA: 0,
+				portA:       "consumer", // TODO: check port mapping


This TODO should probably be addressed

p-offtermatt · 2023-08-14T08:30:14Z

x/ccv/provider/keeper/misbehaviour.go

+func (k Keeper) HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour exported.Misbehaviour) error {
+	logger := ctx.Logger()
+
+	// Check that the validity of the misbehaviour


Suggested change

// Check that the validity of the misbehaviour

// Check the validity of the misbehaviour

p-offtermatt · 2023-08-14T09:02:40Z

x/ccv/provider/keeper/misbehaviour.go

+		// convert consumer consensus address
+		consuAddr := sdk.ConsAddress(v.Address.Bytes())
+		provAddr := k.GetProviderAddrFromConsumerAddr(ctx, tmMisbehaviour.Header1.Header.ChainID, types.NewConsumerConsAddress(consuAddr))
+		k.stakingKeeper.ValidatorByConsAddr(ctx, consuAddr)


Is this line doing anything? The return value is not used, might be worth commenting explicitly if this has some expected side effects

p-offtermatt · 2023-08-14T09:05:48Z

x/ccv/provider/keeper/misbehaviour.go

+		ConflictingBlock: conflicted,
+	}
+
+	if ev.ConflictingHeaderIsInvalid(trusted.Header) {


my understanding is that regardless of the outcome of this function,
the height, timestamp and totalvotingpower come from trusted -
either they are taken directly from the trusted block, or they are the same in both anyways.

Is this if/else even needed? If yes, could you clarify in a comment?

p-offtermatt · 2023-08-14T09:36:14Z

tests/integration/misbehaviour.go

+)
+
+// TestHandleConsumerMisbehaviour verifies first that ICS misbehaviour is handled
+// only if its conlflicting headers are valid. Then, it checks


Suggested change

// only if its conlflicting headers are valid. Then, it checks

// only if its conflicting headers are valid. Then, it checks

p-offtermatt · 2023-08-14T09:40:01Z

tests/integration/misbehaviour.go

+			cs, ok := s.providerApp.GetIBCKeeper().ClientKeeper.GetClientState(s.providerCtx(), s.path.EndpointB.ClientID)
+			s.Require().True(ok)
+
+			if tc.expPass {


Could it make sense to check for error types to avoid failing due to unexpected errors?
From a quick look at the code, it doesn't seem the actual errors are produced in the misbehavior code,
so this might be too annoying to do, but if it's easy, it might be worth it.

p-offtermatt · 2023-08-14T09:45:23Z

tests/integration/misbehaviour.go

+
+// TestMsgSubmitConsumerClientHandler tests that the provider
+// handler can parse MsgSubmitConsumerMisbehaviour
+// TODO: move this to x/provider/handler_test.go


The TODO should probably be addressed - is there something blocking this?

yaruwangway · 2023-08-14T15:18:41Z

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

+
+	flags.AddTxFlagsToCmd(cmd)
+
+	_ = cmd.MarkFlagRequired(flags.FlagFrom)


why not just cmd.MarkFlagRequired(flags.FlagFrom)

sainoe added 30 commits June 21, 2023 15:03

define msg to submit misbehaviour to provider

73f8078

implement msg handling logic e2e test msg handling logic

wip: get byzantine validators in misbehavioiur handling

0b6fd43

add tx handler

16d940d

format HandleConsumerMisbehaviour

4750f1f

add tx handler

2e5f8af

add debugging stuff

b076fd4

Add misbehaviour handler

457fe20

create message for consumer double voting evidence

ae522e1

add DRAFT double vote handler

b52daff

Add cli cmd for submit consumer double voting

8eea677

Add double-vote handler

81dd6ba

add last update

0cb86c9

fix jailing

b6cf938

pass first jailing integration test

1444690

format tests

9c31932

doc

42204e1

save

806a005

update e2e tests'

2c796cd

fix typo and improve docs

7310e42

remove unwanted changes

3114807

fix hermes config with assigned key

fffeb07

revert unwanted changes

61d8012

revert local setup

a11ee29

remove log file

e79af1e

typo

0a6cd87

update doc

9ad0721

update ICS misbehaviour test

e0de36d

update ICS misbehaviour test

1b718be

revert mixed commits

4357efa

update ICS misbehaviour test

2be79d7

update ICS misbehaviour test

e734e53

sainoe requested review from jtremback, mpoke, shaspitz, MSalopek, mmulji-ic, glnro and yaruwangway as code owners July 11, 2023 09:48

Add test for MsgSubmitConsumerMisbehaviour parsing

865e43b

sainoe changed the base branch from feat/upgrade-e2e-testing to feat/ics-misbehaviour-handling July 12, 2023 09:12

sainoe requested a review from a team as a code owner July 12, 2023 09:12

sainoe force-pushed the sainoe/ics-misb-exported branch from 8e48493 to 865e43b Compare July 12, 2023 09:16

fix linter

6874b00

sainoe changed the base branch from feat/ics-misbehaviour-handling to sainoe/ics-equivocation-handling July 17, 2023 06:51

sainoe changed the base branch from sainoe/ics-equivocation-handling to sainoe/ics-misbehaviour-handling July 17, 2023 06:51

sainoe changed the base branch from sainoe/ics-misbehaviour-handling to feat/ics-misbehaviour-handling July 17, 2023 06:52

ancazamfir reviewed Jul 23, 2023

View reviewed changes

mpoke linked an issue Jul 24, 2023 that may be closed by this pull request

Update ICS misbehaviour msg definition #1125

Closed

p-offtermatt approved these changes Aug 14, 2023

View reviewed changes

mpoke assigned yaruwangway Aug 14, 2023

yaruwangway reviewed Aug 14, 2023

View reviewed changes

mpoke marked this pull request as draft September 7, 2023 14:44

sainoe closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update ICS misbehaviour to work with all IBC client implementations #1128

feat: update ICS misbehaviour to work with all IBC client implementations #1128

sainoe commented Jul 11, 2023 •

edited by p-offtermatt

Loading

ancazamfir Jul 23, 2023

ancazamfir Jul 23, 2023

p-offtermatt left a comment

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

p-offtermatt Aug 14, 2023

yaruwangway Aug 14, 2023

		// Assign the Tendermint client misbehaviour concrete type
		tmMisbehaviour := misbehaviour.(*ibctmtypes.Misbehaviour)

	// and that the other validators hold less than 5% so they won't get jailed thanks to the sof opt-out mechanism.
	// and that the other validators hold less than 5% so they won't get jailed thanks to the soft opt-out mechanism.

	// Check that the validity of the misbehaviour
	// Check the validity of the misbehaviour

	// only if its conlflicting headers are valid. Then, it checks
	// only if its conflicting headers are valid. Then, it checks


		flags.AddTxFlagsToCmd(cmd)

		_ = cmd.MarkFlagRequired(flags.FlagFrom)

feat: update ICS misbehaviour to work with all IBC client implementations #1128

feat: update ICS misbehaviour to work with all IBC client implementations #1128

Conversation

sainoe commented Jul 11, 2023 • edited by p-offtermatt Loading

Description

Author Checklist

Reviewers Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sainoe commented Jul 11, 2023 •

edited by p-offtermatt

Loading