diff --git a/bin/node/runtime/pangolin/polkadot-compatible-types.json b/bin/node/runtime/pangolin/polkadot-compatible-types.json index 4c55629eea..fc90318d32 100644 --- a/bin/node/runtime/pangolin/polkadot-compatible-types.json +++ b/bin/node/runtime/pangolin/polkadot-compatible-types.json @@ -235,6 +235,10 @@ "stake": "Balance", "term": "BlockNumber" }, + "ScheduledAuthoritiesChangeT": { + "next_authorities": "Vec", + "deadline": "BlockNumber" + }, "MMRRoot": "Hash", "__[pangolin.runtime]__": { "ProxyType": { diff --git a/bin/node/runtime/pangolin/types.json b/bin/node/runtime/pangolin/types.json index 4c9524b5b2..910d6bf76a 100644 --- a/bin/node/runtime/pangolin/types.json +++ b/bin/node/runtime/pangolin/types.json @@ -227,7 +227,7 @@ "stake": "Balance", "term": "BlockNumber" }, - "ScheduledAuthoritiesChange": { + "ScheduledAuthoritiesChangeT": { "next_authorities": "Vec", "deadline": "BlockNumber" }, diff --git a/frame/bridge/ethereum/backing/src/lib.rs b/frame/bridge/ethereum/backing/src/lib.rs index c81f8cf7ac..a24ad60c33 100644 --- a/frame/bridge/ethereum/backing/src/lib.rs +++ b/frame/bridge/ethereum/backing/src/lib.rs @@ -348,7 +348,7 @@ decl_module! { )?; } - T::EcdsaAuthorities::sync_authorities_change(); + T::EcdsaAuthorities::sync_authorities_change()?; VerifiedProof::insert(tx_index, true); } diff --git a/frame/bridge/ethereum/backing/src/mock.rs b/frame/bridge/ethereum/backing/src/mock.rs index 89948673ba..a3ad484ea2 100644 --- a/frame/bridge/ethereum/backing/src/mock.rs +++ b/frame/bridge/ethereum/backing/src/mock.rs @@ -68,7 +68,9 @@ macro_rules! decl_tests { Ok(()) } - fn sync_authorities_change() {} + fn sync_authorities_change() -> DispatchResult { + Ok(()) + } } parameter_types! { pub const EthereumBackingModuleId: ModuleId = ModuleId(*b"da/backi"); diff --git a/frame/bridge/relay-authorities/src/lib.rs b/frame/bridge/relay-authorities/src/lib.rs index 332496f617..ff897b3403 100644 --- a/frame/bridge/relay-authorities/src/lib.rs +++ b/frame/bridge/relay-authorities/src/lib.rs @@ -62,7 +62,7 @@ use frame_system::ensure_signed; use sp_runtime::{DispatchError, DispatchResult, Perbill, SaturatedConversion}; #[cfg(not(feature = "std"))] use sp_std::borrow::ToOwned; -use sp_std::{mem, prelude::*}; +use sp_std::prelude::*; // --- darwinia --- use darwinia_relay_primitives::relay_authorities::*; use darwinia_support::balance::lock::*; @@ -164,7 +164,7 @@ decl_storage! { pub NextAuthorities get(fn next_authorities): Option>; /// A term index counter, play the same role as nonce in extrinsic - pub AuthorityTerm get(fn authority_term): Term; + pub NextTerm get(fn next_term): Term; /// The authorities change requirements /// @@ -551,8 +551,10 @@ decl_module! { { Self::apply_authorities_change()?; Self::deposit_event(RawEvent::AuthoritiesChangeSigned( - >::get(), - >::get() + >::get(), + >::get() + .ok_or(>::NextAuthoritiesNE)? + .next_authorities .into_iter() .map(|authority| authority.signer) .collect(), @@ -591,6 +593,7 @@ decl_module! { T::ResetOrigin::ensure_origin(origin)?; Self::apply_authorities_change()?; + Self::sync_authorities_change()?; >::kill(); } @@ -689,7 +692,7 @@ where &_S { _1: T::Version::get().spec_name, _2: T::OpCodes::get().1, - _3: >::get(), + _3: >::get(), _4: next_authorities .iter() .map(|authority| authority.signer.clone()) @@ -715,24 +718,27 @@ where } pub fn apply_authorities_change() -> DispatchResult { - >::kill(); - >::try_mutate(|maybe_scheduled_authorities_change| { - let scheduled_authorities_change = maybe_scheduled_authorities_change - .as_mut() - .ok_or(>::NextAuthoritiesNE)?; - - >::mutate(|authorities| { - let next_authorities = - mem::take(&mut scheduled_authorities_change.next_authorities); - let previous_authorities = mem::replace(authorities, next_authorities); - - for RelayAuthority { account_id, .. } in previous_authorities { - >::remove_lock(T::LockId::get(), &account_id); - } - }); + let next_authorities = >::get() + .ok_or(>::NextAuthoritiesNE)? + .next_authorities; + let authorities = >::get(); + + for RelayAuthority { account_id, .. } in authorities { + if next_authorities + .iter() + .position( + |RelayAuthority { + account_id: account_id_, + .. + }| account_id_ == &account_id, + ) + .is_none() + { + >::remove_lock(T::LockId::get(), &account_id); + } + } - DispatchResult::Ok(()) - })?; + >::kill(); >::kill(); Ok(()) @@ -826,7 +832,10 @@ where term: Term, mut authorities: Vec, ) -> DispatchResult { - ensure!(term == >::get(), >::TermMis); + ensure!( + term == >::get(), + >::TermMis + ); let mut chain_authorities = >::get() .into_iter() @@ -843,9 +852,15 @@ where } } - fn sync_authorities_change() { - >::kill(); - >::mutate(|authority_term| *authority_term += 1); + fn sync_authorities_change() -> DispatchResult { + let next_authorities = >::take() + .ok_or(>::NextAuthoritiesNE)? + .next_authorities; + + >::put(next_authorities); + >::mutate(|next_term| *next_term += 1); + + Ok(()) } } diff --git a/frame/bridge/relay-authorities/src/test.rs b/frame/bridge/relay-authorities/src/test.rs index aeb8d21488..bc1fc5716d 100644 --- a/frame/bridge/relay-authorities/src/test.rs +++ b/frame/bridge/relay-authorities/src/test.rs @@ -90,7 +90,7 @@ fn renounce_authority_should_work() { ); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); let term_duration = >::get(); @@ -181,7 +181,7 @@ fn remove_authority_should_work() { ); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_err!( RelayAuthorities::remove_authority(Origin::root(), vec![10]), @@ -191,7 +191,7 @@ fn remove_authority_should_work() { assert!(Ring::locks(1).is_empty()); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_err!( RelayAuthorities::remove_authority(Origin::root(), vec![9]), @@ -207,7 +207,7 @@ fn remove_authority_should_work() { )); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_eq!( RelayAuthorities::authorities(), @@ -245,7 +245,7 @@ fn remove_authority_should_work() { )); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_eq!( RelayAuthorities::authorities(), @@ -285,15 +285,16 @@ fn authority_term_should_work() { let max_candidates = >::get(); for i in 1..=max_candidates { - assert_eq!(RelayAuthorities::authority_term(), i as Term - 1); + assert_eq!(RelayAuthorities::next_term(), i as Term - 1); assert_ok!(request_authority(i as _)); assert_ok!(RelayAuthorities::add_authority( Origin::root(), vec![i as _] )); - RelayAuthorities::sync_authorities_change(); - assert_eq!(RelayAuthorities::authority_term(), i as Term); + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); + assert_eq!(RelayAuthorities::next_term(), i as Term); } }); } @@ -360,7 +361,8 @@ fn mmr_root_signed_event_should_work() { [0; 65] )); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); System::reset_events(); RelayAuthorities::schedule_mmr_root(10); @@ -412,7 +414,8 @@ fn authorities_change_signed_event_should_work() { ))] ); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_ok!(request_authority(2)); assert_ok!(RelayAuthorities::add_authority(Origin::root(), vec![2])); @@ -450,81 +453,50 @@ fn schedule_authorities_change_should_work() { assert_ok!(request_authority(1)); assert_ok!(RelayAuthorities::add_authority(Origin::root(), vec![1])); - assert_eq!( - RelayAuthorities::authorities(), - vec![RelayAuthority { - account_id: 9, - signer: [0; 20], - stake: 1, - term: 10 - }] - ); - assert_eq!( - RelayAuthorities::next_authorities(), - Some(ScheduledAuthoritiesChange { - next_authorities: vec![ - RelayAuthority { - account_id: 9, - signer: [0; 20], - stake: 1, - term: 10 - }, - RelayAuthority { - account_id: 1, - signer: [0; 20], - stake: 1, - term: 10 - } - ], - deadline: 3 - }) - ); - - RelayAuthorities::apply_authorities_change().unwrap(); - - assert_eq!( - RelayAuthorities::authorities(), - vec![ + let authorities = vec![RelayAuthority { + account_id: 9, + signer: [0; 20], + stake: 1, + term: 10, + }]; + let schedule_authorities_change = ScheduledAuthoritiesChange { + next_authorities: vec![ RelayAuthority { account_id: 9, signer: [0; 20], stake: 1, - term: 10 + term: 10, }, RelayAuthority { account_id: 1, signer: [0; 20], stake: 1, - term: 10 - } - ] + term: 10, + }, + ], + deadline: 3, + }; + + assert_eq!(RelayAuthorities::authorities(), authorities); + assert_eq!( + RelayAuthorities::next_authorities(), + Some(schedule_authorities_change.clone()) ); + + RelayAuthorities::apply_authorities_change().unwrap(); + + assert_eq!(RelayAuthorities::authorities(), authorities); assert_eq!( RelayAuthorities::next_authorities(), - Some(ScheduledAuthoritiesChange { - next_authorities: vec![], - deadline: 3 - }) + Some(schedule_authorities_change.clone()) ); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_eq!( RelayAuthorities::authorities(), - vec![ - RelayAuthority { - account_id: 9, - signer: [0; 20], - stake: 1, - term: 10 - }, - RelayAuthority { - account_id: 1, - signer: [0; 20], - stake: 1, - term: 10 - } - ] + schedule_authorities_change.next_authorities ); assert!(RelayAuthorities::next_authorities().is_none()); }); @@ -537,7 +509,7 @@ fn kill_authorities_and_force_new_term_should_work() { assert_ok!(RelayAuthorities::add_authority(Origin::root(), vec![1])); RelayAuthorities::apply_authorities_change().unwrap(); - RelayAuthorities::sync_authorities_change(); + RelayAuthorities::sync_authorities_change().unwrap(); assert_eq!( RelayAuthorities::authorities(), @@ -613,3 +585,39 @@ fn kill_authorities_and_force_new_term_should_work() { assert_eq!(RelayAuthorities::submit_duration(), SubmitDuration::get()); }); } + +#[test] +fn lock_after_authorities_change_should_work() { + new_test_ext().execute_with(|| { + assert!(!Ring::locks(9).is_empty()); + assert!(Ring::locks(1).is_empty()); + assert!(Ring::locks(2).is_empty()); + + assert_ok!(request_authority(1)); + assert_ok!(request_authority(2)); + assert_ok!(RelayAuthorities::add_authority(Origin::root(), vec![1, 2])); + + assert!(!Ring::locks(9).is_empty()); + assert!(!Ring::locks(1).is_empty()); + assert!(!Ring::locks(2).is_empty()); + + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); + + assert!(!Ring::locks(9).is_empty()); + assert!(!Ring::locks(1).is_empty()); + assert!(!Ring::locks(2).is_empty()); + + assert_ok!(RelayAuthorities::remove_authority( + Origin::root(), + vec![9, 2] + )); + + RelayAuthorities::apply_authorities_change().unwrap(); + RelayAuthorities::sync_authorities_change().unwrap(); + + assert!(Ring::locks(9).is_empty()); + assert!(!Ring::locks(1).is_empty()); + assert!(Ring::locks(2).is_empty()); + }); +} diff --git a/primitives/relay/src/relay_authorities.rs b/primitives/relay/src/relay_authorities.rs index 0b1a03773e..d4e160cb06 100644 --- a/primitives/relay/src/relay_authorities.rs +++ b/primitives/relay/src/relay_authorities.rs @@ -48,9 +48,12 @@ pub trait RelayAuthorityProtocol { fn schedule_mmr_root(block_number: BlockNumber); - fn check_authorities_change_to_sync(term: Term, authorities: Vec) -> DispatchResult; + fn check_authorities_change_to_sync( + term: Term, + authorities: Vec, + ) -> DispatchResult; - fn sync_authorities_change(); + fn sync_authorities_change() -> DispatchResult; } pub trait MMR { @@ -106,7 +109,6 @@ where pub _4: _4, } - /// The scheduled change of authority set #[derive(Clone, Default, PartialEq, Encode, Decode, RuntimeDebug)] pub struct ScheduledAuthoritiesChange {