diff --git a/prdoc/pr_8533.prdoc b/prdoc/pr_8533.prdoc new file mode 100644 index 0000000000000..1c08e9db5349b --- /dev/null +++ b/prdoc/pr_8533.prdoc @@ -0,0 +1,8 @@ +title: '`fatxpool`: add fallback for ready at light' +doc: +- audience: Node Dev + description: | + Add fallback for `ready_at_light` for the case of not finding a best view that can be used to return a set of ready transactions. Optimised as well how the best view is searched. +crates: +- name: sc-transaction-pool + bump: major diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs index 2faf5f4ebd30d..3553465668e31 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs @@ -474,7 +474,10 @@ where /// /// The method attempts to build a temporary view and create an iterator of ready transactions /// for a specific `at` hash. If a valid view is found, it collects and prunes - /// transactions already included in the blocks and returns the valid set. + /// transactions already included in the blocks and returns the valid set. Not finding a view + /// returns with the ready transaction set found in the most recent view processed by the + /// fork-aware txpool. Not being able to query for block number for the provided `at` block hash + /// results in returning an empty transaction set. /// /// Pruning is just rebuilding the underlying transactions graph, no validations are executed, /// so this process shall be fast. @@ -487,35 +490,27 @@ where "fatp::ready_at_light" ); - let Ok(block_number) = self.api.resolve_block_number(at) else { - return Box::new(std::iter::empty()) - }; - - let best_result = { - api.tree_route(self.enactment_state.lock().recent_finalized_block(), at).map( - |tree_route| { - if let Some((index, view)) = - tree_route.enacted().iter().enumerate().rev().skip(1).find_map(|(i, b)| { - self.view_store.get_view_at(b.hash, true).map(|(view, _)| (i, view)) - }) { - let e = tree_route.enacted()[index..].to_vec(); - (TreeRoute::new(e, 0).ok(), Some(view)) - } else { - (None, None) - } - }, - ) - }; - - if let Ok((Some(best_tree_route), Some(best_view))) = best_result { - let (tmp_view, _, _): (View, _, _) = - View::new_from_other(&best_view, &HashAndNumber { hash: at, number: block_number }); - + let at_number = self.api.resolve_block_number(at).ok(); + let finalized_number = self + .api + .resolve_block_number(self.enactment_state.lock().recent_finalized_block()) + .ok(); + + // Prune all txs from the best view found, considering the extrinsics part of the blocks + // that are more recent than the view itself. + if let Some((view, enacted_blocks, at_hn)) = at_number.and_then(|at_number| { + let at_hn = HashAndNumber { hash: at, number: at_number }; + finalized_number.and_then(|finalized_number| { + self.view_store + .find_view_descendent_up_to_number(&at_hn, finalized_number) + .map(|(view, enacted_blocks)| (view, enacted_blocks, at_hn)) + }) + }) { + let (tmp_view, _, _): (View, _, _) = View::new_from_other(&view, &at_hn); let mut all_extrinsics = vec![]; - - for h in best_tree_route.enacted() { + for h in enacted_blocks { let extrinsics = api - .block_body(h.hash) + .block_body(h) .await .unwrap_or_else(|error| { warn!( @@ -546,7 +541,7 @@ where debug!( target: LOG_TARGET, ?at, - best_view_hash = ?best_view.at.hash, + best_view_hash = ?view.at.hash, before_count, to_be_removed = all_extrinsics.len(), after_count, @@ -554,6 +549,17 @@ where "fatp::ready_at_light" ); Box::new(tmp_view.pool.validated_pool().ready()) + } else if let Some((most_recent_view, _)) = self + .view_store + .most_recent_view + .read() + .and_then(|at| self.view_store.get_view_at(at, true)) + { + // Fallback for the case when `at` is not on the already known fork. + // Falls back to the most recent view, which may include txs which + // are invalid or already included in the blocks but can still yield a + // partially valid ready set, which is still better than including nothing. + Box::new(most_recent_view.pool.validated_pool().ready()) } else { let empty: ReadyIteratorFor = Box::new(std::iter::empty()); debug!( diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs index fce2d4ad6b27e..b778042d33cc3 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -209,14 +209,16 @@ //! ### Light maintain //! The [maintain](#maintain) procedure can sometimes be quite heavy, and it may not be accomplished //! within the time window expected by the block builder. On top of that block builder may want to -//! build few blocks in the raw, not giving the pool enough time to accomplish possible ongoing +//! build few blocks in the row, not giving the pool enough time to accomplish possible ongoing //! maintain process. //! //! To address this, there is a [light version][`ready_at_light`] of the maintain procedure. It -//! [finds the best view][find_best_view], clones it and prunes all the transactions that were -//! included in enacted part of [tree route][`TreeRoute`] from the base view to the block at which a -//! ready iterator was requested. No new [transaction validations][runtime_api::validate] are -//! required to accomplish it. +//! [finds the first descendent view][`find_view_descendent_up_to_number`] up to the recent +//! finalized block, clones it and prunes all the transactions that were included in enacted part of +//! the traversed route, from the base view to the block at which a ready iterator was requested. No +//! new [transaction validations][runtime_api::validate] are required to accomplish it. If no view +//! is found, it will return the ready transactions of the most recent view processed by the +//! transaction pool. //! //! ### Providing ready transactions: `ready_at` //! The asynchronous [`ready_at`] function resolves to the [ready transactions @@ -314,6 +316,7 @@ //! [`ViewStore`]: crate::fork_aware_txpool::view_store::ViewStore //! [`finish_background_revalidations`]: crate::fork_aware_txpool::view_store::ViewStore::finish_background_revalidations //! [find_best_view]: crate::fork_aware_txpool::view_store::ViewStore::find_best_view +//! [`find_view_descendent_up_to_number`]: crate::fork_aware_txpool::view_store::ViewStore::find_view_descendent_up_to_number //! [`active_views`]: crate::fork_aware_txpool::view_store::ViewStore::active_views //! [`inactive_views`]: crate::fork_aware_txpool::view_store::ViewStore::inactive_views //! [`TxMemPool`]: crate::fork_aware_txpool::tx_mem_pool::TxMemPool diff --git a/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs b/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs index 3e7e230f7d31d..bcf1e8d5ceef9 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/view_store.rs @@ -40,7 +40,7 @@ use sc_transaction_pool_api::{ use sp_blockchain::{HashAndNumber, TreeRoute}; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, Saturating}, + traits::{Block as BlockT, Header, One, Saturating}, transaction_validity::{InvalidTransaction, TransactionValidityError}, }; use std::{ @@ -338,6 +338,39 @@ where self.active_views.read().is_empty() && self.inactive_views.read().is_empty() } + /// Searches in the view store for the first descendant view by iterating through the fork of + /// the `at` block, up to the provided `block_number`. + /// + /// Returns with a maybe pair of a view and a set of enacted blocks when the first view is + /// found. + pub(super) fn find_view_descendent_up_to_number( + &self, + at: &HashAndNumber, + up_to: <::Header as Header>::Number, + ) -> Option<(Arc>, Vec)> { + let mut enacted_blocks = Vec::new(); + let mut at_hash = at.hash; + let mut at_number = at.number; + + // Search for a view that can be used to get and return an approximate ready + // transaction set. + while at_number >= up_to { + // Found a view, stop searching. + if let Some((view, _)) = self.get_view_at(at_hash, true) { + return Some((view, enacted_blocks)); + } + + enacted_blocks.push(at_hash); + + // Move up into the fork. + let header = self.api.block_header(at_hash).ok().flatten()?; + at_hash = *header.parent_hash(); + at_number = at_number.saturating_sub(One::one()); + } + + None + } + /// Finds the best existing active view to clone from along the path. /// /// ```text diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index ba676715bdc13..c1411b29dafb9 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2328,12 +2328,80 @@ fn fatp_ready_light_long_fork_retracted_works() { let header02b = api.push_block_with_parent(header01b.hash(), vec![xt1.clone()], true); let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true); + // Returns the most recent view (`header01a`) ready transactions set. + let ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + assert_eq!(ready_iterator.count(), 4); + + let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); + block_on(pool.maintain(event)); + let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + let ready01 = ready_iterator.next(); + assert_eq!(ready01.unwrap().hash, api.hash_and_length(&xt3).0); + let ready02 = ready_iterator.next(); + assert_eq!(ready02.unwrap().hash, api.hash_and_length(&xt4).0); assert!(ready_iterator.next().is_none()); +} + +#[test] +fn fatp_ready_light_fallback_gets_triggered() { + sp_tracing::try_init_simple(); + + let (pool, api, _) = pool(); + api.set_nonce(api.genesis_hash(), Bob.into(), 200); + api.set_nonce(api.genesis_hash(), Charlie.into(), 200); + api.set_nonce(api.genesis_hash(), Dave.into(), 200); + api.set_nonce(api.genesis_hash(), Eve.into(), 200); + + let genesis = api.genesis_hash(); + + let xt0 = uxt(Alice, 200); + let xt1 = uxt(Bob, 200); + let xt2 = uxt(Charlie, 200); + let xt3 = uxt(Dave, 200); + let xt4 = uxt(Eve, 200); + + let submissions = vec![pool.submit_at(genesis, SOURCE, vec![xt0.clone(), xt1.clone()])]; + let results = block_on(futures::future::join_all(submissions)); + assert!(results.iter().all(|r| { r.is_ok() })); + + let header01a = api.push_block_with_parent(genesis, vec![xt4.clone()], true); + let event = new_best_block_event(&pool, Some(genesis), header01a.hash()); + block_on(pool.maintain(event)); + + let header01b = api.push_block_with_parent(genesis, vec![xt0.clone()], true); + // Call `ready_at_light` at genesis direct descendent, even if not notified as best or + // finalized. Should still return ready txs based on the most recent view processed by the + // txpool. + let ready_iterator = pool.ready_at_light(header01b.hash()).now_or_never().unwrap(); + assert_eq!(ready_iterator.count(), 2); + + let header02b = api.push_block_with_parent(header01b.hash(), vec![xt1.clone()], true); + let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true); + + // Submit a few more txs to the pool. + let submissions = vec![pool.submit_at( + // `at` is ignored. + genesis, + SOURCE, + vec![xt2.clone(), xt3.clone()], + )]; + let results = block_on(futures::future::join_all(submissions)); + assert!(results.iter().all(|r| { r.is_ok() })); + + // Calling `ready_at_light` now on the last block of a fork, with no block notified as best. + // We should still get the ready txs from the most recent view processed by the txpool, + // but now with a few more txs which were submitted previously. + let ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + assert_eq!(ready_iterator.count(), 4); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); block_on(pool.maintain(event)); + // Calling `ready_at_light` on the new best block (`header03b`) should consider its fork up to + // the finalized block for the search of the best view, and coincidentaly, that's the only view + // of the tree route, being the view created for NBB `header03b`. The returned ready txs are the + // ones left in the best view's pool after prunning the txs. let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); let ready01 = ready_iterator.next(); assert_eq!(ready01.unwrap().hash, api.hash_and_length(&xt3).0);