-
Notifications
You must be signed in to change notification settings - Fork 750
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
evm: add blobgasfee eip 7516 #3035
Changes from 2 commits
f208388
8a25c22
2f8e578
bcfedc8
1e84dde
7665573
7aac40c
f8e3152
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 | ||||
---|---|---|---|---|---|---|
|
@@ -471,4 +471,17 @@ export const EIPs: EIPsDict = { | |||||
minimumHardfork: Hardfork.London, | ||||||
requiredEIPs: [], | ||||||
}, | ||||||
7516: { | ||||||
comment: 'BLOBBASEFEE opcode', | ||||||
url: 'https://eips.ethereum.org/EIPS/eip-7516', | ||||||
status: Status.Draft, | ||||||
minimumHardfork: Hardfork.London, | ||||||
requiredEIPs: [4844], | ||||||
gasPrices: { | ||||||
blobbasefee: { | ||||||
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.
Suggested change
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. actually the opcode gas fee lookup in evm is: const baseFee = Number(common.param('gasPrices', opcodeBuilder[key].name.toLowerCase())) so it has to be all lowercase i think 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. You're right. I loked across the 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. @acolytec3 Note that the line which @g11tech references to is in 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. Won't die on this hill but in most cases we do have the camel case and I do somewhat like it for readability, but it is also true that this is a lot more inconsistent than I anticipated when looking through Common naming. Atm we can't update anyhow since this would be a breaking change. So for here I guess we can keep both ways. |
||||||
v: 2, | ||||||
d: 'Gas cost of the BLOBBASEFEE opcode', | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -760,6 +760,18 @@ | |
return baseFee | ||
} | ||
|
||
/** | ||
* Returns the Blob Base Fee of the block as proposed in [EIP-7516](https;//eips.etheruem.org/EIPS/eip-7516) | ||
*/ | ||
getBlobBaseFee(): bigint { | ||
const blobBaseFee = this._env.block.header.getBlobGasPrice() | ||
if (blobBaseFee === undefined) { | ||
// Sanity check | ||
throw new Error('Block has no Blob Base Fee') | ||
} | ||
return blobBaseFee | ||
} | ||
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. Not so deep in the blob pricing topic: so is "blobBaseFee === blobGasPrice" here? If so: I think we then should consider still renaming all 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. yes, the confusion is real- here is an actual discussion happening on it lol: https://discord.com/channels/595666850260713488/745077610685661265/1153600913932554270 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. Guess we might want to wait with merging here until this is settled? 🤔 Since this is the very last question on ACD channel and still un-answered? |
||
|
||
/** | ||
* Returns the chain ID for current chain. Introduced for the | ||
* CHAINID opcode proposed in [EIP-1344](https://eips.ethereum.org/EIPS/eip-1344). | ||
|
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.
London is too low, should be Shanghai
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.
updated min hf to
Cancun
as blobs will be comingCancun
onwardsThere 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, no, that's always the same discussion: the definition of this is "the minimum HF where this EIP (+ eventually other EIPs which are necessary for this one, so this would be 4844, but this part is covered by the EIP dependency setting) can be run in isolation". So this would be Shanghai.
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.
updated to Paris post discussion to confirm with
4844
settings 👍