Adds ability to clear obsolete reports in Offences pallet #14048
Conversation
gupnik
added
A0-please_review
gupnik
added
A3-in_progress
| Offence { validator_set_count: 5, session_index, time_slot, offenders: vec![5] }; | ||
| assert_eq!(Offences::report_offence(vec![], offence), Err(OffenceError::ObsoleteReport)); | ||
|
|
||
| with_on_offence_fractions(|f| { |
There was a problem hiding this comment.
Not sure why we use this convoluted helper function here. Isn't it equal to assert!(OnOffencePerbill::get().is_empty())?
There was a problem hiding this comment.
using the helper function to just assert equality is a bit weird to me.
There was a problem hiding this comment.
or else at least a const SESSION_INDEX:u32 = 10;
|
I have a further testing plan as well: Recently, someone developed a The goal would be to add this pallet to eg. Westend and Rococo (probably the former, as it has a lot of nominators) and test and e2e slash going through correctly. Part of that would also be to observe how the pallet behaves with concurrent offences and such, and when the offences are cleared. I highly encourage this as this is a super critical code path that is not often exercised in eg. even Kusama. |
primitives/session/src/lib.rs
Outdated
|
|
||
| /// A trait to be notified when the session changes. | ||
| /// This is in contrast to `SessionHandler` which handles events for specific keys. | ||
| #[impl_trait_for_tuples::impl_for_tuples(30)] |
There was a problem hiding this comment.
This always comes with an increased compile time.
Maybe we can start with 8 or 16 until someone complains that they need more?
frame/offences/src/lib.rs
Outdated
| @@ -92,6 +120,22 @@ pub mod pallet { | |||
| ValueQuery, | |||
| >; | |||
|
|
|||
| /// A map that stores all reports along with | |||
There was a problem hiding this comment.
Please try to break lines at 100 chars.
I am not sure if the formatter does it, but many editors support a vertical line that makes it easy to do it manually.
frame/offences/src/lib.rs
Outdated
| let session_reports = SessionReports::<T>::take(obsolete_session_index); | ||
| let session_reports = | ||
| Vec::<(Kind, OpaqueTimeSlot, ReportIdOf<T>)>::decode(&mut &session_reports[..]) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
No. If it is empty it just returns Ok(vec![]), an error here means that the storage is corrupted.
DQ but why is this double-encode anyway? Why not store Vec<StorageReportOf<T>>?
| Vec::<SessionReportOf<T>>::decode(&mut &session_reports[..]) | ||
| .unwrap_or_default(); | ||
|
|
||
| for (kind, time_slot, report_id) in &session_reports { |
There was a problem hiding this comment.
Is there any upper limit to the length of this vector?
We always need to keep the weight of on_initialize bounded…
There was a problem hiding this comment.
Good point, I'd expect that the session reports can be very large, especially if there's a bug. We should be able to clear the offences through governance when these are resulting from a bug, but still it's better to bound a loop on_initialize.
Perhaps we should instead queue clearing offences and process a fixed number on_initialize, until the queue is empty (simple multi-block clearing process).
frame/offences/src/lib.rs
Outdated
| fn get_session_reports<T: Config>(session_index: SessionIndex) -> Vec<SessionReportOf<T>> { | ||
| let session_reports = SessionReports::<T>::get(session_index); | ||
| Vec::<SessionReportOf<T>>::decode(&mut &session_reports[..]) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
This unwrap_or_default also seems to ignore the fact that either: There is nothing in storage and it should therefore return vec![] or there is corrupted storage.
defensive_unwrap_or_default should be better. Or you return an error and bubble it up 🤷♂️
There was a problem hiding this comment.
It's actually fine to have an empty vec here as a session might not have any reports.
| Vec::<SessionReportOf<T>>::decode(&mut &session_reports[..]) | ||
| .unwrap_or_default(); | ||
|
|
||
| for (kind, time_slot, report_id) in &session_reports { |
There was a problem hiding this comment.
Good point, I'd expect that the session reports can be very large, especially if there's a bug. We should be able to clear the offences through governance when these are resulting from a bug, but still it's better to bound a loop on_initialize.
Perhaps we should instead queue clearing offences and process a fixed number on_initialize, until the queue is empty (simple multi-block clearing process).
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Gonçalo Pestana <[email protected]>
Co-authored-by: Gonçalo Pestana <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
…offence-pallet-optimizations
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
…tytech/substrate into gupnik/offence-pallet-optimizations
Fixes https://github.com/paritytech/srlabs_findings/issues/224
As discussed in #13936 (review), this PR introduces
SessionReportsthat are sorted bySessionIndex. On start of a new session, reports that are older thanmax_session_report_ageare removed.Todo:
clear_obsolete_reportsat the appropriate time.max_session_report_ageat the time of reporting itselfpolkadot companion: paritytech/polkadot#7183
Will need a migration to clear existing data once
max_session_report_agehas passed.