From aa5bea3bb224d1b3d9656d614c5661b56dc7c4a0 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 14:39:27 +0300 Subject: [PATCH 01/29] add ready_at_light fallback Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 9 +++++++++ .../client/transaction-pool/src/fork_aware_txpool/mod.rs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) 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..efd9e36cc2182 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 @@ -554,6 +554,15 @@ 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)) + { + // Fall-back for the case of not having a valid view we could use + // for getting a ready transactions set. + 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..79ec6ba2fef0d 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -209,7 +209,7 @@ //! ### 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 From b54e528fe1690112a56793229404c51412e8610c Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 15:48:36 +0300 Subject: [PATCH 02/29] update test Signed-off-by: Iulian Barbu --- .../client/transaction-pool/tests/fatp.rs | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index ba676715bdc13..c868e35486300 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2312,11 +2312,7 @@ fn fatp_ready_light_long_fork_retracted_works() { let xt3 = uxt(Dave, 200); let xt4 = uxt(Eve, 200); - let submissions = vec![pool.submit_at( - genesis, - SOURCE, - vec![xt0.clone(), xt1.clone(), xt2.clone(), xt3.clone()], - )]; + 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() })); @@ -2325,15 +2321,44 @@ fn fatp_ready_light_long_fork_retracted_works() { 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 mut ready_iterator = pool.ready_at_light(header01b.hash()).now_or_never().unwrap(); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_none()); + 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 mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); assert!(ready_iterator.next().is_none()); 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); From 9e4b754af739ef286ca59164571b904448bee32a Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 15:56:03 +0300 Subject: [PATCH 03/29] improve comment Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 efd9e36cc2182..b8dfe551759cb 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 @@ -560,8 +560,10 @@ where .read() .and_then(|at| self.view_store.get_view_at(at, true)) { - // Fall-back for the case of not having a valid view we could use - // for getting a ready transactions set. + // Fallback for the case of not having a best view to use + // for getting a ready transactions set. Even if the ready txs provided by the most + // rcent view are invalid or already included in the blocks, some of them might still + // be valid for `at` hash. Box::new(most_recent_view.pool.validated_pool().ready()) } else { let empty: ReadyIteratorFor = Box::new(std::iter::empty()); From be1d1120840db5ea86519ca2c107ec02614680a8 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 15:58:16 +0300 Subject: [PATCH 04/29] fix comment Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 b8dfe551759cb..e14146bf7bf0d 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 @@ -561,9 +561,10 @@ where .and_then(|at| self.view_store.get_view_at(at, true)) { // Fallback for the case of not having a best view to use - // for getting a ready transactions set. Even if the ready txs provided by the most - // rcent view are invalid or already included in the blocks, some of them might still - // be valid for `at` hash. + // for getting a ready transactions set. Even if the some of + // the ready txs provided by the most recent view are invalid or already + // included in the blocks, a few of them might still be valid for `at` hash, + // 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()); From a42f4141d0badc345cb18d913a6cfb835962ee86 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 14 May 2025 13:03:22 +0000 Subject: [PATCH 05/29] Update from github-actions[bot] running command 'prdoc' --- prdoc/pr_8533.prdoc | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 prdoc/pr_8533.prdoc diff --git a/prdoc/pr_8533.prdoc b/prdoc/pr_8533.prdoc new file mode 100644 index 0000000000000..baefe48fdfd7e --- /dev/null +++ b/prdoc/pr_8533.prdoc @@ -0,0 +1,22 @@ +title: '`fatxpool`: add fallback for ready at light' +doc: +- audience: Todo + description: |- + # Description + + Proposing a new block on top of an existing block considers the best ready transactions provided by the txpool. Whenever the given parent hash to build on top is part of a fork up to the finalized block which has no best block notified to the pool, it might be the case that the proposer will rely on `ready_at_light` (due to various reason not in our control), and when that's the case, the ready transactions set will be empty. + + This PR adds a fallback for the `ready_at_light` where we consider the ready txs of the most recent view processed by the txpool, even if those txs might be invalid + + Closes #8213 + + ## Integration + + N/A + + ## Review Notes + + In terms of testing, I updated an existing test which already exercises `ready_at_light` in the scope of the newly added fallback. +crates: +- name: sc-transaction-pool + bump: major From 489e1c97668f1179951e467454c12d01c760c811 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 16:29:05 +0300 Subject: [PATCH 06/29] fix prdoc Signed-off-by: Iulian Barbu --- prdoc/pr_8533.prdoc | 21 +++---------------- .../fork_aware_txpool/fork_aware_txpool.rs | 9 ++++---- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/prdoc/pr_8533.prdoc b/prdoc/pr_8533.prdoc index baefe48fdfd7e..2c527e0783337 100644 --- a/prdoc/pr_8533.prdoc +++ b/prdoc/pr_8533.prdoc @@ -1,22 +1,7 @@ -title: '`fatxpool`: add fallback for ready at light' +title: '`fatxpool`: add fallback for ready at light' doc: -- audience: Todo - description: |- - # Description - - Proposing a new block on top of an existing block considers the best ready transactions provided by the txpool. Whenever the given parent hash to build on top is part of a fork up to the finalized block which has no best block notified to the pool, it might be the case that the proposer will rely on `ready_at_light` (due to various reason not in our control), and when that's the case, the ready transactions set will be empty. - - This PR adds a fallback for the `ready_at_light` where we consider the ready txs of the most recent view processed by the txpool, even if those txs might be invalid - - Closes #8213 - - ## Integration - - N/A - - ## Review Notes - - In terms of testing, I updated an existing test which already exercises `ready_at_light` in the scope of the newly added fallback. +- 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. 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 e14146bf7bf0d..abc5a29be4cd2 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 @@ -561,10 +561,11 @@ where .and_then(|at| self.view_store.get_view_at(at, true)) { // Fallback for the case of not having a best view to use - // for getting a ready transactions set. Even if the some of - // the ready txs provided by the most recent view are invalid or already - // included in the blocks, a few of them might still be valid for `at` hash, - // which is still better than including nothing. + // for getting a ready transactions set. Even if some of + // the ready txs provided by the most recent view are invalid + // or already included in the blocks, a few of them might still + // be valid for `at` hash, 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()); From 67bc04fa348e7f3ddcafdb33411de32cdd1727e8 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 14 May 2025 17:04:08 +0300 Subject: [PATCH 07/29] try another prdoc fix Signed-off-by: Iulian Barbu --- prdoc/pr_8533.prdoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_8533.prdoc b/prdoc/pr_8533.prdoc index 2c527e0783337..4b5eb8ee22a08 100644 --- a/prdoc/pr_8533.prdoc +++ b/prdoc/pr_8533.prdoc @@ -1,7 +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. + 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. crates: - name: sc-transaction-pool bump: major From 83f970bffafd39e78a4890d46a08ef02ccec9e35 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Thu, 15 May 2025 19:07:52 +0300 Subject: [PATCH 08/29] revert old test and create a new one Signed-off-by: Iulian Barbu --- .../client/transaction-pool/tests/fatp.rs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index c868e35486300..e51ebb77075d3 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2312,6 +2312,53 @@ fn fatp_ready_light_long_fork_retracted_works() { let xt3 = uxt(Dave, 200); let xt4 = uxt(Eve, 200); + let submissions = vec![pool.submit_at( + genesis, + SOURCE, + vec![xt0.clone(), xt1.clone(), xt2.clone(), xt3.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); + 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); + + let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + + 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() })); From 6c0b3821188276624710d78648590c5c4a502e0d Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Thu, 15 May 2025 19:10:07 +0300 Subject: [PATCH 09/29] add missing assert Signed-off-by: Iulian Barbu --- substrate/client/transaction-pool/tests/fatp.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index e51ebb77075d3..f81e041b8275f 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2329,6 +2329,7 @@ fn fatp_ready_light_long_fork_retracted_works() { let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true); let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + assert!(ready_iterator.next().is_none()); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); block_on(pool.maintain(event)); From 9a296568906786f7bd4ff490155e710140c9ee61 Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Thu, 15 May 2025 19:11:48 +0300 Subject: [PATCH 10/29] Update substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 abc5a29be4cd2..5177844ffa1ff 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 @@ -560,12 +560,10 @@ where .read() .and_then(|at| self.view_store.get_view_at(at, true)) { - // Fallback for the case of not having a best view to use - // for getting a ready transactions set. Even if some of - // the ready txs provided by the most recent view are invalid - // or already included in the blocks, a few of them might still - // be valid for `at` hash, which is still better than including - // nothing. + // 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()); From 672794205186736cbfb266179e9a6692da0fb97d Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Thu, 15 May 2025 21:16:58 +0300 Subject: [PATCH 11/29] refactor the ready_at_light best view search Signed-off-by: Iulian Barbu --- .../fork_aware_txpool/fork_aware_txpool.rs | 68 +++++++++++++------ .../client/transaction-pool/tests/fatp.rs | 5 ++ 2 files changed, 52 insertions(+), 21 deletions(-) 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 5177844ffa1ff..4bea286cfd693 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 @@ -60,7 +60,7 @@ use sp_blockchain::{HashAndNumber, TreeRoute}; use sp_core::traits::SpawnEssentialNamed; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, NumberFor}, + traits::{Block as BlockT, Header, NumberFor}, transaction_validity::{TransactionTag as Tag, TransactionValidityError, ValidTransaction}, Saturating, }; @@ -487,35 +487,61 @@ where "fatp::ready_at_light" ); - let Ok(block_number) = self.api.resolve_block_number(at) else { + // Return an empty ready transactions set if we can not find the number of `at` block hash. + let Ok(at_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) - } - }, - ) + let mut to_process = Vec::new(); + let mut best_view = None; + let mut tmp_at_number = at_number; + let mut tmp_at = at; + + // Return an empty ready transaction set if we can't find a finalized block to report to, + // given we can not look for a view that can provide an approximate ready transaction set + // on the fork ending with `at` up to the finalized block. + let last_finalized = self.enactment_state.lock().recent_finalized_block(); + let Ok(last_finalized_number) = self.api.resolve_block_number(last_finalized) else { + return Box::new(std::iter::empty()) }; - if let Ok((Some(best_tree_route), Some(best_view))) = best_result { + // Search for a view that can be used to get and return an approximate ready + // transaction set. + while tmp_at_number > last_finalized_number { + // Found a view, now prune its pool based on the blocks in `to_process`. + if let Some((view, _)) = self.view_store.get_view_at(tmp_at, true) { + best_view = Some(view); + break; + } + + to_process.push(tmp_at); + if let Ok(tmp_at_header) = self.api.block_header(tmp_at) { + let Some(parent_number) = tmp_at_header.and_then(|header| { + tmp_at = *header.parent_hash(); + self.api.resolve_block_number(*header.parent_hash()).ok() + }) else { + break; + }; + + tmp_at_number = parent_number; + } else { + // Not finding the header of any block on the fork means we can not continue looking + // for a view up to the finalized block, so we break early. + break; + } + } + + // 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) = best_view { let (tmp_view, _, _): (View, _, _) = - View::new_from_other(&best_view, &HashAndNumber { hash: at, number: block_number }); + View::new_from_other(&view, &HashAndNumber { hash: at, number: at_number }); let mut all_extrinsics = vec![]; - for h in best_tree_route.enacted() { + for h in to_process { let extrinsics = api - .block_body(h.hash) + .block_body(h) .await .unwrap_or_else(|error| { warn!( @@ -546,7 +572,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, diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index f81e041b8275f..a9967b588b2d1 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2328,7 +2328,12 @@ 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 mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); + assert!(ready_iterator.next().is_some()); assert!(ready_iterator.next().is_none()); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); From b858a23e0f6b7af39262f2813f480d6e52a32bbe Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Thu, 15 May 2025 23:38:15 +0300 Subject: [PATCH 12/29] adjust docs Signed-off-by: Iulian Barbu --- .../client/transaction-pool/src/fork_aware_txpool/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 79ec6ba2fef0d..e22e0b5e1cf4f 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -214,9 +214,10 @@ //! //! 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 +//! included in enacted part of [tree route][`TreeRoute`] from the best view to the block at which a //! ready iterator was requested. No new [transaction validations][runtime_api::validate] are -//! required to accomplish it. +//! 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 From c2e0bd97d307a1f4d0b681056ce5cfb0b955e718 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 15 May 2025 20:48:01 +0000 Subject: [PATCH 13/29] Update from github-actions[bot] running command 'fmt' --- .../transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4bea286cfd693..3be8644781710 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 @@ -587,7 +587,7 @@ where .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 + // 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()) From ebc78b0bda81e14dcb65119666284d0c799f6dc9 Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Fri, 16 May 2025 08:57:02 +0300 Subject: [PATCH 14/29] Update prdoc/pr_8533.prdoc --- prdoc/pr_8533.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_8533.prdoc b/prdoc/pr_8533.prdoc index 4b5eb8ee22a08..1c08e9db5349b 100644 --- a/prdoc/pr_8533.prdoc +++ b/prdoc/pr_8533.prdoc @@ -2,7 +2,7 @@ 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. + 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 From eae2c1da04c59ce1024d7001073cf10aea7199b6 Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Fri, 16 May 2025 08:57:13 +0300 Subject: [PATCH 15/29] Update substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs --- .../transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3be8644781710..c61b5453aa568 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 @@ -508,7 +508,7 @@ where // Search for a view that can be used to get and return an approximate ready // transaction set. while tmp_at_number > last_finalized_number { - // Found a view, now prune its pool based on the blocks in `to_process`. + // Found a view, stop searching.. if let Some((view, _)) = self.view_store.get_view_at(tmp_at, true) { best_view = Some(view); break; From 5940d413281adb426f3df7cfe327256274cc4266 Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Fri, 16 May 2025 08:57:23 +0300 Subject: [PATCH 16/29] Update substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs --- .../transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs | 1 + 1 file changed, 1 insertion(+) 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 c61b5453aa568..736063a549976 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 @@ -515,6 +515,7 @@ where } to_process.push(tmp_at); + // Move up the hierarchy by getting the parent block. if let Ok(tmp_at_header) = self.api.block_header(tmp_at) { let Some(parent_number) = tmp_at_header.and_then(|header| { tmp_at = *header.parent_hash(); From d11101e3360100c5a95b546a8d020b3758865b97 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Fri, 16 May 2025 14:27:09 +0000 Subject: [PATCH 17/29] polish iterating through enacted blocks Signed-off-by: Iulian Barbu --- .../fork_aware_txpool/fork_aware_txpool.rs | 101 ++++++++++-------- 1 file changed, 54 insertions(+), 47 deletions(-) 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 736063a549976..02bce5cc4561f 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 @@ -469,12 +469,59 @@ where self.view_store.futures_at(at) } + /// Searches in the view store for a view by iterating through the fork of + /// the `at` block, up to the last finalized block. + /// + /// Returns with a pair of a maybe view and a set of enacted blocks (possibly empty if no view + /// is found), usually starting at the last known block of the fork, up to the block + /// corresponding to the found view. + fn find_best_view( + &self, + at: &HashAndNumber, + ) -> (Option>>, Vec) { + let mut enacted_blocks = Vec::new(); + let mut best_view = None; + + let mut at_hash = at.hash; + let mut at_number = at.number; + + let last_finalized = self.enactment_state.lock().recent_finalized_block(); + let Some(last_finalized_number) = self.api.resolve_block_number(last_finalized).ok() else { + return (None, Vec::new()); + }; + + // Search for a view that can be used to get and return an approximate ready + // transaction set. + while at_number > last_finalized_number { + // Found a view, stop searching.. + if let Some((view, _)) = self.view_store.get_view_at(at_hash, true) { + best_view = Some(view); + break; + } + + enacted_blocks.push(at_hash); + + // Move up into the fork. Return with no view or enacted blocks list if + // we can't access the header of the current block. + let Some(header) = self.api.block_header(at_hash).ok().flatten() else { + return (None, Vec::new()); + }; + at_hash = *header.parent_hash(); + at_number = at_number.saturating_sub(1u32.into()); + } + + return (best_view, enacted_blocks); + } + /// Returns a best-effort set of ready transactions for a given block, without executing full /// maintain process. /// /// 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,60 +534,20 @@ where "fatp::ready_at_light" ); - // Return an empty ready transactions set if we can not find the number of `at` block hash. + // Return an empty ready txs set if we can not resolve `at` block number. let Ok(at_number) = self.api.resolve_block_number(at) else { - return Box::new(std::iter::empty()) - }; - - let mut to_process = Vec::new(); - let mut best_view = None; - let mut tmp_at_number = at_number; - let mut tmp_at = at; - - // Return an empty ready transaction set if we can't find a finalized block to report to, - // given we can not look for a view that can provide an approximate ready transaction set - // on the fork ending with `at` up to the finalized block. - let last_finalized = self.enactment_state.lock().recent_finalized_block(); - let Ok(last_finalized_number) = self.api.resolve_block_number(last_finalized) else { - return Box::new(std::iter::empty()) + return Box::new(std::iter::empty()); }; - // Search for a view that can be used to get and return an approximate ready - // transaction set. - while tmp_at_number > last_finalized_number { - // Found a view, stop searching.. - if let Some((view, _)) = self.view_store.get_view_at(tmp_at, true) { - best_view = Some(view); - break; - } - - to_process.push(tmp_at); - // Move up the hierarchy by getting the parent block. - if let Ok(tmp_at_header) = self.api.block_header(tmp_at) { - let Some(parent_number) = tmp_at_header.and_then(|header| { - tmp_at = *header.parent_hash(); - self.api.resolve_block_number(*header.parent_hash()).ok() - }) else { - break; - }; - - tmp_at_number = parent_number; - } else { - // Not finding the header of any block on the fork means we can not continue looking - // for a view up to the finalized block, so we break early. - break; - } - } - // 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) = best_view { - let (tmp_view, _, _): (View, _, _) = - View::new_from_other(&view, &HashAndNumber { hash: at, number: at_number }); + let at_hn = HashAndNumber { hash: at, number: at_number }; + if let (Some(view), enacted_blocks) = self.find_best_view(&at_hn) { + let (tmp_view, _, _): (View, _, _) = View::new_from_other(&view, &at_hn); let mut all_extrinsics = vec![]; - for h in to_process { + for h in enacted_blocks { let extrinsics = api .block_body(h) .await From ebcf7aa39b0773fba18165ef8e386298ab4a1114 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Sun, 18 May 2025 15:40:58 +0000 Subject: [PATCH 18/29] fix comment Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 02bce5cc4561f..0b6bde02fe808 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 @@ -501,9 +501,9 @@ where enacted_blocks.push(at_hash); - // Move up into the fork. Return with no view or enacted blocks list if - // we can't access the header of the current block. - let Some(header) = self.api.block_header(at_hash).ok().flatten() else { + // Move up into the fork. Return with no view and an empty enacted blocks + // list if we can't access the header of the current block. + let header = self.api.block_header(at_hash).ok().flatten() else { return (None, Vec::new()); }; at_hash = *header.parent_hash(); From 64d0e7a5e2ade12ea0d52c63e018cebd18c9dcc8 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Sun, 18 May 2025 18:38:46 +0000 Subject: [PATCH 19/29] fix build error Signed-off-by: Iulian Barbu --- .../transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0b6bde02fe808..87b6a56bf946c 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 @@ -503,7 +503,7 @@ where // Move up into the fork. Return with no view and an empty enacted blocks // list if we can't access the header of the current block. - let header = self.api.block_header(at_hash).ok().flatten() else { + let Some(header) = self.api.block_header(at_hash).ok().flatten() else { return (None, Vec::new()); }; at_hash = *header.parent_hash(); From ccc8cf2997521922259a483f8fbed9477cdb25bb Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Mon, 19 May 2025 12:41:22 +0300 Subject: [PATCH 20/29] Update substrate/client/transaction-pool/tests/fatp.rs Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> --- substrate/client/transaction-pool/tests/fatp.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index a9967b588b2d1..76fd559507a03 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2330,10 +2330,7 @@ fn fatp_ready_light_long_fork_retracted_works() { // Returns the most recent view (`header01a`) ready transactions set. let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); + assert_eq!(ready_iterator.count(), 4); assert!(ready_iterator.next().is_none()); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); From 1fe3507755b8752aaaafabfa5ebf627e7c0d7dd3 Mon Sep 17 00:00:00 2001 From: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com> Date: Mon, 19 May 2025 12:41:42 +0300 Subject: [PATCH 21/29] Update substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> --- substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e22e0b5e1cf4f..6ad2e456061a5 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -214,7 +214,7 @@ //! //! 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 best view to the block at which a +//! 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. If no view is found, it will return the ready transactions of the //! most recent view processed by the transaction pool. From 9d47e15417b4cf3fe54ec942fc883f852e494b32 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Mon, 19 May 2025 10:05:53 +0000 Subject: [PATCH 22/29] more polishing after review Signed-off-by: Iulian Barbu --- .../fork_aware_txpool/fork_aware_txpool.rs | 59 ++++--------------- .../src/fork_aware_txpool/view_store.rs | 36 ++++++++++- 2 files changed, 47 insertions(+), 48 deletions(-) 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 87b6a56bf946c..994e4c39883eb 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 @@ -60,7 +60,7 @@ use sp_blockchain::{HashAndNumber, TreeRoute}; use sp_core::traits::SpawnEssentialNamed; use sp_runtime::{ generic::BlockId, - traits::{Block as BlockT, Header, NumberFor}, + traits::{Block as BlockT, NumberFor}, transaction_validity::{TransactionTag as Tag, TransactionValidityError, ValidTransaction}, Saturating, }; @@ -469,50 +469,6 @@ where self.view_store.futures_at(at) } - /// Searches in the view store for a view by iterating through the fork of - /// the `at` block, up to the last finalized block. - /// - /// Returns with a pair of a maybe view and a set of enacted blocks (possibly empty if no view - /// is found), usually starting at the last known block of the fork, up to the block - /// corresponding to the found view. - fn find_best_view( - &self, - at: &HashAndNumber, - ) -> (Option>>, Vec) { - let mut enacted_blocks = Vec::new(); - let mut best_view = None; - - let mut at_hash = at.hash; - let mut at_number = at.number; - - let last_finalized = self.enactment_state.lock().recent_finalized_block(); - let Some(last_finalized_number) = self.api.resolve_block_number(last_finalized).ok() else { - return (None, Vec::new()); - }; - - // Search for a view that can be used to get and return an approximate ready - // transaction set. - while at_number > last_finalized_number { - // Found a view, stop searching.. - if let Some((view, _)) = self.view_store.get_view_at(at_hash, true) { - best_view = Some(view); - break; - } - - enacted_blocks.push(at_hash); - - // Move up into the fork. Return with no view and an empty enacted blocks - // list if we can't access the header of the current block. - let Some(header) = self.api.block_header(at_hash).ok().flatten() else { - return (None, Vec::new()); - }; - at_hash = *header.parent_hash(); - at_number = at_number.saturating_sub(1u32.into()); - } - - return (best_view, enacted_blocks); - } - /// Returns a best-effort set of ready transactions for a given block, without executing full /// maintain process. /// @@ -539,10 +495,19 @@ where return Box::new(std::iter::empty()); }; + let at_hn = HashAndNumber { hash: at, number: at_number }; + let last_finalized = self.enactment_state.lock().recent_finalized_block(); + // Return an empty ready txs set as well if we can not resolve the recent finalized block + // number. + let Some(last_finalized_number) = self.api.resolve_block_number(last_finalized).ok() else { + return Box::new(std::iter::empty()); + }; + // Prune all txs from the best view found, considering the extrinsics part of the blocks // that are more recent than the view itself. - let at_hn = HashAndNumber { hash: at, number: at_number }; - if let (Some(view), enacted_blocks) = self.find_best_view(&at_hn) { + if let Some((view, enacted_blocks)) = + self.view_store.find_view_upto_block_number(&at_hn, last_finalized_number) + { let (tmp_view, _, _): (View, _, _) = View::new_from_other(&view, &at_hn); let mut all_extrinsics = vec![]; 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..4b04b27718641 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, Saturating}, transaction_validity::{InvalidTransaction, TransactionValidityError}, }; use std::{ @@ -338,6 +338,40 @@ where self.active_views.read().is_empty() && self.inactive_views.read().is_empty() } + /// Searches in the view store for first 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_upto_block_number( + &self, + at: &HashAndNumber, + block_number: <::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 > block_number { + // 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. Return with no view and an empty enacted blocks + // list if we can't access the header of the current block. + let header = self.api.block_header(at_hash).ok().flatten()?; + at_hash = *header.parent_hash(); + at_number = at_number.saturating_sub(1u32.into()); + } + + return None; + } + /// Finds the best existing active view to clone from along the path. /// /// ```text From 5d7c1fb93a339fe98078354f57e28b676ce44de7 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Mon, 19 May 2025 10:22:08 +0000 Subject: [PATCH 23/29] more polishing Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 2 +- .../transaction-pool/src/fork_aware_txpool/view_store.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) 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 994e4c39883eb..4b2291c7bc491 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 @@ -506,7 +506,7 @@ where // 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)) = - self.view_store.find_view_upto_block_number(&at_hn, last_finalized_number) + self.view_store.find_view_descendent_upto_number(&at_hn, last_finalized_number) { let (tmp_view, _, _): (View, _, _) = View::new_from_other(&view, &at_hn); 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 4b04b27718641..530c9a3c9eaa7 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 @@ -338,12 +338,12 @@ where self.active_views.read().is_empty() && self.inactive_views.read().is_empty() } - /// Searches in the view store for first view by iterating through the fork of - /// the `at` block, up to the provided block number. + /// 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_upto_block_number( + pub(super) fn find_view_descendent_upto_number( &self, at: &HashAndNumber, block_number: <::Header as Header>::Number, From ee17e1e79e8078121820b57709c94d6426cbca49 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 19 May 2025 10:25:38 +0000 Subject: [PATCH 24/29] Update from github-actions[bot] running command 'fmt' --- substrate/client/transaction-pool/tests/fatp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index 76fd559507a03..ae61e62f13434 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2330,7 +2330,7 @@ fn fatp_ready_light_long_fork_retracted_works() { // Returns the most recent view (`header01a`) ready transactions set. let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); - assert_eq!(ready_iterator.count(), 4); + assert_eq!(ready_iterator.count(), 4); assert!(ready_iterator.next().is_none()); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); From 897ddfb7c8fa7d7b1c35f2cad1ff715ef3b031b6 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Mon, 19 May 2025 12:51:24 +0000 Subject: [PATCH 25/29] tests polishing Signed-off-by: Iulian Barbu --- substrate/client/transaction-pool/tests/fatp.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/substrate/client/transaction-pool/tests/fatp.rs b/substrate/client/transaction-pool/tests/fatp.rs index ae61e62f13434..c1411b29dafb9 100644 --- a/substrate/client/transaction-pool/tests/fatp.rs +++ b/substrate/client/transaction-pool/tests/fatp.rs @@ -2329,9 +2329,8 @@ fn fatp_ready_light_long_fork_retracted_works() { let header03b = api.push_block_with_parent(header02b.hash(), vec![xt2.clone()], true); // Returns the most recent view (`header01a`) ready transactions set. - let mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); + let ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); assert_eq!(ready_iterator.count(), 4); - assert!(ready_iterator.next().is_none()); let event = new_best_block_event(&pool, Some(header01a.hash()), header01b.hash()); block_on(pool.maintain(event)); @@ -2374,10 +2373,8 @@ fn fatp_ready_light_fallback_gets_triggered() { // 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 mut ready_iterator = pool.ready_at_light(header01b.hash()).now_or_never().unwrap(); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_none()); + 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); @@ -2395,12 +2392,8 @@ fn fatp_ready_light_fallback_gets_triggered() { // 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 mut ready_iterator = pool.ready_at_light(header03b.hash()).now_or_never().unwrap(); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_some()); - assert!(ready_iterator.next().is_none()); + 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)); From 15afbb349c9a4d6932c819e878ad9933febdc066 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Mon, 19 May 2025 14:14:55 +0000 Subject: [PATCH 26/29] return most recent view ready txs if finalized block number can't be resolved Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) 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 4b2291c7bc491..04e05179f79d1 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 @@ -496,18 +496,17 @@ where }; let at_hn = HashAndNumber { hash: at, number: at_number }; - let last_finalized = self.enactment_state.lock().recent_finalized_block(); - // Return an empty ready txs set as well if we can not resolve the recent finalized block - // number. - let Some(last_finalized_number) = self.api.resolve_block_number(last_finalized).ok() else { - return Box::new(std::iter::empty()); - }; + let descendent_view_and_enacted_blocks = self + .api + .resolve_block_number(self.enactment_state.lock().recent_finalized_block()) + .ok() + .and_then(|last_finalized_number| { + self.view_store.find_view_descendent_upto_number(&at_hn, last_finalized_number) + }); // 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)) = - self.view_store.find_view_descendent_upto_number(&at_hn, last_finalized_number) - { + if let Some((view, enacted_blocks)) = descendent_view_and_enacted_blocks { let (tmp_view, _, _): (View, _, _) = View::new_from_other(&view, &at_hn); let mut all_extrinsics = vec![]; From f0bc263b91fdfdd4983f8c0f69817ddc7e9b0b16 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Mon, 19 May 2025 15:34:07 +0000 Subject: [PATCH 27/29] one more round of polish Signed-off-by: Iulian Barbu --- .../fork_aware_txpool/fork_aware_txpool.rs | 25 ++++++++----------- .../src/fork_aware_txpool/mod.rs | 12 +++++---- 2 files changed, 18 insertions(+), 19 deletions(-) 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 04e05179f79d1..a1dcc985a263c 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 @@ -490,27 +490,24 @@ where "fatp::ready_at_light" ); - // Return an empty ready txs set if we can not resolve `at` block number. - let Ok(at_number) = self.api.resolve_block_number(at) else { - return Box::new(std::iter::empty()); - }; - - let at_hn = HashAndNumber { hash: at, number: at_number }; - let descendent_view_and_enacted_blocks = self + 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() - .and_then(|last_finalized_number| { - self.view_store.find_view_descendent_upto_number(&at_hn, last_finalized_number) - }); + .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)) = descendent_view_and_enacted_blocks { + 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_upto_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 enacted_blocks { let extrinsics = api .block_body(h) 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 6ad2e456061a5..d09b8a145bd87 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -213,11 +213,12 @@ //! 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. If no view is found, it will return the ready transactions of the -//! most recent view processed by the transaction pool. +//! [finds the first descendent view][`find_view_descendent_upto_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 @@ -315,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_upto_number`]: crate::fork_aware_txpool::view_store::ViewStore::find_view_descendent_upto_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 From 2570fa35c3c0e202b6e3cd2719ba4ce22cb3b44d Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Wed, 21 May 2025 20:58:45 +0000 Subject: [PATCH 28/29] address nit Signed-off-by: Iulian Barbu --- .../transaction-pool/src/fork_aware_txpool/view_store.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 530c9a3c9eaa7..d1f308e9d4746 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, Header, Saturating}, + traits::{Block as BlockT, Header, One, Saturating}, transaction_validity::{InvalidTransaction, TransactionValidityError}, }; use std::{ @@ -366,7 +366,7 @@ where // list if we can't access the header of the current block. let header = self.api.block_header(at_hash).ok().flatten()?; at_hash = *header.parent_hash(); - at_number = at_number.saturating_sub(1u32.into()); + at_number = at_number.saturating_sub(One::one()); } return None; From a9a337d7cc2cbb2332c24eaec4b8517ecd22aea0 Mon Sep 17 00:00:00 2001 From: Iulian Barbu Date: Thu, 22 May 2025 10:21:44 +0000 Subject: [PATCH 29/29] address basti comments Signed-off-by: Iulian Barbu --- .../src/fork_aware_txpool/fork_aware_txpool.rs | 2 +- .../transaction-pool/src/fork_aware_txpool/mod.rs | 8 ++++---- .../src/fork_aware_txpool/view_store.rs | 13 ++++++------- 3 files changed, 11 insertions(+), 12 deletions(-) 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 a1dcc985a263c..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 @@ -502,7 +502,7 @@ where let at_hn = HashAndNumber { hash: at, number: at_number }; finalized_number.and_then(|finalized_number| { self.view_store - .find_view_descendent_upto_number(&at_hn, finalized_number) + .find_view_descendent_up_to_number(&at_hn, finalized_number) .map(|(view, enacted_blocks)| (view, enacted_blocks, at_hn)) }) }) { 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 d09b8a145bd87..b778042d33cc3 100644 --- a/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs +++ b/substrate/client/transaction-pool/src/fork_aware_txpool/mod.rs @@ -213,9 +213,9 @@ //! maintain process. //! //! To address this, there is a [light version][`ready_at_light`] of the maintain procedure. It -//! [finds the first descendent view][`find_view_descendent_upto_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 +//! [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. @@ -316,7 +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_upto_number`]: crate::fork_aware_txpool::view_store::ViewStore::find_view_descendent_upto_number +//! [`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 d1f308e9d4746..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 @@ -343,10 +343,10 @@ where /// /// 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_upto_number( + pub(super) fn find_view_descendent_up_to_number( &self, at: &HashAndNumber, - block_number: <::Header as Header>::Number, + up_to: <::Header as Header>::Number, ) -> Option<(Arc>, Vec)> { let mut enacted_blocks = Vec::new(); let mut at_hash = at.hash; @@ -354,22 +354,21 @@ where // Search for a view that can be used to get and return an approximate ready // transaction set. - while at_number > block_number { - // Found a view, stop searching.. + 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. Return with no view and an empty enacted blocks - // list if we can't access the header of the current block. + // 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()); } - return None; + None } /// Finds the best existing active view to clone from along the path.