Skip to content

fix: use fastlz sizes for tracking da usage #7

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danyalprout
Copy link
Collaborator

📝 Summary

Migrated from rbuilder PR flashbots/rbuilder#615

This PR has a couple of changes:

  • Use the FastLZ size of transactions to verify per tx and per block DA limits. This matches the op-geth sequencer behaviour. See this issue for more details on how op-geth behaves.
  • Track cumulative da bytes used

💡 Motivation and Context

Without this change:

  • The DA limiting is based on the uncompressed transaction size, which will result in needlessly throttling very compressible transactions.
  • Throttling does not take into account the amount of bytes currently being used in the block

✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@SozinM
Copy link
Collaborator

SozinM commented May 12, 2025

I have copied implementation of DA limit tracking from reth, do they have this issue too?

@danyalprout
Copy link
Collaborator Author

@SozinM I believe Reth uses the fastlz size not the txn length (builder, txn, fastlz).

Whereas currently we use transaction.length() , which I think is the uncompressed size.

I'm not familiar with Reth, so my understanding of their implementation maybe wrong

@danyalprout danyalprout marked this pull request as ready for review May 12, 2025 17:27
@SozinM
Copy link
Collaborator

SozinM commented May 13, 2025

IMO it's better to keep code closer to the Reth -> we could copy their approach on fastlz
Reason for this is it would be easier for us to rebase other reth changes later

@SozinM
Copy link
Collaborator

SozinM commented May 13, 2025

We could just use chages from this PR https://github.com/paradigmxyz/reth/pull/16153/files from the file builder.rs
We could use already implemented trait and calculate DA size

@avalonche avalonche self-requested a review May 13, 2025 09:25
Copy link
Collaborator

@SozinM SozinM left a comment

Choose a reason for hiding this comment

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

We need to import reth_optimism_txpool::estimated_da_size::DataAvailabilitySized so we could call tx.estimated_da_size()

@danyalprout
Copy link
Collaborator Author

@SozinM nice find, looks like reth_optimism_txpool::estimated_da_size::DataAvailabilitySized isn't currently in a released version. Would you rather we wait for a release or update the deps to use a specific commit?

@danyalprout danyalprout force-pushed the da-settings-fastlz branch from 136f7ed to df07f77 Compare May 13, 2025 22:15
@SozinM
Copy link
Collaborator

SozinM commented May 14, 2025

I think you could rebase on this PR, i hope we will merge it soon #6, i solved all conflicts for the newer version
(Or if needed you could bump reth version even higher in you PR)

@danyalprout
Copy link
Collaborator Author

Sure, will rebase this on main once your PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants