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

doc: update ADR-005 with consumer double voting #1283

Merged
merged 20 commits into from
Oct 9, 2023
Merged

doc: update ADR-005 with consumer double voting #1283

merged 20 commits into from
Oct 9, 2023

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Sep 11, 2023

Description

Closes: #1266

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)

@github-actions github-actions bot added C:Docs Assigned automatically by the PR labeler C:ADR Assigned automatically by the PR labeler labels Sep 11, 2023
@sainoe sainoe changed the title Sainoe/ics equiv adr doc: update ADR-005 with consumer double voting Sep 11, 2023
@sainoe sainoe marked this pull request as ready for review September 12, 2023 14:41
@sainoe sainoe requested a review from a team as a code owner September 12, 2023 14:41
p-offtermatt
p-offtermatt approved these changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@p-offtermatt p-offtermatt left a comment

Choose a reason for hiding this comment

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

Thanks for the work! Couple of comments I believe should be addressed, but nothing particularly serious, so I'm approving!

docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
mpoke
mpoke requested changes Sep 22, 2023
View reviewed changes
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated
referred to as [`infraction_block_header`](https://github.com/cosmos/interchain-security/blob/20b0e35a6d45111bd7bfeb6845417ba752c67c60/proto/interchain_security/ccv/provider/v1/tx.proto#L77).
The latter will provide the malicious validator's public key and the chain ID required to verify the signature of the votes contained in the evidence.

Note that double signing evidence will be verified by using the same conditions than in the CometBFT
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not true. We are not checking the validator's power or the total power for example.

docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md Outdated Show resolved Hide resolved
docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md
which is used to slash the misbehaving validator. In the context of untrusted consumer chains, all their states, including vscIDs,
could be corrupted and therefore cannot be used for slashing purposes.

- For the same reasons explained above, the age of a consumer double signing evidence can't be verified,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this apply to only double signing evidence? Are we verifying the age of light client attack evidence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, the age of the light client attack evidence is verified when we call CheckMisbehaviourAndUpdateState, which checks that the conflicting headers in the LCA have a trusted state that isn't older than the unbonding period.
@insumity could you confirm this please?

@sainoe sainoe merged commit 54e5220 into main Oct 9, 2023
13 checks passed
@sainoe sainoe deleted the sainoe/ics-equiv-adr branch October 9, 2023 08:00
mergify bot pushed a commit that referenced this pull request Oct 9, 2023
@sainoe @mergify
doc: update ADR-005 with consumer double voting (#1283)
b6dcdad 
* save draft

* draft

* save changes

* first draft

* typo

* typo

* last changes

* change TM to Comet

* fix date

* fix links

* address comments

* add ADR link

* change cometBFT and cosmos SDK refs to specific version

* fix links

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

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

* update structure

* save changes

* rework double signing limitations

* fix links

* fix IBC links

---------

Co-authored-by: Marius Poke <[email protected]>
(cherry picked from commit 54e5220)

# Conflicts:
#	docs/docs/adrs/adr-005-cryptographic-equivocation-verification.md
@mergify mergify bot mentioned this pull request Oct 9, 2023
Closed
MSalopek pushed a commit that referenced this pull request Dec 1, 2023
@sainoe @mpoke @MSalopek
doc: update ADR-005 with consumer double voting (#1283)
592dfd9 
* save draft

* draft

* save changes

* first draft

* typo

* typo

* last changes

* change TM to Comet

* fix date

* fix links

* address comments

* add ADR link

* change cometBFT and cosmos SDK refs to specific version

* fix links

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

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

* update structure

* save changes

* rework double signing limitations

* fix links

* fix IBC links

---------

Co-authored-by: Marius Poke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@insumity insumity insumity left review comments

@p-offtermatt p-offtermatt p-offtermatt approved these changes

@mpoke mpoke mpoke approved these changes

Assignees
No one assigned
Labels
C:ADR Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update ADR with consumer double voting
4 participants
@sainoe @insumity @mpoke @p-offtermatt