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

docs: update cryptographic equivocation verification ADR #1168

Merged
merged 23 commits into from
Aug 14, 2023

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Jul 31, 2023

Description

Closes: #1124


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 docs: prefix in the PR title
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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 docs: prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR only changes documentation
  • Reviewed content for consistency
  • Reviewed content for spelling and grammar
  • Tested instructions (if applicable)
  • Checked that the documentation website can be built and deployed successfully (run make build-docs)

@sainoe sainoe changed the title Sainoe/adr 005 2 docs: update cryptographic equivocation verification ADR Jul 31, 2023
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Nice work @sainoe. See my comments bellow.

sainoe and others added 14 commits August 3, 2023 08:32
@sainoe sainoe marked this pull request as ready for review August 3, 2023 14:09
@sainoe sainoe marked this pull request as draft August 3, 2023 14:10
@sainoe sainoe marked this pull request as ready for review August 3, 2023 14:20
@mpoke mpoke linked an issue Aug 3, 2023 that may be closed by this pull request
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

Great writeup!

@@ -16,27 +17,81 @@ Proposed
Currently, we use a governance proposal to slash validators for equivocation (double signing and light client attacks). This has the downside that it takes 2 weeks for the proposal to be approved, effectively reducing the unbonding period in some respects. This does not lead to any pressing real-world security concerns, but since it involves the basis of proof of stake, it would be good to get consumer chain slashing back to parity as soon as possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it meant to say "increasing" here?

To me it seems like the 2 weeks for the prop to get approved would cause us to make an unbonding period that is longer, long enough for the prop to pass.

Suggested change
Currently, we use a governance proposal to slash validators for equivocation (double signing and light client attacks). This has the downside that it takes 2 weeks for the proposal to be approved, effectively reducing the unbonding period in some respects. This does not lead to any pressing real-world security concerns, but since it involves the basis of proof of stake, it would be good to get consumer chain slashing back to parity as soon as possible.
Currently, we use a governance proposal to slash validators for equivocation (double signing and light client attacks). This has the downside that it takes 2 weeks for the proposal to be approved, effectively increasing the unbonding period in some respects. This does not lead to any pressing real-world security concerns, but since it involves the basis of proof of stake, it would be good to get consumer chain slashing back to parity as soon as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the entire paragraph


## Decision

For the first stage of this work, we will only handle light client attacks, since there is already code available in the IBC libraries to handle them. We will introduce a new endpoint- `HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour)`. This endpoint takes evidence of a light client attack of the type used by IBC. It then uses various methods from IBC to cryptographically verify the evidence. It then tombstones validators based on this evidence.
In the first iteration of the feature, we will introduce a new endpoint: `HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour)`.
The main idea is to leverage the current IBC misbehaviour handling and update it to solely jail and slash the validators that
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I understanding things correctly below?

The original IBC misbehavior handling is relevant to evidence submitted to a chain, where the misbehavior happened on that same chain. In ICS we're reusing that code to handle misbehavior evidence on the provider, where the misbehavior happened on a different chain (the consumer).

If my understanding is correct, it might be worth adding a couple sentences explaining how ICS's use of the IBC misbehavior handling is different than how it's conventionally used

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll let @sainoe to address this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarshall-spitzbart IBC misbehaviour handling serves as a security mechanism to protect a chain that runs the client of another chain. For instance, let's say that chain A runs a client of chain B. If chain B were to send invalid states to chain A, it becomes necessary that chain A stops trusting the client of chain B. This can be achieved by submitting an IBC misbehavior message to chain A, which will halt the client if successfully executed.

In the context of ICS, we're uniquely reusing the IBC misbehavior message verification methods. The message handling behaviour, however, differs from the IBC implementation as it involves jailing validators and refrains from modifying the client states.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, all makes sense 👍, this info is prob worth adding to the ADR imo

@mpoke mpoke merged commit 7ebaf9e into jtremback-adr-005 Aug 14, 2023
2 of 3 checks passed
@mpoke mpoke deleted the sainoe/adr-005-2 branch August 14, 2023 21:15
mpoke added a commit that referenced this pull request Aug 14, 2023
* Create adr-005-cryptographic-equivocation-verification.md

* docs: update ADR-005 on cryptographic equivocation verifcation (#1022)

* save first gist

* add first draft

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

---------

Co-authored-by: Marius Poke <[email protected]>

* docs: update cryptographic equivocation verification ADR (#1168)

* add first draft

* udpate refs

* update

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Marius Poke <[email protected]>

* add little changes

* address comments

* fix todos

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Anca Zamfir <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

Co-authored-by: Anca Zamfir <[email protected]>

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

* apply review suggestions

* Update docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md

---------

Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>

* update intro.md

---------

Co-authored-by: Simon Noetzlin <[email protected]>
Co-authored-by: Marius Poke <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create ADR for Cryptographic Verification of Equivocation
4 participants