Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit 1d16c48

Browse files
NikVolftomusdrwbkchr
committed
Produce block always on updated transaction pool state (#5227)
* make sure return ready iterator once state is updated * update sc_basic_authorship tests * update node tests * fix manual seal * actually fix service test * add tests * Update client/basic-authorship/src/basic_authorship.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * helper function * review suggestions * warning and continue * add debug log * use futures::chennel::oneshot * use declaration bound * no option for updated_at * no allocation * ready_at / ready * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
1 parent 5b8c2f1 commit 1d16c48

File tree

12 files changed

+256
-24
lines changed

12 files changed

+256
-24
lines changed

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎bin/node/cli/src/service.rs‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,7 @@ mod tests {
397397
use sc_service::AbstractService;
398398
use crate::service::{new_full, new_light};
399399
use sp_runtime::traits::IdentifyAccount;
400+
use sp_transaction_pool::{MaintainedTransactionPool, ChainEvent};
400401

401402
type AccountPublic = <Signature as Verify>::Signer;
402403

@@ -414,7 +415,21 @@ mod tests {
414415
let dummy_runtime = ::tokio::runtime::Runtime::new().unwrap();
415416
let block_factory = |service: &<Factory as service::ServiceFactory>::FullService| {
416417
let block_id = BlockId::number(service.client().chain_info().best_number);
417-
let parent_header = service.client().header(&block_id).unwrap().unwrap();
418+
let parent_header = service.client().best_header(&block_id)
419+
.expect("db error")
420+
.expect("best block should exist");
421+
422+
futures::executor::block_on(
423+
service.transaction_pool().maintain(
424+
ChainEvent::NewBlock {
425+
is_new_best: true,
426+
id: block_id.clone(),
427+
retracted: vec![],
428+
header: parent_header,
429+
},
430+
)
431+
);
432+
418433
let consensus_net = ConsensusNetwork::new(service.network(), service.client().clone());
419434
let proposer_factory = consensus::ProposerFactory {
420435
client: service.client().clone(),
@@ -464,6 +479,8 @@ mod tests {
464479
}
465480

466481
#[test]
482+
// It is "ignored", but the node-cli ignored tests are running on the CI.
483+
// This can be run locally with `cargo test --release -p node-cli test_sync -- --ignored`.
467484
#[ignore]
468485
fn test_sync() {
469486
let keystore_path = tempfile::tempdir().expect("Creates keystore path");
@@ -504,6 +521,18 @@ mod tests {
504521
let parent_header = service.client().header(&parent_id).unwrap().unwrap();
505522
let parent_hash = parent_header.hash();
506523
let parent_number = *parent_header.number();
524+
525+
futures::executor::block_on(
526+
service.transaction_pool().maintain(
527+
ChainEvent::NewBlock {
528+
is_new_best: true,
529+
id: parent_id.clone(),
530+
retracted: vec![],
531+
header: parent_header.clone(),
532+
},
533+
)
534+
);
535+
507536
let mut proposer_factory = sc_basic_authorship::ProposerFactory::new(
508537
service.client(),
509538
service.transaction_pool()

‎client/basic-authorship/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ sc-telemetry = { version = "2.0.0-alpha.2", path = "../telemetry" }
2323
sp-transaction-pool = { version = "2.0.0-alpha.2", path = "../../primitives/transaction-pool" }
2424
sc-block-builder = { version = "0.8.0-alpha.2", path = "../block-builder" }
2525
tokio-executor = { version = "0.2.0-alpha.6", features = ["blocking"] }
26+
futures-timer = "3.0.1"
2627

2728
[dev-dependencies]
2829
sc-transaction-pool = { version = "2.0.0-alpha.2", path = "../../client/transaction-pool" }

‎client/basic-authorship/src/basic_authorship.rs‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ use sp_transaction_pool::{TransactionPool, InPoolTransaction};
3333
use sc_telemetry::{telemetry, CONSENSUS_INFO};
3434
use sc_block_builder::{BlockBuilderApi, BlockBuilderProvider};
3535
use sp_api::{ProvideRuntimeApi, ApiExt};
36-
use futures::prelude::*;
36+
use futures::{executor, future, future::Either};
3737
use sp_blockchain::{HeaderBackend, ApplyExtrinsicFailed};
3838
use std::marker::PhantomData;
3939

@@ -210,7 +210,18 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A>
210210
let mut is_first = true;
211211
let mut skipped = 0;
212212
let mut unqueue_invalid = Vec::new();
213-
let pending_iterator = self.transaction_pool.ready();
213+
let pending_iterator = match executor::block_on(future::select(
214+
self.transaction_pool.ready_at(self.parent_number),
215+
futures_timer::Delay::new((deadline - (self.now)()) / 8),
216+
)) {
217+
Either::Left((iterator, _)) => iterator,
218+
Either::Right(_) => {
219+
log::warn!(
220+
"Timeout fired waiting for transaction pool to be ready. Proceeding to block production anyway.",
221+
);
222+
self.transaction_pool.ready()
223+
}
224+
};
214225

215226
debug!("Attempting to push transactions from the pool.");
216227
debug!("Pool status: {:?}", self.transaction_pool.status());
@@ -304,10 +315,12 @@ mod tests {
304315
prelude::*,
305316
runtime::{Extrinsic, Transfer},
306317
};
318+
use sp_transaction_pool::{ChainEvent, MaintainedTransactionPool};
307319
use sc_transaction_pool::{BasicPool, FullChainApi};
308320
use sp_api::Core;
309321
use backend::Backend;
310322
use sp_blockchain::HeaderBackend;
323+
use sp_runtime::traits::NumberFor;
311324

312325
fn extrinsic(nonce: u64) -> Extrinsic {
313326
Transfer {
@@ -318,6 +331,17 @@ mod tests {
318331
}.into_signed_tx()
319332
}
320333

334+
fn chain_event<B: BlockT>(block_number: u64, header: B::Header) -> ChainEvent<B>
335+
where NumberFor<B>: From<u64>
336+
{
337+
ChainEvent::NewBlock {
338+
id: BlockId::Number(block_number.into()),
339+
retracted: vec![],
340+
is_new_best: true,
341+
header: header,
342+
}
343+
}
344+
321345
#[test]
322346
fn should_cease_building_block_when_deadline_is_reached() {
323347
// given
@@ -330,16 +354,27 @@ mod tests {
330354
txpool.submit_at(&BlockId::number(0), vec![extrinsic(0), extrinsic(1)])
331355
).unwrap();
332356

357+
futures::executor::block_on(
358+
txpool.maintain(chain_event(
359+
0,
360+
client.header(&BlockId::Number(0u64)).expect("header get error").expect("there should be header")
361+
))
362+
);
363+
333364
let mut proposer_factory = ProposerFactory::new(client.clone(), txpool.clone());
334365

335-
let cell = Mutex::new(time::Instant::now());
366+
let cell = Mutex::new((false, time::Instant::now()));
336367
let mut proposer = proposer_factory.init_with_now(
337368
&client.header(&BlockId::number(0)).unwrap().unwrap(),
338369
Box::new(move || {
339370
let mut value = cell.lock();
340-
let old = *value;
371+
if !value.0 {
372+
value.0 = true;
373+
return value.1;
374+
}
375+
let old = value.1;
341376
let new = old + time::Duration::from_secs(2);
342-
*value = new;
377+
*value = (true, new);
343378
old
344379
})
345380
);
@@ -371,6 +406,13 @@ mod tests {
371406
txpool.submit_at(&BlockId::number(0), vec![extrinsic(0)]),
372407
).unwrap();
373408

409+
futures::executor::block_on(
410+
txpool.maintain(chain_event(
411+
0,
412+
client.header(&BlockId::Number(0u64)).expect("header get error").expect("there should be header")
413+
))
414+
);
415+
374416
let mut proposer_factory = ProposerFactory::new(client.clone(), txpool.clone());
375417

376418
let mut proposer = proposer_factory.init_with_now(
@@ -459,15 +501,26 @@ mod tests {
459501
block
460502
};
461503

504+
futures::executor::block_on(
505+
txpool.maintain(chain_event(
506+
0,
507+
client.header(&BlockId::Number(0u64)).expect("header get error").expect("there should be header")
508+
))
509+
);
510+
462511
// let's create one block and import it
463512
let block = propose_block(&client, 0, 2, 7);
464513
client.import(BlockOrigin::Own, block).unwrap();
465514

466-
// now let's make sure that we can still make some progress
515+
futures::executor::block_on(
516+
txpool.maintain(chain_event(
517+
1,
518+
client.header(&BlockId::Number(1)).expect("header get error").expect("there should be header")
519+
))
520+
);
467521

468-
// This is most likely incorrect, and caused by #5139
469-
let tx_remaining = 0;
470-
let block = propose_block(&client, 1, 2, tx_remaining);
522+
// now let's make sure that we can still make some progress
523+
let block = propose_block(&client, 1, 2, 5);
471524
client.import(BlockOrigin::Own, block).unwrap();
472525
}
473526
}

‎client/consensus/manual-seal/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ jsonrpc-derive = "14.0.5"
1717
log = "0.4.8"
1818
parking_lot = "0.10.0"
1919
serde = { version = "1.0", features=["derive"] }
20+
assert_matches = "1.3.0"
2021

2122
sc-client = { path = "../../../client" , version = "0.8.0-alpha.2"}
2223
sc-client-api = { path = "../../../client/api" , version = "2.0.0-alpha.2"}

‎client/consensus/manual-seal/src/lib.rs‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ mod tests {
224224
txpool::Options,
225225
};
226226
use substrate_test_runtime_transaction_pool::{TestApi, uxt};
227-
use sp_transaction_pool::TransactionPool;
227+
use sp_transaction_pool::{TransactionPool, MaintainedTransactionPool};
228228
use sp_runtime::generic::BlockId;
229229
use sp_blockchain::HeaderBackend;
230230
use sp_consensus::ImportedAux;
@@ -432,14 +432,24 @@ mod tests {
432432
assert!(backend.blockchain().header(BlockId::Number(0)).unwrap().is_some());
433433
assert!(pool.submit_one(&BlockId::Number(1), uxt(Alice, 1)).await.is_ok());
434434

435+
pool.maintain(sp_transaction_pool::ChainEvent::NewBlock {
436+
id: BlockId::Number(1),
437+
header: backend.blockchain().header(BlockId::Number(1)).expect("db error").expect("imported above"),
438+
is_new_best: true,
439+
retracted: vec![],
440+
}).await;
441+
435442
let (tx1, rx1) = futures::channel::oneshot::channel();
436443
assert!(sink.send(EngineCommand::SealNewBlock {
437-
parent_hash: Some(created_block.hash.clone()),
444+
parent_hash: Some(created_block.hash),
438445
sender: Some(tx1),
439446
create_empty: false,
440447
finalize: false,
441448
}).await.is_ok());
442-
assert!(rx1.await.unwrap().is_ok());
449+
assert_matches::assert_matches!(
450+
rx1.await.expect("should be no error receiving"),
451+
Ok(_)
452+
);
443453
assert!(backend.blockchain().header(BlockId::Number(1)).unwrap().is_some());
444454
pool_api.increment_nonce(Alice.into());
445455

‎client/service/test/src/lib.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,11 @@ pub fn sync<G, E, Fb, F, Lb, L, B, ExF, U>(
482482
let first_user_data = &network.full_nodes[0].2;
483483
let best_block = BlockId::number(first_service.get().client().chain_info().best_number);
484484
let extrinsic = extrinsic_factory(&first_service.get(), first_user_data);
485-
futures::executor::block_on(first_service.get().transaction_pool().submit_one(&best_block, extrinsic)).unwrap();
485+
486+
futures::executor::block_on(
487+
first_service.get().transaction_pool().submit_one(&best_block, extrinsic)
488+
).expect("failed to submit extrinsic");
489+
486490
network.run_until_all_full(
487491
|_index, service| service.get().transaction_pool().ready().count() == 1,
488492
|_index, _service| true,

‎client/transaction-pool/graph/src/validated_pool.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ impl<B: ChainApi> ValidatedPool<B> {
545545
}
546546

547547
/// Get an iterator for ready transactions ordered by priority
548-
pub fn ready(&self) -> impl Iterator<Item=TransactionFor<B>> {
548+
pub fn ready(&self) -> impl Iterator<Item=TransactionFor<B>> + Send {
549549
self.pool.read().ready()
550550
}
551551

0 commit comments

Comments
 (0)