Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove Offence delay #8414

Merged
34 commits merged into from
May 3, 2021
Merged

Conversation

Lohann
Copy link
Contributor

@Lohann Lohann commented Mar 22, 2021

Description

An offence can and should be immediately processed by the staking pallet.
Fixes #8343

Checklist:

  • Remove can_report API from OnOffenceHandler
  • Remove DeferredOffences storage from offences pallet
  • Implement the storage migration logic
  • Update unit tests
  • Update benchmarks
  • Test if all deferred offences has been submited after on_runtime_upgrade hook

Context

Due to some technical circumstances the Offence delay has been introduced here #4517
There's a good discussion about how to properly migrate storages here: #8243

polkadot companion: paritytech/polkadot#2966

@Lohann
Copy link
Contributor Author

Lohann commented Mar 22, 2021

One question, the only safe and deterministic way to remove the DeferredOffences storage is if the OnOffenceHandler::on_offence always succeed. Otherwise how do we guarantee that all deferred offences has been emitted?

@Lohann Lohann mentioned this pull request Mar 22, 2021
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
@Lohann Lohann marked this pull request as draft March 22, 2021 17:42
@kianenigma
Copy link
Contributor

One question, the only safe and determinist way to remove the DeferredOffences storage is if the OnOffenceHandler::on_offence always succeed. Otherwise how do we guarantee that all deferred offences has been emitted?

Sorry, don't get what you mean here?

frame/offences/src/lib.rs Outdated Show resolved Hide resolved
@Lohann
Copy link
Contributor Author

Lohann commented Mar 23, 2021

All unit tests passed 🎉

The only thing that is missing is create a test that verifies if all offences has been resubmitted after on_runtime_upgrade

@kianenigma
Copy link
Contributor

All unit tests passed 🎉

The only thing that is missing is create a test that verifies if all offences has been resubmitted after on_runtime_upgrade

Yeah, while generally it is a rare scenario, worth having a test for it.

@Lohann
Copy link
Contributor Author

Lohann commented Mar 24, 2021

Ok all good, but I don't have permission to add the labels and make CI happy.

@Lohann Lohann requested a review from kianenigma March 24, 2021 16:09
docs/CHANGELOG.md Outdated Show resolved Hide resolved
@Lohann
Copy link
Contributor Author

Lohann commented Apr 29, 2021

@thiolliere good catch, WeightSoftLimit removed 👍

@Lohann Lohann requested a review from andresilva as a code owner April 29, 2021 06:17
@Lohann
Copy link
Contributor Author

Lohann commented Apr 29, 2021

Now I'm working on the companion PR

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

lgtm

@Lohann
Copy link
Contributor Author

Lohann commented May 3, 2021

bot merge

@ghost
Copy link

ghost commented May 3, 2021

Error: Error getting organization membership: Status code: 404 Not Found
Body:
Object({
"documentation_url": String(
"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user",
),
"message": String(
"User does not exist or is not a member of the organization",
),
})

@shawntabrizi
Copy link
Member

@Lohann will have Kian merge this when he wakes up and if he is happy

@Lohann
Copy link
Contributor Author

Lohann commented May 3, 2021

@shawntabrizi sounds good! thx for your patience reviewing this PR 🙏

Btw the Companion section of CONTRIBUTING.adoc#updating-polkadot-as-well needs to be updated, using paths = ["path/to/substrate"] in ~/.cargo/config doesn't work for me (and seems deprecated too).

For create the companion PR I used diener instead, which is not documented but I've found it in the check_polkadot_companion_build.sh script.

git clone --depth 20 https://github.com/paritytech/polkadot.git
cargo install -f diener
cd polkadot
diener patch --crates-to-patch ../ --substrate --path Cargo.toml

@shawntabrizi
Copy link
Member

paths = ["path/to/substrate"] in ~/.cargo/config doesn't work for me (and seems deprecated too).

This works for me last I tried. What did you see was the issue when you tried it?

@Lohann
Copy link
Contributor Author

Lohann commented May 3, 2021

Will run again, one minute

@Lohann
Copy link
Contributor Author

Lohann commented May 3, 2021

My ~/.cargo/config.toml

$ cat ~/.cargo/config.toml 
paths = ["/Users/lohannpaternocoutinhoferreira/Documents/Projects/substrate"]

Output of cargo build

