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/{.,state,vm},miner,ethr/tracers: implement 7709 #31015

Closed
wants to merge 1 commit into from

Conversation

gballet
Copy link
Member

@gballet gballet commented Jan 10, 2025

This is the implementation of 7709 as currently running on the verkle testnet. While this is not the final version, the final changes will be minor tweaks:

  • change HistoryServeWindow to 8191
  • use the history contract from 2935 instead of the system address.

Co-authored-by: Ignacio Hagopian <[email protected]>

Signed-off-by: Guillaume Ballet <[email protected]>
@rjl493456442
Copy link
Member

If I understand correctly, in EIP 7709, the main differences are:

- SLOAD gas costs (cold or warm) for the arg % HISTORY_SERVE_WINDOW slot.
- SLOAD after effects on the slot (warming the slot)
- SLOAD accesses added to execution witnesses if Verkle ([EIP-6800](https://eips.ethereum.org/EIPS/eip-6800) and [EIP-4762](https://eips.ethereum.org/EIPS/eip-4762)) is activated

However, only the third point is implemented in this PR right?

ringIndex := (evm.Context.BlockNumber.Uint64() - 1) % params.HistoryServeWindow
var key common.Hash
binary.BigEndian.PutUint64(key[24:], ringIndex)
evm.StateDB.SetState(params.SystemAddress, key, prevHash)
Copy link
Member

Choose a reason for hiding this comment

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

Why to use SystemAddress here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because that's what is used on the testnet, it's older than the current 2935 address.

Copy link
Member

Choose a reason for hiding this comment

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

So the devnet was launched even before the 2935 contract deployment?

binary.BigEndian.PutUint64(key[24:], ringIndex)
evm.StateDB.SetState(params.SystemAddress, key, prevHash)
evm.StateDB.AccessEvents().SlotGas(params.SystemAddress, key, true)
evm.StateDB.Finalise(false)
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if it's necessary.

If Verkle is enabled, SStore will be replaced with SStore4762, the slot gas charging and warming will be done automatically if we register the block hash via invoking the contract, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this doesn't actually invoke any contract, just does the ugly and sets the slot. Which necessitates also setting the access-event.

Copy link
Member

Choose a reason for hiding this comment

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

But why not invoking the contract? The gas charging, slot warming can all be done automatically in this way.

Copy link
Member Author

@gballet gballet Jan 10, 2025

Choose a reason for hiding this comment

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

so you might have missed the gist of conversations I had over this topic. There are several issues with invoking a contract:

  1. it's slower and uglier as you have to instantiate an evm each time, and copy/paste the code to do so all over the place
  2. it's more brittle, since it can revert. Also, what if there is a bug in the contract? Not as easy to fix as a bug in a client.
  3. it's not extensible, if you need to change the behavior, you need to deploy another contract
  4. you must filter the keys that bloat the witness a posteriori
  5. You need to keep a list of system contracts and check that list for each call, as they have a different behavior (see eip 4762)

Only 3 and 4 are relevant to this issue, but system contracts are generally a bad idea in my view.

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 conclusion was that, we would deprecate calling system contracts in verkle.

@gballet gballet added the verkle label Jan 10, 2025
@gballet
Copy link
Member Author

gballet commented Jan 10, 2025

If I understand correctly, in EIP 7709, the main differences are:

- SLOAD gas costs (cold or warm) for the arg % HISTORY_SERVE_WINDOW slot.
- SLOAD after effects on the slot (warming the slot)
- SLOAD accesses added to execution witnesses if Verkle ([EIP-6800](https://eips.ethereum.org/EIPS/eip-6800) and [EIP-4762](https://eips.ethereum.org/EIPS/eip-4762)) is activated

However, only the third point is implemented in this PR right?

nope, it does all three, which is why it calls SlotGas

@rjl493456442
Copy link
Member

rjl493456442 commented Jan 13, 2025

To summarize the situation:

  • Devnet7 was launched without the 2935 contract deployed, so historic block hashes are stored in the SystemAddress.
  • The Verkle team plans to deprecate the system contracts once Verkle is enabled and prefers not to set historic block hashes via a system call.

Although this introduces some technical debt, the change is necessary for joining the devnet7 due to the current storage location of historic block hashes.

Alternatively, we can implement this PR in this way. The slot will also be warmed and collected in the verkle accessEvent.

diff --git a/core/state_processor.go b/core/state_processor.go
index 2fca175630..14d8c4ef91 100644
--- a/core/state_processor.go
+++ b/core/state_processor.go
@@ -17,7 +17,6 @@
 package core
 
 import (
-	"encoding/binary"
 	"fmt"
 	"math/big"
 
@@ -243,15 +242,9 @@ func ProcessParentBlockHash(prevHash common.Hash, evm *vm.EVM, is7709 bool) {
 			defer tracer.OnSystemCallEnd()
 		}
 	}
+	historyStorageAddress := params.HistoryStorageAddress
 	if is7709 {
-		// currently 8192 for kautinen7, to be changed to 8 (and made a constant) for kaustinen8
-		ringIndex := (evm.Context.BlockNumber.Uint64() - 1) % params.HistoryServeWindow
-		var key common.Hash
-		binary.BigEndian.PutUint64(key[24:], ringIndex)
-		evm.StateDB.SetState(params.SystemAddress, key, prevHash)
-		evm.StateDB.AccessEvents().SlotGas(params.SystemAddress, key, true)
-		evm.StateDB.Finalise(false)
-		return
+		historyStorageAddress = params.SystemAddress
 	}
 	msg := &Message{
 		From:      params.SystemAddress,
@@ -259,7 +252,7 @@ func ProcessParentBlockHash(prevHash common.Hash, evm *vm.EVM, is7709 bool) {
 		GasPrice:  common.Big0,
 		GasFeeCap: common.Big0,
 		GasTipCap: common.Big0,
-		To:        &params.HistoryStorageAddress,
+		To:        &historyStorageAddress,
 		Data:      prevHash.Bytes(),
 	}
 	evm.SetTxContext(NewEVMTxContext(msg))

And my personal opinion is to not merge this PR. This change is useless once we have devnet8 (also the subsequent testnets and mainnet) with 2935 contract deployed and we must revert this change in order to join devnet8. I would prefer to keep it as a devnet7 branch only for development/testing purpose.

@rjl493456442
Copy link
Member

rjl493456442 commented Jan 13, 2025

Also, the main scope of 7709 is to change the behavior of BLOCKHASH opcode, rather than the historic block hash registration. The description of this PR is not accurate.

@holiman
Copy link
Contributor

holiman commented Jan 13, 2025

Alternatively, we can implement this PR in this way.

@rjl493456442 but that wouldn't work, since the code isn't deployed, would it?

@gballet
Copy link
Member Author

gballet commented Jan 15, 2025

it wouldn't work because, indeed, the contract isn't deployed, but also because it would add extra items to the witness. The announced goal of this PR is to be able to join the testnet, no inaccuracy there. We don't know yet if things will change for the next testnet, as it still needs to be proven that an efficient filtering system can be implemented.

Also, the main scope of 7709 is to change the behavior of BLOCKHASH opcode, rather than the historic block hash registration.

It also states that direct writes to the state have to be an option:

image

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.

3 participants