evm: add blobgasfee eip 7516 #3035
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
| minimumHardfork: Hardfork.London, | ||
| requiredEIPs: [4844], | ||
| gasPrices: { | ||
| blobbasefee: { |
There was a problem hiding this comment.
| blobbasefee: { | |
| blobBaseFee: { |
There was a problem hiding this comment.
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.
You're right. I loked across the eips.ts file and there's no consistency here. Just a silly thought but maybe a follow-up PR is to just make all of the gasPrices fields in common/src/eips lowercase so we can drop that toLowerCase call here. That's certainly a small performance gain for every single time we get the base fee.
There was a problem hiding this comment.
@acolytec3 Note that the line which @g11tech references to is in codes.ts of EVM (the opcode builder). The opcode builder expects all to be opcode names in common to be lowercase. Note: this method is only called if we get new opcodes (at EVM startup + fork transition) so this is not called each time for all opcodes if one is called.
There was a problem hiding this comment.
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.
thanks for the code comments, updated check casing response. should be good for review/merge now |
Co-authored-by: acolytec3 <[email protected]>
g11tech
force-pushed
the
opcode-blobgasfee
branch
from
698f23b to
8a25c22
Compare
packages/common/src/eips.ts
Outdated
| comment: 'BLOBBASEFEE opcode', | ||
| url: 'https://eips.ethereum.org/EIPS/eip-7516', | ||
| status: Status.Draft, | ||
| minimumHardfork: Hardfork.London, |
There was a problem hiding this comment.
London is too low, should be Shanghai
There was a problem hiding this comment.
updated min hf to Cancun as blobs will be coming Cancun onwards
There was a problem hiding this comment.
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.
updated to Paris post discussion to confirm with 4844 settings 👍
Co-authored-by: Scorbajio <[email protected]>
| throw new Error('Block has no Blob Base Fee') | ||
| } | ||
| return blobBaseFee | ||
| } |
There was a problem hiding this comment.
Not so deep in the blob pricing topic: so is "blobBaseFee === blobGasPrice" here?
If so: I think we then should consider still renaming all blobGasPrice related naming in Block to blobBaseFee? (we can still do since all 4844 functionality is still non-final)
There was a problem hiding this comment.
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.
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?
for devnet 9 (included in cancun) - ethereum/pm#857
EIP https://eips.ethereum.org/EIPS/eip-7516