Add EIP-7594 PeerDAS Max Blob Limit for Txs #4138
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Ok, this is failing in the VM block builder still, so at least within the context of the No imminent idea how this is related to the changes made here, but also need to stop for now. Happy to be picked up if someone has time! 🙂 |
| const msg = Legacy.errorMsg(this, `tx can contain at most ${limitBlobsPerTx} blobs`) | ||
| const msg = Legacy.errorMsg( | ||
| this, | ||
| `tx can contain at most ${limitBlobsPerTx} blobs (maxBlobGasPerBlock/blobGasPerBlob)`, |
There was a problem hiding this comment.
This error message does not look completely correct, because maxBlobGasPerBlock/blobGasPerBlob could be 9 (osaka config: target 6 but max 9), while tx limit is 6
There was a problem hiding this comment.
That's the "old way" of doing it, look at the code, this is just reflecting the implementation three lines above. So this will be superseded by the 6 from Osaka then, currently this code is the active one.
Let me know if my understanding is wrong somewhere here.
|
Ah ok, CI fails also (already mentioned by Holger above), will TAL when I got some more bandwidth 😄 |
|
Ok, so the VM test failures in the block builder were some side effects since I added (a bit on the sideline) new fork timestamps in the Nevertheless unrelated to here, so this is no blocker to move forward with this specific PR (still tests failing though at time of writing). |
|
Another geth genesis related side effect. One should really not do side edits altogether! 🤯 Anyhow, all passing now, ready for review! 🙂 |
| // EIP-7594 PeerDAS: Limit of 6 blobs per transaction | ||
| if (this.common.isActivatedEIP(7594)) { | ||
| const maxBlobsPerTx = this.common.param('maxBlobsPerTx') | ||
| if (this.blobVersionedHashes.length > maxBlobsPerTx) { | ||
| const msg = Legacy.errorMsg( | ||
| this, | ||
| `tx can contain at most ${maxBlobsPerTx} blobs (EIP-7594)`, | ||
| ) | ||
| throw EthereumJSErrorWithoutCode(msg) | ||
| } | ||
| } | ||
|
|
||
| // "Old" limit (superseded by EIP-7594 starting with Osaka) | ||
| const limitBlobsPerTx = | ||
| this.common.param('maxBlobGasPerBlock') / this.common.param('blobGasPerBlob') | ||
| if (this.blobVersionedHashes.length > limitBlobsPerTx) { | ||
| const msg = Legacy.errorMsg(this, `tx can contain at most ${limitBlobsPerTx} blobs`) |
There was a problem hiding this comment.
Should't this be if/else? Currently if 7594 is active we will run both the 7594 limit and the old limit
There was a problem hiding this comment.
I am not fully sure, I think theoretically this "old" limit would technically be still intact, it just is fully superseded by the other one "by the values". So I haven't read that this is dismissed, but if someone can prove with a link happy to add if/else. Practical relevance limited though I guess.
There was a problem hiding this comment.
Ah maybe my understanding was incorrect then, I assumed it would've been overwritten and not just superseded.
|
Will take this in, all adressed. |
This adds the PeerDAS blob per tx limit from here fixing the remaining failing Osaka state tests.
PR also fixes a false positive test for the former max blob per tx case (test cases tx blobs were at 3 and so the
catchblock was simply not reached and so test was passing for the wrong reason (we should rework the test setup here)).Ready for review/merge.