-
Notifications
You must be signed in to change notification settings - Fork 465
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
Worker async sorting #1061
Closed
Closed
Worker async sorting #1061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdowning100
requested review from
wizeguyy,
kiltsonfire and
gameofpointers
as code owners
August 29, 2023 19:35
jdowning100
force-pushed
the
worker-sort
branch
from
August 30, 2023 18:19
6927f05
to
b1062fc
Compare
jdowning100
force-pushed
the
worker-sort
branch
from
August 31, 2023 19:22
e8d50e9
to
1b2ab5b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
@dominant-strategies/core-dev
The worker now asynchronously sorts the txpool (set to every three seconds in sortedPoolCacheInterval) which will allow higher priority transactions to be confirmed even if blocks are full. In addition, there is no longer any priority given to local txs vs remote ones. Note that this sorted list is cached in the worker and sorting is only done in a zone that is processing state.
When constructing a pending block, the worker checks and commits transactions from the sorted list, removing them from the list if they are valid. If the worker is called again quickly (GeneratePendingHeader), the sorted list will be in the next state, i.e. the sorted list will be in a state that assumes the previously constructed block was added to the chain. The sorted list may also be outdated in the case of a data race - that is, a new block is found and the sorter hasn't had a chance to re-sort. In the case of a data race, high-priority transactions that were included in the worker's candidate block but not included in the new current head will not be included in the next candidate block until the worker has a chance to re-sort.
In the case of the reorg, the sorted list is almost guaranteed to be wrong, so it's set to nil and the sorter waits the sortedPoolCacheInterval before attempting to sort again. A possible improvement is to listen to new chainhead events and set the sorted list to nil whenever a new block is found instead of just reorgs. In the case of a nil sorted list, the worker falls back to filling the block without sorting (randomized fill). Another possible improvement is for the worker to generate a new pending header immediately after async sorting so that the candidate block has the latest high-priority transactions (however, interrupting the miner in the middle of mining a valid block might not be a good idea).
One issue is that the sorted list is a heap, so when the top element is removed, it's possible that the heap needs to be re-sorted (worst case O(log n) ) which is not possible to do asynchronously. Currently, the worker pops the top element if there's an error with the transaction which requires a re-sort. If the transaction is a low nonce (data race) or does not have an error, the next transaction for the account, if there is one, is added to the block next without re-sorting.
Please let me know your thoughts.