Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BlockId removal: tx-pool refactor #1678

Merged
merged 14 commits into from
Sep 27, 2023
Prev Previous commit
Next Next commit
fixes
  • Loading branch information
michalkucharczyk committed Sep 22, 2023
commit 3628ccf7638c6db705b0b21865041e01dd559d55
4 changes: 2 additions & 2 deletions cumulus/test/service/benches/transaction_throughput.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use cumulus_test_runtime::{AccountId, BalancesCall, ExistentialDeposit, SudoCall
use futures::{future, StreamExt};
use sc_transaction_pool_api::{TransactionPool as _, TransactionSource, TransactionStatus};
use sp_core::{crypto::Pair, sr25519};
use sp_runtime::{generic::BlockId, OpaqueExtrinsic};
use sp_runtime::OpaqueExtrinsic;

use cumulus_primitives_core::ParaId;
use cumulus_test_service::{
Expand Down Expand Up @@ -117,7 +117,7 @@ async fn submit_tx_and_wait_for_inclusion(
let best_hash = client.chain_info().best_hash;

let mut watch = tx_pool
.submit_and_watch(&BlockId::Hash(best_hash), TransactionSource::External, tx.clone())
.submit_and_watch(best_hash, TransactionSource::External, tx.clone())
.await
.expect("Submits tx to pool")
.fuse();
Expand Down
8 changes: 4 additions & 4 deletions substrate/bin/node/bench/src/construct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use sc_transaction_pool_api::{
};
use sp_consensus::{Environment, Proposer};
use sp_inherents::InherentDataProvider;
use sp_runtime::{generic::BlockId, traits::NumberFor, OpaqueExtrinsic};
use sp_runtime::{traits::NumberFor, OpaqueExtrinsic};

use crate::{
common::SizeType,
Expand Down Expand Up @@ -233,7 +233,7 @@ impl sc_transaction_pool_api::TransactionPool for Transactions {
/// Returns a future that imports a bunch of unverified transactions to the pool.
fn submit_at(
&self,
_at: &BlockId<Self::Block>,
_at: Self::Hash,
_source: TransactionSource,
_xts: Vec<TransactionFor<Self>>,
) -> PoolFuture<Vec<Result<node_primitives::Hash, Self::Error>>, Self::Error> {
Expand All @@ -243,7 +243,7 @@ impl sc_transaction_pool_api::TransactionPool for Transactions {
/// Returns a future that imports one unverified transaction to the pool.
fn submit_one(
&self,
_at: &BlockId<Self::Block>,
_at: Self::Hash,
_source: TransactionSource,
_xt: TransactionFor<Self>,
) -> PoolFuture<TxHash<Self>, Self::Error> {
Expand All @@ -252,7 +252,7 @@ impl sc_transaction_pool_api::TransactionPool for Transactions {

fn submit_and_watch(
&self,
_at: &BlockId<Self::Block>,
_at: Self::Hash,
_source: TransactionSource,
_xt: TransactionFor<Self>,
) -> PoolFuture<Pin<Box<TransactionStatusStreamFor<Self>>>, Self::Error> {
Expand Down
6 changes: 3 additions & 3 deletions substrate/bin/node/bench/src/txpool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use node_testing::bench::{BenchDb, BlockType, DatabaseType, KeyTypes};

use sc_transaction_pool::BasicPool;
use sc_transaction_pool_api::{TransactionPool, TransactionSource};
use sp_runtime::generic::BlockId;

use crate::core::{self, Mode, Path};

Expand Down Expand Up @@ -58,10 +57,11 @@ impl core::BenchmarkDescription for PoolBenchmarkDescription {
impl core::Benchmark for PoolBenchmark {
fn run(&mut self, mode: Mode) -> std::time::Duration {
let context = self.database.create_context();
let genesis_hash = context.client.chain_info().genesis_hash;

let _ = context
.client
.runtime_version_at(context.client.chain_info().genesis_hash)
.runtime_version_at(genesis_hash)
.expect("Failed to get runtime version")
.spec_version;

Expand Down Expand Up @@ -90,7 +90,7 @@ impl core::Benchmark for PoolBenchmark {
let start = std::time::Instant::now();
let submissions = generated_transactions
.into_iter()
.map(|tx| txpool.submit_one(&BlockId::Number(0), TransactionSource::External, tx));
.map(|tx| txpool.submit_one(genesis_hash, TransactionSource::External, tx));
futures::executor::block_on(futures::future::join_all(submissions));
let elapsed = start.elapsed();

Expand Down
4 changes: 2 additions & 2 deletions substrate/bin/node/cli/benches/transaction_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use sc_transaction_pool::PoolLimit;
use sc_transaction_pool_api::{TransactionPool as _, TransactionSource, TransactionStatus};
use sp_core::{crypto::Pair, sr25519};
use sp_keyring::Sr25519Keyring;
use sp_runtime::{generic::BlockId, OpaqueExtrinsic};
use sp_runtime::OpaqueExtrinsic;
use tokio::runtime::Handle;

fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
Expand Down Expand Up @@ -191,7 +191,7 @@ async fn submit_tx_and_wait_for_inclusion(
let best_hash = client.chain_info().best_hash;

let mut watch = tx_pool
.submit_and_watch(&BlockId::Hash(best_hash), TransactionSource::External, tx.clone())
.submit_and_watch(best_hash, TransactionSource::External, tx.clone())
.await
.expect("Submits tx to pool")
.fuse();
Expand Down
5 changes: 2 additions & 3 deletions substrate/client/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,10 +544,9 @@ mod tests {
to: AccountKeyring::Bob.into(),
}
.into_unchecked_extrinsic();
block_on(pool.submit_one(&BlockId::hash(best.hash()), source, transaction.clone()))
.unwrap();
block_on(pool.submit_one(best.hash(), source, transaction.clone())).unwrap();
block_on(pool.submit_one(
&BlockId::hash(best.hash()),
best.hash(),
source,
ExtrinsicBuilder::new_call_do_not_propagate().nonce(1).build(),
))
Expand Down
37 changes: 23 additions & 14 deletions substrate/client/transaction-pool/benches/basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use sp_runtime::{
ValidTransaction,
},
};
use std::sync::Arc;
use substrate_test_runtime::{AccountId, Block, Extrinsic, ExtrinsicBuilder, TransferData, H256};

#[derive(Clone, Debug, Default)]
Expand Down Expand Up @@ -61,7 +62,7 @@ impl ChainApi for TestApi {

fn validate_transaction(
&self,
at: &BlockId<Self::Block>,
at: <Self::Block as BlockT>::Hash,
_source: TransactionSource,
uxt: <Self::Block as BlockT>::Extrinsic,
) -> Self::ValidationFuture {
Expand All @@ -70,7 +71,7 @@ impl ChainApi for TestApi {
let nonce = transfer.nonce;
let from = transfer.from;

match self.block_id_to_number(at) {
match self.block_id_to_number(&BlockId::Hash(at)) {
Ok(Some(num)) if num > 5 => return ready(Ok(Err(InvalidTransaction::Stale.into()))),
_ => {},
}
Expand All @@ -94,6 +95,8 @@ impl ChainApi for TestApi {
) -> Result<Option<NumberFor<Self::Block>>, Self::Error> {
Ok(match at {
BlockId::Number(num) => Some(*num),
BlockId::Hash(hash) if *hash == H256::from_low_u64_be(hash.to_low_u64_be()) =>
michalkucharczyk marked this conversation as resolved.
Show resolved Hide resolved
Some(hash.to_low_u64_be()),
BlockId::Hash(_) => None,
})
}
Expand All @@ -104,7 +107,7 @@ impl ChainApi for TestApi {
) -> Result<Option<<Self::Block as BlockT>::Hash>, Self::Error> {
Ok(match at {
BlockId::Number(num) => Some(H256::from_low_u64_be(*num)).into(),
BlockId::Hash(_) => None,
BlockId::Hash(hash) => Some(*hash),
})
}

Expand Down Expand Up @@ -137,7 +140,7 @@ fn uxt(transfer: TransferData) -> Extrinsic {
ExtrinsicBuilder::new_bench_call(transfer).build()
}

fn bench_configured(pool: Pool<TestApi>, number: u64) {
fn bench_configured(pool: Pool<TestApi>, number: u64, api: Arc<TestApi>) {
let source = TransactionSource::External;
let mut futures = Vec::new();
let mut tags = Vec::new();
Expand All @@ -151,7 +154,12 @@ fn bench_configured(pool: Pool<TestApi>, number: u64) {
});

tags.push(to_tag(nonce, AccountId::from_h256(H256::from_low_u64_be(1))));
futures.push(pool.submit_one(&BlockId::Number(1), source, xt));

futures.push(pool.submit_one(
api.block_id_to_hash(&BlockId::Number(1)).unwrap().unwrap(),
source,
xt,
));
}

let res = block_on(futures::future::join_all(futures.into_iter()));
Expand All @@ -162,7 +170,12 @@ fn bench_configured(pool: Pool<TestApi>, number: u64) {

// Prune all transactions.
let block_num = 6;
block_on(pool.prune_tags(&BlockId::Number(block_num), tags, vec![])).expect("Prune failed");
block_on(pool.prune_tags(
api.block_id_to_hash(&BlockId::Number(block_num)).unwrap().unwrap(),
tags,
vec![],
))
.expect("Prune failed");

// pool is empty
assert_eq!(pool.validated_pool().status().ready, 0);
Expand All @@ -172,19 +185,15 @@ fn bench_configured(pool: Pool<TestApi>, number: u64) {
fn benchmark_main(c: &mut Criterion) {
c.bench_function("sequential 50 tx", |b| {
b.iter(|| {
bench_configured(
Pool::new(Default::default(), true.into(), TestApi::new_dependant().into()),
50,
);
let api = Arc::from(TestApi::new_dependant());
bench_configured(Pool::new(Default::default(), true.into(), api.clone()), 50, api);
});
});

c.bench_function("random 100 tx", |b| {
b.iter(|| {
bench_configured(
Pool::new(Default::default(), true.into(), TestApi::default().into()),
100,
);
let api = Arc::from(TestApi::default());
bench_configured(Pool::new(Default::default(), true.into(), api.clone()), 100, api);
});
});
}
Expand Down