-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
op-batcher: handle EIP-7623 in DA switching calculation #13870
Conversation
2c9e948
to
a001f69
Compare
3aa39ac
to
daecee1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #13870 +/- ##
============================================
- Coverage 46.40% 0 -46.41%
============================================
Files 994 0 -994
Lines 83988 0 -83988
============================================
- Hits 38974 0 -38974
+ Misses 42067 0 -42067
+ Partials 2947 0 -2947
Flags with carried forward coverage won't be shown. Click here to find out more. |
Is it easy to get this from a geth config library someplace, where you can use the L1 chain id (fetched via It also may be possible to detect pectra using the L1 block header, you can use |
ee96a36
to
1e4de7b
Compare
Thanks for the tip! So far I went with your suggestion around EIP-7685. We are already fetching the header just before generating data to send to L1, so no new network requests or other configuration would be needed to detect Pectra activation with this approach 1e4de7b. |
(edited) |
@sebastianst I am wondering whether this e2e test optimism/op-e2e/system/da/eip4844_test.go Lines 252 to 253 in 38f9864
needs updating once we have the ability to activate Pectra on L1. It could be that the numbers are such that the test can cover pre and post Pectra behavior with no changes. Hard to tell before we have the actual Pectra support on L1. We do have a Pectra enabled kurtosis devnet, so perhaps we can point the test at that already? |
@geoknee yes we could use the same test to cover both, pre and post-Pectra L1. But then again, if we unit test the logic in the batcher that does the switching for both, pre and post-Pectra, I don't think that we need to have this e2e test cover both. The job of the e2e test then stays to confirm that the overall switching in the batcher works, which is fine to confirm for only one case IMO, and then the unit tests cover all scenarios. |
bf6edbd
to
bfdac6e
Compare
Did that here bfdac6e. Rabbit holed a bit on this test, but it should work now for both pre and post pectra. It starts the fee ratio at over 40 (asserts batcher using calldata) and reduces it to under 16 (asserts batcher using blobs). |
pass this down to the TxData method so it can make an informed decision on DA type
529b2e3
to
cedf566
Compare
cedf566
to
ab357b6
Compare
This reverts commit f626d8d.
Closes #13791
Updates the batcher's DA switching logic to understand the new calldata costs coming when Pectra activates on L1.
Infers the Pectra activation status from the block header already fetched by the batcher's publishing loop.
Also some refactoring and unit tests added.
TODO