-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(txpool): respect propagate setting in the full tx stream #4362
fix(txpool): respect propagate setting in the full tx stream #4362
Conversation
struct TransactionListener<T: PoolTransaction> { | ||
sender: mpsc::Sender<NewTransactionEvent<T>>, | ||
/// Whether to include transactions that should not be propagated over the network. | ||
kind: PendingTransactionListenerKind, |
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.
Not that happy about reusing PendingTransactionListenerKind
here. If this change is correct, how would you design this?
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.
probably just rename it to TransactionListenerKind
and replace the references in the docs for "pending" with something like "in this pool", wdyt @mattsse?
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.
yeah I like that, TransactionListenerKind
is more appropriate
fn new_pending_pool_transactions_listener( | ||
&self, | ||
) -> NewSubpoolTransactionStream<Self::Transaction> { | ||
NewSubpoolTransactionStream::new(self.new_transactions_listener(), SubPool::Pending) | ||
NewSubpoolTransactionStream::new( | ||
self.new_transactions_listener_for(PendingTransactionListenerKind::PropagateOnly), | ||
SubPool::Pending, | ||
) |
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 didn't do the same refactor for SubPool::BaseFee
or SubPool::Queued
(right below this function) because I didn't see them used anywhere.
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.
this is reasonable, and we don't really have a usecase for this anyway
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.
awesome, sorry for the delay.
this is great, exactly what we wanted,
I only renamed the enum
struct TransactionListener<T: PoolTransaction> { | ||
sender: mpsc::Sender<NewTransactionEvent<T>>, | ||
/// Whether to include transactions that should not be propagated over the network. | ||
kind: PendingTransactionListenerKind, |
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.
yeah I like that, TransactionListenerKind
is more appropriate
fn new_pending_pool_transactions_listener( | ||
&self, | ||
) -> NewSubpoolTransactionStream<Self::Transaction> { | ||
NewSubpoolTransactionStream::new(self.new_transactions_listener(), SubPool::Pending) | ||
NewSubpoolTransactionStream::new( | ||
self.new_transactions_listener_for(PendingTransactionListenerKind::PropagateOnly), | ||
SubPool::Pending, | ||
) |
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.
this is reasonable, and we don't really have a usecase for this anyway
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 4 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR reuses some of the work in #4050 to avoid including non-propagatable tx in the full tx stream. Closes #4351.