Remove OperatorSigningKey pallet storage #3265
Conversation
teor2345
added
improvement
| .runtime_api() | ||
| .operator_id_by_signing_key(consensus_best_hash, signing_key.clone())?; | ||
| let mut maybe_operator_id = None; | ||
| for operator_id in current_operators.keys().chain(next_operators.iter()) { |
There was a problem hiding this comment.
The checks below handle IDs in current and next, so I think we need to search both.
teor2345
force-pushed
the
rem-oper-storage-key
branch
from
46ab2dd
to
4b4895c
Compare
teor2345
requested review from
vedhavyas,
nazar-pc,
rg3l3dr and
NingLin-P
as code owners
| @@ -1655,17 +1644,14 @@ pub(crate) mod tests { | |||
| operator_free_balance - operator_total_stake - ExistentialDeposit::get() | |||
| ); | |||
|
|
|||
| // cannot register with same operator key | |||
| // registering with same operator key is allowed | |||
There was a problem hiding this comment.
Is this what we want, or is there another way we should check for duplicate operator registrations?
There was a problem hiding this comment.
This should be okay. Operators are free to choose their signing key and is no incentive to reuse the signing key albeit operators are at risk using same signing key. But that would not hurt the protocol since all operators need to stake individually and all compromised operators will be slashed accordingly if they used same signing key and key is compromised.
Will also spend sometime thinking on this further but first pass looks good to me
| @@ -325,11 +324,6 @@ pub fn do_register_operator<T: Config>( | |||
| Error::InvalidOperatorSigningKey | |||
| ); | |||
|
|
|||
| ensure!( | |||
| !OperatorSigningKey::<T>::contains_key(config.signing_key.clone()), | |||
| Error::DuplicateOperatorSigningKey | |||
There was a problem hiding this comment.
we can remove this error as well
| @@ -1655,17 +1644,14 @@ pub(crate) mod tests { | |||
| operator_free_balance - operator_total_stake - ExistentialDeposit::get() | |||
| ); | |||
|
|
|||
| // cannot register with same operator key | |||
| // registering with same operator key is allowed | |||
There was a problem hiding this comment.
This should be okay. Operators are free to choose their signing key and is no incentive to reuse the signing key albeit operators are at risk using same signing key. But that would not hurt the protocol since all operators need to stake individually and all compromised operators will be slashed accordingly if they used same signing key and key is compromised.
Will also spend sometime thinking on this further but first pass looks good to me
| @@ -1604,7 +1593,7 @@ pub(crate) mod tests { | |||
| fn test_register_operator() { | |||
| let domain_id = DomainId::new(0); | |||
| let operator_account = 1; | |||
| let operator_free_balance = 1500 * SSC; | |||
| let operator_free_balance = 2500 * SSC; | |||
| @@ -1532,9 +1532,6 @@ sp_api::decl_runtime_apis! { | |||
| /// Returns the current epoch and the next epoch operators of the given domain | |||
| fn domain_operators(domain_id: DomainId) -> Option<(BTreeMap<OperatorId, Balance>, Vec<OperatorId>)>; | |||
|
|
|||
| /// Get operator id by signing key | |||
| fn operator_id_by_signing_key(signing_key: OperatorPublicKey) -> Option<OperatorId>; | |||
There was a problem hiding this comment.
I'm actually surprised this is only used in malicious operator and not the actual one.
maybe a remnent from the bundle equivocation feature we forgot to remove I guess.
| .consensus_client | ||
| .runtime_api() | ||
| .operator_id_by_signing_key(consensus_best_hash, signing_key.clone())?; | ||
| let mut maybe_operator_id = None; |
There was a problem hiding this comment.
if its helpful, I would recommend adding this test runtime api.
We have test specific runtime apis defined that are not implemented in actual runtime.
Just a thought
This PR removes the storage for the
OperatorSigningKeymap.It replaces the usage in the malicious operator with a search though all operator IDs.
Code contributor checklist: