core: implement EIP-4788 BeaconRoot precompile #27289

MariusVanDerWijden · 2023-05-17T09:51:42Z

EIP 4788 : Beacon block root in the EVM

WIP implementation of EIP-4788 for discussion on the eip
cc @ralexstokes

consensus/misc/eip4788.go

core/vm/instructions.go

consensus/misc/eip4788.go

core/vm/contracts.go

consensus/misc/eip4788.go

core/vm/eips.go

params/protocol_params.go

holiman · 2023-07-07T11:00:30Z

core/types/block.go

+
+	// BeaconRoot was added by EIP-4788 and is ignored in legacy headers.
+	BeaconRoot *common.Hash `json:"beaconRoot" rlp:"optional"`


Adding it like this delegates the responsibility of "when to start storing data for EIP-4788" entirely to the CL layer. When/of they start including a beaconroot, we start storing it into the state.

IMO we should

only accept BeaconRoot after the fork time,

require the BeaconRoot after the fork time,

As for RLP, similarly, we need to be careful, and strict.

Is the order between BeaconRoot and DataGasUsed specified? Both are activated in the same fork...

I don't think its specified yet

it should be at the end of all the 4844 stuff; I was trying to specify this abstractly to avoid thrash from other EIPs but I can see how this is unclear so I made it explicit:

ethereum/EIPs#7297

core/state_processor.go

lightclient

Couple nits; really nice overall though.

params/protocol_params.go

lightclient · 2023-07-25T02:48:43Z

consensus/misc/eip4788.go

