AURA offence report system #1766
Open
+1,254
−204
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davxy
added
T0-node
davxy
commented
Oct 1, 2023
Comment on lines
+36
to
+53
| // Checking membership proof | ||
| Weight::from_parts(35u64 * WEIGHT_REF_TIME_PER_MICROS, 0) | ||
| .saturating_add( | ||
| Weight::from_parts(175u64 * WEIGHT_REF_TIME_PER_NANOS, 0) | ||
| .saturating_mul(validator_count), | ||
| ) | ||
| .saturating_add(DbWeight::get().reads(5)) | ||
| // Check equivocation proof | ||
| .saturating_add(Weight::from_parts(110u64 * WEIGHT_REF_TIME_PER_MICROS, 0)) | ||
| // Report offence | ||
| .saturating_add(Weight::from_parts(110u64 * WEIGHT_REF_TIME_PER_MICROS, 0)) | ||
| .saturating_add(Weight::from_parts( | ||
| 25u64 * WEIGHT_REF_TIME_PER_MICROS * max_nominators_per_validator as u64, | ||
| 0, | ||
| )) | ||
| .saturating_add(DbWeight::get().reads(14 + 3 * max_nominators_per_validator as u64)) | ||
| .saturating_add(DbWeight::get().writes(10 + 3 * max_nominators_per_validator as u64)) | ||
| } |
There was a problem hiding this comment.
Honestly I don't know how these hardcoded were computed.
Are these estimation? Empirical results? Guesses 😃
I picked these values from babe/aura/grandpa implementations
davxy
commented
Oct 1, 2023
| }; | ||
|
|
||
| /// Default `WeightInfo` implementation generic over the max number of validator's nominators (`N`). | ||
| pub struct SubstrateWeight<const N: u32>; |
There was a problem hiding this comment.
Here I tried to experiment and detach a bit the consensus from the staking.
That is, instead of adding the MaxNominators directly in the Config, I decided to provide the (const) value as part of the weight type we provide.
IMO this makes sense as:
- relax the dependency to the staking model which the user may don't want to use.
- the
MaxNominatorsvalue is only used here, to compute the weight
Maybe I can also provide the () implementation where we set the max nominators to 0.
If the idea is good, I'm going to do the same for the other consensus protocols (in a follow-up PR)
cc @andresilva
davxy
commented
Oct 1, 2023
Comment on lines
+62
to
+66
| # Provide the default equivocation report system. | ||
| default-equivocation-report-system = [ | ||
| "pallet-authorship", | ||
| "pallet-session", | ||
| ] |
There was a problem hiding this comment.
These pallets are required only if the user decides to use the offence report system which ships with substrate.
These pallets are not used otherwise.
davxy
commented
Oct 1, 2023
Comment on lines
+201
to
+212
| let idx1 = block_num1 | ||
| .saturating_sub(Offset::get()) | ||
| .checked_div(&Period::get()) | ||
| .unwrap_or(Zero::zero()); | ||
| let idx2 = block_num2 | ||
| .saturating_sub(Offset::get()) | ||
| .checked_div(&Period::get()) | ||
| .unwrap_or(Zero::zero()); | ||
|
|
||
| if BlockNumberFor::<T>::from(session_index as u32) != idx1 || idx1 != idx2 { | ||
| return Err(Error::<T>::InvalidKeyOwnershipProof.into()) | ||
| } |
There was a problem hiding this comment.
This is the trick to map block num to session index.
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Mar 26, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Mar 27, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 8, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 8, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 8, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 8, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 8, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 9, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 10, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
serban300
added a commit
to serban300/polkadot-sdk
that referenced
this pull request
Apr 10, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
bkchr
pushed a commit
that referenced
this pull request
Apr 10, 2024
* Define method for checking message lane weights * Docs for public function * Renamings
This was referenced Jun 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Requires tweaks to cumulus, but first I want to be sure that there are not blockers with the implementation)
Due to some characteristics of current aura-pallet implementation, the report system depends on some other pallets.
In particular it requires that:
Runtime implements
pallet-authorshipto get reporter identity when not directly passed to the report system functionRuntime implements
session-palletto map the block number to session index.session-palletConfigmust use thePeriodicSessionstype as itsNextSessionRotationassociated type. This is to reliably map the block number to session number. The session index is simply given by the block number divided by session duration (which in this case is constant).The ideas were mostly picked from BABE, BEEFY and GRANDPA.
So there isn't really anything new beside the trick to compute the session index from the block number.
During the implementation I also spotted a couple of possible improvement areas which I'm going to highlight in the code comments.
Closes #70