Skip to content

Use correct DA transaction compression #61

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

SozinM
Copy link
Collaborator

@SozinM SozinM commented May 21, 2025

πŸ“ Summary

Supersedes #7 and #8
Added some extra integration tests for txpool

πŸ’‘ Motivation and Context


βœ… I have completed the following steps:

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

@SozinM SozinM requested review from ferranbt and avalonche as code owners May 21, 2025 11:15
@SozinM
Copy link
Collaborator Author

SozinM commented May 21, 2025

@danyalprout please review

@SozinM SozinM requested a review from danyalprout May 21, 2025 14:04
@danyalprout
Copy link
Collaborator

I'm not sure this will work as I don't think you're incrementing cumulative_da_bytes_used when a transaction is included in a block, so this check wont be correct.

IMO it maybe worth adding a function to track this/gas used (for example).

@SozinM
Copy link
Collaborator Author

SozinM commented May 22, 2025

Oh, I maybe missed it :)
Will recheck

@SozinM
Copy link
Collaborator Author

SozinM commented May 22, 2025

@danyalprout fixed

@SozinM SozinM force-pushed the msozin/use-tx-da-compression branch 4 times, most recently from 4cb410e to f9ec955 Compare May 22, 2025 12:58
@@ -155,6 +167,16 @@ impl Service for OpRbuilderConfig {
.arg(flashbots_block_time.to_string());
}

if let Some(max_da_tx_size) = self.max_da_tx_size {
cmd.arg("--builder.max-da-tx-size")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a flag of the builder? I though it is the batcher the one that updates these values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question
In reth they have rpc call in miner namespace, should we implement the same πŸ€”
Seems like I need to dig more into DA

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll need to be on the RPC, as it changes dynamically when the batcher is congested.

If you ensure the DA Config is shared between the payload builder and the OPAddOnsBuilder it should work (ref) out of the box.

We're running the two closed PR's on Base Sepolia (and briefly Base Mainnet) last week and it successfully was able to throttle on both networks.

@SozinM SozinM force-pushed the msozin/use-tx-da-compression branch from be4d330 to 886a7de Compare May 23, 2025 06:14
@SozinM
Copy link
Collaborator Author

SozinM commented May 23, 2025

@ferranbt @danyalprout
Fixed usage of DA config and updated test to use miner_setMaxDASize

Fix DA config, set it via miner
Extend tests

Add op-alloy-flz dependency and update related configurations

- Added `op-alloy-flz` as a dependency in `Cargo.toml` and `Cargo.lock`.
- Configured `op-alloy-flz` to be part of the workspace in `op-rbuilder`'s `Cargo.toml`.
- Updated the `payload_builder_vanilla.rs` to utilize `op-alloy-flz` for transaction size estimation.
- Enhanced test framework to include new data availability tests ensuring block size limits are respected.

Add max data availability transaction and block size configuration

- Introduced `max_da_tx_size` and `max_da_block_size` fields in `TestHarnessBuilder` and `OpRbuilderConfig`.
- Added builder methods `with_max_da_tx_size` and `with_max_da_block_size` for setting these values.
- Implemented a new test to ensure transaction size limits are respected in data availability scenarios.
- Updated test module to include the new data availability test.

Add cumulative_da_bytes_used accur

Add da config

Use correct DA transaction compression
@SozinM SozinM force-pushed the msozin/use-tx-da-compression branch from 886a7de to 4643203 Compare May 23, 2025 15:27
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.

4 participants