Deferred Slashing/Offence for offchain Phragmen #5151
Conversation
kianenigma
added
A0-please_review
| @@ -142,19 +142,28 @@ pub trait OnOffenceHandler<Reporter, Offender> { | |||
| /// Zero is a valid value for a fraction. | |||
| /// | |||
| /// The `session` parameter is the session index of the offence. | |||
| /// | |||
| /// The receiver might decide to not accept this offence. In this case, the call site is | |||
| /// responsible for queuing the report and re-submitting again. | |||
There was a problem hiding this comment.
If that's common we should probably extract this logic into some kind of a wrapper.
There was a problem hiding this comment.
We have deferring in quite a few places (in Staking and slashing) but I can't see how you can abstract them away, since IMO each has its own taste and details. Do you have a more concrete example of what you are suggesting here?
frame/offences/src/lib.rs
Outdated
| session_index, | ||
| ).map_err(|_| { | ||
| <DeferredOffences<T>>::mutate(|d| | ||
| d.push((concurrent_offenders.to_vec(), slash_perbill.to_vec(), session_index)) |
There was a problem hiding this comment.
This is unbounded. Are there any guarantees that it won't go over some reasonable size? Also should we limit that?
There was a problem hiding this comment.
If a sane module implement OnOffence, since we check on every on_initialise, then it should be emptied pretty quickly. Yet, no we don't enforce any size. I would be happy to do that from a code PoV. But with slashing, I think it is a tricky situation. A bug here that allows someone that can report but not get slashed due to some buffer being full, IMO, is more catastrophic than temporarily storing lots of data.
| @@ -54,7 +55,16 @@ pub trait Trait: frame_system::Trait { | |||
| decl_storage! { | |||
| trait Store for Module<T: Trait> as Offences { | |||
| /// The primary structure that holds all offence records keyed by report identifiers. | |||
| Reports get(fn reports): map hasher(blake2_256) ReportIdOf<T> => Option<OffenceDetails<T::AccountId, T::IdentificationTuple>>; | |||
| Reports get(fn reports): map hasher(blake2_256) ReportIdOf<T> | |||
| => Option<OffenceDetails<T::AccountId, T::IdentificationTuple>>; | |||
There was a problem hiding this comment.
I would prefer to keep the IdentificationTuple in a separate map if there are multiple reports on the same validator. They are quite large and could bloat the state.
There was a problem hiding this comment.
yeah this can be separate issue since this is actually old storage and I don't want to do migration as well in the current PR. A follow up can migrate both this and DeferredOffences into something that can de-compose T::IdentificationTuple into a Identification and ExposureData (atm this is not possible and not trivial) and then store them as you explained.
|
@tomusdrw I applied almost all reviews. I will merge this into the base branch. If you are unhappy with any of the patches, please report there 👍 |
* Initial skeleton for offchain phragmen * Basic compact encoding decoding for results * add compact files * Bring back Self::ensure_storage_upgraded(); * Make staking use compact stuff. * First seemingly working version of reduce, full of todos * Everything phragmen related works again. * Signing made easier, still issues. * Signing from offchain compile fine 😎 * make compact work with staked asssignment * Evaluation basics are in place. * Move reduce into crate. Document stuff * move reduce into no_std * Add files * Remove other std deps. Runtime compiles * Seemingly it is al stable; cycle implemented but not integrated. * Add fuzzing code. * Cleanup reduce a bit more. * a metric ton of tests for staking; wip 🔨 * Implement a lot more of the tests. * wip getting the unsigned stuff to work * A bit gleanup for unsigned debug * Clean and finalize compact code. * Document reduce. * Still problems with signing * We officaly duct taped the transaction submission stuff. 🤓 * Deadlock with keys again * Runtime builds * Unsigned test works 🙌 * Some cleanups * Make all the tests compile and stuff * Minor cleanup * fix more merge stuff * Most tests work again. * a very nasty bug in reduce * Fix all integrations * Fix more todos * Revamp everything and everything * Remove bogus test * Some review grumbles. * Some fixes * Fix doc test * loop for submission * Fix cli, keyring etc. * some cleanup * Fix staking tests again * fix per-things; bring patches from benchmarking * better score prediction * Add fuzzer, more patches. * Some fixes * More docs * Remove unused generics * Remove max-nominator footgun * Better fuzzer * Disable it ❌ * Bump. * Another round of self-review * Refactor a lot * More major fixes in perThing * Add new fuzz file * Update lock * fix fuzzing code. * Fix nominator retain test * Add slashing check * Update frame/staking/src/tests.rs Co-Authored-By: Joshy Orndorff <JoshOrndorff@users.noreply.github.com> * Some formatting nits * Review comments. * Fix cargo file * Almost all tests work again * Update frame/staking/src/tests.rs Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * Fix review comments * More review stuff * Some nits * Fix new staking / session / babe relation * Update primitives/phragmen/src/lib.rs Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * Update primitives/phragmen/src/lib.rs Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * Update primitives/phragmen/compact/src/lib.rs Co-Authored-By: thiolliere <gui.thiolliere@gmail.com> * Some doc updates to slashing * Fix derive * Remove imports * Remove unimplemented tests * nits * Remove dbg * Better fuzzing params * Remove unused pref map * Deferred Slashing/Offence for offchain Phragmen (#5151) * Some boilerplate * Add test * One more test * Review comments * Fix build * review comments * fix more * fix build * Some cleanups and self-reviews * More minor self reviews * Final nits * Some merge fixes. * opt comment * Fix build * Fix build again. * Update frame/staking/fuzz/fuzz_targets/submit_solution.rs Co-Authored-By: Gavin Wood <gavin@parity.io> * Update frame/staking/src/slashing.rs Co-Authored-By: Gavin Wood <gavin@parity.io> * Update frame/staking/src/offchain_election.rs Co-Authored-By: Gavin Wood <gavin@parity.io> * Fix review comments * fix test * === 🔑 Revamp without staking key. * final round of changes. * Fix cargo-deny * Update frame/staking/src/lib.rs Co-Authored-By: Gavin Wood <gavin@parity.io> Co-authored-by: Joshy Orndorff <JoshOrndorff@users.noreply.github.com> Co-authored-by: thiolliere <gui.thiolliere@gmail.com> Co-authored-by: Gavin Wood <gavin@parity.io>
Some subtle complications:
SlashDefferDuration > 0in Kusama which means that this won't happen, but we can't assume that in substrate. Some chains might have immediate slashing and that simply cannot work with offchain phragmen.There are numerous approaches to these and the risks and attack vectors were not clear, so for now we do something very simple: All reports are ignored while election window is open. This buffering can happen in either staking or offences. For simplicity, and for the sake of staking's code not bloating even more, I do it for now in the offence side.
All in all, it is not an elegant solution but it is rather cheap and assures that no accounting/slashing assumptions are broken. And I don't see it as permanent, we'll probably refactor it into staking once staking itself is in a better shape.