From d1277125b93bbc493cded0d9b6fd3e1a6e06089e Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Mon, 6 Nov 2023 18:13:22 +0100 Subject: [PATCH 01/33] feat: broadcast pause --- state-chain/pallets/cf-broadcast/src/lib.rs | 77 ++++++++++++++++--- state-chain/pallets/cf-emissions/src/lib.rs | 11 ++- state-chain/pallets/cf-funding/src/lib.rs | 2 +- .../pallets/cf-ingress-egress/src/lib.rs | 3 +- .../pallets/cf-vaults/src/vault_rotator.rs | 2 +- state-chain/runtime/src/chainflip.rs | 3 +- .../runtime/src/chainflip/broadcaster.rs | 2 + state-chain/traits/src/lib.rs | 1 + state-chain/traits/src/mocks/broadcaster.rs | 4 +- 9 files changed, 87 insertions(+), 18 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 7dab27ce22..b58d1ebb80 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -19,7 +19,7 @@ use cf_chains::{ }; use cf_traits::{ offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, - EpochKey, OnBroadcastReady, ThresholdSigner, + EpochKey, KeyProvider, OnBroadcastReady, ThresholdSigner, }; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ @@ -29,8 +29,7 @@ use frame_support::{ traits::{Get, OnRuntimeUpgrade, StorageVersion, UnfilteredDispatchable}, Twox64Concat, }; - -use cf_traits::KeyProvider; +use sp_std::vec::Vec; use frame_system::pallet_prelude::OriginFor; pub use pallet::*; @@ -76,7 +75,11 @@ pub mod pallet { use super::*; use cf_chains::benchmarking_value::BenchmarkValue; use cf_traits::{AccountRoleRegistry, BroadcastNomination, KeyProvider, OnBroadcastReady}; - use frame_support::{ensure, pallet_prelude::*, traits::EnsureOrigin}; + use frame_support::{ + ensure, + pallet_prelude::{OptionQuery, *}, + traits::EnsureOrigin, + }; use frame_system::pallet_prelude::*; /// Type alias for the instance's configured Transaction. @@ -291,6 +294,11 @@ pub mod pallet { pub type TransactionFeeDeficit, I: 'static = ()> = StorageMap<_, Twox64Concat, SignerIdFor, ChainAmountFor, ValueQuery>; + /// Whether or not broadcasts are paused for broadcast ids greater than the given broadcast id. + #[pallet::storage] + #[pallet::getter(fn broadcast_pause)] + pub type BroadcastPause = StorageValue<_, BroadcastId, OptionQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event, I: 'static = ()> { @@ -386,8 +394,28 @@ pub mod pallet { let mut retries = BroadcastRetryQueue::::take(); + let mut paused_broadcasts = vec![]; + if let Some(pause_broadcast_id) = BroadcastPause::::get() { + retries.retain(|broadcast| { + if broadcast.broadcast_attempt_id.broadcast_id > pause_broadcast_id { + paused_broadcasts.push(BroadcastAttempt:: { + broadcast_attempt_id: (*broadcast).broadcast_attempt_id.clone(), + transaction_payload: (*broadcast).transaction_payload.clone(), + threshold_signature_payload: (*broadcast) + .threshold_signature_payload + .clone(), + transaction_out_id: (*broadcast).transaction_out_id.clone(), + }); + false + } else { + true + } + }); + } + if retries.len() > num_retries_that_fit { - BroadcastRetryQueue::::put(retries.split_off(num_retries_that_fit)); + paused_broadcasts.append(&mut retries.split_off(num_retries_that_fit)); + BroadcastRetryQueue::::put(paused_broadcasts); } let retries_len = retries.len(); @@ -552,6 +580,12 @@ pub mod pallet { TransactionOutIdToBroadcastId::::take(&tx_out_id) .ok_or(Error::::InvalidPayload)?; + if let Some(pause_broadcast_id) = BroadcastPause::::get() { + if pause_broadcast_id == broadcast_id { + BroadcastPause::::set(None); + } + } + if let Some(expected_tx_metadata) = TransactionMetadata::::take(broadcast_id) { if tx_metadata.verify_metadata(&expected_tx_metadata) { let to_refund = AwaitingBroadcast::::get(BroadcastAttemptId { @@ -678,6 +712,7 @@ impl, I: 'static> Pallet { pub fn threshold_sign_and_broadcast( api_call: >::ApiCall, maybe_callback: Option<>::BroadcastCallable>, + pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { let broadcast_id = BroadcastIdCounter::::mutate(|id| { *id += 1; @@ -707,6 +742,10 @@ impl, I: 'static> Pallet { .into() }, ); + if pause_broadcasts { + BroadcastPause::::set(Some(broadcast_id)); + } + (broadcast_id, signature_request_id) } @@ -735,12 +774,30 @@ impl, I: 'static> Pallet { ThresholdSignatureData::::insert(broadcast_id, (api_call, signature)); let broadcast_attempt_id = BroadcastAttemptId { broadcast_id, attempt_count: 0 }; - Self::start_broadcast_attempt(BroadcastAttempt:: { + + let broadcast_attempt = BroadcastAttempt:: { broadcast_attempt_id, transaction_payload, threshold_signature_payload, transaction_out_id, - }); + }; + + if BroadcastPause::::get() + .and_then( + |pause_broadcast_id| { + if broadcast_id > pause_broadcast_id { + Some(()) + } else { + None + } + }, + ) + .is_some() + { + Self::schedule_for_retry(&broadcast_attempt); + } else { + Self::start_broadcast_attempt(broadcast_attempt); + } broadcast_attempt_id } @@ -783,6 +840,7 @@ impl, I: 'static> Pallet { Self::threshold_sign_and_broadcast( api_call, RequestCallbacks::::get(broadcast_id), + false, ); log::info!( "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", @@ -859,14 +917,15 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, + pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { - Self::threshold_sign_and_broadcast(api_call, None) + Self::threshold_sign_and_broadcast(api_call, None, pause_broadcasts) } fn threshold_sign_and_broadcast_with_callback( api_call: Self::ApiCall, callback: Self::Callback, ) -> (BroadcastId, ThresholdSignatureRequestId) { - Self::threshold_sign_and_broadcast(api_call, Some(callback)) + Self::threshold_sign_and_broadcast(api_call, Some(callback), false) } } diff --git a/state-chain/pallets/cf-emissions/src/lib.rs b/state-chain/pallets/cf-emissions/src/lib.rs index 549b723a7d..95529bf47d 100644 --- a/state-chain/pallets/cf-emissions/src/lib.rs +++ b/state-chain/pallets/cf-emissions/src/lib.rs @@ -302,10 +302,13 @@ impl Pallet { ) { // Emit a threshold signature request. // TODO: See if we can replace an old request if there is one. - T::Broadcaster::threshold_sign_and_broadcast(T::ApiCall::new_unsigned( - total_supply.unique_saturated_into(), - block_number.saturated_into(), - )); + T::Broadcaster::threshold_sign_and_broadcast( + T::ApiCall::new_unsigned( + total_supply.unique_saturated_into(), + block_number.saturated_into(), + ), + false, + ); } } diff --git a/state-chain/pallets/cf-funding/src/lib.rs b/state-chain/pallets/cf-funding/src/lib.rs index a77422fadd..f59571765d 100644 --- a/state-chain/pallets/cf-funding/src/lib.rs +++ b/state-chain/pallets/cf-funding/src/lib.rs @@ -458,7 +458,7 @@ pub mod pallet { Self::deposit_event(Event::RedemptionRequested { account_id, amount: net_amount, - broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call).0, + broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call, false).0, expiry_time: contract_expiry, }); } else { diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index 5ccd7f0c68..3c2eb0877b 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -769,7 +769,8 @@ impl, I: 'static> Pallet { ccm.message.to_vec(), ) { Ok(api_call) => { - let (broadcast_id, _) = T::Broadcaster::threshold_sign_and_broadcast(api_call); + let (broadcast_id, _) = + T::Broadcaster::threshold_sign_and_broadcast(api_call, false); Self::deposit_event(Event::::CcmBroadcastRequested { broadcast_id, egress_id: ccm.egress_id, diff --git a/state-chain/pallets/cf-vaults/src/vault_rotator.rs b/state-chain/pallets/cf-vaults/src/vault_rotator.rs index 97ab9e1cce..f9fd5925e7 100644 --- a/state-chain/pallets/cf-vaults/src/vault_rotator.rs +++ b/state-chain/pallets/cf-vaults/src/vault_rotator.rs @@ -159,7 +159,7 @@ impl, I: 'static> VaultRotator for Pallet { ) { Ok(activation_call) => { let (_, threshold_request_id) = - T::Broadcaster::threshold_sign_and_broadcast(activation_call); + T::Broadcaster::threshold_sign_and_broadcast(activation_call, true); if ::ChainCrypto::optimistic_activation() { // Optimistic activation means we don't need to wait for the activation // transaction to succeed before using the new key. diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 33a4e93351..111d4b1efc 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -391,7 +391,7 @@ impl TokenholderGovernanceBroadcaster { maybe_old_key, Decode::decode(&mut &new_key[..]).or(Err(()))?, )?; - B::threshold_sign_and_broadcast(api_call); + B::threshold_sign_and_broadcast(api_call, false); Ok(()) } @@ -431,6 +431,7 @@ impl CommKeyBroadcaster for TokenholderGovernanceBroadcaster { EthereumBroadcaster::threshold_sign_and_broadcast( SetCommKeyWithAggKey::::new_unsigned(new_key), None::, + false, ); } } diff --git a/state-chain/runtime/src/chainflip/broadcaster.rs b/state-chain/runtime/src/chainflip/broadcaster.rs index 694c2c86f8..379cb5c66d 100644 --- a/state-chain/runtime/src/chainflip/broadcaster.rs +++ b/state-chain/runtime/src/chainflip/broadcaster.rs @@ -38,10 +38,12 @@ impl Broadcaster for GovKeyBroadcaster { AnyChainApi::Ethereum(eth_api_call) => >::threshold_sign_and_broadcast( eth_api_call, + false, ), AnyChainApi::Polkadot(dot_api_call) => >::threshold_sign_and_broadcast( dot_api_call, + false, ), } } diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index 1a8ae59348..491466925c 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -489,6 +489,7 @@ pub trait Broadcaster { /// Request a threshold signature and then build and broadcast the outbound api call. fn threshold_sign_and_broadcast( api_call: Self::ApiCall, + pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId); /// Like `threshold_sign_and_broadcast` but also registers a callback to be dispatched when the diff --git a/state-chain/traits/src/mocks/broadcaster.rs b/state-chain/traits/src/mocks/broadcaster.rs index d761b74f32..2dafe2468f 100644 --- a/state-chain/traits/src/mocks/broadcaster.rs +++ b/state-chain/traits/src/mocks/broadcaster.rs @@ -28,6 +28,7 @@ impl< fn threshold_sign_and_broadcast( api_call: Self::ApiCall, + _pause_broadcasts: bool, ) -> (cf_primitives::BroadcastId, cf_primitives::ThresholdSignatureRequestId) { Self::mutate_value(b"API_CALLS", |api_calls: &mut Option>| { let api_calls = api_calls.get_or_insert(Default::default()); @@ -51,7 +52,8 @@ impl< api_call: Self::ApiCall, callback: Self::Callback, ) -> (BroadcastId, ThresholdSignatureRequestId) { - let ids @ (id, _) = >::threshold_sign_and_broadcast(api_call); + let ids @ (id, _) = + >::threshold_sign_and_broadcast(api_call, false); Self::put_storage(b"CALLBACKS", id, callback); ids } From 5af74d73160c5c4537902957fb04cbe26bf409e8 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 7 Nov 2023 16:58:30 +0100 Subject: [PATCH 02/33] feat: removed key locking and non-optimistic activation --- bouncer/commands/go_bananas.ts | 6 +-- engine/src/witness/eth/key_manager.rs | 8 --- state-chain/chains/src/dot.rs | 5 -- state-chain/chains/src/lib.rs | 7 --- state-chain/chains/src/mocks.rs | 19 ------- state-chain/pallets/cf-broadcast/src/mock.rs | 3 +- .../pallets/cf-threshold-signature/src/lib.rs | 8 +-- state-chain/pallets/cf-vaults/src/lib.rs | 50 +++---------------- state-chain/pallets/cf-vaults/src/tests.rs | 28 ----------- .../pallets/cf-vaults/src/vault_rotator.rs | 36 ++++--------- .../src/chainflip/address_derivation/btc.rs | 8 +-- state-chain/traits/src/lib.rs | 31 ------------ state-chain/traits/src/mocks/key_provider.rs | 16 +----- 13 files changed, 25 insertions(+), 200 deletions(-) diff --git a/bouncer/commands/go_bananas.ts b/bouncer/commands/go_bananas.ts index 4bff0f9e3a..6940905859 100755 --- a/bouncer/commands/go_bananas.ts +++ b/bouncer/commands/go_bananas.ts @@ -93,7 +93,7 @@ function price2tick(price: number): number { async function playLp(asset: string, price: number, liquidity: number) { const spread = 0.01 * price; const liquidityFine = liquidity * 1e6; - for (;;) { + for (; ;) { const offset = (price * (Math.random() - 0.5)) / 20; const buyTick = price2tick(price + offset + spread); const sellTick = price2tick(price + offset - spread); @@ -133,7 +133,7 @@ async function playLp(asset: string, price: number, liquidity: number) { async function launchTornado() { const chainflip = await getChainflipApi(); - const epoch = (await chainflip.query.bitcoinVault.currentVaultEpochAndState()).toJSON()! + const epoch = (await chainflip.query.bitcoinVault.currentVaultEpoch()).toJSON()! .epochIndex as number; const pubkey = ( (await chainflip.query.bitcoinVault.vaults(epoch)).toJSON()!.publicKey.current as string @@ -169,7 +169,7 @@ const swapAmount = new Map([ async function playSwapper() { const assets: Asset[] = ['ETH', 'BTC', 'USDC', 'FLIP', 'DOT']; - for (;;) { + for (; ;) { const src = assets.at(Math.floor(Math.random() * assets.length))!; const dest = assets .filter((x) => x !== src) diff --git a/engine/src/witness/eth/key_manager.rs b/engine/src/witness/eth/key_manager.rs index 804e761d2e..3a6de99c42 100644 --- a/engine/src/witness/eth/key_manager.rs +++ b/engine/src/witness/eth/key_manager.rs @@ -84,14 +84,6 @@ impl ChunkedByVaultBuilder { { info!("Handling event: {event}"); let call: state_chain_runtime::RuntimeCall = match event.event_parameters { - KeyManagerEvents::AggKeySetByAggKeyFilter(_) => pallet_cf_vaults::Call::< - _, - ::Instance, - >::vault_key_rotated { - block_number: header.index, - tx_id: event.tx_hash, - } - .into(), KeyManagerEvents::AggKeySetByGovKeyFilter(AggKeySetByGovKeyFilter { new_agg_key, .. diff --git a/state-chain/chains/src/dot.rs b/state-chain/chains/src/dot.rs index 99f5bc0a50..68ff768bd1 100644 --- a/state-chain/chains/src/dot.rs +++ b/state-chain/chains/src/dot.rs @@ -313,11 +313,6 @@ impl ChainCrypto for PolkadotCrypto { fn sign_with_specific_key() -> bool { true } - - /// We sign with a specific key, so we can optimistically activate the next one. - fn optimistic_activation() -> bool { - true - } } #[derive(Encode, Decode, TypeInfo, Clone, RuntimeDebug, Default, PartialEq, Eq)] diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 7b8f7b2d71..5b737cc5f0 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -187,13 +187,6 @@ pub trait ChainCrypto { Self::UtxoChain::get() } - /// Determines whether the chain crypto allows for optimistic activation of new aggregate keys. - /// - /// By default, this is true for Utxo-based chains, false otherwise. - fn optimistic_activation() -> bool { - Self::UtxoChain::get() - } - /// Determines whether the chain crypto supports key handover. /// /// By default, this is true for Utxo-based chains, false otherwise. diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index bee874a8e7..7ed5c503be 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -16,7 +16,6 @@ pub type MockEthereumChannelId = u128; thread_local! { static MOCK_KEY_HANDOVER_IS_REQUIRED: RefCell = RefCell::new(true); - static MOCK_OPTIMISTIC_ACTIVATION: RefCell = RefCell::new(false); static MOCK_SIGN_WITH_SPECIFIC_KEY: RefCell = RefCell::new(false); static MOCK_VALID_METADATA: RefCell = RefCell::new(true); } @@ -35,20 +34,6 @@ impl Get for MockKeyHandoverIsRequired { } } -pub struct MockOptimisticActivation; - -impl MockOptimisticActivation { - pub fn set(value: bool) { - MOCK_OPTIMISTIC_ACTIVATION.with(|v| *v.borrow_mut() = value); - } -} - -impl Get for MockOptimisticActivation { - fn get() -> bool { - MOCK_OPTIMISTIC_ACTIVATION.with(|v| *v.borrow()) - } -} - pub struct MockFixedKeySigningRequests; impl MockFixedKeySigningRequests { @@ -264,10 +249,6 @@ impl ChainCrypto for MockEthereumChainCrypto { MockFixedKeySigningRequests::get() } - fn optimistic_activation() -> bool { - MockOptimisticActivation::get() - } - fn key_handover_is_required() -> bool { MockKeyHandoverIsRequired::get() } diff --git a/state-chain/pallets/cf-broadcast/src/mock.rs b/state-chain/pallets/cf-broadcast/src/mock.rs index 25d6383de8..5c93c667f1 100644 --- a/state-chain/pallets/cf-broadcast/src/mock.rs +++ b/state-chain/pallets/cf-broadcast/src/mock.rs @@ -17,7 +17,7 @@ use cf_traits::{ block_height_provider::BlockHeightProvider, signer_nomination::MockNominator, threshold_signer::MockThresholdSigner, }, - AccountRoleRegistry, EpochKey, KeyState, OnBroadcastReady, + AccountRoleRegistry, EpochKey, OnBroadcastReady, }; use codec::{Decode, Encode}; use frame_support::{parameter_types, traits::UnfilteredDispatchable}; @@ -98,7 +98,6 @@ impl cf_traits::KeyProvider for MockKeyProvider { Some(EpochKey { key: if VALIDKEY.with(|cell| *cell.borrow()) { VALID_AGG_KEY } else { INVALID_AGG_KEY }, epoch_index: Default::default(), - key_state: KeyState::Unlocked, }) } } diff --git a/state-chain/pallets/cf-threshold-signature/src/lib.rs b/state-chain/pallets/cf-threshold-signature/src/lib.rs index 851c9e52b6..9ec810ffcd 100644 --- a/state-chain/pallets/cf-threshold-signature/src/lib.rs +++ b/state-chain/pallets/cf-threshold-signature/src/lib.rs @@ -695,13 +695,7 @@ impl, I: 'static> Pallet { ( match request_instruction.request_type { RequestType::CurrentKey => T::KeyProvider::active_epoch_key() - .and_then(|EpochKey { key_state, key, epoch_index }| { - if key_state.is_available_for_request(request_id) { - Some((key, epoch_index)) - } else { - None - } - }) + .map(|EpochKey { key, epoch_index }| (key, epoch_index)) .ok_or(Event::::CurrentKeyUnavailable { request_id, attempt_count, diff --git a/state-chain/pallets/cf-vaults/src/lib.rs b/state-chain/pallets/cf-vaults/src/lib.rs index e0f4200c2b..e00852fbc2 100644 --- a/state-chain/pallets/cf-vaults/src/lib.rs +++ b/state-chain/pallets/cf-vaults/src/lib.rs @@ -9,9 +9,8 @@ use cf_primitives::{ use cf_runtime_utilities::{EnumVariant, StorageDecodeVariant}; use cf_traits::{ impl_pallet_safe_mode, offence_reporting::OffenceReporter, AccountRoleRegistry, AsyncResult, - Broadcaster, Chainflip, CurrentEpochIndex, EpochKey, GetBlockHeight, KeyProvider, KeyState, - SafeMode, SetSafeMode, Slashing, ThresholdSigner, VaultKeyWitnessedHandler, VaultRotator, - VaultStatus, + Broadcaster, Chainflip, CurrentEpochIndex, EpochKey, GetBlockHeight, KeyProvider, SafeMode, + SetSafeMode, Slashing, ThresholdSigner, VaultKeyWitnessedHandler, VaultRotator, VaultStatus, }; use frame_support::{ pallet_prelude::*, @@ -140,9 +139,8 @@ pub enum PalletOffence { } #[derive(Encode, Decode, TypeInfo)] -pub struct VaultEpochAndState { +pub struct VaultEpoch { pub epoch_index: EpochIndex, - pub key_state: KeyState, } #[frame_support::pallet] @@ -358,8 +356,7 @@ pub mod pallet { /// The epoch whose authorities control the current vault key. #[pallet::storage] #[pallet::getter(fn current_keyholders_epoch)] - pub type CurrentVaultEpochAndState, I: 'static = ()> = - StorageValue<_, VaultEpochAndState>; + pub type CurrentVaultEpoch, I: 'static = ()> = StorageValue<_, VaultEpoch>; /// Vault rotation statuses for the current epoch rotation. #[pallet::storage] @@ -666,35 +663,6 @@ pub mod pallet { ) } - /// A vault rotation event has been witnessed, we update the vault with the new key. - /// - /// ## Events - /// - /// - [VaultRotationCompleted](Event::VaultRotationCompleted) - /// - /// ## Errors - /// - /// - [NoActiveRotation](Error::NoActiveRotation) - /// - [InvalidRotationStatus](Error::InvalidRotationStatus) - /// - /// ## Dependencies - /// - /// - [Epoch Info Trait](EpochInfo) - #[pallet::call_index(3)] - #[pallet::weight(T::WeightInfo::vault_key_rotated())] - pub fn vault_key_rotated( - origin: OriginFor, - block_number: ChainBlockNumberFor, - - // This field is primarily required to ensure the witness calls are unique per - // transaction (on the external chain) - _tx_id: TransactionInIdFor, - ) -> DispatchResultWithPostInfo { - T::EnsureWitnessedAtCurrentEpoch::ensure_origin(origin)?; - - Self::on_new_key_activated(block_number) - } - /// The vault's key has been updated externally, outside of the rotation /// cycle. This is an unexpected event as far as our chain is concerned, and /// the only thing we can do is to halt and wait for further governance @@ -861,10 +829,7 @@ impl, I: 'static> Pallet { fn set_vault_key_for_epoch(epoch_index: EpochIndex, vault: Vault) { Vaults::::insert(epoch_index, vault); - CurrentVaultEpochAndState::::put(VaultEpochAndState { - epoch_index, - key_state: KeyState::Unlocked, - }); + CurrentVaultEpoch::::put(VaultEpoch { epoch_index }); } // Once we've successfully generated the key, we want to do a signing ceremony to verify that @@ -973,12 +938,11 @@ impl, I: 'static> Pallet { impl, I: 'static> KeyProvider<::ChainCrypto> for Pallet { fn active_epoch_key( ) -> Option::ChainCrypto as ChainCrypto>::AggKey>> { - CurrentVaultEpochAndState::::get().map(|current_vault_epoch_and_state| { + CurrentVaultEpoch::::get().map(|current_vault_epoch_and_state| { EpochKey { key: Vaults::::get(current_vault_epoch_and_state.epoch_index) - .expect("Key must exist if CurrentVaultEpochAndState exists since they get set at the same place: set_next_vault()").public_key, + .expect("Key must exist if CurrentVaultEpoch exists since they get set at the same place: set_next_vault()").public_key, epoch_index: current_vault_epoch_and_state.epoch_index, - key_state: current_vault_epoch_and_state.key_state, } }) } diff --git a/state-chain/pallets/cf-vaults/src/tests.rs b/state-chain/pallets/cf-vaults/src/tests.rs index 77e3834357..0276ec6fbd 100644 --- a/state-chain/pallets/cf-vaults/src/tests.rs +++ b/state-chain/pallets/cf-vaults/src/tests.rs @@ -517,8 +517,6 @@ fn cannot_report_key_handover_outcome_when_awaiting_keygen() { } fn do_full_key_rotation() { - assert!(!MockOptimisticActivation::get(), "Test expects non-optimistic activation"); - let rotation_epoch = ::EpochInfo::epoch_index() + 1; ::keygen( BTreeSet::from_iter(ALL_CANDIDATES.iter().cloned()), @@ -899,31 +897,6 @@ mod vault_key_rotation { }); } - #[test] - fn non_optimistic_activation() { - let ext = setup(Ok(NEW_AGG_PUB_KEY_POST_HANDOVER)).execute_with(|| { - BtcMockThresholdSigner::execute_signature_result_against_last_request(Ok(vec![ - BTC_DUMMY_SIG, - ])); - - MockOptimisticActivation::set(false); - VaultsPallet::activate(); - - assert!(matches!( - PendingVaultRotation::::get().unwrap(), - VaultRotationStatus::AwaitingActivation { .. } - )); - - assert_ok!(VaultsPallet::vault_key_rotated( - RuntimeOrigin::root(), - ACTIVATION_BLOCK_NUMBER, - TX_HASH, - )); - }); - - final_checks(ext, ACTIVATION_BLOCK_NUMBER); - } - #[test] fn optimistic_activation() { const HANDOVER_ACTIVATION_BLOCK: u64 = 420; @@ -933,7 +906,6 @@ mod vault_key_rotation { ])); BlockHeightProvider::::set_block_height(HANDOVER_ACTIVATION_BLOCK); - MockOptimisticActivation::set(true); VaultsPallet::activate(); // No need to call vault_key_rotated. diff --git a/state-chain/pallets/cf-vaults/src/vault_rotator.rs b/state-chain/pallets/cf-vaults/src/vault_rotator.rs index f9fd5925e7..9a8128225f 100644 --- a/state-chain/pallets/cf-vaults/src/vault_rotator.rs +++ b/state-chain/pallets/cf-vaults/src/vault_rotator.rs @@ -152,38 +152,20 @@ impl, I: 'static> VaultRotator for Pallet { if let Some(VaultRotationStatus::::KeyHandoverComplete { new_public_key }) = PendingVaultRotation::::get() { - if let Some(EpochKey { key, key_state, .. }) = Self::active_epoch_key() { + if let Some(EpochKey { key, .. }) = Self::active_epoch_key() { match >::new_unsigned( Some(key), new_public_key, ) { Ok(activation_call) => { - let (_, threshold_request_id) = - T::Broadcaster::threshold_sign_and_broadcast(activation_call, true); - if ::ChainCrypto::optimistic_activation() { - // Optimistic activation means we don't need to wait for the activation - // transaction to succeed before using the new key. - Self::activate_new_key( - new_public_key, - T::ChainTracking::get_block_height(), - ); - } else { - debug_assert!( - matches!(key_state, KeyState::Unlocked), - "Current epoch key must be active to activate next key." - ); - // The key needs to be locked until activation is complete. - CurrentVaultEpochAndState::::mutate(|epoch_end_key| { - epoch_end_key - .as_mut() - .expect("Checked above at if let Some") - .key_state - .lock(threshold_request_id) - }); - PendingVaultRotation::::put( - VaultRotationStatus::::AwaitingActivation { new_public_key }, - ); - } + T::Broadcaster::threshold_sign_and_broadcast(activation_call, true); + + // Optimistic activation means we don't need to wait for the activation + // transaction to succeed before using the new key. + Self::activate_new_key( + new_public_key, + T::ChainTracking::get_block_height(), + ); }, Err(SetAggKeyWithAggKeyError::NotRequired) => { // This can happen if, for example, on a utxo chain there are no funds that diff --git a/state-chain/runtime/src/chainflip/address_derivation/btc.rs b/state-chain/runtime/src/chainflip/address_derivation/btc.rs index ca3414ce28..436df1da74 100644 --- a/state-chain/runtime/src/chainflip/address_derivation/btc.rs +++ b/state-chain/runtime/src/chainflip/address_derivation/btc.rs @@ -48,10 +48,9 @@ fn test_address_generation() { use crate::Runtime; use cf_chains::Bitcoin; use cf_primitives::chains::assets::btc; - use cf_traits::KeyState; use cf_utilities::assert_ok; use pallet_cf_validator::CurrentEpoch; - use pallet_cf_vaults::{CurrentVaultEpochAndState, Vault, VaultEpochAndState, Vaults}; + use pallet_cf_vaults::{CurrentVaultEpoch, Vault, VaultEpoch, Vaults}; frame_support::sp_io::TestExternalities::new_empty().execute_with(|| { CurrentEpoch::::set(1); @@ -67,10 +66,7 @@ fn test_address_generation() { active_from_block: 1, }, ); - CurrentVaultEpochAndState::::put(VaultEpochAndState { - epoch_index: 1, - key_state: KeyState::Unlocked, - }); + CurrentVaultEpoch::::put(VaultEpoch { epoch_index: 1 }); assert_ok!(>::generate_address( btc::Asset::Btc, 1 diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index 491466925c..07ba9944a3 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -362,41 +362,10 @@ pub trait ThresholdSignerNomination { ) -> Option>; } -#[derive(Debug, TypeInfo, Decode, Encode, Clone, Copy, PartialEq, Eq)] -pub enum KeyState { - Unlocked, - /// Key is only available to sign this request id. - Locked(ThresholdSignatureRequestId), -} - -impl KeyState { - pub fn is_available_for_request(&self, request_id: ThresholdSignatureRequestId) -> bool { - match self { - KeyState::Unlocked => true, - KeyState::Locked(locked_request_id) => request_id == *locked_request_id, - } - } - - pub fn unlock(&mut self) { - *self = KeyState::Unlocked; - } - - pub fn lock(&mut self, request_id: ThresholdSignatureRequestId) { - *self = KeyState::Locked(request_id); - } -} - #[derive(Debug, TypeInfo, Decode, Encode, Clone, Copy, PartialEq, Eq)] pub struct EpochKey { pub key: Key, pub epoch_index: EpochIndex, - pub key_state: KeyState, -} - -impl EpochKey { - pub fn lock_for_request(&mut self, request_id: ThresholdSignatureRequestId) { - self.key_state = KeyState::Locked(request_id); - } } /// Provides the currently valid key for multisig ceremonies. diff --git a/state-chain/traits/src/mocks/key_provider.rs b/state-chain/traits/src/mocks/key_provider.rs index 4693a21e85..4b013ae0de 100644 --- a/state-chain/traits/src/mocks/key_provider.rs +++ b/state-chain/traits/src/mocks/key_provider.rs @@ -1,8 +1,7 @@ use cf_chains::ChainCrypto; -use cf_primitives::ThresholdSignatureRequestId; use super::{MockPallet, MockPalletStorage}; -use crate::{EpochKey, KeyState}; +use crate::EpochKey; use std::marker::PhantomData; #[derive(Default)] @@ -16,18 +15,7 @@ const EPOCH_KEY: &[u8] = b"EPOCH_KEY"; impl MockKeyProvider { pub fn add_key(key: C::AggKey) { - Self::put_value( - EPOCH_KEY, - EpochKey { key, epoch_index: Default::default(), key_state: KeyState::Unlocked }, - ); - } - - pub fn lock_key(request_id: ThresholdSignatureRequestId) { - Self::mutate_value::, _, _>(EPOCH_KEY, |maybe_key| { - if let Some(key) = maybe_key.as_mut() { - key.lock_for_request(request_id); - } - }); + Self::put_value(EPOCH_KEY, EpochKey { key, epoch_index: Default::default() }); } } From d77753750ed56a3e0ce1feb35ca10f7a852c78e6 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 8 Nov 2023 17:22:05 +0100 Subject: [PATCH 03/33] fix: tests --- .../cf-integration-tests/src/authorities.rs | 60 ++----------------- .../cf-integration-tests/src/network.rs | 25 +------- state-chain/pallets/cf-broadcast/src/lib.rs | 8 +-- state-chain/pallets/cf-broadcast/src/tests.rs | 2 +- state-chain/pallets/cf-emissions/src/mock.rs | 1 + .../pallets/cf-environment/src/mock.rs | 1 + state-chain/pallets/cf-funding/src/mock.rs | 1 + .../cf-threshold-signature/src/tests.rs | 9 +-- .../pallets/cf-vaults/src/benchmarking.rs | 14 ----- state-chain/pallets/cf-vaults/src/mock.rs | 1 + state-chain/pallets/cf-vaults/src/tests.rs | 52 ++-------------- 11 files changed, 24 insertions(+), 150 deletions(-) diff --git a/state-chain/cf-integration-tests/src/authorities.rs b/state-chain/cf-integration-tests/src/authorities.rs index 71ae0830cf..151198838d 100644 --- a/state-chain/cf-integration-tests/src/authorities.rs +++ b/state-chain/cf-integration-tests/src/authorities.rs @@ -8,12 +8,12 @@ use sp_runtime::AccountId32; use std::collections::{BTreeSet, HashMap}; use cf_primitives::{AuthorityCount, FlipBalance, GENESIS_EPOCH}; -use cf_traits::{AsyncResult, EpochInfo, SafeMode, VaultRotator, VaultStatus}; +use cf_traits::{AsyncResult, EpochInfo, VaultRotator, VaultStatus}; use pallet_cf_environment::SafeModeUpdate; use pallet_cf_validator::{CurrentRotationPhase, RotationPhase}; use state_chain_runtime::{ - safe_mode::RuntimeSafeMode, BitcoinVault, Environment, EthereumInstance, EthereumVault, Flip, - PolkadotInstance, PolkadotVault, Runtime, RuntimeOrigin, Validator, + BitcoinVault, Environment, EthereumInstance, EthereumVault, Flip, PolkadotInstance, + PolkadotVault, Runtime, RuntimeOrigin, Validator, }; // Helper function that creates a network, funds backup nodes, and have them join the auction. @@ -97,11 +97,13 @@ fn authority_rotates_with_correct_sequence() { assert_eq!(AllVaults::status(), AsyncResult::Ready(VaultStatus::KeyHandoverComplete)); // Activate new key. - testnet.move_forward_blocks(2); + // The key is immediately activated in the next block + testnet.move_forward_blocks(1); assert!(matches!( Validator::current_rotation_phase(), RotationPhase::ActivatingKeys(..) )); + assert_eq!( AllVaults::status(), AsyncResult::Ready(VaultStatus::RotationComplete), @@ -293,56 +295,6 @@ fn authority_rotation_can_succeed_after_aborted_by_safe_mode() { }); } -#[test] -fn authority_rotation_cannot_be_aborted_after_key_handover_but_stalls_on_safe_mode() { - const EPOCH_BLOCKS: u32 = 1000; - const MAX_AUTHORITIES: AuthorityCount = 10; - super::genesis::default() - .blocks_per_epoch(EPOCH_BLOCKS) - .max_authorities(MAX_AUTHORITIES) - .build() - .execute_with(|| { - let (mut testnet, _, _) = fund_authorities_and_join_auction(MAX_AUTHORITIES); - - // Resolve Auction - testnet.move_to_the_end_of_epoch(); - - // Run until key handover starts - testnet.move_forward_blocks(5); - assert!( - matches!(AllVaults::status(), AsyncResult::Ready(VaultStatus::KeyHandoverComplete)), - "Key handover should be complete but is {:?}", - AllVaults::status() - ); - - assert_ok!(Environment::update_safe_mode( - pallet_cf_governance::RawOrigin::GovernanceApproval.into(), - SafeModeUpdate::CodeRed - )); - - testnet.move_forward_blocks(3); - - // Authority rotation is stalled while in Code Red because of disabling dispatching - // witness extrinsics and so witnessing vault rotation will be stalled. - assert!(matches!(AllVaults::status(), AsyncResult::Pending)); - - // We activate witnessing calls by setting safe mode to code green just for the - // witnesser pallet. - let mut runtime_safe_mode_with_witnessing = RuntimeSafeMode::CODE_RED; - runtime_safe_mode_with_witnessing.witnesser = - pallet_cf_witnesser::PalletSafeMode::CODE_GREEN; - - assert_ok!(Environment::update_safe_mode( - pallet_cf_governance::RawOrigin::GovernanceApproval.into(), - SafeModeUpdate::CodeAmber(runtime_safe_mode_with_witnessing) - )); - - // rotation should now complete since the witness calls are now dispatched. - testnet.move_forward_blocks(3); - assert_eq!(GENESIS_EPOCH + 1, Validator::epoch_index(), "We should be in a new epoch"); - }); -} - #[test] fn authority_rotation_can_recover_after_keygen_fails() { const EPOCH_BLOCKS: u32 = 1000; diff --git a/state-chain/cf-integration-tests/src/network.rs b/state-chain/cf-integration-tests/src/network.rs index c244a11dc9..5dcf879194 100644 --- a/state-chain/cf-integration-tests/src/network.rs +++ b/state-chain/cf-integration-tests/src/network.rs @@ -13,12 +13,11 @@ use frame_support::{ traits::{IntegrityTest, OnFinalize, OnIdle}, }; use pallet_cf_funding::{MinimumFunding, RedemptionAmount}; -use pallet_cf_validator::RotationPhase; use sp_consensus_aura::SlotDuration; use sp_std::collections::btree_set::BTreeSet; use state_chain_runtime::{ - AccountRoles, AllPalletsWithSystem, BitcoinInstance, EthereumInstance, PolkadotInstance, - Runtime, RuntimeCall, RuntimeEvent, RuntimeOrigin, Weight, + AccountRoles, AllPalletsWithSystem, BitcoinInstance, PolkadotInstance, Runtime, RuntimeCall, + RuntimeEvent, RuntimeOrigin, Weight, }; use std::{ cell::RefCell, @@ -282,26 +281,6 @@ impl Engine { ), RuntimeOrigin::none() ); } - RuntimeEvent::Validator(pallet_cf_validator::Event::RotationPhaseUpdated { new_phase: RotationPhase::ActivatingKeys(_) }) => { - // NOTE: This is a little inaccurate a representation of how it actually works. An event is emitted - // which contains the transaction to broadcast for the rotation tx, which the CFE then broadcasts. - // This is a simpler way to represent this in the tests. Representing in this way in the tests also means - // that for dot, given we don't have a key to sign with initially, it will work without extra test boilerplate. - - // If we rotating let's witness the keys being rotated on the contract - queue_dispatch_extrinsic( - RuntimeCall::Witnesser( - pallet_cf_witnesser::Call::witness_at_epoch { - call: Box::new(pallet_cf_vaults::Call::<_, EthereumInstance>::vault_key_rotated { - block_number: 100, - tx_id: [1u8; 32].into(), - }.into()), - epoch_index: Validator::epoch_index(), - }), - RuntimeOrigin::signed(self.node_id.clone()) - ); - } - RuntimeEvent::PolkadotVault(pallet_cf_vaults::Event::<_, PolkadotInstance>::AwaitingGovernanceActivation { .. }) => { queue_dispatch_extrinsic( RuntimeCall::Environment(pallet_cf_environment::Call::witness_polkadot_vault_creation { diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index b58d1ebb80..fd6d6cd6f3 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -399,12 +399,12 @@ pub mod pallet { retries.retain(|broadcast| { if broadcast.broadcast_attempt_id.broadcast_id > pause_broadcast_id { paused_broadcasts.push(BroadcastAttempt:: { - broadcast_attempt_id: (*broadcast).broadcast_attempt_id.clone(), - transaction_payload: (*broadcast).transaction_payload.clone(), - threshold_signature_payload: (*broadcast) + broadcast_attempt_id: broadcast.broadcast_attempt_id, + transaction_payload: broadcast.transaction_payload.clone(), + threshold_signature_payload: broadcast .threshold_signature_payload .clone(), - transaction_out_id: (*broadcast).transaction_out_id.clone(), + transaction_out_id: broadcast.transaction_out_id.clone(), }); false } else { diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index 99a24a539f..7fe4e34cfe 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -497,7 +497,7 @@ fn threshold_sign_and_broadcast_with_callback() { }; let (broadcast_id, _threshold_request_id) = - Broadcaster::threshold_sign_and_broadcast(api_call.clone(), Some(MockCallback)); + Broadcaster::threshold_sign_and_broadcast(api_call.clone(), Some(MockCallback), false); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); diff --git a/state-chain/pallets/cf-emissions/src/mock.rs b/state-chain/pallets/cf-emissions/src/mock.rs index 4f9f70d391..f1ab973b31 100644 --- a/state-chain/pallets/cf-emissions/src/mock.rs +++ b/state-chain/pallets/cf-emissions/src/mock.rs @@ -180,6 +180,7 @@ impl Broadcaster for MockBroadcast { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, + _pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { Self::call(api_call); (1, 2) diff --git a/state-chain/pallets/cf-environment/src/mock.rs b/state-chain/pallets/cf-environment/src/mock.rs index 04de33cc30..b8f5ef976e 100644 --- a/state-chain/pallets/cf-environment/src/mock.rs +++ b/state-chain/pallets/cf-environment/src/mock.rs @@ -105,6 +105,7 @@ impl Broadcaster for MockPolkadotBroadcaster { fn threshold_sign_and_broadcast( _api_call: Self::ApiCall, + _pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { unimplemented!() } diff --git a/state-chain/pallets/cf-funding/src/mock.rs b/state-chain/pallets/cf-funding/src/mock.rs index 4a262df8f0..20031ecab6 100644 --- a/state-chain/pallets/cf-funding/src/mock.rs +++ b/state-chain/pallets/cf-funding/src/mock.rs @@ -162,6 +162,7 @@ impl Broadcaster for MockBroadcaster { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, + _pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { REDEMPTION_BROADCAST_REQUESTS.with(|cell| { cell.borrow_mut().push(api_call.amount); diff --git a/state-chain/pallets/cf-threshold-signature/src/tests.rs b/state-chain/pallets/cf-threshold-signature/src/tests.rs index a00d981e97..0ed818ad50 100644 --- a/state-chain/pallets/cf-threshold-signature/src/tests.rs +++ b/state-chain/pallets/cf-threshold-signature/src/tests.rs @@ -433,7 +433,7 @@ fn test_not_enough_signers_for_threshold_schedules_retry() { } #[test] -fn test_retries_for_locked_key() { +fn test_retries() { const NOMINEES: [u64; 4] = [1, 2, 3, 4]; const AUTHORITIES: [u64; 5] = [1, 2, 3, 4, 5]; new_test_ext() @@ -447,13 +447,6 @@ fn test_retries_for_locked_key() { .. } = EthereumThresholdSigner::pending_ceremonies(ceremony_id).unwrap(); - MockKeyProvider::::lock_key(request_id); - - // Key is now locked and should be unavailable for new requests. - >::request_signature(*b"SUP?"); - // Ceremony counter should not have changed. - assert_eq!(ceremony_id, current_ceremony_id()); - // Retry should re-use the same key. let retry_block = frame_system::Pallet::::current_block_number() + ThresholdSignatureResponseTimeout::::get(); diff --git a/state-chain/pallets/cf-vaults/src/benchmarking.rs b/state-chain/pallets/cf-vaults/src/benchmarking.rs index 0e636901cc..16901bdb53 100644 --- a/state-chain/pallets/cf-vaults/src/benchmarking.rs +++ b/state-chain/pallets/cf-vaults/src/benchmarking.rs @@ -147,20 +147,6 @@ benchmarks_instance_pallet! { if new_public_key == agg_key )) } - vault_key_rotated { - let new_public_key = AggKeyFor::::benchmark_value(); - PendingVaultRotation::::put( - VaultRotationStatus::::AwaitingActivation { new_public_key }, - ); - let call = Call::::vault_key_rotated { - block_number: 5u32.into(), - tx_id: Decode::decode(&mut &TX_HASH[..]).unwrap() - }; - let origin = T::EnsureWitnessedAtCurrentEpoch::try_successful_origin().unwrap(); - } : { call.dispatch_bypass_filter(origin)? } - verify { - assert!(Vaults::::contains_key(T::EpochInfo::epoch_index())); - } vault_key_rotated_externally { let origin = T::EnsureWitnessedAtCurrentEpoch::try_successful_origin().unwrap(); let call = Call::::vault_key_rotated_externally { diff --git a/state-chain/pallets/cf-vaults/src/mock.rs b/state-chain/pallets/cf-vaults/src/mock.rs index d67985ece8..e6773c81ea 100644 --- a/state-chain/pallets/cf-vaults/src/mock.rs +++ b/state-chain/pallets/cf-vaults/src/mock.rs @@ -151,6 +151,7 @@ impl Broadcaster for MockBroadcaster { fn threshold_sign_and_broadcast( _api_call: Self::ApiCall, + _pause_broadcasts: bool, ) -> (BroadcastId, ThresholdSignatureRequestId) { Self::send_broadcast(); (1, 2) diff --git a/state-chain/pallets/cf-vaults/src/tests.rs b/state-chain/pallets/cf-vaults/src/tests.rs index 0276ec6fbd..11f4497370 100644 --- a/state-chain/pallets/cf-vaults/src/tests.rs +++ b/state-chain/pallets/cf-vaults/src/tests.rs @@ -7,11 +7,7 @@ use crate::{ KeygenFailureVoters, KeygenOutcomeFor, KeygenResolutionPendingSince, KeygenResponseTimeout, KeygenSuccessVoters, PalletOffence, PendingVaultRotation, Vault, VaultRotationStatus, Vaults, }; -use cf_chains::{ - btc::BitcoinCrypto, - evm::EvmCrypto, - mocks::{MockAggKey, MockOptimisticActivation}, -}; +use cf_chains::{btc::BitcoinCrypto, evm::EvmCrypto, mocks::MockAggKey}; use cf_primitives::{AuthorityCount, GENESIS_EPOCH}; use cf_test_utilities::{last_event, maybe_last_event}; use cf_traits::{ @@ -647,19 +643,17 @@ fn do_full_key_rotation() { VaultsPallet::activate(); assert!(!KeygenResolutionPendingSince::::exists()); - assert_eq!(::status(), AsyncResult::Pending); + // assert_eq!(::status(), AsyncResult::Pending); - assert!(matches!( - PendingVaultRotation::::get().unwrap(), - VaultRotationStatus::::AwaitingActivation { new_public_key: k } if k == NEW_AGG_PUB_KEY_POST_HANDOVER - )); + // assert!(matches!( + // PendingVaultRotation::::get().unwrap(), + // VaultRotationStatus::::AwaitingActivation { new_public_key: k } if k == + // NEW_AGG_PUB_KEY_POST_HANDOVER )); // Voting has been cleared. assert_eq!(KeygenSuccessVoters::::iter_keys().next(), None); assert!(!KeygenFailureVoters::::exists()); - assert_ok!(VaultsPallet::vault_key_rotated(RuntimeOrigin::root(), 1, [0xab; 4],)); - assert_last_event!(crate::Event::VaultRotationCompleted); assert_eq!(PendingVaultRotation::::get(), Some(VaultRotationStatus::Complete)); assert_eq!(VaultsPallet::status(), AsyncResult::Ready(VaultStatus::RotationComplete)); @@ -799,9 +793,6 @@ mod vault_key_rotation { use super::*; - const ACTIVATION_BLOCK_NUMBER: u64 = 42; - const TX_HASH: [u8; 4] = [0xab; 4]; - fn setup(key_handover_outcome: KeygenOutcomeFor) -> TestRunner<()> { let ext = new_test_ext(); ext.execute_with(|| { @@ -810,15 +801,6 @@ mod vault_key_rotation { let rotation_epoch_index = ::EpochInfo::epoch_index() + 1; - assert_noop!( - VaultsPallet::vault_key_rotated( - RuntimeOrigin::root(), - ACTIVATION_BLOCK_NUMBER, - TX_HASH, - ), - Error::::NoActiveRotation - ); - ::keygen(candidates.clone(), GENESIS_EPOCH); let ceremony_id = current_ceremony_id(); VaultsPallet::trigger_keygen_verification( @@ -853,16 +835,6 @@ mod vault_key_rotation { fn final_checks(ext: TestRunner<()>, expected_activation_block: u64) { ext.execute_with(|| { - // Can't repeat. - assert_noop!( - VaultsPallet::vault_key_rotated( - RuntimeOrigin::root(), - expected_activation_block, - TX_HASH, - ), - Error::::InvalidRotationStatus - ); - let current_epoch = ::EpochInfo::epoch_index(); let Vault { public_key, active_from_block } = @@ -908,16 +880,6 @@ mod vault_key_rotation { BlockHeightProvider::::set_block_height(HANDOVER_ACTIVATION_BLOCK); VaultsPallet::activate(); - // No need to call vault_key_rotated. - assert_noop!( - VaultsPallet::vault_key_rotated( - RuntimeOrigin::root(), - ACTIVATION_BLOCK_NUMBER, - TX_HASH, - ), - Error::::InvalidRotationStatus - ); - assert!(matches!( PendingVaultRotation::::get().unwrap(), VaultRotationStatus::Complete, @@ -957,8 +919,6 @@ mod vault_key_rotation { BtcMockThresholdSigner::execute_signature_result_against_last_request(Ok(vec![ BTC_DUMMY_SIG, ])); - - MockOptimisticActivation::set(true); VaultsPallet::activate(); }); From 51c49b54df346a4105d3f1ad90c8c13fed4e53ab Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 8 Nov 2023 17:37:56 +0100 Subject: [PATCH 04/33] fix: bouncer prettier check --- bouncer/commands/go_bananas.ts | 4 ++-- bouncer/pnpm-lock.yaml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bouncer/commands/go_bananas.ts b/bouncer/commands/go_bananas.ts index cd9fdb3ede..d1f1bc7494 100755 --- a/bouncer/commands/go_bananas.ts +++ b/bouncer/commands/go_bananas.ts @@ -93,7 +93,7 @@ function price2tick(price: number): number { async function playLp(asset: Asset, price: number, liquidity: number) { const spread = 0.01 * price; const liquidityFine = liquidity * 1e6; - for (; ;) { + for (;;) { const offset = (price * (Math.random() - 0.5)) / 20; const buyTick = price2tick(price + offset + spread); const sellTick = price2tick(price + offset - spread); @@ -169,7 +169,7 @@ const swapAmount = new Map([ async function playSwapper() { const assets: Asset[] = ['ETH', 'BTC', 'USDC', 'FLIP', 'DOT']; - for (; ;) { + for (;;) { const src = assets.at(Math.floor(Math.random() * assets.length))!; const dest = assets .filter((x) => x !== src) diff --git a/bouncer/pnpm-lock.yaml b/bouncer/pnpm-lock.yaml index 2d03e99181..3b726dd4dc 100644 --- a/bouncer/pnpm-lock.yaml +++ b/bouncer/pnpm-lock.yaml @@ -1,4 +1,4 @@ -lockfileVersion: '6.0' +lockfileVersion: '6.1' settings: autoInstallPeers: true From c2a2e20e3e1ca75ee2c8fbcfd0dc7f78352a6117 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Thu, 9 Nov 2023 17:34:14 +0100 Subject: [PATCH 05/33] feat: broadcast pause unit test --- state-chain/pallets/cf-broadcast/src/lib.rs | 2 +- state-chain/pallets/cf-broadcast/src/tests.rs | 134 ++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index c03e827618..113c15cd1a 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -429,8 +429,8 @@ pub mod pallet { if retries.len() > num_retries_that_fit { paused_broadcasts.append(&mut retries.split_off(num_retries_that_fit)); - BroadcastRetryQueue::::put(paused_broadcasts); } + BroadcastRetryQueue::::put(paused_broadcasts); let retries_len = retries.len(); diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index 7fe4e34cfe..6d8492cf35 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -579,3 +579,137 @@ fn transaction_succeeded_results_in_refund_refuse_for_signer() { ); }); } + +#[test] +fn broadcast_pause() { + new_test_ext().execute_with(|| { + const TX_OUT_ID_1: [u8; 4] = [0xaa; 4]; + const TX_OUT_ID_2: [u8; 4] = [0xbb; 4]; + const TX_OUT_ID_3: [u8; 4] = [0xcc; 4]; + + let api_call1 = MockApiCall { + payload: Default::default(), + sig: Default::default(), + tx_out_id: TX_OUT_ID_1, + }; + + let api_call2 = MockApiCall { + payload: Default::default(), + sig: Default::default(), + tx_out_id: TX_OUT_ID_2, + }; + + let api_call3 = MockApiCall { + payload: Default::default(), + sig: Default::default(), + tx_out_id: TX_OUT_ID_3, + }; + + // create and sign 3 txs that are then ready for broadcast + + let (broadcast_id_1, _) = + Broadcaster::threshold_sign_and_broadcast(api_call1.clone(), None, false); + + EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); + + // tx1 emits broadcast request + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::TransactionBroadcastRequest { + transaction_out_id: TX_OUT_ID_1, + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_1, + attempt_count: 0, + }, + transaction_payload: Default::default(), + nominee: MockNominator::get_last_nominee().unwrap(), + }, + )); + + let (broadcast_id_2, _) = + Broadcaster::threshold_sign_and_broadcast(api_call2.clone(), None, true); + + EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); + + // tx2 emits broadcast request and also pauses any further new broadcast requests + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::TransactionBroadcastRequest { + transaction_out_id: TX_OUT_ID_2, + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_2, + attempt_count: 0, + }, + transaction_payload: Default::default(), + nominee: MockNominator::get_last_nominee().unwrap(), + }, + )); + + let (broadcast_id_3, _) = + Broadcaster::threshold_sign_and_broadcast(api_call3.clone(), None, false); + + EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); + + // tx3 is ready for broadcast but since there is a broadcast pause, broadcast request is not + // issued, the broadcast is rescheduled instead. + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::BroadcastRetryScheduled { + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_3, + attempt_count: 0, + }, + }, + )); + + // report successful broadcast of tx1 + assert_ok!(Broadcaster::transaction_succeeded( + RuntimeOrigin::root(), + TX_OUT_ID_1, + Default::default(), + ETH_TX_FEE, + MOCK_TX_METADATA, + )); + + // tx3 should still not be broadcasted because the blocking tx (tx2) has still not + // succeeded. + Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); + assert_eq!( + BroadcastRetryQueue::::get()[0] + .broadcast_attempt_id + .broadcast_id, + broadcast_id_3 + ); + + // Now tx2 succeeds which should allow tx3 to be broadcast + assert_ok!(Broadcaster::transaction_succeeded( + RuntimeOrigin::root(), + TX_OUT_ID_2, + Default::default(), + ETH_TX_FEE, + MOCK_TX_METADATA, + )); + Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); + + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::TransactionBroadcastRequest { + transaction_out_id: TX_OUT_ID_3, + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_3, + // attempt count is 1 because the previous failure to broadcast because of + // broadcast pause is considered an attempt + attempt_count: 1, + }, + transaction_payload: Default::default(), + nominee: MockNominator::get_last_nominee().unwrap(), + }, + )); + + assert!(BroadcastRetryQueue::::get().is_empty()); + + assert_ok!(Broadcaster::transaction_succeeded( + RuntimeOrigin::root(), + TX_OUT_ID_3, + Default::default(), + ETH_TX_FEE, + MOCK_TX_METADATA, + )); + }); +} From fe789b99315e9a0c38303e060af9f2e29764cbe7 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 14 Nov 2023 14:01:33 +0100 Subject: [PATCH 06/33] chore: comments --- state-chain/pallets/cf-broadcast/src/lib.rs | 36 +++++++------------ state-chain/pallets/cf-broadcast/src/tests.rs | 24 +++++++------ state-chain/pallets/cf-emissions/src/mock.rs | 8 ++--- .../pallets/cf-environment/src/mock.rs | 8 ++--- state-chain/pallets/cf-funding/src/lib.rs | 2 +- state-chain/pallets/cf-funding/src/mock.rs | 8 ++--- .../pallets/cf-ingress-egress/src/lib.rs | 4 +-- state-chain/pallets/cf-vaults/src/lib.rs | 32 +++++++++++------ state-chain/pallets/cf-vaults/src/mock.rs | 6 ++-- state-chain/pallets/cf-vaults/src/tests.rs | 6 ---- .../pallets/cf-vaults/src/vault_rotator.rs | 2 -- state-chain/pallets/cf-vaults/src/weights.rs | 35 ------------------ state-chain/runtime/src/chainflip.rs | 1 - .../src/chainflip/address_derivation/btc.rs | 4 +-- state-chain/traits/src/lib.rs | 8 ++--- state-chain/traits/src/mocks/broadcaster.rs | 26 +++++--------- 16 files changed, 80 insertions(+), 130 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 113c15cd1a..fab2add813 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -726,8 +726,7 @@ impl, I: 'static> Pallet { pub fn threshold_sign_and_broadcast( api_call: >::ApiCall, maybe_callback: Option<>::BroadcastCallable>, - pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { let broadcast_id = BroadcastIdCounter::::mutate(|id| { *id += 1; *id @@ -743,7 +742,7 @@ impl, I: 'static> Pallet { let initiated_at = T::ChainTracking::get_block_height(); let threshold_signature_payload = api_call.threshold_signature_payload(); - let signature_request_id = T::ThresholdSigner::request_signature_with_callback( + T::ThresholdSigner::request_signature_with_callback( threshold_signature_payload.clone(), |threshold_request_id| { Call::on_signature_ready { @@ -756,11 +755,8 @@ impl, I: 'static> Pallet { .into() }, ); - if pause_broadcasts { - BroadcastPause::::set(Some(broadcast_id)); - } - (broadcast_id, signature_request_id) + broadcast_id } /// Begin the process of broadcasting a transaction. @@ -797,16 +793,7 @@ impl, I: 'static> Pallet { }; if BroadcastPause::::get() - .and_then( - |pause_broadcast_id| { - if broadcast_id > pause_broadcast_id { - Some(()) - } else { - None - } - }, - ) - .is_some() + .is_some_and(|pause_broadcast_id| broadcast_id > pause_broadcast_id) { Self::schedule_for_retry(&broadcast_attempt); } else { @@ -851,10 +838,9 @@ impl, I: 'static> Pallet { // to retry from the threshold signing stage. else { Self::clean_up_broadcast_storage(broadcast_id); - let (retry_broadcast_id, _) = Self::threshold_sign_and_broadcast( + let retry_broadcast_id = Self::threshold_sign_and_broadcast( api_call, RequestCallbacks::::get(broadcast_id), - false, ); log::info!( "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", @@ -935,14 +921,18 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { - Self::threshold_sign_and_broadcast(api_call, None, pause_broadcasts) + ) -> BroadcastId { + let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); + if pause_broadcasts { + BroadcastPause::::set(Some(broadcast_id)); + } + broadcast_id } fn threshold_sign_and_broadcast_with_callback( api_call: Self::ApiCall, callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { - Self::threshold_sign_and_broadcast(api_call, Some(callback), false) + ) -> BroadcastId { + Self::threshold_sign_and_broadcast(api_call, Some(callback)) } } diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index 6d8492cf35..eddcdf43a9 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -2,7 +2,7 @@ use crate::{ mock::*, AwaitingBroadcast, BroadcastAttemptCount, BroadcastAttemptId, BroadcastId, - BroadcastRetryQueue, Error, Event as BroadcastEvent, FailedBroadcasters, Instance1, + BroadcastRetryQueue, Config, Error, Event as BroadcastEvent, FailedBroadcasters, Instance1, PalletOffence, RequestCallbacks, ThresholdSignatureData, Timeouts, TransactionFeeDeficit, TransactionMetadata, TransactionOutIdToBroadcastId, WeightInfo, }; @@ -17,7 +17,8 @@ use cf_chains::{ }; use cf_traits::{ mocks::{signer_nomination::MockNominator, threshold_signer::MockThresholdSigner}, - AsyncResult, Chainflip, EpochInfo, SetSafeMode, ThresholdSigner, + AsyncResult, Broadcaster as BroadcasterTrait, Chainflip, EpochInfo, SetSafeMode, + ThresholdSigner, }; use frame_support::{assert_noop, assert_ok, dispatch::Weight, traits::Hooks}; use frame_system::RawOrigin; @@ -496,8 +497,8 @@ fn threshold_sign_and_broadcast_with_callback() { tx_out_id: MOCK_TRANSACTION_OUT_ID, }; - let (broadcast_id, _threshold_request_id) = - Broadcaster::threshold_sign_and_broadcast(api_call.clone(), Some(MockCallback), false); + let broadcast_id = + Broadcaster::threshold_sign_and_broadcast(api_call.clone(), Some(MockCallback)); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); @@ -607,8 +608,9 @@ fn broadcast_pause() { // create and sign 3 txs that are then ready for broadcast - let (broadcast_id_1, _) = - Broadcaster::threshold_sign_and_broadcast(api_call1.clone(), None, false); + let broadcast_id_1 = >::TargetChain, + >>::threshold_sign_and_broadcast(api_call1.clone(), false); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); @@ -625,8 +627,9 @@ fn broadcast_pause() { }, )); - let (broadcast_id_2, _) = - Broadcaster::threshold_sign_and_broadcast(api_call2.clone(), None, true); + let broadcast_id_2 = >::TargetChain, + >>::threshold_sign_and_broadcast(api_call2.clone(), true); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); @@ -643,8 +646,9 @@ fn broadcast_pause() { }, )); - let (broadcast_id_3, _) = - Broadcaster::threshold_sign_and_broadcast(api_call3.clone(), None, false); + let broadcast_id_3 = >::TargetChain, + >>::threshold_sign_and_broadcast(api_call3.clone(), false); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); diff --git a/state-chain/pallets/cf-emissions/src/mock.rs b/state-chain/pallets/cf-emissions/src/mock.rs index f1ab973b31..bb6c91062d 100644 --- a/state-chain/pallets/cf-emissions/src/mock.rs +++ b/state-chain/pallets/cf-emissions/src/mock.rs @@ -5,7 +5,7 @@ use cf_chains::{ mocks::{MockEthereum, MockEthereumChainCrypto}, AnyChain, ApiCall, ChainCrypto, UpdateFlipSupply, }; -use cf_primitives::{BroadcastId, FlipBalance, ThresholdSignatureRequestId}; +use cf_primitives::{BroadcastId, FlipBalance}; use cf_traits::{ impl_mock_callback, impl_mock_chainflip, impl_mock_runtime_safe_mode, impl_mock_waived_fees, mocks::{egress_handler::MockEgressHandler, eth_environment_provider::MockEthEnvironment}, @@ -181,15 +181,15 @@ impl Broadcaster for MockBroadcast { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, _pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { Self::call(api_call); - (1, 2) + 1 } fn threshold_sign_and_broadcast_with_callback( _api_call: Self::ApiCall, _callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { unimplemented!() } } diff --git a/state-chain/pallets/cf-environment/src/mock.rs b/state-chain/pallets/cf-environment/src/mock.rs index b8f5ef976e..66e6304be7 100644 --- a/state-chain/pallets/cf-environment/src/mock.rs +++ b/state-chain/pallets/cf-environment/src/mock.rs @@ -7,8 +7,8 @@ use cf_chains::{ eth, ApiCall, Bitcoin, Chain, ChainCrypto, Polkadot, }; use cf_primitives::{ - BroadcastId, SemVer, ThresholdSignatureRequestId, INPUT_UTXO_SIZE_IN_BYTES, - MINIMUM_BTC_TX_SIZE_IN_BYTES, OUTPUT_UTXO_SIZE_IN_BYTES, + BroadcastId, SemVer, INPUT_UTXO_SIZE_IN_BYTES, MINIMUM_BTC_TX_SIZE_IN_BYTES, + OUTPUT_UTXO_SIZE_IN_BYTES, }; use cf_traits::{ impl_mock_callback, impl_mock_chainflip, impl_mock_runtime_safe_mode, impl_pallet_safe_mode, @@ -106,14 +106,14 @@ impl Broadcaster for MockPolkadotBroadcaster { fn threshold_sign_and_broadcast( _api_call: Self::ApiCall, _pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { unimplemented!() } fn threshold_sign_and_broadcast_with_callback( _api_call: Self::ApiCall, _callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { unimplemented!() } } diff --git a/state-chain/pallets/cf-funding/src/lib.rs b/state-chain/pallets/cf-funding/src/lib.rs index f59571765d..e5ad67eb5f 100644 --- a/state-chain/pallets/cf-funding/src/lib.rs +++ b/state-chain/pallets/cf-funding/src/lib.rs @@ -458,7 +458,7 @@ pub mod pallet { Self::deposit_event(Event::RedemptionRequested { account_id, amount: net_amount, - broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call, false).0, + broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call, false), expiry_time: contract_expiry, }); } else { diff --git a/state-chain/pallets/cf-funding/src/mock.rs b/state-chain/pallets/cf-funding/src/mock.rs index 20031ecab6..6907ddf743 100644 --- a/state-chain/pallets/cf-funding/src/mock.rs +++ b/state-chain/pallets/cf-funding/src/mock.rs @@ -1,7 +1,7 @@ use crate as pallet_cf_funding; use crate::PalletSafeMode; use cf_chains::{evm::EvmCrypto, ApiCall, Chain, ChainCrypto, Ethereum}; -use cf_primitives::{AccountRole, BroadcastId, ThresholdSignatureRequestId}; +use cf_primitives::{AccountRole, BroadcastId}; use cf_traits::{ impl_mock_callback, impl_mock_chainflip, impl_mock_runtime_safe_mode, impl_mock_waived_fees, mocks::time_source, AccountRoleRegistry, Broadcaster, WaivedFees, @@ -163,17 +163,17 @@ impl Broadcaster for MockBroadcaster { fn threshold_sign_and_broadcast( api_call: Self::ApiCall, _pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { REDEMPTION_BROADCAST_REQUESTS.with(|cell| { cell.borrow_mut().push(api_call.amount); }); - (0, 1) + 0 } fn threshold_sign_and_broadcast_with_callback( _api_call: Self::ApiCall, _callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { unimplemented!() } } diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index 3c2eb0877b..918e4735a2 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -728,7 +728,7 @@ impl, I: 'static> Pallet { transfer_params, ) { Ok(egress_transaction) => { - let (broadcast_id, _) = T::Broadcaster::threshold_sign_and_broadcast_with_callback( + let broadcast_id = T::Broadcaster::threshold_sign_and_broadcast_with_callback( egress_transaction, Call::finalise_ingress { addresses }.into(), ); @@ -769,7 +769,7 @@ impl, I: 'static> Pallet { ccm.message.to_vec(), ) { Ok(api_call) => { - let (broadcast_id, _) = + let broadcast_id = T::Broadcaster::threshold_sign_and_broadcast(api_call, false); Self::deposit_event(Event::::CcmBroadcastRequested { broadcast_id, diff --git a/state-chain/pallets/cf-vaults/src/lib.rs b/state-chain/pallets/cf-vaults/src/lib.rs index acedf49103..51dad7cc83 100644 --- a/state-chain/pallets/cf-vaults/src/lib.rs +++ b/state-chain/pallets/cf-vaults/src/lib.rs @@ -153,11 +153,6 @@ pub enum PalletOffence { FailedKeyHandover, } -#[derive(Encode, Decode, TypeInfo)] -pub struct VaultEpoch { - pub epoch_index: EpochIndex, -} - #[frame_support::pallet] pub mod pallet { @@ -371,7 +366,7 @@ pub mod pallet { /// The epoch whose authorities control the current vault key. #[pallet::storage] #[pallet::getter(fn current_keyholders_epoch)] - pub type CurrentVaultEpoch, I: 'static = ()> = StorageValue<_, VaultEpoch>; + pub type CurrentVaultEpoch, I: 'static = ()> = StorageValue<_, EpochIndex>; /// Vault rotation statuses for the current epoch rotation. #[pallet::storage] @@ -678,6 +673,21 @@ pub mod pallet { ) } + /// Deprecated! This extrinsic does nothing + #[pallet::call_index(3)] + #[pallet::weight(Weight::zero())] + pub fn vault_key_rotated( + origin: OriginFor, + _block_number: ChainBlockNumberFor, + + // This field is primarily required to ensure the witness calls are unique per + // transaction (on the external chain) + _tx_id: TransactionInIdFor, + ) -> DispatchResultWithPostInfo { + T::EnsureWitnessedAtCurrentEpoch::ensure_origin(origin)?; + Ok(().into()) + } + /// The vault's key has been updated externally, outside of the rotation /// cycle. This is an unexpected event as far as our chain is concerned, and /// the only thing we can do is to halt and wait for further governance @@ -844,7 +854,7 @@ impl, I: 'static> Pallet { fn set_vault_key_for_epoch(epoch_index: EpochIndex, vault: Vault) { Vaults::::insert(epoch_index, vault); - CurrentVaultEpoch::::put(VaultEpoch { epoch_index }); + CurrentVaultEpoch::::put(epoch_index); } // Once we've successfully generated the key, we want to do a signing ceremony to verify that @@ -953,11 +963,11 @@ impl, I: 'static> Pallet { impl, I: 'static> KeyProvider<::ChainCrypto> for Pallet { fn active_epoch_key( ) -> Option::ChainCrypto as ChainCrypto>::AggKey>> { - CurrentVaultEpoch::::get().map(|current_vault_epoch_and_state| { + CurrentVaultEpoch::::get().map(|current_vault_epoch| { EpochKey { - key: Vaults::::get(current_vault_epoch_and_state.epoch_index) - .expect("Key must exist if CurrentVaultEpoch exists since they get set at the same place: set_next_vault()").public_key, - epoch_index: current_vault_epoch_and_state.epoch_index, + key: Vaults::::get(current_vault_epoch) + .expect("Key must exist if CurrentVaultEpoch exists since they get set at the same place: set_vault_key_for_epoch()").public_key, + epoch_index: current_vault_epoch, } }) } diff --git a/state-chain/pallets/cf-vaults/src/mock.rs b/state-chain/pallets/cf-vaults/src/mock.rs index bcd69e41c1..488e40fcc8 100644 --- a/state-chain/pallets/cf-vaults/src/mock.rs +++ b/state-chain/pallets/cf-vaults/src/mock.rs @@ -152,15 +152,15 @@ impl Broadcaster for MockBroadcaster { fn threshold_sign_and_broadcast( _api_call: Self::ApiCall, _pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { Self::send_broadcast(); - (1, 2) + 1 } fn threshold_sign_and_broadcast_with_callback( _api_call: Self::ApiCall, _callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { + ) -> BroadcastId { unimplemented!() } } diff --git a/state-chain/pallets/cf-vaults/src/tests.rs b/state-chain/pallets/cf-vaults/src/tests.rs index b90e66d159..0829737c1d 100644 --- a/state-chain/pallets/cf-vaults/src/tests.rs +++ b/state-chain/pallets/cf-vaults/src/tests.rs @@ -643,12 +643,6 @@ fn do_full_key_rotation() { VaultsPallet::activate(); assert!(!KeygenResolutionPendingSince::::exists()); - // assert_eq!(::status(), AsyncResult::Pending); - - // assert!(matches!( - // PendingVaultRotation::::get().unwrap(), - // VaultRotationStatus::::AwaitingActivation { new_public_key: k } if k == - // NEW_AGG_PUB_KEY_POST_HANDOVER )); // Voting has been cleared. assert_eq!(KeygenSuccessVoters::::iter_keys().next(), None); diff --git a/state-chain/pallets/cf-vaults/src/vault_rotator.rs b/state-chain/pallets/cf-vaults/src/vault_rotator.rs index 9a8128225f..d57c3805e0 100644 --- a/state-chain/pallets/cf-vaults/src/vault_rotator.rs +++ b/state-chain/pallets/cf-vaults/src/vault_rotator.rs @@ -160,8 +160,6 @@ impl, I: 'static> VaultRotator for Pallet { Ok(activation_call) => { T::Broadcaster::threshold_sign_and_broadcast(activation_call, true); - // Optimistic activation means we don't need to wait for the activation - // transaction to succeed before using the new key. Self::activate_new_key( new_public_key, T::ChainTracking::get_block_height(), diff --git a/state-chain/pallets/cf-vaults/src/weights.rs b/state-chain/pallets/cf-vaults/src/weights.rs index 982ffdff84..697ad90f0d 100644 --- a/state-chain/pallets/cf-vaults/src/weights.rs +++ b/state-chain/pallets/cf-vaults/src/weights.rs @@ -36,7 +36,6 @@ pub trait WeightInfo { fn on_initialize_success() -> Weight; fn report_keygen_outcome() -> Weight; fn on_keygen_verification_result() -> Weight; - fn vault_key_rotated() -> Weight; fn vault_key_rotated_externally() -> Weight; fn set_keygen_response_timeout() -> Weight; } @@ -132,23 +131,6 @@ impl WeightInfo for PalletWeight { .saturating_add(T::DbWeight::get().reads(1_u64)) .saturating_add(T::DbWeight::get().writes(2_u64)) } - /// Storage: `EthereumVault::PendingVaultRotation` (r:1 w:1) - /// Proof: `EthereumVault::PendingVaultRotation` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `Validator::CurrentEpoch` (r:1 w:0) - /// Proof: `Validator::CurrentEpoch` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `EthereumVault::Vaults` (r:0 w:1) - /// Proof: `EthereumVault::Vaults` (`max_values`: None, `max_size`: None, mode: `Measured`) - /// Storage: `EthereumVault::CurrentVaultEpochAndState` (r:0 w:1) - /// Proof: `EthereumVault::CurrentVaultEpochAndState` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn vault_key_rotated() -> Weight { - // Proof Size summary in bytes: - // Measured: `493` - // Estimated: `1978` - // Minimum execution time: 23_429_000 picoseconds. - Weight::from_parts(23_875_000, 1978) - .saturating_add(T::DbWeight::get().reads(2_u64)) - .saturating_add(T::DbWeight::get().writes(3_u64)) - } /// Storage: `Validator::CurrentEpoch` (r:1 w:0) /// Proof: `Validator::CurrentEpoch` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) /// Storage: `EthereumVault::Vaults` (r:0 w:1) @@ -271,23 +253,6 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1_u64)) .saturating_add(RocksDbWeight::get().writes(2_u64)) } - /// Storage: `EthereumVault::PendingVaultRotation` (r:1 w:1) - /// Proof: `EthereumVault::PendingVaultRotation` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `Validator::CurrentEpoch` (r:1 w:0) - /// Proof: `Validator::CurrentEpoch` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - /// Storage: `EthereumVault::Vaults` (r:0 w:1) - /// Proof: `EthereumVault::Vaults` (`max_values`: None, `max_size`: None, mode: `Measured`) - /// Storage: `EthereumVault::CurrentVaultEpochAndState` (r:0 w:1) - /// Proof: `EthereumVault::CurrentVaultEpochAndState` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) - fn vault_key_rotated() -> Weight { - // Proof Size summary in bytes: - // Measured: `493` - // Estimated: `1978` - // Minimum execution time: 23_429_000 picoseconds. - Weight::from_parts(23_875_000, 1978) - .saturating_add(RocksDbWeight::get().reads(2_u64)) - .saturating_add(RocksDbWeight::get().writes(3_u64)) - } /// Storage: `Validator::CurrentEpoch` (r:1 w:0) /// Proof: `Validator::CurrentEpoch` (`max_values`: Some(1), `max_size`: None, mode: `Measured`) /// Storage: `EthereumVault::Vaults` (r:0 w:1) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 111d4b1efc..b963655f3e 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -431,7 +431,6 @@ impl CommKeyBroadcaster for TokenholderGovernanceBroadcaster { EthereumBroadcaster::threshold_sign_and_broadcast( SetCommKeyWithAggKey::::new_unsigned(new_key), None::, - false, ); } } diff --git a/state-chain/runtime/src/chainflip/address_derivation/btc.rs b/state-chain/runtime/src/chainflip/address_derivation/btc.rs index 436df1da74..05e5ccc197 100644 --- a/state-chain/runtime/src/chainflip/address_derivation/btc.rs +++ b/state-chain/runtime/src/chainflip/address_derivation/btc.rs @@ -50,7 +50,7 @@ fn test_address_generation() { use cf_primitives::chains::assets::btc; use cf_utilities::assert_ok; use pallet_cf_validator::CurrentEpoch; - use pallet_cf_vaults::{CurrentVaultEpoch, Vault, VaultEpoch, Vaults}; + use pallet_cf_vaults::{CurrentVaultEpoch, Vault, Vaults}; frame_support::sp_io::TestExternalities::new_empty().execute_with(|| { CurrentEpoch::::set(1); @@ -66,7 +66,7 @@ fn test_address_generation() { active_from_block: 1, }, ); - CurrentVaultEpoch::::put(VaultEpoch { epoch_index: 1 }); + CurrentVaultEpoch::::put(1); assert_ok!(>::generate_address( btc::Asset::Btc, 1 diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index 07ba9944a3..a08084bab6 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -456,17 +456,15 @@ pub trait Broadcaster { type Callback: UnfilteredDispatchable; /// Request a threshold signature and then build and broadcast the outbound api call. - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - pause_broadcasts: bool, - ) -> (BroadcastId, ThresholdSignatureRequestId); + fn threshold_sign_and_broadcast(api_call: Self::ApiCall, pause_broadcasts: bool) + -> BroadcastId; /// Like `threshold_sign_and_broadcast` but also registers a callback to be dispatched when the /// signature accepted event has been witnessed. fn threshold_sign_and_broadcast_with_callback( api_call: Self::ApiCall, callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId); + ) -> BroadcastId; } /// The heartbeat of the network diff --git a/state-chain/traits/src/mocks/broadcaster.rs b/state-chain/traits/src/mocks/broadcaster.rs index 2dafe2468f..8b3cb19b93 100644 --- a/state-chain/traits/src/mocks/broadcaster.rs +++ b/state-chain/traits/src/mocks/broadcaster.rs @@ -1,5 +1,5 @@ use cf_chains::{ApiCall, Chain}; -use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; +use cf_primitives::BroadcastId; use core::marker::PhantomData; use frame_support::{ traits::{OriginTrait, UnfilteredDispatchable}, @@ -29,31 +29,23 @@ impl< fn threshold_sign_and_broadcast( api_call: Self::ApiCall, _pause_broadcasts: bool, - ) -> (cf_primitives::BroadcastId, cf_primitives::ThresholdSignatureRequestId) { + ) -> cf_primitives::BroadcastId { Self::mutate_value(b"API_CALLS", |api_calls: &mut Option>| { let api_calls = api_calls.get_or_insert(Default::default()); api_calls.push(api_call); }); - ( - ::mutate_value(b"BROADCAST_ID", |v: &mut Option| { - let v = v.get_or_insert(0); - *v += 1; - *v - }), - ::mutate_value(b"THRESHOLD_ID", |v: &mut Option| { - let v = v.get_or_insert(0); - *v += 1; - *v - }), - ) + ::mutate_value(b"BROADCAST_ID", |v: &mut Option| { + let v = v.get_or_insert(0); + *v += 1; + *v + }) } fn threshold_sign_and_broadcast_with_callback( api_call: Self::ApiCall, callback: Self::Callback, - ) -> (BroadcastId, ThresholdSignatureRequestId) { - let ids @ (id, _) = - >::threshold_sign_and_broadcast(api_call, false); + ) -> BroadcastId { + let ids @ id = >::threshold_sign_and_broadcast(api_call, false); Self::put_storage(b"CALLBACKS", id, callback); ids } From b42ef29e41ca97e6cb05abc916ffb0bc0d08dced Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 14 Nov 2023 15:30:47 +0100 Subject: [PATCH 07/33] chore: test --- state-chain/pallets/cf-broadcast/src/tests.rs | 90 +++++++------------ 1 file changed, 31 insertions(+), 59 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index eddcdf43a9..ecf8ac5090 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -584,27 +584,9 @@ fn transaction_succeeded_results_in_refund_refuse_for_signer() { #[test] fn broadcast_pause() { new_test_ext().execute_with(|| { - const TX_OUT_ID_1: [u8; 4] = [0xaa; 4]; - const TX_OUT_ID_2: [u8; 4] = [0xbb; 4]; - const TX_OUT_ID_3: [u8; 4] = [0xcc; 4]; - - let api_call1 = MockApiCall { - payload: Default::default(), - sig: Default::default(), - tx_out_id: TX_OUT_ID_1, - }; - - let api_call2 = MockApiCall { - payload: Default::default(), - sig: Default::default(), - tx_out_id: TX_OUT_ID_2, - }; - - let api_call3 = MockApiCall { - payload: Default::default(), - sig: Default::default(), - tx_out_id: TX_OUT_ID_3, - }; + let (tx_out_id1, api_call1) = api_call(1); + let (tx_out_id2, api_call2) = api_call(2); + let (tx_out_id3, api_call3) = api_call(3); // create and sign 3 txs that are then ready for broadcast @@ -615,17 +597,7 @@ fn broadcast_pause() { EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); // tx1 emits broadcast request - System::assert_last_event(RuntimeEvent::Broadcaster( - crate::Event::::TransactionBroadcastRequest { - transaction_out_id: TX_OUT_ID_1, - broadcast_attempt_id: BroadcastAttemptId { - broadcast_id: broadcast_id_1, - attempt_count: 0, - }, - transaction_payload: Default::default(), - nominee: MockNominator::get_last_nominee().unwrap(), - }, - )); + assert_transaction_broadcast_request_event(broadcast_id_1, 0, tx_out_id1); let broadcast_id_2 = >::TargetChain, @@ -634,17 +606,7 @@ fn broadcast_pause() { EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); // tx2 emits broadcast request and also pauses any further new broadcast requests - System::assert_last_event(RuntimeEvent::Broadcaster( - crate::Event::::TransactionBroadcastRequest { - transaction_out_id: TX_OUT_ID_2, - broadcast_attempt_id: BroadcastAttemptId { - broadcast_id: broadcast_id_2, - attempt_count: 0, - }, - transaction_payload: Default::default(), - nominee: MockNominator::get_last_nominee().unwrap(), - }, - )); + assert_transaction_broadcast_request_event(broadcast_id_2, 0, tx_out_id2); let broadcast_id_3 = >::TargetChain, @@ -666,7 +628,7 @@ fn broadcast_pause() { // report successful broadcast of tx1 assert_ok!(Broadcaster::transaction_succeeded( RuntimeOrigin::root(), - TX_OUT_ID_1, + tx_out_id1, Default::default(), ETH_TX_FEE, MOCK_TX_METADATA, @@ -685,35 +647,45 @@ fn broadcast_pause() { // Now tx2 succeeds which should allow tx3 to be broadcast assert_ok!(Broadcaster::transaction_succeeded( RuntimeOrigin::root(), - TX_OUT_ID_2, + tx_out_id2, Default::default(), ETH_TX_FEE, MOCK_TX_METADATA, )); Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); - System::assert_last_event(RuntimeEvent::Broadcaster( - crate::Event::::TransactionBroadcastRequest { - transaction_out_id: TX_OUT_ID_3, - broadcast_attempt_id: BroadcastAttemptId { - broadcast_id: broadcast_id_3, - // attempt count is 1 because the previous failure to broadcast because of - // broadcast pause is considered an attempt - attempt_count: 1, - }, - transaction_payload: Default::default(), - nominee: MockNominator::get_last_nominee().unwrap(), - }, - )); + // attempt count is 1 because the previous failure to broadcast because of + // broadcast pause is considered an attempt + assert_transaction_broadcast_request_event(broadcast_id_3, 1, tx_out_id3); assert!(BroadcastRetryQueue::::get().is_empty()); assert_ok!(Broadcaster::transaction_succeeded( RuntimeOrigin::root(), - TX_OUT_ID_3, + tx_out_id3, Default::default(), ETH_TX_FEE, MOCK_TX_METADATA, )); }); } + +fn api_call(i: u8) -> ([u8; 4], MockApiCall) { + let tx_out_id = [i; 4]; + (tx_out_id, MockApiCall { tx_out_id, sig: Default::default(), payload: Default::default() }) +} + +fn assert_transaction_broadcast_request_event( + broadcast_id: BroadcastId, + attempt_count: u32, + tx_out_id: [u8; 4], +) { + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::TransactionBroadcastRequest { + transaction_out_id: tx_out_id, + broadcast_attempt_id: BroadcastAttemptId { broadcast_id, attempt_count }, + transaction_payload: Default::default(), + nominee: MockNominator::get_last_nominee().unwrap(), + }, + )); +} From 06b7df2d3a3144ebac7cab547600acdc845cb41a Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 14 Nov 2023 16:49:12 +0100 Subject: [PATCH 08/33] chore: addressed comments --- state-chain/pallets/cf-broadcast/src/lib.rs | 38 +++++++------------ state-chain/pallets/cf-broadcast/src/tests.rs | 6 +-- state-chain/pallets/cf-emissions/src/lib.rs | 11 ++---- state-chain/pallets/cf-emissions/src/mock.rs | 9 +++-- .../pallets/cf-environment/src/mock.rs | 9 +++-- state-chain/pallets/cf-funding/src/lib.rs | 2 +- state-chain/pallets/cf-funding/src/mock.rs | 9 +++-- .../pallets/cf-ingress-egress/src/lib.rs | 3 +- state-chain/pallets/cf-vaults/src/mock.rs | 9 +++-- .../pallets/cf-vaults/src/vault_rotator.rs | 2 +- state-chain/runtime/src/chainflip.rs | 2 +- state-chain/traits/src/lib.rs | 7 +++- state-chain/traits/src/mocks/broadcaster.rs | 11 +++--- 13 files changed, 56 insertions(+), 62 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index fab2add813..6cbd38c025 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -1,6 +1,7 @@ #![cfg_attr(not(feature = "std"), no_std)] #![doc = include_str!("../README.md")] #![doc = include_str!("../../cf-doc-head.md")] +#![feature(extract_if)] mod benchmarking; mod mock; @@ -410,21 +411,11 @@ pub mod pallet { let mut paused_broadcasts = vec![]; if let Some(pause_broadcast_id) = BroadcastPause::::get() { - retries.retain(|broadcast| { - if broadcast.broadcast_attempt_id.broadcast_id > pause_broadcast_id { - paused_broadcasts.push(BroadcastAttempt:: { - broadcast_attempt_id: broadcast.broadcast_attempt_id, - transaction_payload: broadcast.transaction_payload.clone(), - threshold_signature_payload: broadcast - .threshold_signature_payload - .clone(), - transaction_out_id: broadcast.transaction_out_id.clone(), - }); - false - } else { - true - } - }); + paused_broadcasts = retries + .extract_if(|broadcast| { + broadcast.broadcast_attempt_id.broadcast_id > pause_broadcast_id + }) + .collect::>(); } if retries.len() > num_retries_that_fit { @@ -918,15 +909,8 @@ impl, I: 'static> Broadcaster for Pallet { type ApiCall = T::ApiCall; type Callback = >::BroadcastCallable; - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - pause_broadcasts: bool, - ) -> BroadcastId { - let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); - if pause_broadcasts { - BroadcastPause::::set(Some(broadcast_id)); - } - broadcast_id + fn threshold_sign_and_broadcast(api_call: Self::ApiCall) -> BroadcastId { + Self::threshold_sign_and_broadcast(api_call, None) } fn threshold_sign_and_broadcast_with_callback( @@ -935,4 +919,10 @@ impl, I: 'static> Broadcaster for Pallet { ) -> BroadcastId { Self::threshold_sign_and_broadcast(api_call, Some(callback)) } + + fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { + let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); + BroadcastPause::::set(Some(broadcast_id)); + broadcast_id + } } diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index ecf8ac5090..bb9c129a66 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -592,7 +592,7 @@ fn broadcast_pause() { let broadcast_id_1 = >::TargetChain, - >>::threshold_sign_and_broadcast(api_call1.clone(), false); + >>::threshold_sign_and_broadcast(api_call1.clone()); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); @@ -601,7 +601,7 @@ fn broadcast_pause() { let broadcast_id_2 = >::TargetChain, - >>::threshold_sign_and_broadcast(api_call2.clone(), true); + >>::threshold_sign_and_broadcast_rotation_tx(api_call2.clone()); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); @@ -610,7 +610,7 @@ fn broadcast_pause() { let broadcast_id_3 = >::TargetChain, - >>::threshold_sign_and_broadcast(api_call3.clone(), false); + >>::threshold_sign_and_broadcast(api_call3.clone()); EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); diff --git a/state-chain/pallets/cf-emissions/src/lib.rs b/state-chain/pallets/cf-emissions/src/lib.rs index 95529bf47d..549b723a7d 100644 --- a/state-chain/pallets/cf-emissions/src/lib.rs +++ b/state-chain/pallets/cf-emissions/src/lib.rs @@ -302,13 +302,10 @@ impl Pallet { ) { // Emit a threshold signature request. // TODO: See if we can replace an old request if there is one. - T::Broadcaster::threshold_sign_and_broadcast( - T::ApiCall::new_unsigned( - total_supply.unique_saturated_into(), - block_number.saturated_into(), - ), - false, - ); + T::Broadcaster::threshold_sign_and_broadcast(T::ApiCall::new_unsigned( + total_supply.unique_saturated_into(), + block_number.saturated_into(), + )); } } diff --git a/state-chain/pallets/cf-emissions/src/mock.rs b/state-chain/pallets/cf-emissions/src/mock.rs index bb6c91062d..0b83caf681 100644 --- a/state-chain/pallets/cf-emissions/src/mock.rs +++ b/state-chain/pallets/cf-emissions/src/mock.rs @@ -178,10 +178,7 @@ impl Broadcaster for MockBroadcast { type ApiCall = MockUpdateFlipSupply; type Callback = MockCallback; - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - _pause_broadcasts: bool, - ) -> BroadcastId { + fn threshold_sign_and_broadcast(api_call: Self::ApiCall) -> BroadcastId { Self::call(api_call); 1 } @@ -192,6 +189,10 @@ impl Broadcaster for MockBroadcast { ) -> BroadcastId { unimplemented!() } + + fn threshold_sign_and_broadcast_rotation_tx(_api_call: Self::ApiCall) -> BroadcastId { + unimplemented!() + } } impl_mock_runtime_safe_mode! { emissions: PalletSafeMode } diff --git a/state-chain/pallets/cf-environment/src/mock.rs b/state-chain/pallets/cf-environment/src/mock.rs index 66e6304be7..7174de7a9a 100644 --- a/state-chain/pallets/cf-environment/src/mock.rs +++ b/state-chain/pallets/cf-environment/src/mock.rs @@ -103,10 +103,7 @@ impl Broadcaster for MockPolkadotBroadcaster { type ApiCall = MockCreatePolkadotVault; type Callback = MockCallback; - fn threshold_sign_and_broadcast( - _api_call: Self::ApiCall, - _pause_broadcasts: bool, - ) -> BroadcastId { + fn threshold_sign_and_broadcast(_api_call: Self::ApiCall) -> BroadcastId { unimplemented!() } @@ -116,6 +113,10 @@ impl Broadcaster for MockPolkadotBroadcaster { ) -> BroadcastId { unimplemented!() } + + fn threshold_sign_and_broadcast_rotation_tx(_api_call: Self::ApiCall) -> BroadcastId { + unimplemented!() + } } pub struct MockPolkadotVaultKeyWitnessedHandler; impl VaultKeyWitnessedHandler for MockPolkadotVaultKeyWitnessedHandler { diff --git a/state-chain/pallets/cf-funding/src/lib.rs b/state-chain/pallets/cf-funding/src/lib.rs index e5ad67eb5f..626f0c1d88 100644 --- a/state-chain/pallets/cf-funding/src/lib.rs +++ b/state-chain/pallets/cf-funding/src/lib.rs @@ -458,7 +458,7 @@ pub mod pallet { Self::deposit_event(Event::RedemptionRequested { account_id, amount: net_amount, - broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call, false), + broadcast_id: T::Broadcaster::threshold_sign_and_broadcast(call), expiry_time: contract_expiry, }); } else { diff --git a/state-chain/pallets/cf-funding/src/mock.rs b/state-chain/pallets/cf-funding/src/mock.rs index 6907ddf743..e29cacb784 100644 --- a/state-chain/pallets/cf-funding/src/mock.rs +++ b/state-chain/pallets/cf-funding/src/mock.rs @@ -160,10 +160,7 @@ impl Broadcaster for MockBroadcaster { type ApiCall = MockRegisterRedemption; type Callback = MockCallback; - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - _pause_broadcasts: bool, - ) -> BroadcastId { + fn threshold_sign_and_broadcast(api_call: Self::ApiCall) -> BroadcastId { REDEMPTION_BROADCAST_REQUESTS.with(|cell| { cell.borrow_mut().push(api_call.amount); }); @@ -176,6 +173,10 @@ impl Broadcaster for MockBroadcaster { ) -> BroadcastId { unimplemented!() } + + fn threshold_sign_and_broadcast_rotation_tx(_api_call: Self::ApiCall) -> BroadcastId { + unimplemented!() + } } impl_mock_runtime_safe_mode! { funding: PalletSafeMode } diff --git a/state-chain/pallets/cf-ingress-egress/src/lib.rs b/state-chain/pallets/cf-ingress-egress/src/lib.rs index 918e4735a2..cc114fd542 100644 --- a/state-chain/pallets/cf-ingress-egress/src/lib.rs +++ b/state-chain/pallets/cf-ingress-egress/src/lib.rs @@ -769,8 +769,7 @@ impl, I: 'static> Pallet { ccm.message.to_vec(), ) { Ok(api_call) => { - let broadcast_id = - T::Broadcaster::threshold_sign_and_broadcast(api_call, false); + let broadcast_id = T::Broadcaster::threshold_sign_and_broadcast(api_call); Self::deposit_event(Event::::CcmBroadcastRequested { broadcast_id, egress_id: ccm.egress_id, diff --git a/state-chain/pallets/cf-vaults/src/mock.rs b/state-chain/pallets/cf-vaults/src/mock.rs index 488e40fcc8..d6fa1eec94 100644 --- a/state-chain/pallets/cf-vaults/src/mock.rs +++ b/state-chain/pallets/cf-vaults/src/mock.rs @@ -149,10 +149,7 @@ impl Broadcaster for MockBroadcaster { type ApiCall = MockSetAggKeyWithAggKey; type Callback = MockCallback; - fn threshold_sign_and_broadcast( - _api_call: Self::ApiCall, - _pause_broadcasts: bool, - ) -> BroadcastId { + fn threshold_sign_and_broadcast(_api_call: Self::ApiCall) -> BroadcastId { Self::send_broadcast(); 1 } @@ -163,6 +160,10 @@ impl Broadcaster for MockBroadcaster { ) -> BroadcastId { unimplemented!() } + + fn threshold_sign_and_broadcast_rotation_tx(_api_call: Self::ApiCall) -> BroadcastId { + unimplemented!() + } } parameter_types! { diff --git a/state-chain/pallets/cf-vaults/src/vault_rotator.rs b/state-chain/pallets/cf-vaults/src/vault_rotator.rs index d57c3805e0..4af31a98b5 100644 --- a/state-chain/pallets/cf-vaults/src/vault_rotator.rs +++ b/state-chain/pallets/cf-vaults/src/vault_rotator.rs @@ -158,7 +158,7 @@ impl, I: 'static> VaultRotator for Pallet { new_public_key, ) { Ok(activation_call) => { - T::Broadcaster::threshold_sign_and_broadcast(activation_call, true); + T::Broadcaster::threshold_sign_and_broadcast_rotation_tx(activation_call); Self::activate_new_key( new_public_key, diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index b963655f3e..33a4e93351 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -391,7 +391,7 @@ impl TokenholderGovernanceBroadcaster { maybe_old_key, Decode::decode(&mut &new_key[..]).or(Err(()))?, )?; - B::threshold_sign_and_broadcast(api_call, false); + B::threshold_sign_and_broadcast(api_call); Ok(()) } diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index a08084bab6..173b61eb6e 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -456,8 +456,7 @@ pub trait Broadcaster { type Callback: UnfilteredDispatchable; /// Request a threshold signature and then build and broadcast the outbound api call. - fn threshold_sign_and_broadcast(api_call: Self::ApiCall, pause_broadcasts: bool) - -> BroadcastId; + fn threshold_sign_and_broadcast(api_call: Self::ApiCall) -> BroadcastId; /// Like `threshold_sign_and_broadcast` but also registers a callback to be dispatched when the /// signature accepted event has been witnessed. @@ -465,6 +464,10 @@ pub trait Broadcaster { api_call: Self::ApiCall, callback: Self::Callback, ) -> BroadcastId; + + /// Request a threshold signature and then build and broadcast the outbound api call + /// specifically for a rotation tx.. + fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId; } /// The heartbeat of the network diff --git a/state-chain/traits/src/mocks/broadcaster.rs b/state-chain/traits/src/mocks/broadcaster.rs index 8b3cb19b93..9c06066756 100644 --- a/state-chain/traits/src/mocks/broadcaster.rs +++ b/state-chain/traits/src/mocks/broadcaster.rs @@ -26,10 +26,7 @@ impl< type ApiCall = A; type Callback = C; - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - _pause_broadcasts: bool, - ) -> cf_primitives::BroadcastId { + fn threshold_sign_and_broadcast(api_call: Self::ApiCall) -> cf_primitives::BroadcastId { Self::mutate_value(b"API_CALLS", |api_calls: &mut Option>| { let api_calls = api_calls.get_or_insert(Default::default()); api_calls.push(api_call); @@ -45,10 +42,14 @@ impl< api_call: Self::ApiCall, callback: Self::Callback, ) -> BroadcastId { - let ids @ id = >::threshold_sign_and_broadcast(api_call, false); + let ids @ id = >::threshold_sign_and_broadcast(api_call); Self::put_storage(b"CALLBACKS", id, callback); ids } + + fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { + >::threshold_sign_and_broadcast(api_call) + } } impl< From 4d445bb877506843758f554a03e42733823ad369 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 17 Nov 2023 18:21:01 +0100 Subject: [PATCH 09/33] feat: use same broadcast_id for tx, chain-specific broadcast barriers --- state-chain/chains/src/btc.rs | 7 ++ state-chain/chains/src/dot.rs | 8 ++ state-chain/chains/src/evm.rs | 17 +++ state-chain/chains/src/lib.rs | 7 +- state-chain/chains/src/mocks.rs | 6 + state-chain/chains/src/none.rs | 6 + .../pallets/cf-broadcast/src/benchmarking.rs | 4 +- state-chain/pallets/cf-broadcast/src/lib.rs | 106 +++++++++++------- state-chain/pallets/cf-broadcast/src/tests.rs | 2 +- 9 files changed, 117 insertions(+), 46 deletions(-) diff --git a/state-chain/chains/src/btc.rs b/state-chain/chains/src/btc.rs index ff5e12583d..a128b98750 100644 --- a/state-chain/chains/src/btc.rs +++ b/state-chain/chains/src/btc.rs @@ -248,6 +248,13 @@ impl ChainCrypto for BitcoinCrypto { fn handover_key_matches(current_key: &Self::AggKey, new_key: &Self::AggKey) -> bool { new_key.previous.is_some_and(|previous| current_key.current == previous) } + + fn maybe_broadcast_barriers_on_rotation( + _rotation_broadcast_id: cf_primitives::BroadcastId, + ) -> Vec { + // we dont need to put broadcast barriers for Bitcoin + vec![] + } } fn verify_single_threshold_signature( diff --git a/state-chain/chains/src/dot.rs b/state-chain/chains/src/dot.rs index 68ff768bd1..514371c08d 100644 --- a/state-chain/chains/src/dot.rs +++ b/state-chain/chains/src/dot.rs @@ -313,6 +313,14 @@ impl ChainCrypto for PolkadotCrypto { fn sign_with_specific_key() -> bool { true } + + fn maybe_broadcast_barriers_on_rotation( + rotation_broadcast_id: BroadcastId, + ) -> Vec { + // For polkadot, we need to pause future epoch broadcasts until all the previous epoch + // broadcasts (including the rotation tx) has successfully broadcasted. + vec![rotation_broadcast_id] + } } #[derive(Encode, Decode, TypeInfo, Clone, RuntimeDebug, Default, PartialEq, Eq)] diff --git a/state-chain/chains/src/evm.rs b/state-chain/chains/src/evm.rs index 17a879f7da..15ef871133 100644 --- a/state-chain/chains/src/evm.rs +++ b/state-chain/chains/src/evm.rs @@ -49,6 +49,23 @@ impl ChainCrypto for EvmCrypto { fn agg_key_to_payload(agg_key: Self::AggKey, _for_handover: bool) -> Self::Payload { H256(Blake2_256::hash(&agg_key.to_pubkey_compressed())) } + + fn maybe_broadcast_barriers_on_rotation( + rotation_broadcast_id: BroadcastId, + ) -> Vec { + // For Ethereum, we need to put 2 barriers, the first on the last non-rotation tx of the + // previous epoch, the second on the rotation tx itself. This is because before we execute + // the rotation tx for eth, we need to make sure all previous tx have successfully + // broadcast. Also, we need to pause future new epoch tx from broadcast until the rotation + // broadcast has successfully completed. + // + // If the rotation tx is the first broadcast ever, we dont need the first barrier. + if rotation_broadcast_id > 1 { + vec![rotation_broadcast_id - 1, rotation_broadcast_id] + } else { + vec![rotation_broadcast_id] + } + } } #[derive(Copy, Clone, RuntimeDebug, PartialEq, Eq)] diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 5b737cc5f0..8deda78aac 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -5,7 +5,7 @@ use core::{fmt::Display, iter::Step}; use crate::benchmarking_value::{BenchmarkValue, BenchmarkValueExtended}; pub use address::ForeignChainAddress; use address::{AddressDerivationApi, ToHumanreadableAddress}; -use cf_primitives::{AssetAmount, ChannelId, EgressId, EthAmount, TransactionHash}; +use cf_primitives::{AssetAmount, BroadcastId, ChannelId, EgressId, EthAmount, TransactionHash}; use codec::{Decode, Encode, FullCodec, MaxEncodedLen}; use frame_support::{ pallet_prelude::{MaybeSerializeDeserialize, Member}, @@ -193,6 +193,11 @@ pub trait ChainCrypto { fn key_handover_is_required() -> bool { Self::UtxoChain::get() } + + /// Provides chain specific functionality for providing the broadcast barriers on rotation tx + /// broadcast + fn maybe_broadcast_barriers_on_rotation(rotation_broadcast_id: BroadcastId) + -> Vec; } /// Provides chain-specific replay protection data. diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index 7ed5c503be..5b0b1fa993 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -252,6 +252,12 @@ impl ChainCrypto for MockEthereumChainCrypto { fn key_handover_is_required() -> bool { MockKeyHandoverIsRequired::get() } + + fn maybe_broadcast_barriers_on_rotation( + _rotation_broadcast_id: BroadcastId, + ) -> Vec { + unimplemented!() + } } impl_default_benchmark_value!(MockAggKey); diff --git a/state-chain/chains/src/none.rs b/state-chain/chains/src/none.rs index 119a8b2ec8..98b9f2ea7b 100644 --- a/state-chain/chains/src/none.rs +++ b/state-chain/chains/src/none.rs @@ -53,4 +53,10 @@ impl ChainCrypto for NoneChainCrypto { fn agg_key_to_payload(_agg_key: Self::AggKey, _for_handover: bool) -> Self::Payload { unimplemented!() } + + fn maybe_broadcast_barriers_on_rotation( + _rotation_broadcast_id: BroadcastId, + ) -> Vec { + unimplemented!() + } } diff --git a/state-chain/pallets/cf-broadcast/src/benchmarking.rs b/state-chain/pallets/cf-broadcast/src/benchmarking.rs index 42584f98c8..e0b3e542c1 100644 --- a/state-chain/pallets/cf-broadcast/src/benchmarking.rs +++ b/state-chain/pallets/cf-broadcast/src/benchmarking.rs @@ -49,7 +49,7 @@ fn generate_on_signature_ready_call, I>() -> pallet::Call::benchmark_value(), api_call: Box::new(ApiCallFor::::benchmark_value()), - broadcast_id: 1, + broadcast_attempt_id: BroadcastAttemptId { broadcast_id: 1, attempt_count: 0 }, initiated_at: INITIATED_AT.into(), } } @@ -115,7 +115,7 @@ benchmarks_instance_pallet! { BenchmarkValue::benchmark_value(), signed_api_call, BenchmarkValue::benchmark_value(), - 1, + BroadcastAttemptId{broadcast_id: 1, attempt_count: 0}, INITIATED_AT.into(), ); diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 6cbd38c025..88f2df04db 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -92,7 +92,7 @@ pub mod pallet { use cf_traits::{AccountRoleRegistry, BroadcastNomination, KeyProvider, OnBroadcastReady}; use frame_support::{ ensure, - pallet_prelude::{OptionQuery, *}, + pallet_prelude::{OptionQuery, ValueQuery, *}, traits::EnsureOrigin, }; use frame_system::pallet_prelude::*; @@ -312,7 +312,7 @@ pub mod pallet { /// Whether or not broadcasts are paused for broadcast ids greater than the given broadcast id. #[pallet::storage] #[pallet::getter(fn broadcast_pause)] - pub type BroadcastPause = StorageValue<_, BroadcastId, OptionQuery>; + pub type BroadcastBarriers = StorageValue<_, Vec, ValueQuery>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] @@ -337,7 +337,7 @@ pub mod pallet { transaction_out_id: TransactionOutIdFor, }, /// A broadcast's threshold signature is invalid, we will attempt to re-sign it. - ThresholdSignatureInvalid { broadcast_id: BroadcastId, retry_broadcast_id: BroadcastId }, + ThresholdSignatureInvalid { broadcast_id: BroadcastId }, /// A signature accepted event on the target chain has been witnessed and the callback was /// executed. BroadcastCallbackExecuted { broadcast_id: BroadcastId, result: DispatchResult }, @@ -410,10 +410,10 @@ pub mod pallet { let mut retries = BroadcastRetryQueue::::take(); let mut paused_broadcasts = vec![]; - if let Some(pause_broadcast_id) = BroadcastPause::::get() { + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().last() { paused_broadcasts = retries .extract_if(|broadcast| { - broadcast.broadcast_attempt_id.broadcast_id > pause_broadcast_id + broadcast.broadcast_attempt_id.broadcast_id > *broadcast_barrier_id }) .collect::>(); } @@ -527,7 +527,7 @@ pub mod pallet { threshold_request_id: ThresholdSignatureRequestId, threshold_signature_payload: PayloadFor, api_call: Box<>::ApiCall>, - broadcast_id: BroadcastId, + broadcast_attempt_id: BroadcastAttemptId, initiated_at: ChainBlockNumberFor, ) -> DispatchResultWithPostInfo { let _ = T::EnsureThresholdSigned::ensure_origin(origin)?; @@ -550,7 +550,7 @@ pub mod pallet { T::TransactionBuilder::build_transaction(&signed_api_call), signed_api_call, threshold_signature_payload, - broadcast_id, + broadcast_attempt_id, initiated_at, ); Ok(().into()) @@ -585,9 +585,11 @@ pub mod pallet { TransactionOutIdToBroadcastId::::take(&tx_out_id) .ok_or(Error::::InvalidPayload)?; - if let Some(pause_broadcast_id) = BroadcastPause::::get() { - if pause_broadcast_id == broadcast_id { - BroadcastPause::::set(None); + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().last() { + if *broadcast_barrier_id == broadcast_id { + BroadcastBarriers::::mutate(|broadcast_barriers| { + broadcast_barriers.pop(); + }); } } @@ -740,7 +742,7 @@ impl, I: 'static> Pallet { threshold_request_id, threshold_signature_payload, api_call: Box::new(api_call), - broadcast_id, + broadcast_attempt_id: BroadcastAttemptId { broadcast_id, attempt_count: 0 }, initiated_at, } .into() @@ -760,7 +762,7 @@ impl, I: 'static> Pallet { transaction_payload: TransactionFor, api_call: >::ApiCall, threshold_signature_payload: <::ChainCrypto as ChainCrypto>::Payload, - broadcast_id: BroadcastId, + broadcast_attempt_id: BroadcastAttemptId, initiated_at: ChainBlockNumberFor, ) -> BroadcastAttemptId { let transaction_out_id = api_call.transaction_out_id(); @@ -769,12 +771,13 @@ impl, I: 'static> Pallet { TransactionOutIdToBroadcastId::::insert( &transaction_out_id, - (broadcast_id, initiated_at), + (broadcast_attempt_id.broadcast_id, initiated_at), ); - ThresholdSignatureData::::insert(broadcast_id, (api_call, signature)); - - let broadcast_attempt_id = BroadcastAttemptId { broadcast_id, attempt_count: 0 }; + ThresholdSignatureData::::insert( + broadcast_attempt_id.broadcast_id, + (api_call, signature), + ); let broadcast_attempt = BroadcastAttempt:: { broadcast_attempt_id, @@ -783,9 +786,9 @@ impl, I: 'static> Pallet { transaction_out_id, }; - if BroadcastPause::::get() - .is_some_and(|pause_broadcast_id| broadcast_id > pause_broadcast_id) - { + if BroadcastBarriers::::get().last().is_some_and(|broadcast_barrier_id| { + broadcast_attempt_id.broadcast_id > *broadcast_barrier_id + }) { Self::schedule_for_retry(&broadcast_attempt); } else { Self::start_broadcast_attempt(broadcast_attempt); @@ -800,26 +803,25 @@ impl, I: 'static> Pallet { let EpochKey { key, .. } = T::KeyProvider::active_epoch_key() .expect("Epoch key must exist if we made a broadcast."); + let next_broadcast_attempt_id = broadcast_attempt.broadcast_attempt_id.next_attempt(); + + BroadcastAttemptCount::::mutate( + broadcast_id, + |attempt_count: &mut AttemptCount| { + *attempt_count += 1; + }, + ); + debug_assert_eq!( + BroadcastAttemptCount::::get(broadcast_id), + next_broadcast_attempt_id.attempt_count, + ); + if T::TransactionBuilder::is_valid_for_rebroadcast( &api_call, &broadcast_attempt.threshold_signature_payload, &key, &signature, ) { - let next_broadcast_attempt_id = - broadcast_attempt.broadcast_attempt_id.next_attempt(); - - BroadcastAttemptCount::::mutate( - broadcast_id, - |attempt_count: &mut AttemptCount| { - *attempt_count += 1; - }, - ); - debug_assert_eq!( - BroadcastAttemptCount::::get(broadcast_id), - next_broadcast_attempt_id.attempt_count, - ); - Self::start_broadcast_attempt(BroadcastAttempt:: { broadcast_attempt_id: next_broadcast_attempt_id, ..broadcast_attempt @@ -828,19 +830,35 @@ impl, I: 'static> Pallet { // If the signature verification fails, we want // to retry from the threshold signing stage. else { - Self::clean_up_broadcast_storage(broadcast_id); - let retry_broadcast_id = Self::threshold_sign_and_broadcast( - api_call, - RequestCallbacks::::get(broadcast_id), + // Only storage to be removed, the other storages corresponding to this broadcast + // are either still valid or will get overwritten on re-threshold sign and + // broadcast. + TransactionOutIdToBroadcastId::::remove(broadcast_attempt.transaction_out_id); + + // We update the initiated_at here since as the tx is resigned and broadcast, it is + // not possible for it to be successfully broadcasted before this point. + let initiated_at = T::ChainTracking::get_block_height(); + + let threshold_signature_payload = api_call.threshold_signature_payload(); + T::ThresholdSigner::request_signature_with_callback( + threshold_signature_payload.clone(), + |threshold_request_id| { + Call::on_signature_ready { + threshold_request_id, + threshold_signature_payload, + api_call: Box::new(api_call), + broadcast_attempt_id: next_broadcast_attempt_id, + initiated_at, + } + .into() + }, ); + log::info!( "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", broadcast_id ); - Self::deposit_event(Event::::ThresholdSignatureInvalid { - broadcast_id, - retry_broadcast_id, - }); + Self::deposit_event(Event::::ThresholdSignatureInvalid { broadcast_id }); } } else { log::error!("No threshold signature data is available."); @@ -922,7 +940,11 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); - BroadcastPause::::set(Some(broadcast_id)); + let mut broadcast_barriers = <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation( + broadcast_id, + ); + broadcast_barriers.reverse(); + BroadcastBarriers::::set(broadcast_barriers); broadcast_id } } diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index bb9c129a66..cea9c2c97b 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -130,7 +130,7 @@ fn start_mock_broadcast_tx_out_id( MockTransaction, MockApiCall { tx_out_id, payload: Default::default(), sig: Default::default() }, Default::default(), - 1, + BroadcastAttemptId { broadcast_id: 1, attempt_count: 0 }, 100u64, ) } From 56dbec5675c86752d2279784114ee54160166983 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Mon, 20 Nov 2023 11:19:27 +0100 Subject: [PATCH 10/33] chore: reinstate test --- .../cf-integration-tests/src/authorities.rs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/state-chain/cf-integration-tests/src/authorities.rs b/state-chain/cf-integration-tests/src/authorities.rs index 151198838d..791ec99266 100644 --- a/state-chain/cf-integration-tests/src/authorities.rs +++ b/state-chain/cf-integration-tests/src/authorities.rs @@ -295,6 +295,47 @@ fn authority_rotation_can_succeed_after_aborted_by_safe_mode() { }); } +#[test] +fn authority_rotation_cannot_be_aborted_after_key_handover_and_completes_even_on_safe_mode_enabled() +{ + const EPOCH_BLOCKS: u32 = 1000; + const MAX_AUTHORITIES: AuthorityCount = 10; + super::genesis::default() + .blocks_per_epoch(EPOCH_BLOCKS) + .max_authorities(MAX_AUTHORITIES) + .build() + .execute_with(|| { + let (mut testnet, _, _) = fund_authorities_and_join_auction(MAX_AUTHORITIES); + + // Resolve Auction + testnet.move_to_the_end_of_epoch(); + + // Run until key handover starts + testnet.move_forward_blocks(5); + assert!( + matches!(AllVaults::status(), AsyncResult::Ready(VaultStatus::KeyHandoverComplete)), + "Key handover should be complete but is {:?}", + AllVaults::status() + ); + + assert_ok!(Environment::update_safe_mode( + pallet_cf_governance::RawOrigin::GovernanceApproval.into(), + SafeModeUpdate::CodeRed + )); + + testnet.move_forward_blocks(3); + + // Authority rotation is stalled while in Code Red because of disabling dispatching + // witness extrinsics and so witnessing vault rotation will be stalled. + assert!(matches!( + AllVaults::status(), + AsyncResult::Ready(VaultStatus::RotationComplete) + )); + testnet.move_forward_blocks(3); + assert_eq!(GENESIS_EPOCH + 1, Validator::epoch_index(), "We should be in a new epoch"); + }); +} + #[test] fn authority_rotation_can_recover_after_keygen_fails() { const EPOCH_BLOCKS: u32 = 1000; From 2d3cdc5c2598a46afdfa02f50945a03ec7efd13f Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Mon, 20 Nov 2023 13:46:43 +0100 Subject: [PATCH 11/33] feat: storage migration --- state-chain/pallets/cf-vaults/src/lib.rs | 2 +- .../pallets/cf-vaults/src/migrations.rs | 7 ++- .../pallets/cf-vaults/src/migrations/v3.rs | 63 +++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 state-chain/pallets/cf-vaults/src/migrations/v3.rs diff --git a/state-chain/pallets/cf-vaults/src/lib.rs b/state-chain/pallets/cf-vaults/src/lib.rs index 51dad7cc83..18e5e2d816 100644 --- a/state-chain/pallets/cf-vaults/src/lib.rs +++ b/state-chain/pallets/cf-vaults/src/lib.rs @@ -40,7 +40,7 @@ mod migrations; mod mock; mod tests; -pub const PALLET_VERSION: StorageVersion = StorageVersion::new(2); +pub const PALLET_VERSION: StorageVersion = StorageVersion::new(3); const KEYGEN_CEREMONY_RESPONSE_TIMEOUT_BLOCKS_DEFAULT: u32 = 90; diff --git a/state-chain/pallets/cf-vaults/src/migrations.rs b/state-chain/pallets/cf-vaults/src/migrations.rs index 3eaa166e9b..b8fbc64f29 100644 --- a/state-chain/pallets/cf-vaults/src/migrations.rs +++ b/state-chain/pallets/cf-vaults/src/migrations.rs @@ -1,6 +1,9 @@ pub mod v2; +pub mod v3; use cf_runtime_upgrade_utilities::VersionedMigration; -pub type PalletMigration = - (VersionedMigration, v2::Migration, 1, 2>,); +pub type PalletMigration = ( + VersionedMigration, v2::Migration, 1, 2>, + VersionedMigration, v2::Migration, 2, 3>, +); diff --git a/state-chain/pallets/cf-vaults/src/migrations/v3.rs b/state-chain/pallets/cf-vaults/src/migrations/v3.rs new file mode 100644 index 0000000000..35c4a1353d --- /dev/null +++ b/state-chain/pallets/cf-vaults/src/migrations/v3.rs @@ -0,0 +1,63 @@ +use crate::*; + +#[cfg(feature = "try-runtime")] +use frame_support::dispatch::DispatchError; +use frame_support::traits::OnRuntimeUpgrade; +use sp_std::marker::PhantomData; + +/// v2 migrating storage item KeygenSlashRate -> KeygenSlashAmount +/// Percent -> FlipBalance +pub struct Migration, I: 'static>(PhantomData<(T, I)>); + +mod old { + + use super::*; + + #[derive(Debug, TypeInfo, Decode, Encode, Clone, Copy, PartialEq, Eq)] + pub enum KeyState { + Unlocked, + /// Key is only available to sign this request id. + Locked(ThresholdSignatureRequestId), + } + #[derive(Encode, Decode, TypeInfo)] + pub struct VaultEpochAndState { + pub epoch_index: EpochIndex, + pub key_state: KeyState, + } + + #[frame_support::storage_alias] + pub type CurrentVaultEpochAndState, I: 'static> = + StorageValue, VaultEpochAndState>; +} + +impl, I: 'static> OnRuntimeUpgrade for Migration { + fn on_runtime_upgrade() -> frame_support::weights::Weight { + // remove the old storage containing Percent + if let Some(old::VaultEpochAndState { epoch_index, key_state: _ }) = + old::CurrentVaultEpochAndState::::take() + { + // set the new storage using the EpochIndex from the old storage item above. + CurrentVaultEpoch::::put(epoch_index); + } + + Weight::zero() + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, DispatchError> { + // just check that the old storage item existed + Ok(old::CurrentVaultEpochAndState::::exists().encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), DispatchError> { + let old_state = ::decode(&mut &state[..]) + .map_err(|_| "Failed to decode pre-upgrade state.")?; + + assert!(!old::CurrentVaultEpochAndState::::exists()); + assert!(CurrentVaultEpoch::::exists()); + assert_eq!(old_state.epoch_index, CurrentVaultEpoch::::get().unwrap()); + + Ok(()) + } +} From 41a7f08020e0559b2e88083814ffbfa3cfb24cc0 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 21 Nov 2023 17:40:13 +0100 Subject: [PATCH 12/33] fix: check for all broadcasts before the barrier --- state-chain/pallets/cf-broadcast/src/lib.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 96c3ab6543..2a6cd9e4ce 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -311,9 +311,14 @@ pub mod pallet { /// Whether or not broadcasts are paused for broadcast ids greater than the given broadcast id. #[pallet::storage] - #[pallet::getter(fn broadcast_pause)] + #[pallet::getter(fn broadcast_barriers)] pub type BroadcastBarriers = StorageValue<_, Vec, ValueQuery>; + /// List of broadcasts that are initiated but not witnessed on the external chain. + #[pallet::storage] + #[pallet::getter(fn pending_broadcasts)] + pub type PendingBroadcasts = StorageValue<_, Vec, ValueQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event, I: 'static = ()> { @@ -571,8 +576,16 @@ pub mod pallet { TransactionOutIdToBroadcastId::::take(&tx_out_id) .ok_or(Error::::InvalidPayload)?; + PendingBroadcasts::::mutate(|pending_broadcasts| { + pending_broadcasts.remove(pending_broadcasts.binary_search(&broadcast_id).expect("The broadcast_id should exist in the pending broadcasts list since we added it to the last when the broadcast was initated")) + }); + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().last() { - if *broadcast_barrier_id == broadcast_id { + let maybe_earliest_pending_broadcast = + PendingBroadcasts::::get().first().copied(); + if maybe_earliest_pending_broadcast.is_none() || + (maybe_earliest_pending_broadcast.unwrap() > *broadcast_barrier_id) + { BroadcastBarriers::::mutate(|broadcast_barriers| { broadcast_barriers.pop(); }); @@ -710,6 +723,9 @@ impl, I: 'static> Pallet { *id += 1; *id }); + + PendingBroadcasts::::append(broadcast_id); + if let Some(callback) = maybe_callback { RequestCallbacks::::insert(broadcast_id, callback); } From 24de28da420acc339829b0d89c922dcaccb391d6 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 22 Nov 2023 12:48:34 +0100 Subject: [PATCH 13/33] feat: more tests, test fixes --- state-chain/chains/src/mocks.rs | 35 ++- state-chain/pallets/cf-broadcast/src/mock.rs | 2 +- state-chain/pallets/cf-broadcast/src/tests.rs | 252 +++++++++++++----- state-chain/pallets/cf-vaults/src/mock.rs | 4 +- 4 files changed, 216 insertions(+), 77 deletions(-) diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index 5b0b1fa993..c6d746d295 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -14,10 +14,18 @@ pub struct MockEthereum; pub type MockEthereumChannelId = u128; +#[derive(Clone)] +pub enum ChainChoice { + Ethereum, + Polkadot, + Bitcoin, +} + thread_local! { static MOCK_KEY_HANDOVER_IS_REQUIRED: RefCell = RefCell::new(true); static MOCK_SIGN_WITH_SPECIFIC_KEY: RefCell = RefCell::new(false); static MOCK_VALID_METADATA: RefCell = RefCell::new(true); + static MOCK_BROADCAST_BARRIERS: RefCell = RefCell::new(ChainChoice::Ethereum); } pub struct MockKeyHandoverIsRequired; @@ -34,6 +42,20 @@ impl Get for MockKeyHandoverIsRequired { } } +pub struct MockBroadcastBarriers; + +impl MockBroadcastBarriers { + pub fn set(value: ChainChoice) { + MOCK_BROADCAST_BARRIERS.with(|v| *v.borrow_mut() = value); + } +} + +impl Get for MockBroadcastBarriers { + fn get() -> ChainChoice { + MOCK_BROADCAST_BARRIERS.with(|v| (*v.borrow()).clone()) + } +} + pub struct MockFixedKeySigningRequests; impl MockFixedKeySigningRequests { @@ -254,9 +276,18 @@ impl ChainCrypto for MockEthereumChainCrypto { } fn maybe_broadcast_barriers_on_rotation( - _rotation_broadcast_id: BroadcastId, + rotation_broadcast_id: BroadcastId, ) -> Vec { - unimplemented!() + match MockBroadcastBarriers::get() { + ChainChoice::Ethereum => + if rotation_broadcast_id > 1 { + vec![rotation_broadcast_id - 1, rotation_broadcast_id] + } else { + vec![rotation_broadcast_id] + }, + ChainChoice::Polkadot => vec![rotation_broadcast_id], + ChainChoice::Bitcoin => vec![], + } } } diff --git a/state-chain/pallets/cf-broadcast/src/mock.rs b/state-chain/pallets/cf-broadcast/src/mock.rs index c79529cadf..0f4076ef7a 100644 --- a/state-chain/pallets/cf-broadcast/src/mock.rs +++ b/state-chain/pallets/cf-broadcast/src/mock.rs @@ -163,7 +163,7 @@ cf_test_utilities::impl_test_helpers! { Test, RuntimeGenesisConfig::default(), || { - MockEpochInfo::next_epoch((0..3).collect()); + MockEpochInfo::next_epoch((0..4).collect()); MockNominator::use_current_authorities_as_nominees::(); for id in &MockEpochInfo::current_authorities() { >::register_as_validator(id).unwrap(); diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index cea9c2c97b..0d465cf221 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -9,9 +9,9 @@ use crate::{ use cf_chains::{ evm::SchnorrVerificationComponents, mocks::{ - MockApiCall, MockEthereum, MockEthereumChainCrypto, MockEthereumTransactionMetadata, - MockThresholdSignature, MockTransaction, MockTransactionBuilder, ETH_TX_FEE, - MOCK_TX_METADATA, + ChainChoice, MockApiCall, MockBroadcastBarriers, MockEthereum, MockEthereumChainCrypto, + MockEthereumTransactionMetadata, MockThresholdSignature, MockTransaction, + MockTransactionBuilder, ETH_TX_FEE, MOCK_TX_METADATA, }, ChainCrypto, FeeRefundCalculator, }; @@ -142,7 +142,11 @@ fn start_mock_broadcast() -> BroadcastAttemptId { #[test] fn transaction_succeeded_results_in_refund_for_signer() { new_test_ext().execute_with(|| { - let broadcast_attempt_id = start_mock_broadcast_tx_out_id(MOCK_TRANSACTION_OUT_ID); + let (tx_out_id, apicall) = api_call(1); + let broadcast_id = initiate_and_sign_broadcast(&apicall, TxType::Normal); + + let broadcast_attempt_id = BroadcastAttemptId { broadcast_id, attempt_count: 0 }; + let tx_sig_request = AwaitingBroadcast::::get(broadcast_attempt_id).unwrap(); @@ -150,13 +154,7 @@ fn transaction_succeeded_results_in_refund_for_signer() { assert_eq!(TransactionFeeDeficit::::get(nominee), 0); - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - MOCK_TRANSACTION_OUT_ID, - nominee, - ETH_TX_FEE, - MOCK_TX_METADATA, - )); + witness_broadcast(tx_out_id); let expected_refund = tx_sig_request .broadcast_attempt @@ -175,7 +173,7 @@ fn transaction_succeeded_results_in_refund_for_signer() { }) ); - assert_broadcast_storage_cleaned_up(broadcast_attempt_id.broadcast_id); + assert_broadcast_storage_cleaned_up(broadcast_id); }); } @@ -308,7 +306,8 @@ fn test_sigdata_with_no_match_is_noop() { #[test] fn transaction_succeeded_after_timeout_reports_failed_nodes() { new_test_ext().execute_with(|| { - start_mock_broadcast_tx_out_id(MOCK_TRANSACTION_OUT_ID); + let (tx_out_id, apicall) = api_call(1); + initiate_and_sign_broadcast(&apicall, TxType::Normal); let mut failed_authorities = vec![]; // The last node succeeds @@ -322,13 +321,7 @@ fn transaction_succeeded_after_timeout_reports_failed_nodes() { Broadcaster::on_initialize(0); } - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - MOCK_TRANSACTION_OUT_ID, - Default::default(), - ETH_TX_FEE, - MOCK_TX_METADATA, - )); + witness_broadcast(tx_out_id); MockOffenceReporter::assert_reported( PalletOffence::FailedToBroadcastTransaction, @@ -448,10 +441,8 @@ fn threshold_signature_rerequested(broadcast_attempt_id: BroadcastAttemptId) { assert!( TransactionOutIdToBroadcastId::::get(MOCK_TRANSACTION_OUT_ID).is_none() ); - assert_eq!(BroadcastAttemptCount::::get(broadcast_attempt_id.broadcast_id), 0); - assert!( - ThresholdSignatureData::::get(broadcast_attempt_id.broadcast_id).is_none() - ); + // attempt count incremented for the same broadcast_id + assert_eq!(BroadcastAttemptCount::::get(broadcast_attempt_id.broadcast_id), 1); // Verify that we have a new signature request in the pipeline assert_eq!( MockThresholdSigner::::signature_result(0), @@ -559,18 +550,15 @@ fn ensure_retries_are_skipped_during_safe_mode() { fn transaction_succeeded_results_in_refund_refuse_for_signer() { new_test_ext().execute_with(|| { MockEthereumTransactionMetadata::set_validity(false); - let _ = start_mock_broadcast_tx_out_id(MOCK_TRANSACTION_OUT_ID); + + let (tx_out_id, apicall) = api_call(1); + initiate_and_sign_broadcast(&apicall, TxType::Normal); + let nominee = MockNominator::get_last_nominee().unwrap(); assert_eq!(TransactionFeeDeficit::::get(nominee), 0); - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - MOCK_TRANSACTION_OUT_ID, - nominee, - ETH_TX_FEE, - MOCK_TX_METADATA, - )); + witness_broadcast(tx_out_id); assert_eq!( System::events().get(1).expect("an event").event, @@ -582,40 +570,28 @@ fn transaction_succeeded_results_in_refund_refuse_for_signer() { } #[test] -fn broadcast_pause() { +fn broadcast_barrier_for_polkadot() { new_test_ext().execute_with(|| { + MockBroadcastBarriers::set(ChainChoice::Polkadot); + let (tx_out_id1, api_call1) = api_call(1); let (tx_out_id2, api_call2) = api_call(2); let (tx_out_id3, api_call3) = api_call(3); // create and sign 3 txs that are then ready for broadcast - let broadcast_id_1 = >::TargetChain, - >>::threshold_sign_and_broadcast(api_call1.clone()); - - EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); - + let broadcast_id_1 = initiate_and_sign_broadcast(&api_call1, TxType::Normal); // tx1 emits broadcast request assert_transaction_broadcast_request_event(broadcast_id_1, 0, tx_out_id1); - let broadcast_id_2 = >::TargetChain, - >>::threshold_sign_and_broadcast_rotation_tx(api_call2.clone()); - - EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); - + let broadcast_id_2 = initiate_and_sign_broadcast(&api_call2, TxType::Rotation); // tx2 emits broadcast request and also pauses any further new broadcast requests assert_transaction_broadcast_request_event(broadcast_id_2, 0, tx_out_id2); - let broadcast_id_3 = >::TargetChain, - >>::threshold_sign_and_broadcast(api_call3.clone()); - - EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); + let broadcast_id_3 = initiate_and_sign_broadcast(&api_call3, TxType::Normal); - // tx3 is ready for broadcast but since there is a broadcast pause, broadcast request is not - // issued, the broadcast is rescheduled instead. + // tx3 is ready for broadcast but since there is a broadcast pause, broadcast request is + // not issued, the broadcast is rescheduled instead. System::assert_last_event(RuntimeEvent::Broadcaster( crate::Event::::BroadcastRetryScheduled { broadcast_attempt_id: BroadcastAttemptId { @@ -626,13 +602,7 @@ fn broadcast_pause() { )); // report successful broadcast of tx1 - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - tx_out_id1, - Default::default(), - ETH_TX_FEE, - MOCK_TX_METADATA, - )); + witness_broadcast(tx_out_id1); // tx3 should still not be broadcasted because the blocking tx (tx2) has still not // succeeded. @@ -645,13 +615,7 @@ fn broadcast_pause() { ); // Now tx2 succeeds which should allow tx3 to be broadcast - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - tx_out_id2, - Default::default(), - ETH_TX_FEE, - MOCK_TX_METADATA, - )); + witness_broadcast(tx_out_id2); Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); // attempt count is 1 because the previous failure to broadcast because of @@ -660,13 +624,125 @@ fn broadcast_pause() { assert!(BroadcastRetryQueue::::get().is_empty()); - assert_ok!(Broadcaster::transaction_succeeded( - RuntimeOrigin::root(), - tx_out_id3, - Default::default(), - ETH_TX_FEE, - MOCK_TX_METADATA, + witness_broadcast(tx_out_id3); + }); +} + +#[test] +fn broadcast_barrier_for_bitcoin() { + new_test_ext().execute_with(|| { + MockBroadcastBarriers::set(ChainChoice::Bitcoin); + + let (tx_out_id1, api_call1) = api_call(1); + let (tx_out_id2, api_call2) = api_call(2); + let (tx_out_id3, api_call3) = api_call(3); + + // create and sign 3 txs that are then ready for broadcast + let broadcast_id_1 = initiate_and_sign_broadcast(&api_call1, TxType::Normal); + // tx1 emits broadcast request + assert_transaction_broadcast_request_event(broadcast_id_1, 0, tx_out_id1); + + let broadcast_id_2 = initiate_and_sign_broadcast(&api_call2, TxType::Rotation); + // tx2 emits broadcast request and does not pause future broadcasts in bitcoin + assert_transaction_broadcast_request_event(broadcast_id_2, 0, tx_out_id2); + + let broadcast_id_3 = initiate_and_sign_broadcast(&api_call3, TxType::Normal); + // tx3 emits broadcast request + assert_transaction_broadcast_request_event(broadcast_id_3, 0, tx_out_id3); + + // we successfully witness all txs + witness_broadcast(tx_out_id1); + witness_broadcast(tx_out_id2); + witness_broadcast(tx_out_id3); + }); +} + +#[test] +fn broadcast_barrier_for_ethereum() { + new_test_ext().execute_with(|| { + MockBroadcastBarriers::set(ChainChoice::Ethereum); + + let (tx_out_id1, api_call1) = api_call(1); + let (tx_out_id2, api_call2) = api_call(2); + let (tx_out_id3, api_call3) = api_call(3); + let (tx_out_id4, api_call4) = api_call(4); + + let broadcast_id_1 = initiate_and_sign_broadcast(&api_call1, TxType::Normal); + // tx1 emits broadcast request + assert_transaction_broadcast_request_event(broadcast_id_1, 0, tx_out_id1); + + let broadcast_id_2 = initiate_and_sign_broadcast(&api_call2, TxType::Normal); + // tx2 emits broadcast request + assert_transaction_broadcast_request_event(broadcast_id_2, 0, tx_out_id2); + + // this will put a bbroadcast barrier at tx2 and tx3. tx3 wont be broadcasted yet + let broadcast_id_3 = initiate_and_sign_broadcast(&api_call3, TxType::Rotation); + + // tx3 is ready for broadcast but since there is a broadcast pause, broadcast request is + // not issued, the broadcast is rescheduled instead. + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::BroadcastRetryScheduled { + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_3, + attempt_count: 0, + }, + }, + )); + + // tx4 will be created but not broadcasted yet + let broadcast_id_4 = initiate_and_sign_broadcast(&api_call4, TxType::Normal); + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::BroadcastRetryScheduled { + broadcast_attempt_id: BroadcastAttemptId { + broadcast_id: broadcast_id_4, + attempt_count: 0, + }, + }, )); + + // report successful broadcast of tx2 + witness_broadcast(tx_out_id2); + + // tx3 and tx4 should still not be broadcasted because not all txs before and including tx2 + // have been witnessed + Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); + assert_eq!( + BroadcastRetryQueue::::get()[0] + .broadcast_attempt_id + .broadcast_id, + broadcast_id_3 + ); + + assert_eq!( + BroadcastRetryQueue::::get()[1] + .broadcast_attempt_id + .broadcast_id, + broadcast_id_4 + ); + + // Now tx1 succeeds which should allow tx3 to be broadcast but not tx4 since there will be + // another barrier at tx3 + witness_broadcast(tx_out_id1); + Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); + // attempt count is 1 because the previous failure to broadcast because of + // broadcast pause is considered an attempt + assert_transaction_broadcast_request_event(broadcast_id_3, 1, tx_out_id3); + + // tx4 is still pending + assert_eq!( + BroadcastRetryQueue::::get()[0] + .broadcast_attempt_id + .broadcast_id, + broadcast_id_4 + ); + + // witness tx3 which should allow tx4 to be broadcast + witness_broadcast(tx_out_id3); + Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); + + assert_transaction_broadcast_request_event(broadcast_id_4, 1, tx_out_id4); + assert!(BroadcastRetryQueue::::get().is_empty()); + witness_broadcast(tx_out_id4); }); } @@ -689,3 +765,35 @@ fn assert_transaction_broadcast_request_event( }, )); } + +fn initiate_and_sign_broadcast( + apicall: &MockApiCall, + tx_type: TxType, +) -> BroadcastId { + let broadcast_id = match tx_type { + TxType::Normal => >::TargetChain, + >>::threshold_sign_and_broadcast((*apicall).clone()), + TxType::Rotation => >::TargetChain, + >>::threshold_sign_and_broadcast_rotation_tx((*apicall).clone()), + }; + + EthMockThresholdSigner::execute_signature_result_against_last_request(Ok(ETH_DUMMY_SIG)); + + broadcast_id +} + +fn witness_broadcast(tx_out_id: [u8; 4]) { + assert_ok!(Broadcaster::transaction_succeeded( + RuntimeOrigin::root(), + tx_out_id, + Default::default(), + ETH_TX_FEE, + MOCK_TX_METADATA, + )); +} +enum TxType { + Normal, + Rotation, +} diff --git a/state-chain/pallets/cf-vaults/src/mock.rs b/state-chain/pallets/cf-vaults/src/mock.rs index d6fa1eec94..80c7edcd77 100644 --- a/state-chain/pallets/cf-vaults/src/mock.rs +++ b/state-chain/pallets/cf-vaults/src/mock.rs @@ -161,8 +161,8 @@ impl Broadcaster for MockBroadcaster { unimplemented!() } - fn threshold_sign_and_broadcast_rotation_tx(_api_call: Self::ApiCall) -> BroadcastId { - unimplemented!() + fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { + Self::threshold_sign_and_broadcast(api_call) } } From 0852b66eb278bbbeb3569f147d04a39bd2244e27 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 22 Nov 2023 16:35:56 +0100 Subject: [PATCH 14/33] chore: addressed comments --- state-chain/pallets/cf-broadcast/src/lib.rs | 49 +++++++++++---------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 2a6cd9e4ce..b9106a1684 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -44,7 +44,7 @@ use frame_support::{ traits::{Get, StorageVersion, UnfilteredDispatchable}, Twox64Concat, }; -use sp_std::vec::Vec; +use sp_std::{collections::vec_deque::VecDeque, vec::Vec}; use frame_system::pallet_prelude::OriginFor; pub use pallet::*; @@ -312,7 +312,7 @@ pub mod pallet { /// Whether or not broadcasts are paused for broadcast ids greater than the given broadcast id. #[pallet::storage] #[pallet::getter(fn broadcast_barriers)] - pub type BroadcastBarriers = StorageValue<_, Vec, ValueQuery>; + pub type BroadcastBarriers = StorageValue<_, VecDeque, ValueQuery>; /// List of broadcasts that are initiated but not witnessed on the external chain. #[pallet::storage] @@ -342,7 +342,7 @@ pub mod pallet { transaction_out_id: TransactionOutIdFor, }, /// A broadcast's threshold signature is invalid, we will attempt to re-sign it. - ThresholdSignatureInvalid { broadcast_id: BroadcastId }, + ThresholdSignatureInvalid { broadcast_attempt_id: BroadcastAttemptId }, /// A signature accepted event on the target chain has been witnessed and the callback was /// executed. BroadcastCallbackExecuted { broadcast_id: BroadcastId, result: DispatchResult }, @@ -412,21 +412,18 @@ pub mod pallet { .expect("start_next_broadcast_attempt weight should not be 0") as usize; - let mut retries = BroadcastRetryQueue::::take(); - - let mut paused_broadcasts = vec![]; - if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().last() { - paused_broadcasts = retries + let retries = BroadcastRetryQueue::::mutate(|retry_queue| { + let id_limit = BroadcastBarriers::::get() + .front() + .copied() + .unwrap_or(BroadcastId::max_value()); + retry_queue .extract_if(|broadcast| { - broadcast.broadcast_attempt_id.broadcast_id > *broadcast_barrier_id + broadcast.broadcast_attempt_id.broadcast_id <= id_limit }) - .collect::>(); - } - - if retries.len() > num_retries_that_fit { - paused_broadcasts.append(&mut retries.split_off(num_retries_that_fit)); - } - BroadcastRetryQueue::::put(paused_broadcasts); + .take(num_retries_that_fit) + .collect::>() + }); let retries_len = retries.len(); @@ -580,14 +577,14 @@ pub mod pallet { pending_broadcasts.remove(pending_broadcasts.binary_search(&broadcast_id).expect("The broadcast_id should exist in the pending broadcasts list since we added it to the last when the broadcast was initated")) }); - if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().last() { + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { let maybe_earliest_pending_broadcast = PendingBroadcasts::::get().first().copied(); if maybe_earliest_pending_broadcast.is_none() || (maybe_earliest_pending_broadcast.unwrap() > *broadcast_barrier_id) { BroadcastBarriers::::mutate(|broadcast_barriers| { - broadcast_barriers.pop(); + broadcast_barriers.pop_front(); }); } } @@ -788,7 +785,7 @@ impl, I: 'static> Pallet { transaction_out_id, }; - if BroadcastBarriers::::get().last().is_some_and(|broadcast_barrier_id| { + if BroadcastBarriers::::get().front().is_some_and(|broadcast_barrier_id| { broadcast_attempt_id.broadcast_id > *broadcast_barrier_id }) { Self::schedule_for_retry(&broadcast_attempt); @@ -841,6 +838,10 @@ impl, I: 'static> Pallet { // not possible for it to be successfully broadcasted before this point. let initiated_at = T::ChainTracking::get_block_height(); + Self::deposit_event(Event::::ThresholdSignatureInvalid { + broadcast_attempt_id: broadcast_attempt.broadcast_attempt_id, + }); + let threshold_signature_payload = api_call.threshold_signature_payload(); T::ThresholdSigner::request_signature_with_callback( threshold_signature_payload.clone(), @@ -860,7 +861,6 @@ impl, I: 'static> Pallet { "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", broadcast_id ); - Self::deposit_event(Event::::ThresholdSignatureInvalid { broadcast_id }); } } else { log::error!("No threshold signature data is available."); @@ -942,11 +942,12 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); - let mut broadcast_barriers = <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation( + let mut broadcast_barriers: VecDeque = <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation( broadcast_id, - ); - broadcast_barriers.reverse(); - BroadcastBarriers::::set(broadcast_barriers); + ).into(); + BroadcastBarriers::::mutate(|current_barriers| { + current_barriers.append(&mut broadcast_barriers) + }); broadcast_id } } From 7efd2e524fdd7fb09591863318f24a03cf12d810 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 29 Nov 2023 02:57:57 +0100 Subject: [PATCH 15/33] feat: on_sig_ready call migration --- .../pallets/cf-vaults/src/migrations.rs | 2 +- state-chain/runtime/src/lib.rs | 5 + state-chain/runtime/src/migrations.rs | 50 ++++ .../threshold_signature_callbacks.rs | 260 ++++++++++++++++++ 4 files changed, 316 insertions(+), 1 deletion(-) create mode 100644 state-chain/runtime/src/migrations.rs create mode 100644 state-chain/runtime/src/migrations/threshold_signature_callbacks.rs diff --git a/state-chain/pallets/cf-vaults/src/migrations.rs b/state-chain/pallets/cf-vaults/src/migrations.rs index b8fbc64f29..a5e9e7e5b2 100644 --- a/state-chain/pallets/cf-vaults/src/migrations.rs +++ b/state-chain/pallets/cf-vaults/src/migrations.rs @@ -5,5 +5,5 @@ use cf_runtime_upgrade_utilities::VersionedMigration; pub type PalletMigration = ( VersionedMigration, v2::Migration, 1, 2>, - VersionedMigration, v2::Migration, 2, 3>, + VersionedMigration, v3::Migration, 2, 3>, ); diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index a8073d561b..76568de607 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -3,6 +3,7 @@ #![recursion_limit = "256"] pub mod chainflip; pub mod constants; +pub mod migrations; pub mod runtime_apis; pub mod safe_mode; #[cfg(feature = "std")] @@ -849,6 +850,10 @@ type PalletMigrations = ( pallet_cf_ingress_egress::migrations::PalletMigration, pallet_cf_swapping::migrations::PalletMigration, pallet_cf_lp::migrations::PalletMigration, + migrations::VersionedMigration< + migrations::threshold_signature_callbacks::ThresholdSignatureCallbacks, + 101, + >, ); #[cfg(feature = "runtime-benchmarks")] diff --git a/state-chain/runtime/src/migrations.rs b/state-chain/runtime/src/migrations.rs new file mode 100644 index 0000000000..b25fb6f37a --- /dev/null +++ b/state-chain/runtime/src/migrations.rs @@ -0,0 +1,50 @@ +//! Chainflip runtime storage migrations. +use crate::System; +use frame_support::{traits::OnRuntimeUpgrade, weights::Weight}; +use sp_std::{marker::PhantomData, vec::Vec}; + +pub mod threshold_signature_callbacks; + +#[cfg(feature = "try-runtime")] +use sp_runtime::DispatchError; + +//const SPEC_VERSION: u32 = 101; + +/// A runtime storage migration that will only be applied if the `SPEC_VERSION` matches the +/// post-upgrade runtime's spec version. +pub struct VersionedMigration(PhantomData); + +impl OnRuntimeUpgrade for VersionedMigration +where + U: OnRuntimeUpgrade, +{ + fn on_runtime_upgrade() -> Weight { + if System::runtime_version().spec_version == SPEC_VERSION { + U::on_runtime_upgrade() + } else { + log::info!( + "Skipping storage migration for version {:?} - consider removing this from the runtime.", + SPEC_VERSION + ); + Weight::zero() + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, DispatchError> { + if System::runtime_version().spec_version == SPEC_VERSION { + U::pre_upgrade() + } else { + Ok(vec![]) + } + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), DispatchError> { + if System::runtime_version().spec_version == SPEC_VERSION { + U::post_upgrade(state) + } else { + Ok(()) + } + } +} diff --git a/state-chain/runtime/src/migrations/threshold_signature_callbacks.rs b/state-chain/runtime/src/migrations/threshold_signature_callbacks.rs new file mode 100644 index 0000000000..4bec5ddb94 --- /dev/null +++ b/state-chain/runtime/src/migrations/threshold_signature_callbacks.rs @@ -0,0 +1,260 @@ +use crate::{BitcoinInstance, Box, EthereumInstance, PolkadotInstance, Runtime, RuntimeCall}; +use codec::{Decode, Encode}; +use pallet_cf_broadcast::Call; +use scale_info::TypeInfo; +use sp_std::vec::Vec; + +use frame_support::{traits::OnRuntimeUpgrade, weights::Weight}; +use pallet_cf_broadcast::{BroadcastAttemptId, RequestCallbacks}; + +#[cfg(feature = "try-runtime")] +use sp_runtime::DispatchError; + +pub mod old { + use super::{Decode, Encode, TypeInfo, Vec}; + use cf_chains::{ + btc::{api::BitcoinApi, PreviousOrCurrent, SigningPayload}, + dot::{api::PolkadotApi, EncodedPolkadotPayload}, + eth::api::EthereumApi, + }; + use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; + use frame_support::Twox64Concat; + use sp_core::H256; + + use crate::chainflip::{BtcEnvironment, DotEnvironment, EthEnvironment}; + + #[derive(Debug, TypeInfo, Decode, Encode, Clone, PartialEq, Eq)] + #[allow(non_camel_case_types)] + pub enum EthereumBroadaster { + #[codec(index = 1u8)] + on_signature_ready { + threshold_request_id: ThresholdSignatureRequestId, + threshold_signature_payload: H256, + api_call: EthereumApi, + broadcast_id: BroadcastId, + initiated_at: u64, + }, + } + + #[derive(Debug, TypeInfo, Decode, Encode, Clone, PartialEq, Eq)] + #[allow(non_camel_case_types)] + pub enum PolkadotBroadaster { + #[codec(index = 1u8)] + on_signature_ready { + threshold_request_id: ThresholdSignatureRequestId, + threshold_signature_payload: EncodedPolkadotPayload, + api_call: PolkadotApi, + broadcast_id: BroadcastId, + initiated_at: u32, + }, + } + + #[derive(Debug, TypeInfo, Decode, Encode, Clone, PartialEq, Eq)] + #[allow(non_camel_case_types)] + pub enum BitcoinBroadaster { + #[codec(index = 1u8)] + on_signature_ready { + threshold_request_id: ThresholdSignatureRequestId, + threshold_signature_payload: Vec<(PreviousOrCurrent, SigningPayload)>, + api_call: BitcoinApi, + broadcast_id: BroadcastId, + initiated_at: u64, + }, + } + #[derive(Debug, Decode, Encode, Clone, PartialEq, Eq, TypeInfo)] + pub enum RuntimeCall { + #[codec(index = 27u8)] + EthereumBroadcaster(EthereumBroadaster), + #[codec(index = 28u8)] + PolkadotBroadaster(PolkadotBroadaster), + #[codec(index = 29u8)] + BitcoinBroadaster(BitcoinBroadaster), + } + + impl RuntimeCall { + pub fn unwrap_eth( + self, + ) -> (ThresholdSignatureRequestId, H256, EthereumApi, BroadcastId, u64) { + match self { + Self::EthereumBroadcaster(b) => match b { + EthereumBroadaster::on_signature_ready { + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + } => ( + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + ), + }, + _ => unreachable!(), + } + } + + pub fn unwrap_dot( + self, + ) -> ( + ThresholdSignatureRequestId, + EncodedPolkadotPayload, + PolkadotApi, + BroadcastId, + u32, + ) { + match self { + Self::PolkadotBroadaster(b) => match b { + PolkadotBroadaster::on_signature_ready { + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + } => ( + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + ), + }, + _ => unreachable!(), + } + } + + pub fn unwrap_btc( + self, + ) -> ( + ThresholdSignatureRequestId, + Vec<(PreviousOrCurrent, SigningPayload)>, + BitcoinApi, + BroadcastId, + u64, + ) { + match self { + Self::BitcoinBroadaster(b) => match b { + BitcoinBroadaster::on_signature_ready { + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + } => ( + threshold_request_id, + threshold_signature_payload, + api_call, + broadcast_id, + initiated_at, + ), + }, + _ => unreachable!(), + } + } + } + + #[frame_support::storage_alias] + pub type RequestCallbacks, I: 'static> = + StorageMap, Twox64Concat, BroadcastId, RuntimeCall>; +} +pub struct ThresholdSignatureCallbacks; +impl OnRuntimeUpgrade for ThresholdSignatureCallbacks { + fn on_runtime_upgrade() -> Weight { + RequestCallbacks::::translate(|_k, v: old::RuntimeCall| { + let c = v.unwrap_eth(); + Some(RuntimeCall::EthereumBroadcaster(Call::on_signature_ready { + threshold_request_id: c.0, + threshold_signature_payload: c.1, + api_call: Box::new(c.2), + broadcast_attempt_id: BroadcastAttemptId { broadcast_id: c.3, attempt_count: 0 }, + initiated_at: c.4, + })) + }); + + RequestCallbacks::::translate(|_k, v: old::RuntimeCall| { + let c = v.unwrap_dot(); + Some(RuntimeCall::PolkadotBroadcaster(Call::on_signature_ready { + threshold_request_id: c.0, + threshold_signature_payload: c.1, + api_call: Box::new(c.2), + broadcast_attempt_id: BroadcastAttemptId { broadcast_id: c.3, attempt_count: 0 }, + initiated_at: c.4, + })) + }); + + RequestCallbacks::::translate(|_k, v: old::RuntimeCall| { + let c = v.unwrap_btc(); + Some(RuntimeCall::BitcoinBroadcaster(Call::on_signature_ready { + threshold_request_id: c.0, + threshold_signature_payload: c.1, + api_call: Box::new(c.2), + broadcast_attempt_id: BroadcastAttemptId { broadcast_id: c.3, attempt_count: 0 }, + initiated_at: c.4, + })) + }); + + Weight::default() + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, DispatchError> { + let mut eth_broadcastids = old::RequestCallbacks::::iter() + .map(|(k, v)| (k, v.unwrap_eth().3)) + .collect::>(); + let mut dot_broadcastids = old::RequestCallbacks::::iter() + .map(|(k, v): (u32, old::RuntimeCall)| (k, v.unwrap_dot().3)) + .collect::>(); + let mut btc_broadcastids = old::RequestCallbacks::::iter() + .map(|(k, v): (u32, old::RuntimeCall)| (k, v.unwrap_btc().3)) + .collect::>(); + + eth_broadcastids.append(&mut dot_broadcastids); + eth_broadcastids.append(&mut btc_broadcastids); + Ok(eth_broadcastids.encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), DispatchError> { + use pallet_cf_vaults::ensure_variant; + + let old_storage = >::decode(&mut &state[..]).unwrap(); + + let mut eth_broadcastids = RequestCallbacks::::iter() + .map(|(k, v)| { + let call = ensure_variant!(RuntimeCall::EthereumBroadcaster(call) => call, v, DispatchError::Other("..")); + let broadcast_attempt_id = ensure_variant!(Call::on_signature_ready{broadcast_attempt_id, ..} => broadcast_attempt_id, call, DispatchError::Other("..")); + assert_eq!(broadcast_attempt_id.attempt_count, 0); + Ok((k, broadcast_attempt_id.broadcast_id)) + }) + .collect::, DispatchError>>()?; + + let mut dot_broadcastids = RequestCallbacks::::iter() + .map(|(k, v)| { + let call = ensure_variant!(RuntimeCall::PolkadotBroadcaster(call) => call, v, DispatchError::Other("..")); + let broadcast_attempt_id = ensure_variant!(Call::on_signature_ready{broadcast_attempt_id, ..} => broadcast_attempt_id, call, DispatchError::Other("..")); + assert_eq!(broadcast_attempt_id.attempt_count, 0); + Ok((k, broadcast_attempt_id.broadcast_id)) + }) + .collect::, DispatchError>>()?; + + let mut btc_broadcastids = RequestCallbacks::::iter() + .map(|(k, v)| { + let call = ensure_variant!(RuntimeCall::BitcoinBroadcaster(call) => call, v, DispatchError::Other("..")); + let broadcast_attempt_id = ensure_variant!(Call::on_signature_ready{broadcast_attempt_id, ..} => broadcast_attempt_id, call, DispatchError::Other("..")); + assert_eq!(broadcast_attempt_id.attempt_count, 0); + Ok((k, broadcast_attempt_id.broadcast_id)) + }) + .collect::, DispatchError>>()?; + + eth_broadcastids.append(&mut dot_broadcastids); + eth_broadcastids.append(&mut btc_broadcastids); + + for i in 0..eth_broadcastids.len() { + assert_eq!(eth_broadcastids[i].0, old_storage[i].0); + assert_eq!(eth_broadcastids[i].1, old_storage[i].1); + } + + Ok(()) + } +} From 51f014b0d925413036d0118285069f5f81e4f19a Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 29 Nov 2023 11:09:01 +0100 Subject: [PATCH 16/33] fix: vec import for tr-runtime --- state-chain/runtime/src/migrations.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/state-chain/runtime/src/migrations.rs b/state-chain/runtime/src/migrations.rs index b25fb6f37a..bf994a2136 100644 --- a/state-chain/runtime/src/migrations.rs +++ b/state-chain/runtime/src/migrations.rs @@ -1,8 +1,10 @@ //! Chainflip runtime storage migrations. use crate::System; use frame_support::{traits::OnRuntimeUpgrade, weights::Weight}; -use sp_std::{marker::PhantomData, vec::Vec}; +use sp_std::marker::PhantomData; +#[cfg(feature = "try-runtime")] +use sp_std::{vec, vec::Vec}; pub mod threshold_signature_callbacks; #[cfg(feature = "try-runtime")] From d9a0be7f1b13bde6273341b8fd0ef558822beab8 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 29 Nov 2023 13:18:05 +0100 Subject: [PATCH 17/33] fix: use only specific key for signing --- .../src/threshold_signing.rs | 15 +++++- state-chain/chains/src/dot.rs | 6 --- state-chain/chains/src/lib.rs | 8 --- state-chain/chains/src/mocks.rs | 19 ------- .../pallets/cf-threshold-signature/src/lib.rs | 19 +++---- .../cf-threshold-signature/src/tests.rs | 4 +- .../runtime/src/chainflip/broadcaster.rs | 50 ------------------- 7 files changed, 20 insertions(+), 101 deletions(-) delete mode 100644 state-chain/runtime/src/chainflip/broadcaster.rs diff --git a/state-chain/cf-integration-tests/src/threshold_signing.rs b/state-chain/cf-integration-tests/src/threshold_signing.rs index b92f1b385f..68b3612181 100644 --- a/state-chain/cf-integration-tests/src/threshold_signing.rs +++ b/state-chain/cf-integration-tests/src/threshold_signing.rs @@ -76,6 +76,7 @@ impl KeyUtils for EthKeyComponents { } pub struct ThresholdSigner { + previous_key_components: Option, key_components: KeyComponents, proposed_key_components: Option, _phantom: PhantomData, @@ -90,8 +91,14 @@ where if key == current_key { return self.key_components.sign(message) } - let next_key = self.proposed_key_components.as_ref().unwrap().agg_key(); - if key == next_key { + if self.previous_key_components.is_some() && + self.previous_key_components.as_ref().unwrap().agg_key() == key + { + return self.previous_key_components.as_ref().unwrap().sign(message) + } + if self.proposed_key_components.is_some() && + self.proposed_key_components.as_ref().unwrap().agg_key() == key + { self.proposed_key_components.as_ref().unwrap().sign(message) } else { panic!("Unknown key"); @@ -108,6 +115,7 @@ where // Rotate to the current proposed key and clear the proposed key pub fn use_proposed_key(&mut self) { if self.proposed_key_components.is_some() { + self.previous_key_components = Some(self.key_components.clone()); self.key_components = self.proposed_key_components.as_ref().expect("No key has been proposed").clone(); self.proposed_key_components = None; @@ -120,6 +128,7 @@ pub type EthThresholdSigner = ThresholdSigner Self { ThresholdSigner { + previous_key_components: None, key_components: EthKeyComponents::generate(GENESIS_KEY_SEED, GENESIS_EPOCH), proposed_key_components: None, _phantom: PhantomData, @@ -134,6 +143,7 @@ pub type DotThresholdSigner = ThresholdSigner Self { Self { + previous_key_components: None, key_components: DotKeyComponents::generate(GENESIS_KEY_SEED, GENESIS_EPOCH), proposed_key_components: None, _phantom: PhantomData, @@ -171,6 +181,7 @@ pub type BtcThresholdSigner = ThresholdSigner; impl Default for BtcThresholdSigner { fn default() -> Self { Self { + previous_key_components: None, key_components: BtcKeyComponents::generate(GENESIS_KEY_SEED, GENESIS_EPOCH), proposed_key_components: None, _phantom: PhantomData, diff --git a/state-chain/chains/src/dot.rs b/state-chain/chains/src/dot.rs index 514371c08d..5bb1bbac6c 100644 --- a/state-chain/chains/src/dot.rs +++ b/state-chain/chains/src/dot.rs @@ -308,12 +308,6 @@ impl ChainCrypto for PolkadotCrypto { EncodedPolkadotPayload(Blake2_256::hash(&agg_key.aliased_ref()[..]).to_vec()) } - /// Once authored, polkadot extrinsic must be signed by the key whose nonce was incorporated - /// into the extrinsic. - fn sign_with_specific_key() -> bool { - true - } - fn maybe_broadcast_barriers_on_rotation( rotation_broadcast_id: BroadcastId, ) -> Vec { diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 9f7f7cdc70..bdc50068df 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -179,14 +179,6 @@ pub trait ChainCrypto { true } - /// Determines whether threshold signatures are made with a specific fixed key, or whether the - /// key is refreshed if we need to retry the signature. - /// - /// By default, this is true for Utxo-based chains, false otherwise. - fn sign_with_specific_key() -> bool { - Self::UtxoChain::get() - } - /// Determines whether the chain crypto supports key handover. /// /// By default, this is true for Utxo-based chains, false otherwise. diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index c6d746d295..39a68124d8 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -23,7 +23,6 @@ pub enum ChainChoice { thread_local! { static MOCK_KEY_HANDOVER_IS_REQUIRED: RefCell = RefCell::new(true); - static MOCK_SIGN_WITH_SPECIFIC_KEY: RefCell = RefCell::new(false); static MOCK_VALID_METADATA: RefCell = RefCell::new(true); static MOCK_BROADCAST_BARRIERS: RefCell = RefCell::new(ChainChoice::Ethereum); } @@ -56,20 +55,6 @@ impl Get for MockBroadcastBarriers { } } -pub struct MockFixedKeySigningRequests; - -impl MockFixedKeySigningRequests { - pub fn set(value: bool) { - MOCK_SIGN_WITH_SPECIFIC_KEY.with(|v| *v.borrow_mut() = value); - } -} - -impl Get for MockFixedKeySigningRequests { - fn get() -> bool { - MOCK_SIGN_WITH_SPECIFIC_KEY.with(|v| *v.borrow()) - } -} - #[derive(Debug, Clone, Default, PartialEq, Eq, Encode, Decode, TypeInfo)] pub struct MockEthereumTransactionMetadata; @@ -267,10 +252,6 @@ impl ChainCrypto for MockEthereumChainCrypto { new_key != &BAD_AGG_KEY_POST_HANDOVER } - fn sign_with_specific_key() -> bool { - MockFixedKeySigningRequests::get() - } - fn key_handover_is_required() -> bool { MockKeyHandoverIsRequired::get() } diff --git a/state-chain/pallets/cf-threshold-signature/src/lib.rs b/state-chain/pallets/cf-threshold-signature/src/lib.rs index bfdd80f5bb..4299a17ee5 100644 --- a/state-chain/pallets/cf-threshold-signature/src/lib.rs +++ b/state-chain/pallets/cf-threshold-signature/src/lib.rs @@ -422,11 +422,7 @@ pub mod pallet { request_id, attempt_count.wrapping_add(1), payload, - if ::sign_with_specific_key() { - RequestType::SpecificKey(key, epoch) - } else { - RequestType::CurrentKey - }, + RequestType::SpecificKey(key, epoch), )); Event::::RetryRequested { request_id, ceremony_id } }, @@ -810,14 +806,11 @@ where type ValidatorId = T::ValidatorId; fn request_signature(payload: PayloadFor) -> RequestId { - let request_type = if ::sign_with_specific_key() { - T::KeyProvider::active_epoch_key().defensive_map_or_else( - || RequestType::CurrentKey, - |EpochKey { key, epoch_index, .. }| RequestType::SpecificKey(key, epoch_index), - ) - } else { - RequestType::CurrentKey - }; + let request_type = T::KeyProvider::active_epoch_key().defensive_map_or_else( + || RequestType::SpecificKey(Default::default(), Default::default()), + |EpochKey { key, epoch_index, .. }| RequestType::SpecificKey(key, epoch_index), + ); + Self::inner_request_signature(payload, request_type) } diff --git a/state-chain/pallets/cf-threshold-signature/src/tests.rs b/state-chain/pallets/cf-threshold-signature/src/tests.rs index 0ed818ad50..bfe5caca71 100644 --- a/state-chain/pallets/cf-threshold-signature/src/tests.rs +++ b/state-chain/pallets/cf-threshold-signature/src/tests.rs @@ -4,7 +4,7 @@ use crate::{ self as pallet_cf_threshold_signature, mock::*, AttemptCount, CeremonyContext, CeremonyId, Error, PalletOffence, RequestContext, RequestId, ThresholdSignatureResponseTimeout, }; -use cf_chains::mocks::{MockAggKey, MockEthereumChainCrypto, MockFixedKeySigningRequests}; +use cf_chains::mocks::{MockAggKey, MockEthereumChainCrypto}; use cf_traits::{ mocks::{key_provider::MockKeyProvider, signer_nomination::MockNominator}, AsyncResult, Chainflip, EpochKey, KeyProvider, ThresholdSigner, @@ -479,8 +479,6 @@ fn test_retries_for_immutable_key() { .. } = EthereumThresholdSigner::pending_ceremonies(ceremony_id).unwrap(); - MockFixedKeySigningRequests::set(true); - // Pretend the key has been updated to the next one. MockKeyProvider::::add_key(MockAggKey(*b"NEXT")); diff --git a/state-chain/runtime/src/chainflip/broadcaster.rs b/state-chain/runtime/src/chainflip/broadcaster.rs deleted file mode 100644 index 379cb5c66d..0000000000 --- a/state-chain/runtime/src/chainflip/broadcaster.rs +++ /dev/null @@ -1,50 +0,0 @@ -use core::marker::PhantomData; - -use crate::{EthereumBroadcaster, PolkadotBroadcaster}; -use cf_chains::{ - any::{AnyChainApi, AnyGovKey}, - eth::api::EthereumApi, - AnyChain, ChainCrypto, Ethereum, ForeignChain, Polkadot, SetAggKeyWithAggKey, - SetGovKeyWithAggKey, -}; -use cf_traits::{BroadcastAnyChainGovKey, Broadcaster}; -use sp_std::vec::Vec; - -use super::{DotEnvironment, EthEnvironment}; - -pub enum AnyApi { - Ethereum(EthereumApi), - Polkadot(PolkadotApi), -} - -impl SetGovKeyWithAggKey for AnyApi { - fn new_unsigned(maybe_old_key: Option, new_key: AnyGovKey) -> Result { - match (maybe_old_key, new_key) { - AnyGovKey::Ethereum(key) => - ::new_unsigned(maybe_old_key, new_key), - AnyGovKey::Polkadot(key) => - ::new_unsigned(maybe_old_key, new_key), - } - } -} - -impl Broadcaster for GovKeyBroadcaster { - type ApiCall = AnyApi; - - fn threshold_sign_and_broadcast( - api_call: Self::ApiCall, - ) -> cf_primitives::BroadcastId { - match api_call { - AnyChainApi::Ethereum(eth_api_call) => - >::threshold_sign_and_broadcast( - eth_api_call, - false, - ), - AnyChainApi::Polkadot(dot_api_call) => - >::threshold_sign_and_broadcast( - dot_api_call, - false, - ), - } - } -} From ba13b6bed3c6168836f851dcbe78d559c206c677 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 29 Nov 2023 14:11:34 +0100 Subject: [PATCH 18/33] chore: addressed comments --- state-chain/pallets/cf-broadcast/src/lib.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index b9106a1684..8fe0df9345 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -2,6 +2,7 @@ #![doc = include_str!("../README.md")] #![doc = include_str!("../../cf-doc-head.md")] #![feature(extract_if)] +#![feature(is_sorted)] mod benchmarking; mod mock; @@ -574,14 +575,18 @@ pub mod pallet { .ok_or(Error::::InvalidPayload)?; PendingBroadcasts::::mutate(|pending_broadcasts| { - pending_broadcasts.remove(pending_broadcasts.binary_search(&broadcast_id).expect("The broadcast_id should exist in the pending broadcasts list since we added it to the last when the broadcast was initated")) + debug_assert!(pending_broadcasts.iter().is_sorted()); + if let Ok(id) = pending_broadcasts.binary_search(&broadcast_id) { + pending_broadcasts.remove(id); + } else { + log::error!("The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated"); + } }); if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { - let maybe_earliest_pending_broadcast = - PendingBroadcasts::::get().first().copied(); - if maybe_earliest_pending_broadcast.is_none() || - (maybe_earliest_pending_broadcast.unwrap() > *broadcast_barrier_id) + if PendingBroadcasts::::get() + .first() + .map_or(true, |id| *id > *broadcast_barrier_id) { BroadcastBarriers::::mutate(|broadcast_barriers| { broadcast_barriers.pop_front(); From c267707c5ee1d1e870a75764ffcd99b8cb9c2016 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Thu, 30 Nov 2023 12:06:25 +0100 Subject: [PATCH 19/33] fix: only add barriers if there are already pending txs before it --- state-chain/pallets/cf-broadcast/src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 8fe0df9345..38af11a1e4 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -947,11 +947,15 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { let broadcast_id = Self::threshold_sign_and_broadcast(api_call, None); - let mut broadcast_barriers: VecDeque = <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation( - broadcast_id, - ).into(); + BroadcastBarriers::::mutate(|current_barriers| { - current_barriers.append(&mut broadcast_barriers) + current_barriers.append( + &mut <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation(broadcast_id) + .extract_if(|barrier| { + PendingBroadcasts::::get().first().map_or(false, |id| *id <= *barrier) + }) + .collect::>(), + ); }); broadcast_id } From c9daf2d600ac824f42d2d2ef2f9c40630ff0da26 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 1 Dec 2023 11:25:09 +0100 Subject: [PATCH 20/33] feat: remove aborted broadcasts from pending --- state-chain/pallets/cf-broadcast/src/lib.rs | 28 +++++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 38af11a1e4..c274ad87b6 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -320,6 +320,12 @@ pub mod pallet { #[pallet::getter(fn pending_broadcasts)] pub type PendingBroadcasts = StorageValue<_, Vec, ValueQuery>; + /// List of broadcasts that have been aborted because they were unsuccessful to be broadcast + /// after many retries. + #[pallet::storage] + #[pallet::getter(fn aborted_broadcasts)] + pub type AbortedBroadcasts = StorageValue<_, Vec, ValueQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event, I: 'static = ()> { @@ -491,6 +497,8 @@ pub mod pallet { .broadcast_attempt_id .broadcast_id, }); + Self::remove_pending_broadcast(&broadcast_attempt_id.broadcast_id); + AbortedBroadcasts::::append(broadcast_attempt_id.broadcast_id); } else { Self::schedule_for_retry(&signing_attempt.broadcast_attempt); } @@ -574,14 +582,7 @@ pub mod pallet { TransactionOutIdToBroadcastId::::take(&tx_out_id) .ok_or(Error::::InvalidPayload)?; - PendingBroadcasts::::mutate(|pending_broadcasts| { - debug_assert!(pending_broadcasts.iter().is_sorted()); - if let Ok(id) = pending_broadcasts.binary_search(&broadcast_id) { - pending_broadcasts.remove(id); - } else { - log::error!("The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated"); - } - }); + Self::remove_pending_broadcast(&broadcast_id); if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { if PendingBroadcasts::::get() @@ -716,6 +717,17 @@ impl, I: 'static> Pallet { } } + pub fn remove_pending_broadcast(broadcast_id: &BroadcastId) { + PendingBroadcasts::::mutate(|pending_broadcasts| { + debug_assert!(pending_broadcasts.iter().is_sorted()); + if let Ok(id) = pending_broadcasts.binary_search(broadcast_id) { + pending_broadcasts.remove(id); + } else { + log::error!("The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated"); + } + }); + } + /// Request a threshold signature, providing [Call::on_signature_ready] as the callback. pub fn threshold_sign_and_broadcast( api_call: >::ApiCall, From 20fd42aac092cb60e40aafe9c77303e5eb18f648 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 1 Dec 2023 11:44:20 +0100 Subject: [PATCH 21/33] chore: remove CurrentKey variant in RequestType --- .../pallets/cf-broadcast/src/benchmarking.rs | 12 ++++++------ .../cf-threshold-signature/src/benchmarking.rs | 8 +++++--- .../pallets/cf-threshold-signature/src/lib.rs | 14 -------------- state-chain/pallets/cf-vaults/src/lib.rs | 4 ++-- state-chain/traits/src/lib.rs | 2 +- 5 files changed, 14 insertions(+), 26 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/benchmarking.rs b/state-chain/pallets/cf-broadcast/src/benchmarking.rs index e0b3e542c1..b0e5df5528 100644 --- a/state-chain/pallets/cf-broadcast/src/benchmarking.rs +++ b/state-chain/pallets/cf-broadcast/src/benchmarking.rs @@ -3,7 +3,7 @@ use super::*; -use cf_traits::ThresholdSigner; +use cf_traits::{CurrentEpochIndex, ThresholdSigner}; use frame_benchmarking::{benchmarks_instance_pallet, whitelisted_caller}; use frame_support::{ dispatch::UnfilteredDispatchable, @@ -67,7 +67,7 @@ benchmarks_instance_pallet! { ThresholdSignatureData::::insert(i, (ApiCallFor::::benchmark_value(), ThresholdSignatureFor::::benchmark_value())) } let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key); + T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { Pallet::::on_initialize(timeout_block); } @@ -85,7 +85,7 @@ benchmarks_instance_pallet! { generate_on_signature_ready_call::().dispatch_bypass_filter(T::EnsureThresholdSigned::try_successful_origin().unwrap())?; let expiry_block = frame_system::Pallet::::block_number() + T::BroadcastTimeout::get(); let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key); + T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); }: _(RawOrigin::Signed(caller), broadcast_attempt_id) verify { assert!(Timeouts::::contains_key(expiry_block)); @@ -100,7 +100,7 @@ benchmarks_instance_pallet! { insert_transaction_broadcast_attempt::(whitelisted_caller(), broadcast_attempt_id); let call = generate_on_signature_ready_call::(); let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key); + T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { call.dispatch_bypass_filter(T::EnsureThresholdSigned::try_successful_origin().unwrap())? } verify { assert_eq!(BroadcastIdCounter::::get(), 0); @@ -119,7 +119,7 @@ benchmarks_instance_pallet! { INITIATED_AT.into(), ); - T::KeyProvider::set_key(AggKeyFor::::benchmark_value()); + T::KeyProvider::set_key(AggKeyFor::::benchmark_value(), CurrentEpochIndex::::get()); let transaction_payload = TransactionFor::::benchmark_value(); } : { @@ -151,7 +151,7 @@ benchmarks_instance_pallet! { tx_metadata: TransactionMetadataFor::::benchmark_value(), }; let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key); + T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { call.dispatch_bypass_filter(T::EnsureWitnessedAtCurrentEpoch::try_successful_origin().unwrap())? } verify { // We expect the unwrap to error if the extrinsic didn't fire an event - if an event has been emitted we reached the end of the extrinsic diff --git a/state-chain/pallets/cf-threshold-signature/src/benchmarking.rs b/state-chain/pallets/cf-threshold-signature/src/benchmarking.rs index 8cf4bb8f99..2a4255c94a 100644 --- a/state-chain/pallets/cf-threshold-signature/src/benchmarking.rs +++ b/state-chain/pallets/cf-threshold-signature/src/benchmarking.rs @@ -4,7 +4,7 @@ use super::*; use cf_chains::{benchmarking_value::BenchmarkValue, ChainCrypto}; -use cf_traits::{AccountRoleRegistry, Chainflip, ThresholdSigner}; +use cf_traits::{AccountRoleRegistry, Chainflip, CurrentEpochIndex, ThresholdSigner}; use frame_benchmarking::{account, benchmarks_instance_pallet, whitelist_account}; use frame_support::{ assert_ok, @@ -94,12 +94,14 @@ benchmarks_instance_pallet! { let a in 10..150; // r: number of retries let r in 0..50; - T::KeyProvider::set_key(::AggKey::benchmark_value()); + let key = ::AggKey::benchmark_value(); + let current_epoch = CurrentEpochIndex::::get(); + T::KeyProvider::set_key(key, current_epoch); CurrentAuthorities::::put(BTreeSet::<::ValidatorId>::new()); // These attempts will fail because there are no authorities to do the signing. for _ in 0..r { - Pallet::::new_ceremony_attempt(RequestInstruction::new(1, 1, PayloadFor::::benchmark_value(), RequestType::CurrentKey)); + Pallet::::new_ceremony_attempt(RequestInstruction::new(1, 1, PayloadFor::::benchmark_value(), RequestType::SpecificKey(key, current_epoch))); } assert_eq!( diff --git a/state-chain/pallets/cf-threshold-signature/src/lib.rs b/state-chain/pallets/cf-threshold-signature/src/lib.rs index 4299a17ee5..89868d577e 100644 --- a/state-chain/pallets/cf-threshold-signature/src/lib.rs +++ b/state-chain/pallets/cf-threshold-signature/src/lib.rs @@ -57,9 +57,6 @@ pub enum PalletOffence { #[derive(Clone, RuntimeDebug, PartialEq, Eq, Encode, Decode, TypeInfo)] pub enum RequestType { - /// Will use the current key and current authority set (which might change between retries). - /// This signing request will be retried until success. - CurrentKey, /// Uses the provided key and selects new participants from the provided epoch. /// This signing request will be retried until success. SpecificKey(Key, EpochIndex), @@ -365,11 +362,6 @@ pub mod pallet { request_id: RequestId, attempt_count: AttemptCount, }, - /// We cannot sign because the key is unavailable. - CurrentKeyUnavailable { - request_id: RequestId, - attempt_count: AttemptCount, - }, /// The threshold signature response timeout has been updated ThresholdSignatureResponseTimeoutUpdated { new_timeout: BlockNumberFor, @@ -671,12 +663,6 @@ impl, I: 'static> Pallet { } else { ( match request_instruction.request_type { - RequestType::CurrentKey => T::KeyProvider::active_epoch_key() - .map(|EpochKey { key, epoch_index }| (key, epoch_index)) - .ok_or(Event::::CurrentKeyUnavailable { - request_id, - attempt_count, - }), RequestType::SpecificKey(key, epoch_index) => Ok((key, epoch_index)), _ => unreachable!("RequestType::KeygenVerification is handled above"), } diff --git a/state-chain/pallets/cf-vaults/src/lib.rs b/state-chain/pallets/cf-vaults/src/lib.rs index 7cbf8642cb..ad801e9114 100644 --- a/state-chain/pallets/cf-vaults/src/lib.rs +++ b/state-chain/pallets/cf-vaults/src/lib.rs @@ -953,9 +953,9 @@ impl, I: 'static> KeyProvider<::ChainCrypto> for } #[cfg(feature = "runtime-benchmarks")] - fn set_key(key: <::ChainCrypto as ChainCrypto>::AggKey) { + fn set_key(key: <::ChainCrypto as ChainCrypto>::AggKey, epoch: EpochIndex) { Vaults::::insert( - CurrentEpochIndex::::get(), + epoch, Vault { public_key: key, active_from_block: ChainBlockNumberFor::::from(0u32) }, ); } diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index bf15377348..38f81f8a51 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -378,7 +378,7 @@ pub trait KeyProvider { fn active_epoch_key() -> Option>; #[cfg(feature = "runtime-benchmarks")] - fn set_key(_key: C::AggKey) { + fn set_key(_key: C::AggKey, _epoch: EpochIndex) { unimplemented!() } } From 7ddeea6de684da212de3daaca55707d40e583401 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 2 Dec 2023 01:24:36 +0100 Subject: [PATCH 22/33] fix: all tests passing (bouncer tbc) --- Cargo.lock | 1 + .../cf-integration-tests/src/funding.rs | 20 +- .../cf-integration-tests/src/network.rs | 3 +- .../cf-integration-tests/src/swapping.rs | 53 +++-- state-chain/pallets/cf-broadcast/Cargo.toml | 2 + .../pallets/cf-broadcast/src/benchmarking.rs | 12 +- state-chain/pallets/cf-broadcast/src/lib.rs | 190 +++++++++--------- state-chain/pallets/cf-broadcast/src/tests.rs | 97 ++++----- 8 files changed, 186 insertions(+), 192 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6b284f469b..f2de3243c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7251,6 +7251,7 @@ dependencies = [ "cf-chains", "cf-primitives", "cf-runtime-upgrade-utilities", + "cf-runtime-utilities", "cf-test-utilities", "cf-traits", "frame-benchmarking", diff --git a/state-chain/cf-integration-tests/src/funding.rs b/state-chain/cf-integration-tests/src/funding.rs index ae998ba7c2..4845c24c59 100644 --- a/state-chain/cf-integration-tests/src/funding.rs +++ b/state-chain/cf-integration-tests/src/funding.rs @@ -156,33 +156,31 @@ fn backup_reward_is_calculated_linearly() { // 3 backup will split the backup reward. assert_eq!(Validator::highest_funded_qualified_backup_node_bids().count(), 3); + const N: u128 = 100; - let rewards_per_block = &calculate_backup_rewards::( + let rewards_per_heartbeat = &calculate_backup_rewards::( Validator::highest_funded_qualified_backup_node_bids().collect::>(), Validator::bond(), - 1u128, + HEARTBEAT_BLOCK_INTERVAL as u128, Emissions::backup_node_emission_per_block(), Emissions::current_authority_emission_per_block(), Validator::current_authority_count() as u128, ); - let rewards_per_heartbeat = &calculate_backup_rewards::( + let rewards_per_n_heartbeats = &calculate_backup_rewards::( Validator::highest_funded_qualified_backup_node_bids().collect::>(), Validator::bond(), - HEARTBEAT_BLOCK_INTERVAL as u128, + HEARTBEAT_BLOCK_INTERVAL as u128 * N, Emissions::backup_node_emission_per_block(), Emissions::current_authority_emission_per_block(), Validator::current_authority_count() as u128, ); - for i in 0..rewards_per_block.len() { + for i in 0..rewards_per_heartbeat.len() { // Validator account should match - assert_eq!(rewards_per_block[i].0, rewards_per_heartbeat[i].0); + assert_eq!(rewards_per_heartbeat[i].0, rewards_per_heartbeat[i].0); // Reward per heartbeat should be scaled linearly. - assert_eq!( - rewards_per_heartbeat[i].1, - rewards_per_block[i].1 * HEARTBEAT_BLOCK_INTERVAL as u128 - ); + assert_eq!(rewards_per_n_heartbeats[i].1, rewards_per_heartbeat[i].1 * N); } }); } @@ -270,7 +268,7 @@ fn apy_can_be_above_100_percent() { MAX_AUTHORITIES as u128; let apy_basis_point = FixedU64::from_rational(reward, total).checked_mul_int(10_000u32).unwrap(); - assert_eq!(apy_basis_point, 241_377_726u32); + assert_eq!(apy_basis_point, 241_377_727u32); assert_eq!(calculate_account_apy(&validator), Some(apy_basis_point)); }); } diff --git a/state-chain/cf-integration-tests/src/network.rs b/state-chain/cf-integration-tests/src/network.rs index 4e6448ab2c..5ea02ad770 100644 --- a/state-chain/cf-integration-tests/src/network.rs +++ b/state-chain/cf-integration-tests/src/network.rs @@ -575,8 +575,7 @@ impl Network { true => true, false => engine.auto_submit_heartbeat && - engine.last_heartbeat + Validator::blocks_per_epoch() - 1 <= - current_block, + engine.last_heartbeat + HEARTBEAT_BLOCK_INTERVAL - 1 <= current_block, } { assert_ok!(Reputation::heartbeat(RuntimeOrigin::signed(engine.node_id.clone()))); engine.last_heartbeat = current_block; diff --git a/state-chain/cf-integration-tests/src/swapping.rs b/state-chain/cf-integration-tests/src/swapping.rs index 94b6a704ee..947e8846d0 100644 --- a/state-chain/cf-integration-tests/src/swapping.rs +++ b/state-chain/cf-integration-tests/src/swapping.rs @@ -11,6 +11,7 @@ use cf_chains::{ address::{AddressConverter, AddressDerivationApi, EncodedAddress}, assets::eth::Asset as EthAsset, eth::{api::EthereumApi, EthereumTrackedData}, + evm::TransactionFee, CcmChannelMetadata, CcmDepositMetadata, Chain, ChainState, Ethereum, ExecutexSwapAndCall, ForeignChain, ForeignChainAddress, SwapOrigin, TransactionBuilder, TransferAssetParams, }; @@ -18,15 +19,15 @@ use cf_primitives::{ AccountId, AccountRole, Asset, AssetAmount, AuthorityCount, GENESIS_EPOCH, STABLE_ASSET, }; use cf_test_utilities::{assert_events_eq, assert_events_match}; -use cf_traits::{AccountRoleRegistry, EpochInfo, LpBalanceApi}; +use cf_traits::{AccountRoleRegistry, Chainflip, EpochInfo, LpBalanceApi}; use frame_support::{ assert_ok, instances::Instance1, traits::{OnFinalize, OnIdle, OnNewAccount}, }; use pallet_cf_broadcast::{ - AwaitingBroadcast, BroadcastAttemptId, RequestFailureCallbacks, RequestSuccessCallbacks, - ThresholdSignatureData, TransactionSigningAttempt, + AwaitingBroadcast, BroadcastAttemptId, BroadcastIdCounter, RequestFailureCallbacks, + RequestSuccessCallbacks, ThresholdSignatureData, TransactionSigningAttempt, }; use pallet_cf_ingress_egress::{DepositWitness, FailedCcm}; use pallet_cf_pools::{OrderId, RangeOrderSize}; @@ -770,6 +771,33 @@ fn can_resign_failed_ccm() { } testnet.move_to_the_next_epoch(); + let tx_out_id = AwaitingBroadcast::::get(BroadcastAttemptId { + broadcast_id: 1, + attempt_count: 0, + }) + .unwrap() + .broadcast_attempt + .transaction_out_id; + + for node in Validator::current_authorities() { + // Broadcast success for id 1, which is the rotation transaction. + // This needs to succeed because it's a barrier broadcast. + assert_ok!(Witnesser::witness_at_epoch( + RuntimeOrigin::signed(node), + Box::new(RuntimeCall::EthereumBroadcaster( + pallet_cf_broadcast::Call::transaction_succeeded { + tx_out_id, + signer_id: Default::default(), + tx_fee: TransactionFee { + effective_gas_price: Default::default(), + gas_used: Default::default() + }, + tx_metadata: Default::default(), + } + )), + ::EpochInfo::current_epoch() + )); + } setup_pool_and_accounts(vec![Asset::Eth, Asset::Flip]); // Deposit CCM and process the swap @@ -801,26 +829,19 @@ fn can_resign_failed_ccm() { } // Process the swap -> egress -> threshold sign -> broadcast - let broadcast_id = 2; - assert!(EthereumBroadcaster::threshold_signature_data(broadcast_id).is_none()); testnet.move_forward_blocks(3); - - // Threshold signature is ready and the call is ready to be broadcasted. - assert!(EthereumBroadcaster::threshold_signature_data(broadcast_id).is_some()); - - let validators = Validator::current_authorities(); + let broadcast_id = BroadcastIdCounter::::get(); let mut broadcast_attempt_id = BroadcastAttemptId { broadcast_id, attempt_count: 0 }; // Fail the broadcast - for _ in 0..validators.len() { + for _ in Validator::current_authorities() { let TransactionSigningAttempt { broadcast_attempt: _attempt, nominee } = AwaitingBroadcast::::get(broadcast_attempt_id) .unwrap_or_else(|| { panic!( - "Failed to get the transaction signing attempt for {:?}. Available attempts: {:?}", - broadcast_attempt_id, - AwaitingBroadcast::::iter().collect::>() - ) + "Failed to get the transaction signing attempt for {:?}.", + broadcast_attempt_id, + ) }); assert_ok!(EthereumBroadcaster::transaction_signing_failure( @@ -828,7 +849,7 @@ fn can_resign_failed_ccm() { broadcast_attempt_id, )); testnet.move_forward_blocks(1); - broadcast_attempt_id = broadcast_attempt_id.next_attempt(); + broadcast_attempt_id = broadcast_attempt_id.peek_next(); } // Upon broadcast failure, the Failure callback is called, and failed CCM is stored. diff --git a/state-chain/pallets/cf-broadcast/Cargo.toml b/state-chain/pallets/cf-broadcast/Cargo.toml index 96a0bb3dca..eca2ea6423 100644 --- a/state-chain/pallets/cf-broadcast/Cargo.toml +++ b/state-chain/pallets/cf-broadcast/Cargo.toml @@ -18,6 +18,7 @@ cf-chains = { path = '../../chains', default-features = false } cf-primitives = { path = '../../primitives', default-features = false } cf-runtime-upgrade-utilities = { path = '../../runtime-upgrade-utilities', default-features = false } cf-traits = { path = '../../traits', default-features = false } +cf-runtime-utilities = { path = '../../runtime-utilities', default-features = false } log = { version = '0.4.16', default-features = false } @@ -46,6 +47,7 @@ std = [ 'cf-chains/std', 'cf-primitives/std', 'cf-runtime-upgrade-utilities/std', + 'cf-runtime-utilities/std', 'cf-traits/std', 'codec/std', 'frame-benchmarking?/std', diff --git a/state-chain/pallets/cf-broadcast/src/benchmarking.rs b/state-chain/pallets/cf-broadcast/src/benchmarking.rs index 4b26e3afdf..7c4c3e143c 100644 --- a/state-chain/pallets/cf-broadcast/src/benchmarking.rs +++ b/state-chain/pallets/cf-broadcast/src/benchmarking.rs @@ -111,13 +111,13 @@ benchmarks_instance_pallet! { start_next_broadcast_attempt { let api_call = ApiCallFor::::benchmark_value(); let signed_api_call = api_call.signed(&BenchmarkValue::benchmark_value()); - let broadcast_attempt_id = Pallet::::start_broadcast( + let broadcast_id = as Broadcaster<_>>::threshold_sign_and_broadcast( BenchmarkValue::benchmark_value(), - signed_api_call, - BenchmarkValue::benchmark_value(), - BroadcastAttemptId{broadcast_id: 1, attempt_count: 0}, - INITIATED_AT.into(), ); + let broadcast_attempt_id = BroadcastAttemptId { + broadcast_id, + attempt_count: BroadcastAttemptCount::::get(broadcast_id), + }; T::KeyProvider::set_key(AggKeyFor::::benchmark_value(), CurrentEpochIndex::::get()); let transaction_payload = TransactionFor::::benchmark_value(); @@ -131,7 +131,7 @@ benchmarks_instance_pallet! { }) } verify { - assert!(AwaitingBroadcast::::contains_key(broadcast_attempt_id.next_attempt())); + assert!(AwaitingBroadcast::::contains_key(broadcast_attempt_id.peek_next())); } transaction_succeeded { let caller: T::AccountId = whitelisted_caller(); diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index b07cbb8cf4..ec8c386011 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -12,7 +12,10 @@ pub mod migrations; pub mod weights; use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; use cf_traits::{GetBlockHeight, SafeMode}; -use frame_support::{traits::OriginTrait, RuntimeDebug}; +use frame_support::{ + traits::{Defensive, OriginTrait}, + RuntimeDebug, +}; use sp_std::marker; pub use weights::WeightInfo; @@ -35,7 +38,7 @@ use cf_chains::{ }; use cf_traits::{ offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, - EpochKey, KeyProvider, OnBroadcastReady, ThresholdSigner, + EpochKey, KeyProvider, ThresholdSigner, }; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ @@ -63,9 +66,23 @@ pub struct BroadcastAttemptId { } impl BroadcastAttemptId { - /// Increment the attempt count for a particular BroadcastAttemptId - pub fn next_attempt(&self) -> Self { - Self { attempt_count: self.attempt_count.wrapping_add(1), ..*self } + /// Get the next BroadcastAttemptId. + pub fn peek_next(&self) -> Self { + Self { attempt_count: self.attempt_count + 1, ..*self } + } + + /// Increment the attempt counter and return the next BroadcastAttemptId. + pub fn into_next, I: 'static>(self) -> Self { + Self { + attempt_count: BroadcastAttemptCount::::mutate( + self.broadcast_id, + |attempt_count: &mut AttemptCount| { + *attempt_count += 1; + *attempt_count + }, + ), + ..self + } } } @@ -400,11 +417,18 @@ pub mod pallet { let expiries = Timeouts::::take(block_number); if T::SafeMode::get().retry_enabled { for attempt_id in expiries.iter() { - if let Some(attempt) = Self::take_awaiting_broadcast(*attempt_id) { + if PendingBroadcasts::::get() + .binary_search(&attempt_id.broadcast_id) + .is_ok() + { Self::deposit_event(Event::::BroadcastAttemptTimeout { broadcast_attempt_id: *attempt_id, }); - Self::start_next_broadcast_attempt(attempt); + if let Some(broadcast_attempt) = Self::take_awaiting_broadcast(*attempt_id) + .defensive_proof("take_awaiting_broadcast should not fail") + { + Self::start_next_broadcast_attempt(broadcast_attempt); + } } } } else { @@ -443,7 +467,11 @@ pub mod pallet { let retries_len = retries.len(); for retry in retries { - if Self::take_awaiting_broadcast(retry.broadcast_attempt_id).is_some() { + // Check if the broadcast is pending + if PendingBroadcasts::::get() + .binary_search(&retry.broadcast_attempt_id.broadcast_id) + .is_ok() + { Self::start_next_broadcast_attempt(retry); } } @@ -541,7 +569,7 @@ pub mod pallet { /// /// ## Events /// - /// - If `should_broadcast` see [Call::start_broadcast] + /// - [Event::CallResigned] If the call was re-signed. /// /// /// ## Errors @@ -572,6 +600,7 @@ pub mod pallet { .expect("signature can not be unavailable"); let signed_api_call = api_call.signed(&signature); + ThresholdSignatureData::::insert( broadcast_attempt_id.broadcast_id, (signed_api_call.clone(), signature), @@ -579,13 +608,30 @@ pub mod pallet { // If a signed call already exists, update the storage and do not broadcast. if should_broadcast { - Self::start_broadcast( - T::TransactionBuilder::build_transaction(&signed_api_call), - signed_api_call, - threshold_signature_payload, - broadcast_attempt_id, - initiated_at, + let transaction_out_id = signed_api_call.transaction_out_id(); + + T::BroadcastReadyProvider::on_broadcast_ready(&signed_api_call); + + // The Engine uses this. + TransactionOutIdToBroadcastId::::insert( + &transaction_out_id, + (broadcast_attempt_id.broadcast_id, initiated_at), ); + + let broadcast_attempt = BroadcastAttempt:: { + broadcast_attempt_id, + transaction_payload: T::TransactionBuilder::build_transaction(&signed_api_call), + threshold_signature_payload, + transaction_out_id, + }; + + if BroadcastBarriers::::get().front().is_some_and(|broadcast_barrier_id| { + broadcast_attempt_id.broadcast_id > *broadcast_barrier_id + }) { + Self::schedule_for_retry(&broadcast_attempt); + } else { + Self::start_broadcast_attempt(broadcast_attempt); + } } else { Self::deposit_event(Event::::CallResigned { broadcast_id: broadcast_attempt_id.broadcast_id, @@ -639,23 +685,30 @@ pub mod pallet { if let Some(expected_tx_metadata) = TransactionMetadata::::take(broadcast_id) { if tx_metadata.verify_metadata(&expected_tx_metadata) { - let to_refund = AwaitingBroadcast::::get(BroadcastAttemptId { + if let Some(to_refund) = AwaitingBroadcast::::get(BroadcastAttemptId { broadcast_id, attempt_count: BroadcastAttemptCount::::get(broadcast_id), }) - .ok_or(Error::::InvalidBroadcastAttemptId)? - .broadcast_attempt - .transaction_payload - .return_fee_refund(tx_fee); - - TransactionFeeDeficit::::mutate(signer_id.clone(), |fee_deficit| { - *fee_deficit = fee_deficit.saturating_add(to_refund); - }); + .map(|signing_attempt| { + signing_attempt + .broadcast_attempt + .transaction_payload + .return_fee_refund(tx_fee) + }) { + TransactionFeeDeficit::::mutate(signer_id.clone(), |fee_deficit| { + *fee_deficit = fee_deficit.saturating_add(to_refund); + }); - Self::deposit_event(Event::::TransactionFeeDeficitRecorded { - beneficiary: signer_id, - amount: to_refund, - }); + Self::deposit_event(Event::::TransactionFeeDeficitRecorded { + beneficiary: signer_id, + amount: to_refund, + }); + } else { + log::warn!( + "Unable to attribute transaction fee refundfor broadcast {}.", + broadcast_id + ); + } } else { Self::deposit_event(Event::::TransactionFeeDeficitRefused { beneficiary: signer_id, @@ -723,26 +776,16 @@ pub mod pallet { impl, I: 'static> Pallet { pub fn clean_up_broadcast_storage(broadcast_id: BroadcastId) { - let first_attempt = AttemptCount::default(); - - if let Some(transaction_signing_attempt) = - AwaitingBroadcast::::get(BroadcastAttemptId { - broadcast_id, - attempt_count: first_attempt, - }) { - TransactionOutIdToBroadcastId::::remove( - transaction_signing_attempt.broadcast_attempt.transaction_out_id, - ); - }; - - for attempt_count in first_attempt..=(BroadcastAttemptCount::::take(broadcast_id)) { + for attempt_count in 0..=(BroadcastAttemptCount::::take(broadcast_id)) { AwaitingBroadcast::::remove(BroadcastAttemptId { broadcast_id, attempt_count }); } TransactionMetadata::::remove(broadcast_id); RequestSuccessCallbacks::::remove(broadcast_id); RequestFailureCallbacks::::remove(broadcast_id); - ThresholdSignatureData::::remove(broadcast_id); + if let Some((api_call, _)) = ThresholdSignatureData::::take(broadcast_id) { + TransactionOutIdToBroadcastId::::remove(api_call.transaction_out_id()); + } } pub fn take_awaiting_broadcast( @@ -766,7 +809,9 @@ impl, I: 'static> Pallet { if let Ok(id) = pending_broadcasts.binary_search(broadcast_id) { pending_broadcasts.remove(id); } else { - log::error!("The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated"); + cf_runtime_utilities::log_or_panic!( + "The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated" + ); } }); } @@ -828,44 +873,6 @@ impl, I: 'static> Pallet { ) } - /// Begin the process of broadcasting a transaction. - /// - /// ## Events - /// - /// - [TransactionBroadcastRequest](Event::TransactionBroadcastRequest) - fn start_broadcast( - transaction_payload: TransactionFor, - api_call: >::ApiCall, - threshold_signature_payload: <::ChainCrypto as ChainCrypto>::Payload, - broadcast_attempt_id: BroadcastAttemptId, - initiated_at: ChainBlockNumberFor, - ) -> BroadcastAttemptId { - let transaction_out_id = api_call.transaction_out_id(); - - T::BroadcastReadyProvider::on_broadcast_ready(&api_call); - - TransactionOutIdToBroadcastId::::insert( - &transaction_out_id, - (broadcast_attempt_id.broadcast_id, initiated_at), - ); - - let broadcast_attempt = BroadcastAttempt:: { - broadcast_attempt_id, - transaction_payload, - threshold_signature_payload, - transaction_out_id, - }; - - if BroadcastBarriers::::get().front().is_some_and(|broadcast_barrier_id| { - broadcast_attempt_id.broadcast_id > *broadcast_barrier_id - }) { - Self::schedule_for_retry(&broadcast_attempt); - } else { - Self::start_broadcast_attempt(broadcast_attempt); - } - broadcast_attempt_id - } - fn start_next_broadcast_attempt(broadcast_attempt: BroadcastAttempt) { let broadcast_id = broadcast_attempt.broadcast_attempt_id.broadcast_id; @@ -873,18 +880,8 @@ impl, I: 'static> Pallet { let EpochKey { key, .. } = T::KeyProvider::active_epoch_key() .expect("Epoch key must exist if we made a broadcast."); - let next_broadcast_attempt_id = broadcast_attempt.broadcast_attempt_id.next_attempt(); - - BroadcastAttemptCount::::mutate( - broadcast_id, - |attempt_count: &mut AttemptCount| { - *attempt_count += 1; - }, - ); - debug_assert_eq!( - BroadcastAttemptCount::::get(broadcast_id), - next_broadcast_attempt_id.attempt_count, - ); + let next_broadcast_attempt_id = + broadcast_attempt.broadcast_attempt_id.into_next::(); if T::TransactionBuilder::is_valid_for_rebroadcast( &api_call, @@ -900,13 +897,10 @@ impl, I: 'static> Pallet { // If the signature verification fails, we want // to retry from the threshold signing stage. else { - // Only storage to be removed, the other storages corresponding to this broadcast - // are either still valid or will get overwritten on re-threshold sign and - // broadcast. - TransactionOutIdToBroadcastId::::remove(broadcast_attempt.transaction_out_id); - // We update the initiated_at here since as the tx is resigned and broadcast, it is // not possible for it to be successfully broadcasted before this point. + // This `initiated_at` block will be associated with the new transaction_out_id + // so should not interfere with witnessing the previous one. let initiated_at = T::ChainTracking::get_block_height(); Self::deposit_event(Event::::ThresholdSignatureInvalid { diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index 9765b16719..849d2a2f1c 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -11,8 +11,8 @@ use cf_chains::{ evm::SchnorrVerificationComponents, mocks::{ ChainChoice, MockApiCall, MockBroadcastBarriers, MockEthereum, MockEthereumChainCrypto, - MockEthereumTransactionMetadata, MockTransaction, MockTransactionBuilder, ETH_TX_FEE, - MOCK_TX_METADATA, + MockEthereumTransactionMetadata, MockTransactionBuilder, ETH_TX_FEE, + MOCK_TRANSACTION_OUT_ID, MOCK_TX_METADATA, }, ChainCrypto, FeeRefundCalculator, }; @@ -43,8 +43,6 @@ thread_local! { // When calling on_idle, we should broadcast everything with this excess weight. const LARGE_EXCESS_WEIGHT: Weight = Weight::from_parts(20_000_000_000, 0); -const MOCK_TRANSACTION_OUT_ID: [u8; 4] = [0xbc; 4]; - struct MockCfe; impl MockCfe { @@ -128,27 +126,15 @@ fn assert_broadcast_storage_cleaned_up(broadcast_id: BroadcastId) { } fn start_mock_broadcast_tx_out_id( - tx_out_id: ::TransactionOutId, -) -> BroadcastAttemptId { - let api_call = - MockApiCall { tx_out_id, payload: Default::default(), sig: Some(Default::default()) }; - let broadcast_id = 1; - // Insert threshold signature into storage. - ThresholdSignatureData::::insert( - broadcast_id, - (api_call.clone(), api_call.sig.unwrap()), - ); - Broadcaster::start_broadcast( - MockTransaction, - api_call, - Default::default(), - BroadcastAttemptId { broadcast_id: 1, attempt_count: 0 }, - 100u64, - ) + i: u8, +) -> (BroadcastAttemptId, ::TransactionOutId) { + let (tx_out_id, apicall) = api_call(i); + let broadcast_id = initiate_and_sign_broadcast(&apicall, TxType::Normal); + (BroadcastAttemptId { broadcast_id, attempt_count: 0 }, tx_out_id) } fn start_mock_broadcast() -> BroadcastAttemptId { - start_mock_broadcast_tx_out_id(Default::default()) + start_mock_broadcast_tx_out_id(Default::default()).0 } #[test] @@ -192,15 +178,16 @@ fn transaction_succeeded_results_in_refund_for_signer() { #[test] fn test_abort_after_number_of_attempts_is_equal_to_the_number_of_authorities() { new_test_ext().execute_with(|| { - let broadcast_attempt_id = start_mock_broadcast(); + let (_tx_out_id, apicall) = api_call(1); + let broadcast_id = initiate_and_sign_broadcast(&apicall, TxType::Normal); for i in 0..MockEpochInfo::current_authority_count() { // Nominated signer responds that they can't sign the transaction. // retry should kick off at end of block if sufficient block space is free. assert_eq!( - BroadcastAttemptCount::::get(broadcast_attempt_id.broadcast_id), - broadcast_attempt_id.attempt_count + i, - "Failed for {broadcast_attempt_id:?} at iteration {i}" + BroadcastAttemptCount::::get(broadcast_id), + i, + "Failed for {broadcast_id:?} at iteration {i}" ); MockCfe::respond(Scenario::SigningFailure); Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); @@ -208,9 +195,7 @@ fn test_abort_after_number_of_attempts_is_equal_to_the_number_of_authorities() { assert_eq!( System::events().pop().expect("an event").event, - RuntimeEvent::Broadcaster(crate::Event::BroadcastAborted { - broadcast_id: broadcast_attempt_id.broadcast_id - }) + RuntimeEvent::Broadcaster(crate::Event::BroadcastAborted { broadcast_id }) ); }); } @@ -236,8 +221,9 @@ fn on_idle_caps_broadcasts_when_not_enough_weight() { Broadcaster::on_initialize(0); // only the first one should have retried, incremented attempt count - assert!(AwaitingBroadcast::::get(broadcast_attempt_id.next_attempt()) - .is_some()); + assert!( + AwaitingBroadcast::::get(broadcast_attempt_id.peek_next()).is_some() + ); // the other should be still in the retry queue let retry_queue = BroadcastRetryQueue::::get(); assert_eq!(retry_queue.len(), 1); @@ -272,8 +258,9 @@ fn test_transaction_signing_failed() { Broadcaster::on_idle(0, LARGE_EXCESS_WEIGHT); Broadcaster::on_initialize(0); - assert!(AwaitingBroadcast::::get(broadcast_attempt_id.next_attempt()) - .is_some()); + assert!( + AwaitingBroadcast::::get(broadcast_attempt_id.peek_next()).is_some() + ); }); } @@ -384,7 +371,7 @@ fn test_signature_request_expiry() { // New attempt is live with same broadcast_id and incremented attempt_count. assert!({ let new_attempt = - AwaitingBroadcast::::get(broadcast_attempt_id.next_attempt()) + AwaitingBroadcast::::get(broadcast_attempt_id.peek_next()) .unwrap(); new_attempt.broadcast_attempt.broadcast_attempt_id.attempt_count == 1 && new_attempt.broadcast_attempt.broadcast_attempt_id.broadcast_id == @@ -427,7 +414,7 @@ fn test_transmission_request_expiry() { // New attempt is live with same broadcast_id and incremented attempt_count. assert!({ let new_attempt = - AwaitingBroadcast::::get(broadcast_attempt_id.next_attempt()) + AwaitingBroadcast::::get(broadcast_attempt_id.peek_next()) .unwrap(); new_attempt.broadcast_attempt.broadcast_attempt_id.attempt_count == 1 && new_attempt.broadcast_attempt.broadcast_attempt_id.broadcast_id == @@ -444,46 +431,37 @@ fn test_transmission_request_expiry() { check_end_state(); }); } - -fn threshold_signature_rerequested(broadcast_attempt_id: BroadcastAttemptId) { - // Expect the original broadcast to be deleted - assert!(AwaitingBroadcast::::get(broadcast_attempt_id).is_none()); - // Verify storage has been deleted - assert!( - TransactionOutIdToBroadcastId::::get(MOCK_TRANSACTION_OUT_ID).is_none() - ); - // attempt count incremented for the same broadcast_id - assert_eq!(BroadcastAttemptCount::::get(broadcast_attempt_id.broadcast_id), 1); - // Verify that we have a new signature request in the pipeline - assert_eq!( - MockThresholdSigner::::signature_result(0), - AsyncResult::Pending - ); -} - // One particular case where this occurs is if the Polkadot Runtime upgrade occurs after we've // already signed a tx. In this case we know it will continue to fail if we keep rebroadcasting so // we should stop and rethreshold sign using the new runtime version. #[test] fn re_request_threshold_signature_on_invalid_tx_params() { new_test_ext().execute_with(|| { - let broadcast_attempt_id = start_mock_broadcast(); + let (_, apicall) = api_call(1); + let broadcast_id = initiate_and_sign_broadcast(&apicall, TxType::Normal); + let broadcast_attempt_id = BroadcastAttemptId { broadcast_id, attempt_count: 0 }; assert_eq!( MockThresholdSigner::::signature_result(0), AsyncResult::Void ); assert!(AwaitingBroadcast::::get(broadcast_attempt_id).is_some()); - assert_eq!( - BroadcastAttemptCount::::get(broadcast_attempt_id.broadcast_id), - 0 - ); + assert_eq!(BroadcastAttemptCount::::get(broadcast_id), 0); MockTransactionBuilder::::set_invalid_for_rebroadcast(); // If invalid on retry then we should re-threshold sign Broadcaster::on_initialize(BROADCAST_EXPIRY_BLOCKS + 1); - threshold_signature_rerequested(broadcast_attempt_id); + // Verify storage has been deleted + assert!(TransactionOutIdToBroadcastId::::get(MOCK_TRANSACTION_OUT_ID) + .is_none()); + // attempt count incremented for the same broadcast_id + assert_eq!(BroadcastAttemptCount::::get(broadcast_id), 1); + // Verify that we have a new signature request in the pipeline + assert_eq!( + MockThresholdSigner::::signature_result(0), + AsyncResult::Pending + ); }); } @@ -681,9 +659,10 @@ fn retry_and_success_in_same_block() { #[test] fn retry_with_threshold_signing_still_allows_late_success_witness_second_attempt() { let mut expected_expiry_block = 0; + const MOCK_TRANSACTION_OUT_ID: [u8; 4] = [0xbc; 4]; new_test_ext() .execute_with(|| { - let broadcast_attempt_id = start_mock_broadcast_tx_out_id(MOCK_TRANSACTION_OUT_ID); + let (broadcast_attempt_id, _) = start_mock_broadcast_tx_out_id(0xbc); let awaiting_broadcast = AwaitingBroadcast::::get(broadcast_attempt_id).unwrap(); From 0c056b566a80783cab307d3c5cb1d19a72f4aae1 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 12:29:24 +0100 Subject: [PATCH 23/33] fix: remove defenisve_proof --- state-chain/pallets/cf-broadcast/src/lib.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index ec8c386011..e3e6b8d646 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -12,10 +12,7 @@ pub mod migrations; pub mod weights; use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; use cf_traits::{GetBlockHeight, SafeMode}; -use frame_support::{ - traits::{Defensive, OriginTrait}, - RuntimeDebug, -}; +use frame_support::{traits::OriginTrait, RuntimeDebug}; use sp_std::marker; pub use weights::WeightInfo; @@ -425,7 +422,6 @@ pub mod pallet { broadcast_attempt_id: *attempt_id, }); if let Some(broadcast_attempt) = Self::take_awaiting_broadcast(*attempt_id) - .defensive_proof("take_awaiting_broadcast should not fail") { Self::start_next_broadcast_attempt(broadcast_attempt); } From dd06205b26682ccc93e38a67a258cec937574715 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 12:30:09 +0100 Subject: [PATCH 24/33] fix: always remove barrier with pending broadcast --- state-chain/pallets/cf-broadcast/src/lib.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index e3e6b8d646..9651cef4c8 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -668,17 +668,6 @@ pub mod pallet { Self::remove_pending_broadcast(&broadcast_id); - if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { - if PendingBroadcasts::::get() - .first() - .map_or(true, |id| *id > *broadcast_barrier_id) - { - BroadcastBarriers::::mutate(|broadcast_barriers| { - broadcast_barriers.pop_front(); - }); - } - } - if let Some(expected_tx_metadata) = TransactionMetadata::::take(broadcast_id) { if tx_metadata.verify_metadata(&expected_tx_metadata) { if let Some(to_refund) = AwaitingBroadcast::::get(BroadcastAttemptId { @@ -809,6 +798,13 @@ impl, I: 'static> Pallet { "The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated" ); } + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { + if pending_broadcasts.first().map_or(true, |id| *id > *broadcast_barrier_id) { + BroadcastBarriers::::mutate(|broadcast_barriers| { + broadcast_barriers.pop_front(); + }); + } + } }); } From 6f3ff2ad535c4699344b56611ecd016a4f937967 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 12:30:24 +0100 Subject: [PATCH 25/33] chore: readability --- state-chain/pallets/cf-broadcast/src/lib.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 9651cef4c8..e7f798f3b5 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -670,16 +670,16 @@ pub mod pallet { if let Some(expected_tx_metadata) = TransactionMetadata::::take(broadcast_id) { if tx_metadata.verify_metadata(&expected_tx_metadata) { - if let Some(to_refund) = AwaitingBroadcast::::get(BroadcastAttemptId { - broadcast_id, - attempt_count: BroadcastAttemptCount::::get(broadcast_id), - }) - .map(|signing_attempt| { - signing_attempt + if let Some(signing_attempt) = + AwaitingBroadcast::::get(BroadcastAttemptId { + broadcast_id, + attempt_count: BroadcastAttemptCount::::get(broadcast_id), + }) { + let to_refund = signing_attempt .broadcast_attempt .transaction_payload - .return_fee_refund(tx_fee) - }) { + .return_fee_refund(tx_fee); + TransactionFeeDeficit::::mutate(signer_id.clone(), |fee_deficit| { *fee_deficit = fee_deficit.saturating_add(to_refund); }); From 5ce1a170a6f4b61ac8372a1b2a43288ca9f3368d Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 12:31:31 +0100 Subject: [PATCH 26/33] fix: typo --- state-chain/pallets/cf-broadcast/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index e7f798f3b5..b0a2fdfc07 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -690,7 +690,7 @@ pub mod pallet { }); } else { log::warn!( - "Unable to attribute transaction fee refundfor broadcast {}.", + "Unable to attribute transaction fee refund for broadcast {}.", broadcast_id ); } From 6abf06fced4388ef94e4247bb2412510122a5d2d Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 13:00:03 +0100 Subject: [PATCH 27/33] fix: use BTreeSet to avoid duplicates. --- state-chain/pallets/cf-broadcast/src/lib.rs | 87 +++++++++------------ 1 file changed, 38 insertions(+), 49 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index b0a2fdfc07..97f0d79740 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -10,10 +10,26 @@ mod tests; pub mod migrations; pub mod weights; +use cf_chains::{ + ApiCall, Chain, ChainCrypto, FeeRefundCalculator, TransactionBuilder, TransactionMetadata as _, +}; use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; -use cf_traits::{GetBlockHeight, SafeMode}; -use frame_support::{traits::OriginTrait, RuntimeDebug}; -use sp_std::marker; +use cf_traits::{ + offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, + EpochKey, GetBlockHeight, KeyProvider, SafeMode, ThresholdSigner, +}; +use codec::{Decode, Encode, MaxEncodedLen}; +use frame_support::{ + dispatch::DispatchResultWithPostInfo, + pallet_prelude::DispatchResult, + sp_runtime::traits::Saturating, + traits::{Defensive, Get, OriginTrait, StorageVersion, UnfilteredDispatchable}, + RuntimeDebug, Twox64Concat, +}; +use frame_system::pallet_prelude::OriginFor; +pub use pallet::*; +use scale_info::TypeInfo; +use sp_std::{collections::btree_set::BTreeSet, marker, marker::PhantomData, prelude::*, vec::Vec}; pub use weights::WeightInfo; #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Copy, Clone, PartialEq, Eq, RuntimeDebug)] @@ -30,28 +46,6 @@ impl SafeMode for PalletSafeMode { const CODE_GREEN: Self = PalletSafeMode { retry_enabled: true, _phantom: marker::PhantomData }; } -use cf_chains::{ - ApiCall, Chain, ChainCrypto, FeeRefundCalculator, TransactionBuilder, TransactionMetadata as _, -}; -use cf_traits::{ - offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, - EpochKey, KeyProvider, ThresholdSigner, -}; -use codec::{Decode, Encode, MaxEncodedLen}; -use frame_support::{ - dispatch::DispatchResultWithPostInfo, - pallet_prelude::DispatchResult, - sp_runtime::traits::Saturating, - traits::{Get, StorageVersion, UnfilteredDispatchable}, - Twox64Concat, -}; -use sp_std::{collections::vec_deque::VecDeque, vec::Vec}; - -use frame_system::pallet_prelude::OriginFor; -pub use pallet::*; -use scale_info::TypeInfo; -use sp_std::{marker::PhantomData, prelude::*}; - /// The number of broadcast attempts that were made before this one. pub type AttemptCount = u32; @@ -333,12 +327,12 @@ pub mod pallet { /// Whether or not broadcasts are paused for broadcast ids greater than the given broadcast id. #[pallet::storage] #[pallet::getter(fn broadcast_barriers)] - pub type BroadcastBarriers = StorageValue<_, VecDeque, ValueQuery>; + pub type BroadcastBarriers = StorageValue<_, BTreeSet, ValueQuery>; /// List of broadcasts that are initiated but not witnessed on the external chain. #[pallet::storage] #[pallet::getter(fn pending_broadcasts)] - pub type PendingBroadcasts = StorageValue<_, Vec, ValueQuery>; + pub type PendingBroadcasts = StorageValue<_, BTreeSet, ValueQuery>; /// List of broadcasts that have been aborted because they were unsuccessful to be broadcast /// after many retries. @@ -414,10 +408,7 @@ pub mod pallet { let expiries = Timeouts::::take(block_number); if T::SafeMode::get().retry_enabled { for attempt_id in expiries.iter() { - if PendingBroadcasts::::get() - .binary_search(&attempt_id.broadcast_id) - .is_ok() - { + if PendingBroadcasts::::get().contains(&attempt_id.broadcast_id) { Self::deposit_event(Event::::BroadcastAttemptTimeout { broadcast_attempt_id: *attempt_id, }); @@ -449,7 +440,7 @@ pub mod pallet { let retries = BroadcastRetryQueue::::mutate(|retry_queue| { let id_limit = BroadcastBarriers::::get() - .front() + .first() .copied() .unwrap_or(BroadcastId::max_value()); retry_queue @@ -465,8 +456,7 @@ pub mod pallet { for retry in retries { // Check if the broadcast is pending if PendingBroadcasts::::get() - .binary_search(&retry.broadcast_attempt_id.broadcast_id) - .is_ok() + .contains(&retry.broadcast_attempt_id.broadcast_id) { Self::start_next_broadcast_attempt(retry); } @@ -621,7 +611,7 @@ pub mod pallet { transaction_out_id, }; - if BroadcastBarriers::::get().front().is_some_and(|broadcast_barrier_id| { + if BroadcastBarriers::::get().first().is_some_and(|broadcast_barrier_id| { broadcast_attempt_id.broadcast_id > *broadcast_barrier_id }) { Self::schedule_for_retry(&broadcast_attempt); @@ -791,17 +781,15 @@ impl, I: 'static> Pallet { pub fn remove_pending_broadcast(broadcast_id: &BroadcastId) { PendingBroadcasts::::mutate(|pending_broadcasts| { debug_assert!(pending_broadcasts.iter().is_sorted()); - if let Ok(id) = pending_broadcasts.binary_search(broadcast_id) { - pending_broadcasts.remove(id); - } else { + if !pending_broadcasts.remove(broadcast_id) { cf_runtime_utilities::log_or_panic!( "The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated" ); } - if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().front() { + if let Some(broadcast_barrier_id) = BroadcastBarriers::::get().first() { if pending_broadcasts.first().map_or(true, |id| *id > *broadcast_barrier_id) { BroadcastBarriers::::mutate(|broadcast_barriers| { - broadcast_barriers.pop_front(); + broadcast_barriers.pop_first(); }); } } @@ -1012,15 +1000,16 @@ impl, I: 'static> Broadcaster for Pallet { fn threshold_sign_and_broadcast_rotation_tx(api_call: Self::ApiCall) -> BroadcastId { let broadcast_id = >::threshold_sign_and_broadcast(api_call); - BroadcastBarriers::::mutate(|current_barriers| { - current_barriers.append( - &mut <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation(broadcast_id) - .extract_if(|barrier| { - PendingBroadcasts::::get().first().map_or(false, |id| *id <= *barrier) - }) - .collect::>(), - ); - }); + if let Some(earliest_pending_broadcast_id) = PendingBroadcasts::::get() + .first() + .defensive_proof("Broadcast ID was just inserted, so at least this one must exist.") + { + for barrier in <<>::TargetChain as Chain>::ChainCrypto as ChainCrypto>::maybe_broadcast_barriers_on_rotation(broadcast_id) { + if barrier >= *earliest_pending_broadcast_id { + BroadcastBarriers::::append(barrier); + } + } + } broadcast_id } } From 8b46d1a1442419f7dc2f6060e09f1ab7e20e9346 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 13:33:50 +0100 Subject: [PATCH 28/33] fix: check attempt count >= not == --- state-chain/pallets/cf-broadcast/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 97f0d79740..51c3271152 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -503,7 +503,7 @@ pub mod pallet { // Schedule a failed attempt for retry when the next block is authored. // We will abort the broadcast once all authorities have attempt to sign the // transaction - if signing_attempt.broadcast_attempt.broadcast_attempt_id.attempt_count == + if signing_attempt.broadcast_attempt.broadcast_attempt_id.attempt_count >= T::EpochInfo::current_authority_count() .checked_sub(1) .expect("We must have at least one authority") @@ -780,7 +780,6 @@ impl, I: 'static> Pallet { pub fn remove_pending_broadcast(broadcast_id: &BroadcastId) { PendingBroadcasts::::mutate(|pending_broadcasts| { - debug_assert!(pending_broadcasts.iter().is_sorted()); if !pending_broadcasts.remove(broadcast_id) { cf_runtime_utilities::log_or_panic!( "The broadcast_id should exist in the pending broadcasts list since we added it to the list when the broadcast was initated" From 7267a8b5c582fcc215a29a2d533aed2d22aeb4e7 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 4 Dec 2023 10:23:05 +0100 Subject: [PATCH 29/33] test: test that the broadcast is aborted when all broadcasters attempt --- state-chain/pallets/cf-broadcast/src/tests.rs | 54 +++++++++++++++++++ .../traits/src/mocks/signer_nomination.rs | 4 ++ 2 files changed, 58 insertions(+) diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index 849d2a2f1c..e442057ebb 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -200,6 +200,60 @@ fn test_abort_after_number_of_attempts_is_equal_to_the_number_of_authorities() { }); } +#[test] +fn broadcasts_aborted_after_all_report_failures_after_retry() { + new_test_ext() + .execute_with(|| { + let (_tx_out_id1, api_call1) = api_call(1); + + // Mock when all the possible broadcasts have failed another broadcast, and are + // therefore suspended. + MockNominator::set_nominees(Some(Default::default())); + + let broadcast_id = initiate_and_sign_broadcast(&api_call1, TxType::Normal); + + // No nominees, so we need to reschedule + System::assert_last_event(RuntimeEvent::Broadcaster( + crate::Event::::BroadcastRetryScheduled { + broadcast_attempt_id: BroadcastAttemptId { broadcast_id, attempt_count: 0 }, + }, + )); + broadcast_id + }) + // schedule some retries within each block - these do not result in + // TransactionBroadcastRequests + .then_execute_at_next_block(|broadcast_id| broadcast_id) + .then_execute_at_next_block(|broadcast_id| broadcast_id) + .then_execute_at_next_block(|broadcast_id| { + // The nominees are no longer suspended, so the retry in on_idle will nominate a + // broadcaster + MockNominator::reset_last_nominee(); + MockNominator::use_current_authorities_as_nominees::(); + broadcast_id + }) + .then_execute_at_next_block(|broadcast_id| { + // we should have attempt 3 here now. + assert_ok!(Broadcaster::transaction_signing_failure( + RawOrigin::Signed(0).into(), + BroadcastAttemptId { broadcast_id, attempt_count: 3 }, + )); + assert_eq!( + System::events().pop().expect("an event").event, + RuntimeEvent::Broadcaster(crate::Event::BroadcastAborted { broadcast_id }) + ); + broadcast_id + }) + .then_execute_at_next_block(|broadcast_id| { + assert_noop!( + Broadcaster::transaction_signing_failure( + RawOrigin::Signed(1).into(), + BroadcastAttemptId { broadcast_id, attempt_count: 4 }, + ), + Error::::InvalidBroadcastAttemptId + ); + }); +} + #[test] fn on_idle_caps_broadcasts_when_not_enough_weight() { new_test_ext().execute_with(|| { diff --git a/state-chain/traits/src/mocks/signer_nomination.rs b/state-chain/traits/src/mocks/signer_nomination.rs index 9c92e7d490..673d789e36 100644 --- a/state-chain/traits/src/mocks/signer_nomination.rs +++ b/state-chain/traits/src/mocks/signer_nomination.rs @@ -45,6 +45,10 @@ impl MockNominator { THRESHOLD_NOMINEES.with(|cell| cell.borrow().clone()) } + pub fn reset_last_nominee() { + LAST_NOMINATED_INDEX.with(|cell| *cell.borrow_mut() = None); + } + pub fn set_nominees(nominees: Option>) { THRESHOLD_NOMINEES.with(|cell| *cell.borrow_mut() = nominees); } From 558a57ac514f1a781fff8677391bcd4f8132cf70 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 15:03:48 +0100 Subject: [PATCH 30/33] fix: simplify signature refresh logic --- state-chain/chains/src/lib.rs | 4 +-- state-chain/chains/src/mocks.rs | 12 ++++---- .../pallets/cf-broadcast/src/benchmarking.rs | 7 +---- state-chain/pallets/cf-broadcast/src/lib.rs | 29 ++++++------------- state-chain/pallets/cf-broadcast/src/mock.rs | 28 ++---------------- state-chain/pallets/cf-broadcast/src/tests.rs | 4 +-- state-chain/runtime/src/chainflip.rs | 25 +++++----------- state-chain/runtime/src/lib.rs | 3 -- 8 files changed, 27 insertions(+), 85 deletions(-) diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 18567b9ae2..947c895be5 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -237,11 +237,9 @@ where fn refresh_unsigned_data(tx: &mut C::Transaction); /// Checks if the payload is still valid for the call. - fn is_valid_for_rebroadcast( + fn requires_signature_refresh( call: &Call, payload: &<::ChainCrypto as ChainCrypto>::Payload, - current_key: &<::ChainCrypto as ChainCrypto>::AggKey, - signature: &<::ChainCrypto as ChainCrypto>::ThresholdSignature, ) -> bool; /// Calculate the Units of gas that is allowed to make this call. diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index c3170c1322..72aaaa0954 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -333,14 +333,14 @@ impl ApiCall for MockApiCall { } thread_local! { - pub static IS_VALID_BROADCAST: std::cell::RefCell = RefCell::new(true); + pub static REQUIRES_REFRESH: std::cell::RefCell = RefCell::new(false); } pub struct MockTransactionBuilder(PhantomData<(C, Call)>); impl MockTransactionBuilder { - pub fn set_invalid_for_rebroadcast() { - IS_VALID_BROADCAST.with(|is_valid| *is_valid.borrow_mut() = false) + pub fn set_requires_refresh() { + REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow_mut() = true) } } @@ -355,12 +355,10 @@ impl, Call: ApiCall> // refresh nothing } - fn is_valid_for_rebroadcast( + fn requires_signature_refresh( _call: &Call, _payload: &<::ChainCrypto as ChainCrypto>::Payload, - _current_key: &<::ChainCrypto as ChainCrypto>::AggKey, - _signature: &<::ChainCrypto as ChainCrypto>::ThresholdSignature, ) -> bool { - IS_VALID_BROADCAST.with(|is_valid| *is_valid.borrow()) + REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow()) } } diff --git a/state-chain/pallets/cf-broadcast/src/benchmarking.rs b/state-chain/pallets/cf-broadcast/src/benchmarking.rs index 7c4c3e143c..e72ce83dfa 100644 --- a/state-chain/pallets/cf-broadcast/src/benchmarking.rs +++ b/state-chain/pallets/cf-broadcast/src/benchmarking.rs @@ -3,7 +3,7 @@ use super::*; -use cf_traits::{CurrentEpochIndex, ThresholdSigner}; +use cf_traits::ThresholdSigner; use frame_benchmarking::{benchmarks_instance_pallet, whitelisted_caller}; use frame_support::{ dispatch::UnfilteredDispatchable, @@ -68,7 +68,6 @@ benchmarks_instance_pallet! { ThresholdSignatureData::::insert(i, (ApiCallFor::::benchmark_value(), ThresholdSignatureFor::::benchmark_value())) } let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { Pallet::::on_initialize(timeout_block); } @@ -86,7 +85,6 @@ benchmarks_instance_pallet! { generate_on_signature_ready_call::().dispatch_bypass_filter(T::EnsureThresholdSigned::try_successful_origin().unwrap())?; let expiry_block = frame_system::Pallet::::block_number() + T::BroadcastTimeout::get(); let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); }: _(RawOrigin::Signed(caller), broadcast_attempt_id) verify { assert!(Timeouts::::contains_key(expiry_block)); @@ -101,7 +99,6 @@ benchmarks_instance_pallet! { insert_transaction_broadcast_attempt::(whitelisted_caller(), broadcast_attempt_id); let call = generate_on_signature_ready_call::(); let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { call.dispatch_bypass_filter(T::EnsureThresholdSigned::try_successful_origin().unwrap())? } verify { assert_eq!(BroadcastIdCounter::::get(), 0); @@ -119,7 +116,6 @@ benchmarks_instance_pallet! { attempt_count: BroadcastAttemptCount::::get(broadcast_id), }; - T::KeyProvider::set_key(AggKeyFor::::benchmark_value(), CurrentEpochIndex::::get()); let transaction_payload = TransactionFor::::benchmark_value(); } : { @@ -151,7 +147,6 @@ benchmarks_instance_pallet! { tx_metadata: TransactionMetadataFor::::benchmark_value(), }; let valid_key = AggKeyFor::::benchmark_value(); - T::KeyProvider::set_key(valid_key, CurrentEpochIndex::::get()); } : { call.dispatch_bypass_filter(T::EnsureWitnessedAtCurrentEpoch::try_successful_origin().unwrap())? } verify { // We expect the unwrap to error if the extrinsic didn't fire an event - if an event has been emitted we reached the end of the extrinsic diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 51c3271152..21084c9d4f 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -16,7 +16,7 @@ use cf_chains::{ use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; use cf_traits::{ offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, - EpochKey, GetBlockHeight, KeyProvider, SafeMode, ThresholdSigner, + GetBlockHeight, SafeMode, ThresholdSigner, }; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ @@ -98,7 +98,7 @@ pub const PALLET_VERSION: StorageVersion = StorageVersion::new(2); pub mod pallet { use super::*; use cf_chains::benchmarking_value::BenchmarkValue; - use cf_traits::{AccountRoleRegistry, BroadcastNomination, KeyProvider, OnBroadcastReady}; + use cf_traits::{AccountRoleRegistry, BroadcastNomination, OnBroadcastReady}; use frame_support::{ ensure, pallet_prelude::{OptionQuery, ValueQuery, *}, @@ -221,9 +221,6 @@ pub mod pallet { #[pallet::constant] type BroadcastTimeout: Get>; - /// Something that provides the current key for signing. - type KeyProvider: KeyProvider<::ChainCrypto>; - /// Safe Mode access. type SafeMode: Get>; @@ -855,27 +852,14 @@ impl, I: 'static> Pallet { fn start_next_broadcast_attempt(broadcast_attempt: BroadcastAttempt) { let broadcast_id = broadcast_attempt.broadcast_attempt_id.broadcast_id; - if let Some((api_call, signature)) = ThresholdSignatureData::::get(broadcast_id) { - let EpochKey { key, .. } = T::KeyProvider::active_epoch_key() - .expect("Epoch key must exist if we made a broadcast."); - + if let Some((api_call, _signature)) = ThresholdSignatureData::::get(broadcast_id) { let next_broadcast_attempt_id = broadcast_attempt.broadcast_attempt_id.into_next::(); - if T::TransactionBuilder::is_valid_for_rebroadcast( + if T::TransactionBuilder::requires_signature_refresh( &api_call, &broadcast_attempt.threshold_signature_payload, - &key, - &signature, ) { - Self::start_broadcast_attempt(BroadcastAttempt:: { - broadcast_attempt_id: next_broadcast_attempt_id, - ..broadcast_attempt - }); - } - // If the signature verification fails, we want - // to retry from the threshold signing stage. - else { // We update the initiated_at here since as the tx is resigned and broadcast, it is // not possible for it to be successfully broadcasted before this point. // This `initiated_at` block will be associated with the new transaction_out_id @@ -906,6 +890,11 @@ impl, I: 'static> Pallet { "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", broadcast_id ); + } else { + Self::start_broadcast_attempt(BroadcastAttempt:: { + broadcast_attempt_id: next_broadcast_attempt_id, + ..broadcast_attempt + }); } } else { log::error!("No threshold signature data is available."); diff --git a/state-chain/pallets/cf-broadcast/src/mock.rs b/state-chain/pallets/cf-broadcast/src/mock.rs index 0f4076ef7a..0202e34ad5 100644 --- a/state-chain/pallets/cf-broadcast/src/mock.rs +++ b/state-chain/pallets/cf-broadcast/src/mock.rs @@ -6,9 +6,7 @@ use crate::{self as pallet_cf_broadcast, Instance1, PalletOffence, PalletSafeMod use cf_chains::{ eth::Ethereum, evm::EvmCrypto, - mocks::{ - MockAggKey, MockApiCall, MockEthereum, MockEthereumChainCrypto, MockTransactionBuilder, - }, + mocks::{MockApiCall, MockEthereum, MockEthereumChainCrypto, MockTransactionBuilder}, Chain, ChainCrypto, }; use cf_traits::{ @@ -17,7 +15,7 @@ use cf_traits::{ block_height_provider::BlockHeightProvider, signer_nomination::MockNominator, threshold_signer::MockThresholdSigner, }, - AccountRoleRegistry, EpochKey, OnBroadcastReady, + AccountRoleRegistry, OnBroadcastReady, }; use codec::{Decode, Encode}; use frame_support::{parameter_types, traits::UnfilteredDispatchable}; @@ -79,10 +77,6 @@ parameter_types! { pub type MockOffenceReporter = cf_traits::mocks::offence_reporting::MockOffenceReporter; -pub const VALID_AGG_KEY: MockAggKey = MockAggKey([0, 0, 0, 0]); - -pub const INVALID_AGG_KEY: MockAggKey = MockAggKey([1, 1, 1, 1]); - thread_local! { pub static SIGNATURE_REQUESTS: RefCell::ChainCrypto as ChainCrypto>::Payload>> = RefCell::new(vec![]); pub static CALLBACK_CALLED: RefCell = RefCell::new(false); @@ -91,17 +85,6 @@ thread_local! { pub type EthMockThresholdSigner = MockThresholdSigner; -pub struct MockKeyProvider; - -impl cf_traits::KeyProvider for MockKeyProvider { - fn active_epoch_key() -> Option::AggKey>> { - Some(EpochKey { - key: if VALIDKEY.with(|cell| *cell.borrow()) { VALID_AGG_KEY } else { INVALID_AGG_KEY }, - epoch_index: Default::default(), - }) - } -} - #[derive(Debug, Clone, Default, PartialEq, Eq, Encode, Decode, TypeInfo)] pub struct MockCallback; @@ -123,12 +106,6 @@ impl UnfilteredDispatchable for MockCallback { } } -impl MockKeyProvider { - pub fn set_valid(valid: bool) { - VALIDKEY.with(|cell| *cell.borrow_mut() = valid); - } -} - pub struct MockBroadcastReadyProvider; impl OnBroadcastReady for MockBroadcastReadyProvider { type ApiCall = MockApiCall; @@ -149,7 +126,6 @@ impl pallet_cf_broadcast::Config for Test { type EnsureThresholdSigned = NeverFailingOriginCheck; type BroadcastTimeout = BroadcastTimeout; type WeightInfo = (); - type KeyProvider = MockKeyProvider; type RuntimeOrigin = RuntimeOrigin; type BroadcastCallable = MockCallback; type SafeMode = MockRuntimeSafeMode; diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index e442057ebb..4717ff541d 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -502,7 +502,7 @@ fn re_request_threshold_signature_on_invalid_tx_params() { assert!(AwaitingBroadcast::::get(broadcast_attempt_id).is_some()); assert_eq!(BroadcastAttemptCount::::get(broadcast_id), 0); - MockTransactionBuilder::::set_invalid_for_rebroadcast(); + MockTransactionBuilder::::set_requires_refresh(); // If invalid on retry then we should re-threshold sign Broadcaster::on_initialize(BROADCAST_EXPIRY_BLOCKS + 1); @@ -752,7 +752,7 @@ fn retry_with_threshold_signing_still_allows_late_success_witness_second_attempt ), 1 ); - MockTransactionBuilder::::set_invalid_for_rebroadcast(); + MockTransactionBuilder::::set_requires_refresh(); MockCfe::respond(Scenario::Timeout); (nominee, awaiting_broadcast) }) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 7e8b199c77..48b3f6029d 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -164,18 +164,11 @@ impl TransactionBuilder> for EthTransactio } } - fn is_valid_for_rebroadcast( - call: &EthereumApi, + fn requires_signature_refresh( + _call: &EthereumApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, - current_key: &<::ChainCrypto as ChainCrypto>::AggKey, - signature: &<::ChainCrypto as ChainCrypto>::ThresholdSignature, ) -> bool { - // Check if signature is valid - <::ChainCrypto as ChainCrypto>::verify_threshold_signature( - current_key, - &call.threshold_signature_payload(), - signature, - ) + false } /// Calculate the gas limit for a Ethereum call, using the current gas price. @@ -217,15 +210,13 @@ impl TransactionBuilder> for DotTransactio // TODO: For now this is a noop until we actually have dot chain tracking } - fn is_valid_for_rebroadcast( + fn requires_signature_refresh( call: &PolkadotApi, payload: &<::ChainCrypto as ChainCrypto>::Payload, - _current_key: &<::ChainCrypto as ChainCrypto>::AggKey, - _signature: &<::ChainCrypto as ChainCrypto>::ThresholdSignature, ) -> bool { // Current key and signature are irrelevant. The only thing that can invalidate a polkadot // transaction is if the payload changes due to a runtime version update. - &call.threshold_signature_payload() == payload + &call.threshold_signature_payload() != payload } } @@ -243,11 +234,9 @@ impl TransactionBuilder> for BtcTransactionB // refreshing btc tx is not trivial. We leave it a no-op for now. } - fn is_valid_for_rebroadcast( + fn requires_signature_refresh( _call: &BitcoinApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, - _current_key: &<::ChainCrypto as ChainCrypto>::AggKey, - _signature: &<::ChainCrypto as ChainCrypto>::ThresholdSignature, ) -> bool { // The payload for a Bitcoin transaction will never change and so it doesnt need to be // checked here. We also dont need to check for the signature here because even if we are in @@ -255,7 +244,7 @@ impl TransactionBuilder> for BtcTransactionB // since its based on those old input UTXOs. In fact, we never have to resign btc txs and // the btc tx is always valid as long as the input UTXOs are valid. Therefore, we don't have // to check anything here and just rebroadcast. - true + false } } diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index 549d9d2344..c493002187 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -675,7 +675,6 @@ impl pallet_cf_broadcast::Config for Runtime { type WeightInfo = pallet_cf_broadcast::weights::PalletWeight; type SafeMode = RuntimeSafeMode; type SafeModeBlockMargin = ConstU32<10>; - type KeyProvider = EthereumVault; type ChainTracking = EthereumChainTracking; } @@ -698,7 +697,6 @@ impl pallet_cf_broadcast::Config for Runtime { type WeightInfo = pallet_cf_broadcast::weights::PalletWeight; type SafeMode = RuntimeSafeMode; type SafeModeBlockMargin = ConstU32<10>; - type KeyProvider = PolkadotVault; type ChainTracking = PolkadotChainTracking; } @@ -721,7 +719,6 @@ impl pallet_cf_broadcast::Config for Runtime { type WeightInfo = pallet_cf_broadcast::weights::PalletWeight; type SafeMode = RuntimeSafeMode; type SafeModeBlockMargin = ConstU32<10>; - type KeyProvider = BitcoinVault; type ChainTracking = BitcoinChainTracking; } From 7a0ab457c1d3fa1d5ea02ffa3c461320b4afc79b Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 4 Dec 2023 15:20:31 +0100 Subject: [PATCH 31/33] fix: add PendingBroadcast migration --- state-chain/pallets/cf-broadcast/src/lib.rs | 2 +- .../pallets/cf-broadcast/src/migrations.rs | 8 ++++- .../pallets/cf-broadcast/src/migrations/v3.rs | 33 +++++++++++++++++++ state-chain/runtime/src/lib.rs | 6 ++-- 4 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 state-chain/pallets/cf-broadcast/src/migrations/v3.rs diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 21084c9d4f..ae609f3f0e 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -92,7 +92,7 @@ pub enum PalletOffence { FailedToBroadcastTransaction, } -pub const PALLET_VERSION: StorageVersion = StorageVersion::new(2); +pub const PALLET_VERSION: StorageVersion = StorageVersion::new(3); #[frame_support::pallet] pub mod pallet { diff --git a/state-chain/pallets/cf-broadcast/src/migrations.rs b/state-chain/pallets/cf-broadcast/src/migrations.rs index fd701ecdd2..3d70f36435 100644 --- a/state-chain/pallets/cf-broadcast/src/migrations.rs +++ b/state-chain/pallets/cf-broadcast/src/migrations.rs @@ -1 +1,7 @@ -pub type PalletMigration = (); +pub mod v3; + +use cf_runtime_upgrade_utilities::VersionedMigration; + +use crate::Pallet; + +pub type PalletMigration = (VersionedMigration, v3::Migration, 2, 3>,); diff --git a/state-chain/pallets/cf-broadcast/src/migrations/v3.rs b/state-chain/pallets/cf-broadcast/src/migrations/v3.rs new file mode 100644 index 0000000000..2c0fb08817 --- /dev/null +++ b/state-chain/pallets/cf-broadcast/src/migrations/v3.rs @@ -0,0 +1,33 @@ +use crate::*; +#[cfg(feature = "try-runtime")] +use frame_support::sp_runtime::DispatchError; +use frame_support::{traits::OnRuntimeUpgrade, weights::Weight}; +use sp_std::marker::PhantomData; +#[cfg(feature = "try-runtime")] +use sp_std::prelude::Vec; + +pub struct Migration, I: 'static>(PhantomData<(T, I)>); + +impl, I: 'static> OnRuntimeUpgrade for Migration { + fn on_runtime_upgrade() -> frame_support::weights::Weight { + let pending_broadcasts = AwaitingBroadcast::::iter_keys() + .map(|BroadcastAttemptId { broadcast_id, .. }| broadcast_id) + .collect::>(); + PendingBroadcasts::::put(pending_broadcasts); + Weight::zero() + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, DispatchError> { + Ok(Default::default()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), DispatchError> { + let pending_broadcasts = PendingBroadcasts::::get(); + for id in AwaitingBroadcast::::iter_keys() { + assert!(pending_broadcasts.contains(&id.broadcast_id),); + } + Ok(()) + } +} diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index c493002187..7b88fb5650 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -838,9 +838,9 @@ type PalletMigrations = ( pallet_cf_threshold_signature::migrations::PalletMigration, pallet_cf_threshold_signature::migrations::PalletMigration, pallet_cf_threshold_signature::migrations::PalletMigration, - pallet_cf_broadcast::migrations::PalletMigration, - pallet_cf_broadcast::migrations::PalletMigration, - pallet_cf_broadcast::migrations::PalletMigration, + pallet_cf_broadcast::migrations::PalletMigration, + pallet_cf_broadcast::migrations::PalletMigration, + pallet_cf_broadcast::migrations::PalletMigration, pallet_cf_chain_tracking::migrations::PalletMigration, pallet_cf_chain_tracking::migrations::PalletMigration, pallet_cf_chain_tracking::migrations::PalletMigration, From 43d44ebd0f759091c2ef5e5e71dc35128b6e5d27 Mon Sep 17 00:00:00 2001 From: kylezs Date: Mon, 4 Dec 2023 17:48:38 +0100 Subject: [PATCH 32/33] fix: migrations --- state-chain/pallets/cf-broadcast/src/lib.rs | 2 +- state-chain/pallets/cf-broadcast/src/migrations.rs | 4 ++-- .../pallets/cf-broadcast/src/migrations/{v3.rs => v2.rs} | 0 state-chain/pallets/cf-vaults/src/migrations/v3.rs | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) rename state-chain/pallets/cf-broadcast/src/migrations/{v3.rs => v2.rs} (100%) diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index ae609f3f0e..21084c9d4f 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -92,7 +92,7 @@ pub enum PalletOffence { FailedToBroadcastTransaction, } -pub const PALLET_VERSION: StorageVersion = StorageVersion::new(3); +pub const PALLET_VERSION: StorageVersion = StorageVersion::new(2); #[frame_support::pallet] pub mod pallet { diff --git a/state-chain/pallets/cf-broadcast/src/migrations.rs b/state-chain/pallets/cf-broadcast/src/migrations.rs index 3d70f36435..ca1b007b32 100644 --- a/state-chain/pallets/cf-broadcast/src/migrations.rs +++ b/state-chain/pallets/cf-broadcast/src/migrations.rs @@ -1,7 +1,7 @@ -pub mod v3; +pub mod v2; use cf_runtime_upgrade_utilities::VersionedMigration; use crate::Pallet; -pub type PalletMigration = (VersionedMigration, v3::Migration, 2, 3>,); +pub type PalletMigration = (VersionedMigration, v2::Migration, 1, 2>,); diff --git a/state-chain/pallets/cf-broadcast/src/migrations/v3.rs b/state-chain/pallets/cf-broadcast/src/migrations/v2.rs similarity index 100% rename from state-chain/pallets/cf-broadcast/src/migrations/v3.rs rename to state-chain/pallets/cf-broadcast/src/migrations/v2.rs diff --git a/state-chain/pallets/cf-vaults/src/migrations/v3.rs b/state-chain/pallets/cf-vaults/src/migrations/v3.rs index 35c4a1353d..927ba73f3e 100644 --- a/state-chain/pallets/cf-vaults/src/migrations/v3.rs +++ b/state-chain/pallets/cf-vaults/src/migrations/v3.rs @@ -45,8 +45,9 @@ impl, I: 'static> OnRuntimeUpgrade for Migration { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, DispatchError> { - // just check that the old storage item existed - Ok(old::CurrentVaultEpochAndState::::exists().encode()) + let state = old::CurrentVaultEpochAndState::::get().unwrap(); + + Ok(state.encode()) } #[cfg(feature = "try-runtime")] From cd84ee070fa85bb358eb1306ea8270b2d881a24c Mon Sep 17 00:00:00 2001 From: kylezs Date: Tue, 5 Dec 2023 09:44:22 +0100 Subject: [PATCH 33/33] fix: use correct spec version --- state-chain/runtime/src/lib.rs | 2 +- state-chain/runtime/src/migrations.rs | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index 7b88fb5650..da095f21a9 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -854,7 +854,7 @@ type PalletMigrations = ( pallet_cf_lp::migrations::PalletMigration, migrations::VersionedMigration< migrations::threshold_signature_callbacks::ThresholdSignatureCallbacks, - 101, + 103, >, pallet_cf_pools::migrations::PalletMigration, ); diff --git a/state-chain/runtime/src/migrations.rs b/state-chain/runtime/src/migrations.rs index bf994a2136..891b0b68ea 100644 --- a/state-chain/runtime/src/migrations.rs +++ b/state-chain/runtime/src/migrations.rs @@ -10,8 +10,6 @@ pub mod threshold_signature_callbacks; #[cfg(feature = "try-runtime")] use sp_runtime::DispatchError; -//const SPEC_VERSION: u32 = 101; - /// A runtime storage migration that will only be applied if the `SPEC_VERSION` matches the /// post-upgrade runtime's spec version. pub struct VersionedMigration(PhantomData);