-
Notifications
You must be signed in to change notification settings - Fork 2
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
On-chain unresponsiveness reporting #1215
Changes from 14 commits
7c4504f
82aa757
c3ec986
6caa324
ab7490e
4939236
b689d7d
3b9055d
0e4eb56
2dbe4ee
c11704a
1b92f1c
7ae006c
efa04c6
7b834a3
1f31753
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -90,6 +90,7 @@ pub mod pallet { | |||||
+ frame_system::Config | ||||||
+ pallet_staking::Config | ||||||
+ pallet_parameters::Config | ||||||
+ pallet_slashing::Config | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is gonna trigger Jesse, but in a future PR I want to move this to be loosely coupled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idk I just think it makes the code more unreadable and less flexible but wtv |
||||||
{ | ||||||
/// Because this pallet emits events, it depends on the runtime's definition of an event. | ||||||
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>; | ||||||
|
@@ -336,6 +337,7 @@ pub mod pallet { | |||||
InvalidValidatorId, | ||||||
SigningGroupError, | ||||||
TssAccountAlreadyExists, | ||||||
NotSigner, | ||||||
NotNextSigner, | ||||||
ReshareNotInProgress, | ||||||
AlreadyConfirmed, | ||||||
|
@@ -735,6 +737,57 @@ pub mod pallet { | |||||
// signers see https://github.com/entropyxyz/entropy-core/issues/985 | ||||||
Ok(Pays::No.into()) | ||||||
} | ||||||
|
||||||
/// An on-chain hook for TSS servers in the signing committee to report other TSS servers in | ||||||
/// the committee for misbehaviour. | ||||||
/// | ||||||
/// Any "conequences" are handled by the configured Slashing pallet and not this pallet | ||||||
/// itself. | ||||||
#[pallet::call_index(7)] | ||||||
#[pallet::weight(<T as Config>::WeightInfo::report_unstable_peer(MAX_SIGNERS as u32))] | ||||||
pub fn report_unstable_peer( | ||||||
origin: OriginFor<T>, | ||||||
offender_tss_account: T::AccountId, | ||||||
) -> DispatchResultWithPostInfo { | ||||||
let reporter_tss_account = ensure_signed(origin)?; | ||||||
|
||||||
// For reporting purposes we need to know the validator account tied to the TSS account. | ||||||
let reporter_validator_id = Self::threshold_to_stash(&reporter_tss_account) | ||||||
.ok_or(Error::<T>::NoThresholdKey)?; | ||||||
let offender_validator_id = Self::threshold_to_stash(&offender_tss_account) | ||||||
.ok_or(Error::<T>::NoThresholdKey)?; | ||||||
|
||||||
// Note: This operation is O(n), but with a small enough Signer group this should be | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like a formal justification for the capitilization of Signer.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The storage struct is called Pls don't make me run CI again for this 😅 |
||||||
// fine to do on-chain. | ||||||
let signers = Self::signers(); | ||||||
ensure!(signers.contains(&reporter_validator_id), Error::<T>::NotSigner); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means relayers cannot use this to report that they were unable to relay a signature request to a member of the signing committee. Which is fine for now but maybe at some point we will want a separate extrinsic for that purpose. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's a good point. I meant to bring it up in the PR description but forgot. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #1223 |
||||||
ensure!(signers.contains(&offender_validator_id), Error::<T>::NotSigner); | ||||||
|
||||||
// We do a bit of a weird conversion here since we want the validator's underlying | ||||||
// `AccountId` for the reporting mechanism, not their `ValidatorId`. | ||||||
// | ||||||
// The Session pallet should have this configured to be the same thing, but we can't | ||||||
// prove that to the compiler. | ||||||
let encoded_validator_id = T::ValidatorId::encode(&reporter_validator_id); | ||||||
let reporter_validator_account = T::AccountId::decode(&mut &encoded_validator_id[..]) | ||||||
.expect("A `ValidatorId` should be equivalent to an `AccountId`."); | ||||||
|
||||||
let encoded_validator_id = T::ValidatorId::encode(&offender_validator_id); | ||||||
let offending_peer_validator_account = | ||||||
T::AccountId::decode(&mut &encoded_validator_id[..]) | ||||||
.expect("A `ValidatorId` should be equivalent to an `AccountId`."); | ||||||
|
||||||
// We don't actually take any action here, we offload the reporting to the Slashing | ||||||
// pallet. | ||||||
pallet_slashing::Pallet::<T>::note_report( | ||||||
reporter_validator_account, | ||||||
offending_peer_validator_account, | ||||||
)?; | ||||||
|
||||||
let actual_weight = | ||||||
<T as Config>::WeightInfo::report_unstable_peer(signers.len() as u32); | ||||||
Ok(Some(actual_weight).into()) | ||||||
} | ||||||
} | ||||||
|
||||||
impl<T: Config> Pallet<T> { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you are not going to run in to problems with needing the staking extension pallet to be a dependency of the slashing pallet in order to call
chill
? Or can you call chill onpallet-staking
directly without having circular dependencies?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed below