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

disable consumer initiated slashing + fix slash acks bug #692

Merged
merged 29 commits into from
Feb 6, 2023

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Feb 1, 2023

Description

Disable consumer-initiated slashing and also jailing for double-signing evidence.

Also includes #708

Linked issues

Closes: #689 and #693

Type of change

If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs!

  • Feature: Changes and/or adds code behavior, irrelevant to bug fixes
  • Refactor: Changes existing code style, naming, structure, etc.

Regression tests

  • Disable consumer-initiated slashing feature
  • Update e2e tests
  • Update integration tests
  • Update diff tests

New behavior tests

All tests should be updated to follow the new expected behavior when the consumer-initiated slashing feature is disabled.

@sainoe sainoe assigned sainoe and shaspitz and unassigned sainoe Feb 1, 2023
@shaspitz
Copy link
Contributor

shaspitz commented Feb 1, 2023

Imo HandleSlashPacket should be renamed to HandleDowntimeSlashPacket, JailFromDowntimeSlashPacket, or something that can now be more descriptive

@shaspitz
Copy link
Contributor

shaspitz commented Feb 2, 2023

@mpoke do you think it's appropriate to remove TestSlashUndelegation altogether in this PR? I realize this test was recently added in #633. We can keep some parts of TestSlashUndelegation, but it seems the only testing that'd be left is already covered by e2e tests in unbonding.go. Any opinions here?

@shaspitz
Copy link
Contributor

shaspitz commented Feb 2, 2023

@mpoke do you think it's appropriate to remove TestSlashUndelegation altogether in this PR? I realize this test was recently added in #633. We can keep some parts of TestSlashUndelegation, but it seems the only testing that'd be left is already covered by e2e tests in unbonding.go. Any opinions here?

This test was removed

infractionHeight, _ := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId)

// TODO: would be better to have a warning, but there is no Warn() function
k.Logger(ctx).Error("SlashPacket received for double-signing",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an event on the provider so double-signs can be queried at a later time.

@shaspitz
Copy link
Contributor

shaspitz commented Feb 2, 2023

e2e/UTs are fixed ✅

@sainoe
Copy link
Contributor Author

sainoe commented Feb 3, 2023

Diff-tests updated ✅

tests/integration/actions.go Outdated Show resolved Hide resolved
tests/integration/steps_democracy.go Outdated Show resolved Hide resolved
tests/integration/steps_downtime.go Outdated Show resolved Hide resolved
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.

Some comments but nothing that should block merging, LGTM

@jtremback
Copy link
Contributor

jtremback commented Feb 3, 2023

Looks good, except for changes required in model.ts line 428. Please fix it and then this can be merged

@shaspitz
Copy link
Contributor

shaspitz commented Feb 3, 2023

Imo HandleSlashPacket should be renamed to HandleDowntimeSlashPacket, JailFromDowntimeSlashPacket, or something that can now be more descriptive

Naming refactors can be included in a future PR. See #706

@shaspitz shaspitz mentioned this pull request Feb 4, 2023
5 tasks
sainoe and others added 3 commits February 6, 2023 09:35
* refactor Slash processing on provider and consumer

* add unittest for slash acks correctness

* update using review comments

* fix and tests

* relay test

* smol

---------

Co-authored-by: Matija Salopek <[email protected]>
Co-authored-by: MSalopek <[email protected]>
@shaspitz shaspitz changed the title disable consumer initiated slashing disable consumer initiated slashing + fix slash acks bug Feb 6, 2023
@shaspitz
Copy link
Contributor

shaspitz commented Feb 6, 2023

#708 should have been merged into main after this PR, all good tho, this PR will close two issues now. Description updated

@shaspitz shaspitz merged commit 00fd295 into main Feb 6, 2023
@shaspitz shaspitz deleted the 689-disable-slashing branch February 6, 2023 16:43
@shaspitz shaspitz mentioned this pull request Feb 6, 2023
2 tasks
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.

Disable consumer initiated slashing
5 participants