Skip to content
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

Deprecate best_transactions_with_base_fee #6394

Merged
merged 7 commits into from
Feb 5, 2024

Conversation

tcoratger
Copy link
Contributor

Should close #6382.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is a good start,

what's missing is blob fee handling:

after we check the directional swing of the base fee we also need to take the swing of the blob into account because a lower blobfee can unlock more eip4844 txs from the blob pool and a higher can prevent currently pending blobs in the pending pool

@mattsse mattsse added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Feb 5, 2024
@tcoratger
Copy link
Contributor Author

@mattsse As we will migrate to Cancun, why not using normal BestTransactionsAttributes initialization with get_blob_gasprice everywhere instead of base_fee initializer. If there is no blob, the option will just return a None right?

@tcoratger
Copy link
Contributor Author

this is a good start,

what's missing is blob fee handling:

after we check the directional swing of the base fee we also need to take the swing of the blob into account because a lower blobfee can unlock more eip4844 txs from the blob pool and a higher can prevent currently pending blobs in the pending pool

@mattsse We agree that these changes should be made in a separate PR in response to #6383 no?

@@ -421,7 +421,7 @@ where
&self,
base_fee: u64,
) -> Box<dyn BestTransactions<Item = Arc<ValidPoolTransaction<Self::Transaction>>>> {
self.pool.best_transactions_with_base_fee(base_fee)
self.pool.best_transactions_with_attributes(BestTransactionsAttributes::base_fee(base_fee))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse I just did an update, this is the only place where I use the base fee initializer. On the other places, I use an initialization taking into account the gas price of the blob and if there is no blob, then the Option will just return a None and it will have the same effect as the base fee initializer in my opinion.

Tell me what you think?

@mattsse
Copy link
Collaborator

mattsse commented Feb 5, 2024

We agree that these changes should be made in a separate PR in response to #6383 no?

right, that is appropriate, so we limit this pr to deprecating the function

@mattsse mattsse enabled auto-merge February 5, 2024 14:47
@mattsse mattsse disabled auto-merge February 5, 2024 15:41
@mattsse mattsse merged commit b9ade51 into paradigmxyz:main Feb 5, 2024
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tx-pool Related to the transaction mempool C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Transactionpool::best_transactions_with_base_fee
2 participants