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

Revalidation tweak & logging for transaction pool #6258

Merged
gavofyork merged 7 commits into from
Jun 9, 2020
Merged

Revalidation tweak & logging for transaction pool #6258

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aa6dd5e
updates and logging
NikVolf Jun 2, 2020
c13bf38
fix length
NikVolf Jun 4, 2020
1fca9b3
Update client/transaction-pool/src/lib.rs
NikVolf Jun 4, 2020
5125782
rename
NikVolf Jun 4, 2020
7f4b63c
Merge branch 'nv-txpool-tweaks' of github.com:paritytech/substrate in…
NikVolf Jun 4, 2020
3551782
address sequential concerns also
NikVolf Jun 8, 2020
f78d215
Update client/transaction-pool/src/lib.rs
NikVolf Jun 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion client/basic-authorship/src/basic_authorship.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,8 @@ impl<A, B, Block, C> Proposer<B, Block, C, A>
Either::Left((iterator, _)) => iterator,
Either::Right(_) => {
log::warn!(
"Timeout fired waiting for transaction pool to be ready. Proceeding to block production anyway.",
"Timeout fired waiting for transaction pool at block #{}. Proceeding with production.",
self.parent_number,
);
self.transaction_pool.ready()
}
Expand Down
6 changes: 6 additions & 0 deletions client/transaction-pool/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ impl<PoolApi, Block> TransactionPool for BasicPool<PoolApi, Block>

fn ready_at(&self, at: NumberFor<Self::Block>) -> PolledIterator<PoolApi> {
if self.ready_poll.lock().updated_at() >= at {
log::trace!(target: "txpool", "Transaction pool is already processed block #{}", at);
let iterator: ReadyIteratorFor<PoolApi> = Box::new(self.pool.validated_pool().ready());
return Box::pin(futures::future::ready(iterator));
}
Expand Down Expand Up @@ -437,6 +438,9 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block>
fn maintain(&self, event: ChainEvent<Self::Block>) -> Pin<Box<dyn Future<Output=()> + Send>> {
match event {
ChainEvent::NewBlock { id, retracted, .. } => {

log::trace!(target: "txpool", "Processing block #{} in the transaction pool. Retracted: {:?}", id, retracted);

let id = id.clone();
let pool = self.pool.clone();
let api = self.api.clone();
Expand Down Expand Up @@ -472,6 +476,8 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block>
.map(|tx| pool.hash_of(&tx))
.collect::<Vec<_>>();

log::trace!(target: "txpool", "Pruning transactions: {:?}", hashes);

if let Err(e) = pool.prune_known(&id, &hashes) {
log::error!("Cannot prune known in the pool {:?}!", e);
}
Expand Down
2 changes: 1 addition & 1 deletion client/transaction-pool/src/revalidation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ impl<Api: ChainApi> RevalidationWorker<Api> {

fn prepare_batch(&mut self) -> Vec<ExHash<Api>> {
let mut queued_exts = Vec::new();
let mut left = BACKGROUND_REVALIDATION_BATCH_SIZE;
let mut left = std::cmp::max(BACKGROUND_REVALIDATION_BATCH_SIZE, self.members.len() / 4);
Copy link
Member

@bkchr bkchr Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, actually when reading this again, what should this change bring?

And BACKGROUND_REVALIDATION_BATCH_SIZE should now be MIN_BACKGROUND_REVALIDATION_BATCH_SIZE

Copy link
Contributor Author

@NikVolf NikVolf Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I commented, this should dissolve revalidation queue faster after block events
(when it is most full, since block event is what dumps transactions for revalidation)

Copy link
Member

@bkchr bkchr Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we run into any problems related to transactions not being discarded fast enough?

And blocks only dump transaction for revalidation when we had a Re-org and than one of the retracted blocks need to have a lot of transactions.

I assume we don't revalidate transactions that are pruned through being included in a block?

Copy link
Contributor Author

@NikVolf NikVolf Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, there are problem with revalidation queue bloating when the chaintip is substantial and new transactions also arrive at the substantial rate

Revalidation dumps everything in the pool (which can be not empty after pruning included transactions, unlike what you probably observe on non-stressed node) to the queue, so it is not only about something retracted. And it is best that most of this revalidated before next block import/production start so that revalidation won't interfere with that.

Copy link
Contributor Author

@NikVolf NikVolf Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we don't revalidate transactions that are pruned through being included in a block?

We don't

Copy link
Contributor Author

@NikVolf NikVolf Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the batch size is too big, we will re-validate one batch and directly after this re-validate the next batch

I know this is a bit suboptimal, but the good thing is that validation is parallelized and limited to 2 threads, so validation tasks will just queue on those threads if overflows.

Copy link
Member

@bkchr bkchr Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we spawn 2 threads? I just see that we spawn one future for the re-validation worker.

Copy link
Contributor Author

@NikVolf NikVolf Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revalidation worker does not validate on it's own, it does it via ChainApi, which spawns 2 threads

Copy link
Member

@bkchr bkchr Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that we spawn the validation on a different pool here. However, here we directly await on the future, which means that we will always only check one transaction at a time. We would need to use a join_all to wait for all together to have a validation going on in parallel.

Copy link
Contributor Author

@NikVolf NikVolf Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed those concerns as well @bkchr


// Take maximum of count transaction by order
// which they got into the pool
Expand Down