This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Revalidation tweak & logging for transaction pool #6258
Revalidation tweak & logging for transaction pool #6258
Changes from 2 commits
aa6dd5e
c13bf38
1fca9b3
5125782
7f4b63c
3551782
f78d215
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 beMIN_BACKGROUND_REVALIDATION_BATCH_SIZE
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 threadsThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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