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

MD867 - Enable/Disable Slashing #874

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

augustocollado
Copy link
Contributor

This PR introduces changes to the pallet external-validator-slashes.
It adds a new sudo-only extrinsic to switch between three modes for slashing:

  • Enabled (default mode) - Slashing events generate an event (log), offenders are slashed
  • LogOnly - Slashing events generates an event (log), and offenders are not slashed
  • Disabled - Slashing events don't generate events (log), and offenders are not slashed

It also adds tests for the three modes using babe offences.

Note: Weights are not correct, they need to be properly benchmarked.

@augustocollado augustocollado added the a0-pleasereview Pull request needs code review. label Feb 15, 2025
Copy link
Contributor

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Good job @augustocollado !

Copy link
Contributor

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

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

Changes look good! Only a few small comments.

Also, I see a lot of formatting changes that should disappear after running cargo fmt. Can you please check that as well? Thanks!

Other than that the PR looks good. Great job 🥇

@augustocollado
Copy link
Contributor Author

Also, I see a lot of formatting changes that should disappear after running cargo fmt. Can you please check that as well?

All those files changed as a result of running cargo fmt. In fact, the CI rust fmt check is failing only because I manually undid one change (image attached) that caused the compilation to fail. (Tomasz mentioned that this is a know issue related to the rust compiler version)
image

@girazoki
Copy link
Contributor

girazoki commented Feb 27, 2025

Also, I see a lot of formatting changes that should disappear after running cargo fmt. Can you please check that as well?

All those files changed as a result of running cargo fmt. In fact, the CI rust fmt check is failing only because I manually undid one change (image attached) that caused the compilation to fail. (Tomasz mentioned that this is a know issue related to the rust compiler version) ![image](https://private-user-images.githubusercontent.com/56117494/417240224-118514ee-c91e-444f-86c3-ea999bc639cf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDA2NDcyODQsIm5iZiI6MTc0MDY0Njk4NCwicGF0aCI6Ii81NjExNzQ5NC80MTcyNDAyMjQtMTE4NTE0ZWUtYzkxZS00NDRmLTg2YzMtZWE5OTliYzYzOWNmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjI3VDA5MDMwNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTExZTU0NzhmMWFhMDg2NzY4NGU2Y2Y2ZmRiNmRlNWY0MGJkZmRmMDUwODIzNDc2N2U5NWI3YWExOTMzOTY0N2QmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.3PXKDSqBlEP1fcq-AghtpLtUG1FEFwZDO2giWTTH9ks

did you undo the change because it was failing in the CI?

So a couple of suggestions, you should be using ruct 1.81.0 stable, which is what we use in the CI. However I would not mind your fmt changes if they pass the CI (this is, I dont mind you using nightly as long as they pass the rust stable fmt checks)

@augustocollado
Copy link
Contributor Author

augustocollado commented Feb 27, 2025

Also, I see a lot of formatting changes that should disappear after running cargo fmt. Can you please check that as well?

All those files changed as a result of running cargo fmt. In fact, the CI rust fmt check is failing only because I manually undid one change (image attached) that caused the compilation to fail. (Tomasz mentioned that this is a know issue related to the rust compiler version) ![image](https://private-user-images.githubusercontent.com/56117494/417240224-118514ee-c91e-444f-86c3-ea999bc639cf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDA2NDcyODQsIm5iZiI6MTc0MDY0Njk4NCwicGF0aCI6Ii81NjExNzQ5NC80MTcyNDAyMjQtMTE4NTE0ZWUtYzkxZS00NDRmLTg2YzMtZWE5OTliYzYzOWNmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjI3VDA5MDMwNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTExZTU0NzhmMWFhMDg2NzY4NGU2Y2Y2ZmRiNmRlNWY0MGJkZmRmMDUwODIzNDc2N2U5NWI3YWExOTMzOTY0N2QmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.3PXKDSqBlEP1fcq-AghtpLtUG1FEFwZDO2giWTTH9ks

did you undo the change because it was failing in the CI?

So a couple of suggestions, you should be using ruct 1.81.0 stable, which is what we use in the CI. However I would not mind your fmt changes if they pass the CI (this is, I dont mind you using nightly as long as they pass the rust stable fmt checks)

I undid this change because it wouldn't compile in my env. Probably it will compile and pass the fmt check if I change my rust version. I'll try that later!

@augustocollado
Copy link
Contributor Author

Also, I see a lot of formatting changes that should disappear after running cargo fmt. Can you please check that as well?

All those files changed as a result of running cargo fmt. In fact, the CI rust fmt check is failing only because I manually undid one change (image attached) that caused the compilation to fail. (Tomasz mentioned that this is a know issue related to the rust compiler version) ![image](https://private-user-images.githubusercontent.com/56117494/417240224-118514ee-c91e-444f-86c3-ea999bc639cf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDA2NDcyODQsIm5iZiI6MTc0MDY0Njk4NCwicGF0aCI6Ii81NjExNzQ5NC80MTcyNDAyMjQtMTE4NTE0ZWUtYzkxZS00NDRmLTg2YzMtZWE5OTliYzYzOWNmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjI3VDA5MDMwNFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTExZTU0NzhmMWFhMDg2NzY4NGU2Y2Y2ZmRiNmRlNWY0MGJkZmRmMDUwODIzNDc2N2U5NWI3YWExOTMzOTY0N2QmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.3PXKDSqBlEP1fcq-AghtpLtUG1FEFwZDO2giWTTH9ks

did you undo the change because it was failing in the CI?

So a couple of suggestions, you should be using ruct 1.81.0 stable, which is what we use in the CI. However I would not mind your fmt changes if they pass the CI (this is, I dont mind you using nightly as long as they pass the rust stable fmt checks)

I've set my rust version to 1.81.0 stable, ran fmt, and among the many fmt changes (already pushed in this PR), there's one that breaks compilation:
image
That one change I manually undid to make the build check pass, but then the fmt check fails.
Not sure how to make both things (build & fmt) work.

@Agusrodri Agusrodri added breaking Needs to be mentioned in breaking changes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 27, 2025
@Agusrodri Agusrodri added the A8-mergeoncegreen Pull request is reviewed well. label Feb 27, 2025
@augustocollado
Copy link
Contributor Author

There was something wrong with my environment, but now using 1.81.0 stable fmt & builds are fine. Thanks @Agusrodri for the support!

@girazoki girazoki added A8-mergeoncegreen Pull request is reviewed well. and removed A8-mergeoncegreen Pull request is reviewed well. labels Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a0-pleasereview Pull request needs code review. A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants