Skip to content
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

core: implement blobfee opcode #28098

Merged
merged 7 commits into from
Oct 2, 2023

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Sep 12, 2023

Implements the https://eips.ethereum.org/EIPS/eip-7516 opcode for cancun

core/vm/eips.go Outdated Show resolved Hide resolved
core/evm.go Outdated
@@ -64,6 +69,7 @@ func NewEVMBlockContext(header *types.Header, chain ChainContext, author *common
Time: header.Time,
Difficulty: new(big.Int).Set(header.Difficulty),
BaseFee: baseFee,
BlobFee: blobFee,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of places that doesn't use this constructor. (Maybe we ought to make them use a constructor, becuause it's very easy to miss updating one of them)
Screenshot from 2023-09-14 09-51-50

46 places where we create a vm.BlockContext struct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that most of these cases are in tests where we only set one or two of the fields. We can't really update them to use the constructor since that will need a header to populate the fields. We could theoretically create a constructor that will set every field, but that will result in a lot of NewBlockContext(nil, nil, .-.. nil). Also we pass the struct by value very often so most of the initializatios are actually BlockCOntext{}

core/vm/opcodes.go Outdated Show resolved Hide resolved
core/vm/eips.go Outdated Show resolved Hide resolved
internal/ethapi/api.go Outdated Show resolved Hide resolved
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the ExcessBlobGas from the vm context, its redundant with the fee

if vmContext.ExcessBlobGas != nil {
execRs.CurrentExcessBlobGas = (*math.HexOrDecimal64)(vmContext.ExcessBlobGas)
if vmContext.BlobBaseFee != nil {
// TODO (MariusVanDerWijden): I have no idea why we have this field at all.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea here is that if you provide parentExcessBlobGas and parentBlobGasUsed t8n will automatically calculate that block's excessBlobGas. In these instances you would probably want the calculated value output so that you can pipe it in for the next transition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, sry

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think that this is really needed

@holiman holiman added the cancun label Oct 2, 2023
@@ -138,7 +137,7 @@ func applyTransaction(msg *Message, config *params.ChainConfig, gp *GasPool, sta

if tx.Type() == types.BlobTxType {
receipt.BlobGasUsed = uint64(len(tx.BlobHashes()) * params.BlobTxBlobGasPerBlob)
receipt.BlobGasPrice = eip4844.CalcBlobFee(*evm.Context.ExcessBlobGas)
receipt.BlobGasPrice = evm.Context.BlobBaseFee
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come that the "gasprice" is calculated as the basefee? I mean, the base is only one part of the price, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no blob priority fee yet, so it's just base fee!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was ExcessBlobGas exactly before?
And the concept of ExcessBlobGas no longer exists?

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@holiman holiman added this to the 1.13.3 milestone Oct 2, 2023
@holiman holiman merged commit c39cbc1 into ethereum:master Oct 2, 2023
1 check passed
hyunchel pushed a commit to hyunchel/go-ethereum that referenced this pull request Oct 6, 2023
Implements "EIP-7516: BLOBBASEFEE opcode" for cancun, as per spec: https://eips.ethereum.org/EIPS/eip-7516
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
Implements "EIP-7516: BLOBBASEFEE opcode" for cancun, as per spec: https://eips.ethereum.org/EIPS/eip-7516
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 16, 2023
Implements "EIP-7516: BLOBBASEFEE opcode" for cancun, as per spec: https://eips.ethereum.org/EIPS/eip-7516
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Implements "EIP-7516: BLOBBASEFEE opcode" for cancun, as per spec: https://eips.ethereum.org/EIPS/eip-7516
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Feb 26, 2024
implement BLOBBASEFEE opcode (0x4a) (ethereum#28098)

 Conflicts:
  core/evm.go
  core/vm/evm.go
Both nitro and upstream added fields to BlockContextx. Using both.

 Additions:
  accounts/abi/bind/base.go
  eth/tracers/js/goja.go
common.Address no longer has the Hash() function that we sometimes used.
It now, however, has a Big() function.
Previous calls to Address.Hash().Big() were converted to .Big()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants