-
Notifications
You must be signed in to change notification settings - Fork 838
Add EIP-7594 PeerDAS Max Blob Limit for Txs #4138
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,10 +187,26 @@ export class Blob4844Tx implements TransactionInterface<typeof TransactionType.B | |
| } | ||
| } | ||
|
|
||
| // 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`) | ||
| const msg = Legacy.errorMsg( | ||
| this, | ||
| `tx can contain at most ${limitBlobsPerTx} blobs (maxBlobGasPerBlock/blobGasPerBlob)`, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error message does not look completely correct, because
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ) | ||
| throw EthereumJSErrorWithoutCode(msg) | ||
| } else if (this.blobVersionedHashes.length === 0) { | ||
| const msg = Legacy.errorMsg(this, `tx should contain at least one blob`) | ||
|
|
||
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.
Should't this be
if/else? Currently if 7594 is active we will run both the 7594 limit and the old limitThere 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah maybe my understanding was incorrect then, I assumed it would've been overwritten and not just superseded.