warning: path override for crate `sp-trie` has altered the original list of
dependencies; the dependency on `sp-core` was either added or
modified to not match the previously resolved version

This is currently allowed but is known to produce buggy behavior with spurious
recompiles and changes to the crate graph. Path overrides unfortunately were
never intended to support this feature, so for now this message is just a
warning. In the future, however, this message will become a hard error.

To change the dependency graph via an override it's recommended to use the
`[replace]` feature of Cargo instead of the path override feature. This is
documented online at the url below for more information.

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

error: failed to select a version for `semver-parser`.
    ... required by package `semver v0.11.0`
    ... which is depended on by `cargo_metadata v0.13.1`
    ... which is depended on by `substrate-wasm-builder v4.0.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/substrate)`
    ... which is depended on by `kusama-runtime v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot/runtime/kusama)`
    ... which is depended on by `polkadot-service v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot/node/service)`
    ... which is depended on by `polkadot v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot)`
versions that meet the requirements `=0.10.0` are: 0.10.0

all possible versions conflict with previously selected packages.

  previously selected package `semver-parser v0.10.2`
    ... which is depended on by `cargo_metadata v0.13.1`
    ... which is depended on by `substrate-wasm-builder v4.0.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/substrate)`
    ... which is depended on by `kusama-runtime v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot/runtime/kusama)`
    ... which is depended on by `polkadot-service v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot/node/service)`
    ... which is depended on by `polkadot v0.9.0 (/Users/lohannpaternocoutinhoferreira/Documents/Projects/polkadot)`

failed to select a version for `semver-parser` which could resolve this conflict

@Lohann Lohann requested a review from kianenigma May 3, 2021 04:19
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thanks!

@kianenigma
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented May 3, 2021

Waiting for commit status.

@ghost ghost merged commit 5740270 into paritytech:master May 3, 2021
shawntabrizi pushed a commit that referenced this pull request Jun 17, 2021
* Removed can_report api from OnOffenceHandler

* Removed DeferredOffences and create a storage migration

* Removed missing comments

* Mock set_deferred_offences and deferred_offences methods

* OnOffenceHandler::on_offence always succeed

* Fix benchmark tests

* Fix runtime-benchmark cfg methods

* Removed 'applied' attribute from Offence event

* refactor deprecated deferred offences getter

* Validate if offences are submited after on_runtime_upgrade

* update changelog

* Remove empty lines

* Fix remove_deferred_storage weights

* Remove Offence::on_runtime_upgrade benchmark

* Revert CHANGELOG.md update

* Deprecate DeferredOffenceOf type

* Update copyright

Co-authored-by: Kian Paimani <[email protected]>

* Add migration logs

Co-authored-by: Kian Paimani <[email protected]>

* Fix migration log

* Remove unused import

* Add migration tests

* rustfmt

* use generate_storage_alias! macro

* Refactor should_resubmit_deferred_offences test

* Replace spaces by tabs

* Refactor should_resubmit_deferred_offences test

* Removed WeightSoftLimit

* Removed WeightSoftLimit from tests and mocks

* Remove unused imports

* Apply suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
* Removed can_report api from OnOffenceHandler

* Removed DeferredOffences and create a storage migration

* Removed missing comments

* Mock set_deferred_offences and deferred_offences methods

* OnOffenceHandler::on_offence always succeed

* Fix benchmark tests

* Fix runtime-benchmark cfg methods

* Removed 'applied' attribute from Offence event

* refactor deprecated deferred offences getter

* Validate if offences are submited after on_runtime_upgrade

* update changelog

* Remove empty lines

* Fix remove_deferred_storage weights

* Remove Offence::on_runtime_upgrade benchmark

* Revert CHANGELOG.md update

* Deprecate DeferredOffenceOf type

* Update copyright

Co-authored-by: Kian Paimani <[email protected]>

* Add migration logs

Co-authored-by: Kian Paimani <[email protected]>

* Fix migration log

* Remove unused import

* Add migration tests

* rustfmt

* use generate_storage_alias! macro

* Refactor should_resubmit_deferred_offences test

* Replace spaces by tabs

* Refactor should_resubmit_deferred_offences test

* Removed WeightSoftLimit

* Removed WeightSoftLimit from tests and mocks

* Remove unused imports

* Apply suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi 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.

Remove Offence delay API
6 participants