From 864e6d2b5724ee55d0e0e84876d527ba2bf6a924 Mon Sep 17 00:00:00 2001 From: Jamie Ford Date: Wed, 9 Oct 2024 01:49:33 +1100 Subject: [PATCH] feat: cleanup aborted broadcasts (#5301) * feat: cleanup aborted broadcasts * refactor: use housekeeping migration * refactor: remove stale from perseverance as well * chore: remove left over structs --- state-chain/pallets/cf-broadcast/src/lib.rs | 5 +- .../pallets/cf-broadcast/src/migrations.rs | 2 + .../migrations/remove_aborted_broadcasts.rs | 24 +++++++ state-chain/pallets/cf-broadcast/src/tests.rs | 24 +++++++ .../runtime/src/migrations/housekeeping.rs | 63 ++++++++++++++++++- 5 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 state-chain/pallets/cf-broadcast/src/migrations/remove_aborted_broadcasts.rs diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index f8eb5498ac..d6f5d847a6 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -688,6 +688,9 @@ impl, I: 'static> Pallet { .ok_or(Error::::InvalidPayload)?; Self::remove_pending_broadcast(&broadcast_id); + AbortedBroadcasts::::mutate(|aborted| { + aborted.remove(&broadcast_id); + }); if IncomingKeyAndBroadcastId::::exists() { let (incoming_key, rotation_broadcast_id) = @@ -1089,7 +1092,7 @@ impl, I: 'static> Broadcaster for Pallet { } fn expire_broadcast(broadcast_id: BroadcastId) { - // These would otherwise be cleaned up when the broadacst succeeds or aborts. + // These would otherwise be cleaned up when the broadcast succeeds or aborts. RequestSuccessCallbacks::::remove(broadcast_id); RequestFailureCallbacks::::remove(broadcast_id); Self::clean_up_broadcast_storage(broadcast_id); diff --git a/state-chain/pallets/cf-broadcast/src/migrations.rs b/state-chain/pallets/cf-broadcast/src/migrations.rs index 3cb192c8a8..1d9cadbd3e 100644 --- a/state-chain/pallets/cf-broadcast/src/migrations.rs +++ b/state-chain/pallets/cf-broadcast/src/migrations.rs @@ -3,9 +3,11 @@ use cf_runtime_upgrade_utilities::{PlaceholderMigration, VersionedMigration}; mod initialize_broadcast_timeout_storage; mod migrate_timeouts; +pub mod remove_aborted_broadcasts; pub type PalletMigration = ( VersionedMigration, initialize_broadcast_timeout_storage::Migration, 6, 7>, VersionedMigration, migrate_timeouts::Migration, 7, 8>, PlaceholderMigration, 8>, + // Migration 8->9 is SerializeSolanaBroadcastMigration in runtime lib. ); diff --git a/state-chain/pallets/cf-broadcast/src/migrations/remove_aborted_broadcasts.rs b/state-chain/pallets/cf-broadcast/src/migrations/remove_aborted_broadcasts.rs new file mode 100644 index 0000000000..3161b13277 --- /dev/null +++ b/state-chain/pallets/cf-broadcast/src/migrations/remove_aborted_broadcasts.rs @@ -0,0 +1,24 @@ +use crate::*; + +// Highest stale aborted broadcasts as of 3/10/2024: +// Mainnet +pub const ETHEREUM_MAX_ABORTED_BROADCAST_BERGHAIN: BroadcastId = 11592; +pub const ARBITRUM_MAX_ABORTED_BROADCAST_BERGHAIN: BroadcastId = 426; +// Perseverance testnet +pub const ETHEREUM_MAX_ABORTED_BROADCAST_PERSEVERANCE: BroadcastId = 1609; +pub const ARBITRUM_MAX_ABORTED_BROADCAST_PERSEVERANCE: BroadcastId = 665; +pub const POLKADOT_MAX_ABORTED_BROADCAST_PERSEVERANCE: BroadcastId = 634; + +pub fn remove_stale_and_all_older, I: 'static>(latest_stale_broadcast: BroadcastId) { + AbortedBroadcasts::::mutate(|aborted| { + aborted.retain(|id| id > &latest_stale_broadcast); + }); +} + +#[cfg(feature = "try-runtime")] +pub fn assert_removed, I: 'static>(latest_stale_broadcast: BroadcastId) { + let aborted_broadcasts = AbortedBroadcasts::::get(); + if let Some(first) = aborted_broadcasts.first() { + assert!(*first > latest_stale_broadcast, "Aborted broadcast {first} was not removed"); + } +} diff --git a/state-chain/pallets/cf-broadcast/src/tests.rs b/state-chain/pallets/cf-broadcast/src/tests.rs index a14dee7f4a..d217a3003a 100644 --- a/state-chain/pallets/cf-broadcast/src/tests.rs +++ b/state-chain/pallets/cf-broadcast/src/tests.rs @@ -1562,3 +1562,27 @@ fn changing_broadcast_timeout() { ); }); } + +#[test] +fn aborted_broadcast_is_cleaned_up_on_success() { + new_test_ext().execute_with(|| { + // Abort a broadcast + let (broadcast_id, tx_id) = start_mock_broadcast(); + let nominee = ready_to_abort_broadcast(broadcast_id); + assert_ok!(Broadcaster::transaction_failed(RuntimeOrigin::signed(nominee), broadcast_id)); + assert!(AbortedBroadcasts::::get().contains(&broadcast_id)); + + // Witness a successful broadcast as if it was manually broadcast + assert_ok!(Broadcaster::transaction_succeeded( + RuntimeOrigin::root(), + tx_id, + Default::default(), + ETH_TX_FEE, + MOCK_TX_METADATA, + Default::default(), + )); + + // Storage should be cleaned, event emitted. + assert!(AbortedBroadcasts::::get().is_empty()); + }); +} diff --git a/state-chain/runtime/src/migrations/housekeeping.rs b/state-chain/runtime/src/migrations/housekeeping.rs index e7dd513b19..8e8a296285 100644 --- a/state-chain/runtime/src/migrations/housekeeping.rs +++ b/state-chain/runtime/src/migrations/housekeeping.rs @@ -1,17 +1,38 @@ use crate::Runtime; +use cf_chains::instances::{ArbitrumInstance, EthereumInstance, PolkadotInstance}; +use cf_runtime_upgrade_utilities::genesis_hashes; use frame_support::{traits::OnRuntimeUpgrade, weights::Weight}; +use pallet_cf_broadcast::migrations::remove_aborted_broadcasts; +#[cfg(feature = "try-runtime")] +use sp_runtime::DispatchError; +#[cfg(feature = "try-runtime")] +use sp_std::vec::Vec; pub struct Migration; impl OnRuntimeUpgrade for Migration { fn on_runtime_upgrade() -> Weight { - use cf_runtime_upgrade_utilities::genesis_hashes; match genesis_hashes::genesis_hash::() { genesis_hashes::BERGHAIN => { - log::info!("🧹 No housekeeping required for Berghain."); + log::info!("🧹 Housekeeping, removing stale aborted broadcasts"); + remove_aborted_broadcasts::remove_stale_and_all_older::( + remove_aborted_broadcasts::ETHEREUM_MAX_ABORTED_BROADCAST_BERGHAIN, + ); + remove_aborted_broadcasts::remove_stale_and_all_older::( + remove_aborted_broadcasts::ARBITRUM_MAX_ABORTED_BROADCAST_BERGHAIN, + ); }, genesis_hashes::PERSEVERANCE => { - log::info!("🧹 No housekeeping required for Perseverance."); + log::info!("🧹 Housekeeping, removing stale aborted broadcasts"); + remove_aborted_broadcasts::remove_stale_and_all_older::( + remove_aborted_broadcasts::ETHEREUM_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); + remove_aborted_broadcasts::remove_stale_and_all_older::( + remove_aborted_broadcasts::ARBITRUM_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); + remove_aborted_broadcasts::remove_stale_and_all_older::( + remove_aborted_broadcasts::POLKADOT_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); }, genesis_hashes::SISYPHOS => { log::info!("🧹 No housekeeping required for Sisyphos."); @@ -21,4 +42,40 @@ impl OnRuntimeUpgrade for Migration { Weight::zero() } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), DispatchError> { + match genesis_hashes::genesis_hash::() { + genesis_hashes::BERGHAIN => { + log::info!( + "Housekeeping post_upgrade, checking stale aborted broadcasts are removed." + ); + remove_aborted_broadcasts::assert_removed::( + remove_aborted_broadcasts::ETHEREUM_MAX_ABORTED_BROADCAST_BERGHAIN, + ); + remove_aborted_broadcasts::assert_removed::( + remove_aborted_broadcasts::ARBITRUM_MAX_ABORTED_BROADCAST_BERGHAIN, + ); + }, + genesis_hashes::PERSEVERANCE => { + log::info!( + "Housekeeping post_upgrade, checking stale aborted broadcasts are removed." + ); + remove_aborted_broadcasts::assert_removed::( + remove_aborted_broadcasts::ETHEREUM_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); + remove_aborted_broadcasts::assert_removed::( + remove_aborted_broadcasts::ARBITRUM_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); + remove_aborted_broadcasts::assert_removed::( + remove_aborted_broadcasts::POLKADOT_MAX_ABORTED_BROADCAST_PERSEVERANCE, + ); + }, + genesis_hashes::SISYPHOS => { + log::info!("Skipping housekeeping post_upgrade for Sisyphos."); + }, + _ => {}, + } + Ok(()) + } }