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.

Sign up for GitHub

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

Jump to bottom

feat!: backport cryptographic equivocation to SDKv45 #1360

Merged
merged 42 commits into from
Oct 17, 2023
Merged

feat!: backport cryptographic equivocation to SDKv45 #1360

merged 42 commits into from
Oct 17, 2023

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Oct 16, 2023

Description

Closes: #1346

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...

  • Included the correct type prefix in the PR title
  • Added ! to the type prefix if state-machine breaking change (i.e., requires coordinated upgrade)
  • Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Followed the guidelines for building SDK modules
  • Included the necessary unit and integration tests
  • Added a changelog entry to CHANGELOG.md
  • Included comments for documenting Go code
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed
  • If this PR is library API breaking, bump the go.mod version string of the repo, and follow through on a new major release for both the consumer and provider

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

sainoe and others added 30 commits August 14, 2023 23:24
@sainoe @ancazamfir @mpoke
feat!: add ICS misbehaviour handling (#826)
a77eea1 
* 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>
@sainoe
feat: improve ICS misbehaviour E2E testing coverage (#1225)
21e3d83 
* update e2e tests

* update the chain halt assertion
@sainoe
refactor: address comments of ICS Misbehaviour PRs #826 and #1148 (#1223
292ad75 
)

* remove interface

* improve comment

* update godoc

* address last comments
@sainoe
feat: add handler for consumer double voting (#1232)
f168b9b 
* 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
@sainoe
fix: tiny bug in NewSubmitConsumerDoubleVotingCmd (#1247)
f12a5c0 
* fix double voting cli

* fix bug double signing handler

* godoc

* nits

* revert wrong push of lasts commits
@sainoe
fix: make HandleConsumerDoubleVoting works with provider pubkeys (#…
2501e83 
…1254)

* fix double voting cli

* fix bug double signing handler

* godoc

* nits

* lint

* nit
@sainoe
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
@sainoe
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
@sainoe @p-offtermatt
test: add E2E tests for double voting evidence handling (#1256)
c881a1a 
* 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>
@mergify @shaspitz
feat!: provider proposal for changing reward denoms (backport #1280) (#…
494c4d2 
…1291)

* feat!: provider proposal for changing reward denoms (#1280)

* new provider prop type

* add methods and tests for new prop, update docs

* remove old tx, fix tests

* e2e handling

* fix command type

* boilerplate

* fix e2e tests

* Update CHANGELOG.md

* lint

* validate denoms

* Update proposal.go

* rm msg string

* fix tests

* rm chain in change denom action

* lint

* test for invalid denom

* events for both add and remove

* Update proposal_test.go

(cherry picked from commit 48a2186)

# Conflicts:
#	CHANGELOG.md
#	app/provider/app.go
#	proto/interchain_security/ccv/provider/v1/provider.proto
#	proto/interchain_security/ccv/provider/v1/tx.proto
#	tests/e2e/actions.go
#	tests/integration/distribution.go
#	x/ccv/provider/client/cli/tx.go
#	x/ccv/provider/client/proposal_handler.go
#	x/ccv/provider/keeper/distribution.go
#	x/ccv/provider/keeper/distribution_test.go
#	x/ccv/provider/proposal_handler_test.go
#	x/ccv/provider/types/codec.go
#	x/ccv/provider/types/proposal.go
#	x/ccv/provider/types/provider.pb.go
#	x/ccv/provider/types/tx.pb.go

* fix conflicts

* fix rest handler

* Update CHANGELOG.md

* rm uneeded tx proto

---------

Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>
@sainoe
save
a71f1fe
@sainoe
fix nits
3be76ad
@sainoe
Merge remote-tracking branch 'upstream/release/v2.1.x-lsm' into feat/…
691d206 
…ics-misbehaviour-handling
@sainoe
update changelog and fix nits
88e0717
@insumity
feat: implement slashing functionality on the provider chain (ADR-013) (
022cd5d 
#1275)

Implementing the slashing functionality, as described in ADDR, on the provider chain.
@insumity
remove equivocation proposal
14bf5a6
@insumity
bring back evidencekeeper
f7a411a
@insumity
go.sum added
4f8b0a7
@insumity
rebase and fix lint issues
e8a6408
@insumity
fix mocks
ec76a52
@insumity
clean up protos & delete unecessary file
c6ed9ab
@insumity
fix E2E test
23a25f8
@insumity
fix Dockerfile
650ccb5
@insumity
more fixes
a654ebe
@insumity
increase wait attempt
236153f
@insumity
increase wait attempt
6fc6721
@insumity
wait 1 block in gov proposal
c9702c2
@insumity
fix numbers
34aa490
@sainoe
fix double voting security hole
48a4b51
@sainoe
fix error logging
6a355fe
@sainoe sainoe requested a review from a team as a code owner October 16, 2023 07:52
@sainoe sainoe changed the title feat: add ICS misbehaviour handling (SDKv45) feat!: add ICS misbehaviour handling (SDKv45) Oct 16, 2023
@sainoe sainoe changed the title feat!: add ICS misbehaviour handling (SDKv45) feat!: backport cryptographic equivocation to SDKv45 Oct 16, 2023
insumity
insumity reviewed Oct 16, 2023
View reviewed changes
x/ccv/provider/keeper/misbehaviour.go
// but does NOT update the light client state.
// Note that the IBC CheckMisbehaviourAndUpdateState method returns an error if the trusted consensus states are expired,
// see ibc-go/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
_, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, &misbehaviour)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something in regards to light client attacks but I was wondering the following: At the end of the day, it would seem that a lunatic light client attack consists of having > 1/ 3 validators sign 2 conflicting blocks (i.e., with different BlockID). In other words, we still have a case of double voting here (i.e., someone provided 2 votes for different blocks). If that's true, then what is the difference in slashing those malicious validators here due to misbehaviour and not just slashing them in the double-voting case. In the general case, we do freeze the client but here we do not. So, why are we making a distinction between misbehaviour and double voting?

insumity
insumity reviewed Oct 16, 2023
View reviewed changes
x/ccv/provider/keeper/misbehaviour.go
// but does NOT update the light client state.
// Note that the IBC CheckMisbehaviourAndUpdateState method returns an error if the trusted consensus states are expired,
// see ibc-go/modules/light-clients/07-tendermint/types/misbehaviour_handle.go
_, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, &misbehaviour)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ot seems that CheckMisbehaviourAndUpdateState under the hoods would end up calling checkTrustedHeader that checks:

consState.NextValidatorsHash __equal_to__ tvalHash

In other words, if MsgSubmitConsumerMisbehaviour for a light client attack provides a trusted height X and a new header height Y with Y > X + 1 this evidence would fail. Could this be something that we check in CheckMisbehaviour and stop verifying the evidence early if that's the case?

@sainoe sainoe linked an issue Oct 16, 2023 that may be closed by this pull request
backport ics-misbehaviour-handling to SDK v45 (ICS 2.x) #1346
Closed
insumity
insumity approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@insumity insumity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I focused my review on double_vote.go and misbehaviour.go. The security issues raised in #1340 have been fixed and for this I'm accepting. Thanks Simon!

Note that I did not revisit the tests part of this PR, proto file changes, tx.go and other code related to submitting the appropriate messages.

MSalopek
MSalopek approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@MSalopek MSalopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This has been reviewed multiple times by multiple people. My approval is just administrative.

@sainoe sainoe merged commit 208eb68 into release/v2.2.x-provider-lsm Oct 17, 2023
12 of 13 checks passed
tbruyelle
tbruyelle reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@tbruyelle tbruyelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At @sainoe's request, @giunatale and I reviewed the PR and it's all good for us. Personally I found it very well documented and implemented, impressive work 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbruyelle tbruyelle tbruyelle left review comments

@insumity insumity insumity approved these changes

@MSalopek MSalopek MSalopek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

backport ics-misbehaviour-handling to SDK v45 (ICS 2.x)
4 participants
@sainoe @tbruyelle @insumity @MSalopek