From eeb490ecfd5bf683f664348e9f7692c4ba8c12c3 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Mon, 16 Jan 2023 23:15:22 +0100 Subject: [PATCH] txpool: don't maintain the pool during major sync (#13004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * txpool: don't maintain the pool during major sync Fix shall prevent from wasting the CPU during the major sync. No actions are actually required in transaction pool during the major sync. Fixes: #12903 * passing sync_oracle to maintain method * fixed: builder, txpool tests * do not maintain tx-pool if node gone out of sync * EnactmentAction: all logic moved to EnactmentState Tests to be done. * maintain guard logic moved directly to MaintainedTransactionPool * minor fixes * EnactmentAction: all logic moved to EnactmentState (again) * SyncOracle fixes here and there * Update client/transaction-pool/src/enactment_state.rs Co-authored-by: Bastian Köcher * Update client/transaction-pool/src/enactment_state.rs Co-authored-by: Bastian Köcher * sync_oracle removed * spelling + fmt + doc * Review suggestions applied * log::info -> debug * Update client/transaction-pool/src/enactment_state.rs Co-authored-by: Bastian Köcher * ".git/.scripts/commands/fmt/fmt.sh" Co-authored-by: parity-processbot <> Co-authored-by: Bastian Köcher --- Cargo.lock | 1 + client/transaction-pool/Cargo.toml | 1 + .../transaction-pool/src/enactment_state.rs | 185 +++++++++++++----- client/transaction-pool/src/lib.rs | 14 +- 4 files changed, 145 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e4e3e1b84625c..446638277202a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9046,6 +9046,7 @@ dependencies = [ "futures-timer", "linked-hash-map", "log", + "num-traits", "parity-scale-codec", "parking_lot 0.12.1", "sc-block-builder", diff --git a/client/transaction-pool/Cargo.toml b/client/transaction-pool/Cargo.toml index 7a3ab042d5a13..4b69b6d0f2fe0 100644 --- a/client/transaction-pool/Cargo.toml +++ b/client/transaction-pool/Cargo.toml @@ -19,6 +19,7 @@ futures = "0.3.21" futures-timer = "3.0.2" linked-hash-map = "0.5.4" log = "0.4.17" +num-traits = "0.2.8" parking_lot = "0.12.1" serde = { version = "1.0.136", features = ["derive"] } thiserror = "1.0.30" diff --git a/client/transaction-pool/src/enactment_state.rs b/client/transaction-pool/src/enactment_state.rs index 6aac98641cf85..382b2683156b6 100644 --- a/client/transaction-pool/src/enactment_state.rs +++ b/client/transaction-pool/src/enactment_state.rs @@ -18,13 +18,21 @@ //! Substrate transaction pool implementation. +use num_traits::CheckedSub; use sc_transaction_pool_api::ChainEvent; use sp_blockchain::TreeRoute; -use sp_runtime::traits::Block as BlockT; +use sp_runtime::traits::{Block as BlockT, NumberFor}; + +/// The threshold since the last update where we will skip any maintenance for blocks. +/// +/// This includes tracking re-orgs and sending out certain notifications. In general this shouldn't +/// happen and may only happen when the node is doing a full sync. +const SKIP_MAINTENANCE_THRESHOLD: u16 = 20; /// Helper struct for keeping track of the current state of processed new best /// block and finalized events. The main purpose of keeping track of this state -/// is to figure out if a transaction pool enactment is needed or not. +/// is to figure out which phases (enactment / finalization) of transaction pool +/// maintenance are needed. /// /// Given the following chain: /// @@ -54,6 +62,17 @@ where recent_finalized_block: Block::Hash, } +/// Enactment action that should be performed after processing the `ChainEvent` +#[derive(Debug)] +pub enum EnactmentAction { + /// Both phases of maintenance shall be skipped + Skip, + /// Both phases of maintenance shall be performed + HandleEnactment(TreeRoute), + /// Enactment phase of maintenance shall be skipped + HandleFinalization, +} + impl EnactmentState where Block: BlockT, @@ -71,23 +90,38 @@ where /// Updates the state according to the given `ChainEvent`, returning /// `Some(tree_route)` with a tree route including the blocks that need to /// be enacted/retracted. If no enactment is needed then `None` is returned. - pub fn update( + pub fn update( &mut self, event: &ChainEvent, - tree_route: &F, - ) -> Result>, String> + tree_route: &TreeRouteF, + hash_to_number: &BlockNumberF, + ) -> Result, String> where - F: Fn(Block::Hash, Block::Hash) -> Result, String>, + TreeRouteF: Fn(Block::Hash, Block::Hash) -> Result, String>, + BlockNumberF: Fn(Block::Hash) -> Result>, String>, { - let (new_hash, finalized) = match event { - ChainEvent::NewBestBlock { hash, .. } => (*hash, false), - ChainEvent::Finalized { hash, .. } => (*hash, true), + let (new_hash, current_hash, finalized) = match event { + ChainEvent::NewBestBlock { hash, .. } => (*hash, self.recent_best_block, false), + ChainEvent::Finalized { hash, .. } => (*hash, self.recent_finalized_block, true), }; + // do not proceed with txpool maintain if block distance is to high + let skip_maintenance = match (hash_to_number(new_hash), hash_to_number(current_hash)) { + (Ok(Some(new)), Ok(Some(current))) => + new.checked_sub(¤t) > Some(SKIP_MAINTENANCE_THRESHOLD.into()), + _ => true, + }; + + if skip_maintenance { + log::debug!(target: "txpool", "skip maintain: tree_route would be too long"); + self.force_update(event); + return Ok(EnactmentAction::Skip) + } + // block was already finalized if self.recent_finalized_block == new_hash { log::debug!(target: "txpool", "handle_enactment: block already finalized"); - return Ok(None) + return Ok(EnactmentAction::Skip) } // compute actual tree route from best_block to notified block, and use @@ -109,7 +143,7 @@ where "Recently finalized block {} would be retracted by ChainEvent {}, skipping", self.recent_finalized_block, new_hash ); - return Ok(None) + return Ok(EnactmentAction::Skip) } if finalized { @@ -124,7 +158,7 @@ where target: "txpool", "handle_enactment: no newly enacted blocks since recent best block" ); - return Ok(None) + return Ok(EnactmentAction::HandleFinalization) } // otherwise enacted finalized block becomes best block... @@ -132,7 +166,7 @@ where self.recent_best_block = new_hash; - Ok(Some(tree_route)) + Ok(EnactmentAction::HandleEnactment(tree_route)) } /// Forces update of the state according to the given `ChainEvent`. Intended to be used as a @@ -148,9 +182,10 @@ where #[cfg(test)] mod enactment_state_tests { - use super::EnactmentState; + use super::{EnactmentAction, EnactmentState}; use sc_transaction_pool_api::ChainEvent; use sp_blockchain::{HashAndNumber, TreeRoute}; + use sp_runtime::traits::NumberFor; use std::sync::Arc; use substrate_test_runtime_client::runtime::{Block, Hash}; @@ -170,6 +205,9 @@ mod enactment_state_tests { fn e1() -> HashAndNumber { HashAndNumber { number: 5, hash: Hash::from([0xE1; 32]) } } + fn x1() -> HashAndNumber { + HashAndNumber { number: 22, hash: Hash::from([0x1E; 32]) } + } fn b2() -> HashAndNumber { HashAndNumber { number: 2, hash: Hash::from([0xB2; 32]) } } @@ -182,11 +220,22 @@ mod enactment_state_tests { fn e2() -> HashAndNumber { HashAndNumber { number: 5, hash: Hash::from([0xE2; 32]) } } + fn x2() -> HashAndNumber { + HashAndNumber { number: 22, hash: Hash::from([0x2E; 32]) } + } + + fn test_chain() -> Vec> { + vec![x1(), e1(), d1(), c1(), b1(), a(), b2(), c2(), d2(), e2(), x2()] + } + + fn block_hash_to_block_number(hash: Hash) -> Result>, String> { + Ok(test_chain().iter().find(|x| x.hash == hash).map(|x| x.number)) + } /// mock tree_route computing function for simple two-forks chain fn tree_route(from: Hash, to: Hash) -> Result, String> { - let chain = vec![e1(), d1(), c1(), b1(), a(), b2(), c2(), d2(), e2()]; - let pivot = 4_usize; + let chain = test_chain(); + let pivot = chain.iter().position(|x| x.number == a().number).unwrap(); let from = chain .iter() @@ -197,13 +246,13 @@ mod enactment_state_tests { .position(|bn| bn.hash == to) .ok_or("existing block should be given")?; - // B1-C1-D1-E1 + // B1-C1-D1-E1-..-X1 // / // A // \ - // B2-C2-D2-E2 + // B2-C2-D2-E2-..-X2 // - // [E1 D1 C1 B1 A B2 C2 D2 E2] + // [X1 E1 D1 C1 B1 A B2 C2 D2 E2 X2] let vec: Vec> = if from < to { chain.into_iter().skip(from).take(to - from + 1).collect() @@ -373,13 +422,20 @@ mod enactment_state_tests { let expected = TreeRoute::new(vec![a()], 0); assert_treeroute_eq(result, expected); } + + #[test] + fn tree_route_mock_test_17() { + let result = tree_route(x2().hash, b1().hash); + let expected = TreeRoute::new(vec![x2(), e2(), d2(), c2(), b2(), a(), b1()], 5); + assert_treeroute_eq(result, expected); + } } fn trigger_new_best_block( state: &mut EnactmentState, from: HashAndNumber, acted_on: HashAndNumber, - ) -> bool { + ) -> EnactmentAction { let (from, acted_on) = (from.hash, acted_on.hash); let event_tree_route = tree_route(from, acted_on).expect("Tree route exists"); @@ -391,16 +447,16 @@ mod enactment_state_tests { tree_route: Some(Arc::new(event_tree_route)), }, &tree_route, + &block_hash_to_block_number, ) .unwrap() - .is_some() } fn trigger_finalized( state: &mut EnactmentState, from: HashAndNumber, acted_on: HashAndNumber, - ) -> bool { + ) -> EnactmentAction { let (from, acted_on) = (from.hash, acted_on.hash); let v = tree_route(from, acted_on) @@ -411,9 +467,12 @@ mod enactment_state_tests { .collect::>(); state - .update(&ChainEvent::Finalized { hash: acted_on, tree_route: v.into() }, &tree_route) + .update( + &ChainEvent::Finalized { hash: acted_on, tree_route: v.into() }, + &tree_route, + &block_hash_to_block_number, + ) .unwrap() - .is_some() } fn assert_es_eq( @@ -437,51 +496,51 @@ mod enactment_state_tests { // B2-C2-D2-E2 let result = trigger_new_best_block(&mut es, a(), d1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, d1(), a()); let result = trigger_new_best_block(&mut es, d1(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), a()); let result = trigger_finalized(&mut es, a(), d2()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, d2(), d2()); let result = trigger_new_best_block(&mut es, d2(), e1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_finalized(&mut es, a(), b2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_finalized(&mut es, a(), b1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_new_best_block(&mut es, a(), d2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_finalized(&mut es, a(), d2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_new_best_block(&mut es, a(), c2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_new_best_block(&mut es, a(), c1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, d2(), d2()); let result = trigger_new_best_block(&mut es, d2(), e2()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e2(), d2()); let result = trigger_finalized(&mut es, d2(), e2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::HandleFinalization)); assert_es_eq(&es, e2(), e2()); } @@ -493,27 +552,27 @@ mod enactment_state_tests { // A-B1-C1-D1-E1 let result = trigger_new_best_block(&mut es, a(), b1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, b1(), a()); let result = trigger_new_best_block(&mut es, b1(), c1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, c1(), a()); let result = trigger_new_best_block(&mut es, c1(), d1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, d1(), a()); let result = trigger_new_best_block(&mut es, d1(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), a()); let result = trigger_finalized(&mut es, a(), c1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::HandleFinalization)); assert_es_eq(&es, e1(), c1()); let result = trigger_finalized(&mut es, c1(), e1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::HandleFinalization)); assert_es_eq(&es, e1(), e1()); } @@ -525,11 +584,11 @@ mod enactment_state_tests { // A-B1-C1-D1-E1 let result = trigger_new_best_block(&mut es, a(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), a()); let result = trigger_finalized(&mut es, a(), b1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::HandleFinalization)); assert_es_eq(&es, e1(), b1()); } @@ -541,11 +600,11 @@ mod enactment_state_tests { // A-B1-C1-D1-E1 let result = trigger_finalized(&mut es, a(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), e1()); let result = trigger_finalized(&mut es, e1(), b1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, e1(), e1()); } @@ -561,11 +620,11 @@ mod enactment_state_tests { // B2-C2-D2-E2 let result = trigger_finalized(&mut es, a(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), e1()); let result = trigger_finalized(&mut es, e1(), e2()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, e1(), e1()); } @@ -577,19 +636,19 @@ mod enactment_state_tests { // A-B1-C1-D1-E1 let result = trigger_new_best_block(&mut es, a(), b1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, b1(), a()); let result = trigger_finalized(&mut es, a(), d1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, d1(), d1()); let result = trigger_new_best_block(&mut es, a(), e1()); - assert!(result); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); assert_es_eq(&es, e1(), d1()); let result = trigger_new_best_block(&mut es, a(), c1()); - assert_eq!(result, false); + assert!(matches!(result, EnactmentAction::Skip)); assert_es_eq(&es, e1(), d1()); } @@ -610,4 +669,26 @@ mod enactment_state_tests { es.force_update(&ChainEvent::Finalized { hash: b1().hash, tree_route: Arc::from([]) }); assert_es_eq(&es, a(), b1()); } + + #[test] + fn test_enactment_skip_long_enacted_path() { + sp_tracing::try_init_simple(); + let mut es = EnactmentState::new(a().hash, a().hash); + + // A-B1-C1-..-X1 + let result = trigger_new_best_block(&mut es, a(), x1()); + assert!(matches!(result, EnactmentAction::Skip)); + assert_es_eq(&es, x1(), a()); + } + + #[test] + fn test_enactment_proceed_with_enacted_path_at_threshold() { + sp_tracing::try_init_simple(); + let mut es = EnactmentState::new(b1().hash, b1().hash); + + // A-B1-C1-..-X1 + let result = trigger_new_best_block(&mut es, b1(), x1()); + assert!(matches!(result, EnactmentAction::HandleEnactment { .. })); + assert_es_eq(&es, x1(), b1()); + } } diff --git a/client/transaction-pool/src/lib.rs b/client/transaction-pool/src/lib.rs index f3797b180f14d..1cd9bef77bc69 100644 --- a/client/transaction-pool/src/lib.rs +++ b/client/transaction-pool/src/lib.rs @@ -33,7 +33,7 @@ mod tests; pub use crate::api::FullChainApi; use async_trait::async_trait; -use enactment_state::EnactmentState; +use enactment_state::{EnactmentAction, EnactmentState}; use futures::{ channel::oneshot, future::{self, ready}, @@ -732,16 +732,22 @@ where )), } }; + let block_id_to_number = + |hash| self.api.block_id_to_number(&BlockId::Hash(hash)).map_err(|e| format!("{}", e)); - let result = self.enactment_state.lock().update(&event, &compute_tree_route); + let result = + self.enactment_state + .lock() + .update(&event, &compute_tree_route, &block_id_to_number); match result { Err(msg) => { log::debug!(target: "txpool", "{msg}"); self.enactment_state.lock().force_update(&event); }, - Ok(None) => {}, - Ok(Some(tree_route)) => { + Ok(EnactmentAction::Skip) => return, + Ok(EnactmentAction::HandleFinalization) => {}, + Ok(EnactmentAction::HandleEnactment(tree_route)) => { self.handle_enactment(tree_route).await; }, };