-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Send transactions to all peers instead of a sub-set #1261
Conversation
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 it is inspired from code that exists priorly, but I just want to raise some concerns here about closing and emptying channels, checking all errors, and trying to be synchronous to avoid some flakiness.
Co-authored-by: baptiste-b-pegasys <[email protected]>
@baptiste-b-pegasys changes done. please review again |
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.
LGTM
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.
LGTM
Summary
The introduction of EIP-2464 (
eth65
version) reduces the bandwidth used for transaction propagation from linear complexity in the number of peers to logarithmic one. While this approach removes the naive transfer of full transactions, it does create transaction pool consistency issues where some nodes are unable to promote transactions properly to be included in a block and thus creating empty blocks.Moreover, it happens more specifically when there is a combination of the following:
--txpool.accountqueue
)The following image illustrates the issue:
Therefore, this PR addresses the issue by sending the transactions to all peers, while in the meantime we work on a logarithmic propagation solution for IBFT/QBFT.
Changes