-
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
feat: implement EIP-7702 span batch tests #14197
base: develop
Are you sure you want to change the base?
feat: implement EIP-7702 span batch tests #14197
Conversation
Generally want to think on the design here quick. This doesn't check for activation of isthmus when decoding which means it opens up the case that a 7702 tx could be included in a batch before isthmus activates on L2. We generally want to fail fast to keep the work done in the proof system low. In the case of a 7702 tx being included before isthmus in this implementation, i believe the EL should return an invalid block error and that should trigger the steady batch derivation of it being replaced with a deposit only block. This is all work that will need to be replicated in the proof. If we have validation in the batch decoder itself, then we would fail fast and just drop the batch as an invalid batch. I think this is less work in the proof. can i get a sanity check on this? cc @sebastianst @clabby |
Shouldn't EIP-7702 txs just be rejected from the tx pool validator pre-isthmus? Confused how it would even end up in a canonical L2 block. |
You dont need to build a block where all txs go thru the mempool. We have to defend against malicious actors |
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.
From the specs:
Span batches with transaction type 4 should only be accepted after Isthmus is enabled.
this line is important here. With this implementation change as-is, we currently just accept the span-batch if it contains a 7702 tx pre-isthmus. It's only during block-processing where it becomes invalid.
If there are any protocol difference between an invalid batch and an invalid block, then this becomes a chain divergence pre-isthmus:
- nodes with this logic will produce an invalid block
- nodes without this logic will produce an invalid batch
If the result is still the same (a deposit-only block) then that's fine. I think with Holocene we have that guarantee, but this should be double-checked ideally.
Edit:
seb:
So with Holocene, invalid batches are dropped, as is the remaining channel. It is not replaced by deposits-only batch. This only happens once attributes have been generated. So when the EL returns INVALID, the attributes are retried with deposit-only attribs.
So this is not a safe change, because of the difference between invalid batch and invalid block.
I think one solution might be to scan the batch-txs for 7702 type, after parsing the batch. This then keeps the spanbatch code simple, while conforming with the spec, and always resulting in the invalid-batch case on both nodes with the old pre-isthmus logic as well as nodes with this updated spanbatch code before isthmus. |
Summarizing from Discord, I agree with @protolambda that it makes most sense to check for forbidden SetCode transactions in each individual batch (Inside This also means that we don't need to add versioning to span batches, and the en/decoding implementation in this PR works fine. We just need to add this check to |
TODO: check that a singular batch containing a 7702 transaction before Pectra is enabled is dropped, but not other singular batches inside the span batch.
Ensure span batch containing Blocks 1-3 contain the normal tx, don't contain the 7702 tx before activation, and contain the 7702 tx after activation. |
Not sure this works though, IIUC. If batch 2 gets dropped, all future batches (no matter if singular or from the same span) are dropped as well (from the same channel). This test still makes sense, but then assert that only batch 1 is accepted. Another scenario is the positive one, so 2 blocks in a span batch, first block pre-Isthmus, type 2, second block post-Isthmus, type 4, and we assert that both are valid. |
store42Program := program.New().Sstore(0x42, 0x42) | ||
callBobProgram := program.New().Call(nil, dp.Addresses.Bob, 1, 0, 0, 0, 0) | ||
|
||
alloc := actionsHelpers.DefaultAlloc |
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.
TODO: don't overwrite default alloc
I added impl/test here to ensure that batches with SetCode txs are dropped before Isthmus: #14288 |
e17d250
to
64a4eda
Compare
Description
This PR creates two contracts and tests that a transaction that relies on authorization lists can be mined. Then, the span batches are posted to L1 and we ensure that the verifier can sync up with the authorization list tx.
I tested replacing the authorization list with an empty array on serialization of the span batch and the test errored because the tx couldn't be included on the verifier. This shows that the test validates that the span batch spec properly serializes and deserializes set code transactions.
Also included are a few fixes to ensure set code txs can be properly serialized/deserialized.
This is dependent on ethereum/go-ethereum#31073 from geth which allows 7702 txs in the mempool, but this can be tested with: ethereum-optimism/op-geth#479
Fixes #14156