+	state.SetState(historicalStorageAddress, rootKey, root)
+}
+
+func calcBeaconRootIndices(header *types.Header) (timeKey, time, rootKey, root common.Hash) {


This function is kinda superfluous. It's just doing a bunch of one-line conversions. I think it is actually more comprehensible all in the same function, otherwise takes a bit of time to realize almost not computation is happening here.

I think Martin proposed spinning this out, so it can be tested easier, I'll add some test cases, so it actually makes sense to move this

params/protocol_params.go

core/vm/contracts.go

holiman · 2023-07-27T12:31:36Z

consensus/misc/eip4788_test.go

+	"github.com/ethereum/go-ethereum/params"
+)
+
+func TestCalcBeaconRootIndices(t *testing.T) {


This is a good start. Mind if I move the actual test vectors of it out to a json file, so we can share it cross-client?

Sure would be nice to have some more test cases

holiman

LGTM

puma314 · 2023-07-28T23:41:31Z

@ralexstokes have you considered passing the entirety of the evm EVM object instead of just the stateDB StateReader object to the precompile? In particular, information in the EVM object might be useful for enabling more expressive future precompiles (and also more expressive custom precompile implementations on custom op-stack chains as another example), including things like more complex gas cost estimation (based on current gas remaining), ability to access values in the EVM config, surrounding call context, etc.

For some additional context, I've been working on implementing a proof of concept for remote static call for Optimism (ethereum-optimism/ecosystem-contributions#76). This involves implementing a precompile in op-geth that would issue a network call to an L1 archive node to get information about L1 state and return that within the contract execution on L2.

It would be useful to be able to access evm.config information in the remotestaticcall precompile to know what the L1 RPC URL is and also to know about the current amount of gas in the call to understand how much gas to pass to eth_call.

There might be very good reasons to not do this, but just curious if it would make sense.

karalabe · 2023-07-31T05:26:13Z

core/vm/contracts.go

@@ -121,6 +125,19 @@ var PrecompiledContractsBLS = map[common.Address]PrecompiledContract{
 	common.BytesToAddress([]byte{18}): &bls12381MapG2{},
 }

+var PrecompiledContracts4788 = map[common.Address]PrecompiledContract{


Why this instead of in Cancun? Now we have cancun with KZG but no beaconnroot, and we have this with beaconroot but no KZG

karalabe · 2023-07-31T05:26:55Z

core/vm/contracts.go

+	common.BytesToAddress([]byte{7}): &bn256ScalarMulIstanbul{},
+	common.BytesToAddress([]byte{8}): &bn256PairingIstanbul{},
+	common.BytesToAddress([]byte{9}): &blake2F{},
+	params.BeaconRootsStorageAddress: &beaconRoot{},


Given that all other contracts are hard coded with value addresses, I'd rather stick to that vs adding a param here

This one is slightly different though as it needs to be accessed in both consensus/misc/eip4788.go and in core/vm/contracts.go. It's weird it is the only non-hardcoded one here though, but is it better to hardcode in both places?

It should be consistent. If there's a need to move the beaconRoot into a param, then move all of them imo.

karalabe · 2023-07-31T05:28:07Z

params/protocol_params.go

@@ -160,6 +164,9 @@ const (
 	RefundQuotient        uint64 = 2
 	RefundQuotientEIP3529 uint64 = 5

+	HistoricalRootsModulus  uint64 = 98304 // Limits how many historical roots are stored by EIP-4788


I'd recommend starting to prefix constants belonging to the same EIP/fork/whatnot since we already have tons of params and it's very hard to see what goes where. I.e. BeaconRootHistoricalRootsModulus

karalabe · 2023-07-31T05:28:56Z

core/vm/contracts.go

@@ -34,12 +34,16 @@ import (
 	"golang.org/x/crypto/ripemd160"
 )

+type StateReader interface {


karalabe · 2023-07-31T05:29:37Z

core/vm/contracts.go

-	RequiredGas(input []byte) uint64  // RequiredPrice calculates the contract gas use
-	Run(input []byte) ([]byte, error) // Run runs the precompiled contract
+	RequiredGas(input []byte) uint64                       // RequiredPrice calculates the contract gas use
+	Run(statedb StateReader, input []byte) ([]byte, error) // Run runs the precompiled contract


Nitpick, we have statedb and stateDB naming in this single PR. Can we just have state instead?

holiman · 2023-07-31T15:14:18Z

The 4788 EIP needs to be amended, currently geth is doing something which is correct, but definitely not intended behaviour.

Since the beaconroot trie account is empty, by the consensus-definition of empty,

An account is empty when it has no code, zero nonce and zero balance

It is touch-deleted at the end of every block. So it works "fine" during a block, but then it becomes wiped at the end of the block. Discussion is happening regarding how to get around this behaviour. One example candidate solution is to amend the eip so that:

When updating the storage for the precompile, set nonce=1 iff nonce==0.

I see no point in merging this EIP until this has been resolved.

holiman · 2023-07-31T17:32:48Z

I have now updated this PR to set the nonce to 1, if it is zero. That change makes it not according to spec, but I made it so we can experiment with generating tests using this PR.

lightclient · 2023-07-31T21:40:15Z

core/state_processor.go

@@ -71,6 +71,15 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
 	if p.config.DAOForkSupport && p.config.DAOForkBlock != nil && p.config.DAOForkBlock.Cmp(block.Number()) == 0 {
 		misc.ApplyDAOHardFork(statedb)
 	}
+	if p.config.IsCancun(blockNumber, block.Time()) {


should this be in Prepare in the consensus interface?

Please clarify, @lightclient, I don't understand what you are suggesting

holiman · 2023-08-01T13:41:09Z

@puma314

have you considered passing the entirety of the evm EVM object instead of just the stateDB StateReader object to the precompile?

The original PR did that, but I suggested passing in a smaller interface. For downstream projects, it should be a pretty small change to just take the method(s) from EVM that you need, and add them to that interface. The big bulky change of passing the thing into all precompiles is already done via this PR. Hope that's good enough for you

holiman · 2023-08-02T17:15:47Z

core/vm/contracts.go

+func (c *beaconRoot) Run(stateDB StateReader, input []byte) ([]byte, error) {
+	if len(input) != common.HashLength {
+		return nil, errors.New("invalid input length")
+	}


This is (subtly wrong). From the EIP:

If the input is more than 32 bytes, the precompile only takes the first 32 bytes of the input buffer and ignores the rest. If the input is less than 32 bytes, the precompile should revert.

cc @shemnon will this make the next testnet go boom? Or does besu contain the same error?

we can also update the EIP here, if that's simpler

I don't know if there is precedent for precompile input handling one way or the other...

Besu hasn't implemented it yet. @pinges is supposed to be implementing that.

This change implements "EIP 4788 : Beacon block root in the EVM". It implements version-2 of EPI-4788, main difference being that the contract is an actual contract rather than a precompile, as in #27289.

holiman · 2023-08-22T12:34:08Z

Superseded by #27849

…27849) This change implements "EIP 4788 : Beacon block root in the EVM". It implements version-2 of EPI-4788, main difference being that the contract is an actual contract rather than a precompile, as in ethereum#27289.

MariusVanDerWijden force-pushed the eip-4788-mod branch from 2662722 to 0c446f1 Compare May 17, 2023 09:56

ralexstokes reviewed May 24, 2023

View reviewed changes

consensus/misc/eip4788.go Outdated Show resolved Hide resolved

ralexstokes reviewed May 24, 2023

View reviewed changes

consensus/misc/eip4788.go Outdated Show resolved Hide resolved

MariusVanDerWijden added 7 commits June 29, 2023 10:18

core: implement modified version of EIP-4788

e851002

consensus/misc: add eip4788

6f11378

consensus/misc: header

d6d0d6d

core/vm: allow for stateful precompiles

10390cf

core/vm: implement 4788 as a stateful precompile

3d4ecc3

core, consensus/misc: update to new 4788 spec

f115bf6

core: fix rebase

2bd0102

MariusVanDerWijden force-pushed the eip-4788-mod branch from 53d13db to 2bd0102 Compare June 29, 2023 08:25

MariusVanDerWijden changed the title ~~core: implement modified version of EIP-4788~~ core: implement EIP-4788 BeaconRoot precompile Jun 29, 2023

core/vm: add activator

bd221a9

holiman added the cancun label Jul 6, 2023

holiman reviewed Jul 6, 2023

View reviewed changes

core/vm/instructions.go Outdated Show resolved Hide resolved

consensus/misc/eip4788.go Outdated Show resolved Hide resolved