On-chain unresponsiveness reporting #1215
Conversation
| @@ -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.
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.
idk I just think it makes the code more unreadable and less flexible but wtv
HCastano
added
the
Bump `spec_version`
There was a problem hiding this comment.
⭐ Looks great.
My only concern would be possible circular dependency problems - as even if we can call pallet-staking's chill from the slashing pallet without needing pallet-staking-extension, we are probably anyway going to need stuff from the staking extension pallet there for example to be able to mark a bad signer for removal from the signer set at the next reshare, or remove a TSS node, or something along those lines. So i would probably have put this extrinsic in the slashing pallet.
Thinking about it, i expect your plan is to put a trait in entropy-shared in order to do this, like with the AttestationHandler.
| @@ -34,6 +34,7 @@ p256 ={ version="0.13.2", default-features=false, features=["ecdsa" | |||
| rand ={ version="0.8.5", default-features=false, features=["alloc"] } | |||
|
|
|||
| pallet-parameters={ version="0.3.0", path="../parameters", default-features=false } | |||
| pallet-slashing={ version="0.3.0", path="../slashing", default-features=false } | |||
There was a problem hiding this comment.
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 on pallet-staking directly without having circular dependencies?
| // Note: This operation is O(n), but with a small enough Signer group this should be | ||
| // 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.
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.
Yes that's a good point. I meant to bring it up in the PR description but forgot. The
essence of the question here is whether or not we give relayers the power to report. I'm
not entirely sure yet, but it's something we can decide on later
| 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.
I would like a formal justification for the capitilization of Signer.
| // Note: This operation is O(n), but with a small enough Signer group this should be | |
| // Note: This operation is O(n), but with a small enough signer group this should be |
There was a problem hiding this comment.
The storage struct is called Signers, QED? lol
Pls don't make me run CI again for this 😅
Yeah so I'm not entirely sure how we'll be doing the last bit (calling
The problem with putting it in the Slashing pallet is that we don't have access to the
Yeah I do want to do this eventually so the Slashing mechanism can be somewhat flexible |
This is another take on #667 but without subgroups.
This PR adds a new extrinsic,
report_unstable_peer(), to the Staking pallet allowingTSS nodes in the signing committee to report other TSS nodes in the committee.
Internally this extrinsic calls out to the Slashing pallet to note these reports.
Eventually something will be done to punish any repeat offenders (probably
chillthemfrom the validator set), but that'll be work for a later PR.
I'm in the middle of another PR which adds the triggering mechanism to the TSS itself.