-
Notifications
You must be signed in to change notification settings - Fork 20k
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
cmd, core, params, trie: add verkle access witness gas charging #29338
cmd, core, params, trie: add verkle access witness gas charging #29338
Conversation
8373191
to
d4019ed
Compare
core/state_processor.go
Outdated
@@ -78,7 +78,7 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg | |||
signer = types.MakeSigner(p.config, header.Number, header.Time) | |||
) | |||
if beaconRoot := block.BeaconRoot(); beaconRoot != nil { | |||
ProcessBeaconBlockRoot(*beaconRoot, vmenv, statedb) | |||
ProcessBeaconBlockRoot(*beaconRoot, vmenv, statedb, header.Number, header.Time) |
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.
vmenv actually has an unexported rules
field that I would like to be able to access instead of having to pass extra headers. Would that be an option?
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.
Yeah could either make the chainRules
accessible, or use a vmenv.ChainConfig().Rules(header.Number, header.Time)
new instance
core/state/access_witness.go
Outdated
for i := utils.VersionLeafKey; i <= utils.CodeSizeLeafKey; i++ { | ||
gas += aw.touchAddressAndChargeGas(addr, zeroTreeIndex, byte(i), isWrite) | ||
} |
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.
This little thing iterates form VersionLeafKey
to CodeSizeLeafKey
, it's not apparent to me why this does so.In TouchAndChageMessageCall
, we only use those two, not the things in between. Please add some comments
consensus/beacon/consensus.go
Outdated
@@ -360,6 +360,10 @@ func (beacon *Beacon) Finalize(chain consensus.ChainHeaderReader, header *types. | |||
amount := new(uint256.Int).SetUint64(w.Amount) | |||
amount = amount.Mul(amount, uint256.NewInt(params.GWei)) | |||
state.AddBalance(w.Address, amount, tracing.BalanceIncreaseWithdrawal) | |||
|
|||
if chain.Config().IsEIP4762(header.Number, header.Time) { | |||
state.Witness().TouchFullAccount(w.Address[:], true) |
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.
WHy, nonce is not needed here, it's just a balance increase?
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.
Have the same feeling.
As account fields are stored in different position in leafNode. The state access granularity is actually a field. It brings lots of complexity for gas metering.
Nothing to against the implementation, just want to mention that gas metering could be wrong very easy.
core/state/access_witness.go
Outdated
return gas | ||
} | ||
|
||
func (aw *AccessWitness) TouchTxOriginAndComputeGas(originAddr []byte) uint64 { |
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.
All of these implicitly ComputeGas
, no? But some have it in the name and some don't, so just drop it imo
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.
indeed some don't, I changed their names and the API to reflect that.
core/vm/instructions.go
Outdated
@@ -382,8 +390,23 @@ func opExtCodeCopy(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) | |||
uint64CodeOffset = math.MaxUint64 | |||
} | |||
addr := common.Address(a.Bytes20()) | |||
codeCopy := getData(interpreter.evm.StateDB.GetCode(addr), uint64CodeOffset, length.Uint64()) | |||
scope.Memory.Set(memOffset.Uint64(), length.Uint64(), codeCopy) | |||
if interpreter.evm.chainRules.IsEIP4762 { |
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.
I'd prefer if you make opExtCodeCopyVerkle
and put in eips
. That way we know that the verkle-things do not interfere with pre-verkle, not even adding an extra if-clause check. Same for other changed ops in this file
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.
Ping
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.
it's not great, because if any operation is changed between the time I copy the code and the time the Osaka fork is activated, there is a regression risk. I would rather try to move it to a dynamic gas function if I can.
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.
and who knows how these functions, especially the introspecting ones, are going to change when/if EOF is added to Prague and the spec finalized.
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.
right, I guess I could just call the previous handler and just add the extra gas costs, to avoid regressions.
core/vm/evm.go
Outdated
} else { | ||
err = ErrCodeStoreOutOfGas | ||
// Contract creation completed, touch the missing fields in the contract | ||
if !contract.UseGas(evm.Accesses.TouchFullAccount(address.Bytes()[:], true), evm.Config.Tracer, tracing.GasChangeUnspecified) { |
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.
Probably needs a new GasChange reason here
consensus/beacon/consensus.go
Outdated
@@ -360,6 +360,10 @@ func (beacon *Beacon) Finalize(chain consensus.ChainHeaderReader, header *types. | |||
amount := new(uint256.Int).SetUint64(w.Amount) | |||
amount = amount.Mul(amount, uint256.NewInt(params.GWei)) | |||
state.AddBalance(w.Address, amount, tracing.BalanceIncreaseWithdrawal) | |||
|
|||
if chain.Config().IsEIP4762(header.Number, header.Time) { | |||
state.Witness().TouchFullAccount(w.Address[:], true) |
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.
Have the same feeling.
As account fields are stored in different position in leafNode. The state access granularity is actually a field. It brings lots of complexity for gas metering.
Nothing to against the implementation, just want to mention that gas metering could be wrong very easy.
core/chain_makers.go
Outdated
@@ -102,7 +102,7 @@ func (b *BlockGen) SetParentBeaconRoot(root common.Hash) { | |||
blockContext = NewEVMBlockContext(b.header, b.cm, &b.header.Coinbase) | |||
vmenv = vm.NewEVM(blockContext, vm.TxContext{}, b.statedb, b.cm.config, vm.Config{}) | |||
) | |||
ProcessBeaconBlockRoot(root, vmenv, b.statedb) | |||
ProcessBeaconBlockRoot(root, vmenv, b.statedb, b.Number(), b.Timestamp()) |
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.
Please replace it with github.com/ethereum/go-verkle
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.
not sure what you mean here, this is what is already happening line 35? 🤔
core/state_processor.go
Outdated
statedb.AddAddressToAccessList(params.BeaconRootsAddress) | ||
txctx := NewEVMTxContext(msg) | ||
vmenv.Reset(txctx, statedb) | ||
if vmenv.ChainConfig().Rules(num, true, time).IsEIP2929 { |
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.
Shouldn't it be IsEIP4762
?
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.
nope, access lists are a concept of 2929 that is deactivated with 4762
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.
Is there any particular reason why you are placing all these calls to AccessList
within isEIP2929
clauses?
I mean, you could just let the call happen, couldn't you?
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.
I could, but it's not supposed to happen (even if it has no obvious impact afaict)
core/state_transition.go
Outdated
|
||
// add the coinbase to the witness iff the fee is greater than 0 | ||
if rules.IsEIP4762 && fee.Sign() != 0 { | ||
st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true) |
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.
Shouldn't we only mutate the balance field?
2044918
to
7de652d
Compare
core/state_processor.go
Outdated
@@ -185,7 +190,7 @@ func ApplyTransaction(config *params.ChainConfig, bc ChainContext, author *commo | |||
|
|||
// ProcessBeaconBlockRoot applies the EIP-4788 system call to the beacon block root | |||
// contract. This method is exported to be used in tests. | |||
func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *state.StateDB) { | |||
func ProcessBeaconBlockRoot(beaconRoot common.Hash, vmenv *vm.EVM, statedb *state.StateDB, num *big.Int, time uint64) { |
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.
And this could just take the params.Rules
, couldn't it?
core/state_transition.go
Outdated
if msg.To != nil { | ||
st.evm.Accesses.AddTxDestination(targetAddr.Bytes(), msg.Value.Sign() != 0) |
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.
Why is this needed? We're going to Call
into that address, shouldn't that add it later on?
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.
because when it comes to witnesses, these costs are paid for the 21000 gas. All other calls are paid for. If this is moved to the Call
, I have to add an if
somewhere to distinguish between the top-level call, and any other, nested call.
Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>
Co-authored-by: Martin HS <martin@swende.se>
788d16c
to
d4f2768
Compare
Co-authored-by: Gary Rong <garyrong0905@gmail.com>
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.
One thing that bothers me a bit, with how the access events are designed: you do e.g. BalanceGas
, which does two things:
- Adds it to the access events
- Returns how much it cost
This means that the evm cannot first check if the cost is covered, and then proceed (or not). As it is, the effective gas cost is the "available gas", not the cost.
This is problematic in the following case.
1.Y
: Call contract X
with near-zero gas
2. X
: load slot 1
3. X
: The load
operation fails because insufficient gas for adding to witness
4.Y
: Call contract X
with near-zero gas
5. X
: load slot 2
6. X
: The load
operation fails because insufficient gas for adding to witness
7. repeat
The end result is that we've added N
slots to the witness, bloating it, at the cost of ~100
(the cost of a call to a warm address, plus a little more). Whereas the 'true' cost would have been something like N x ( (WitnessBranchReadCost @ 1900) + (WitnessChunkReadCost @ 200))
Now, the same is true for a few other ops, notably the access lists. However, the access lists are journalled, which makes this attack moot. This can be fixed, if the design is changed so that either
- The methods to "add and return cost" takes a
availableGas
parameter, and aborts if the cost is higher than that - The methods to "add and return cost" is split into two:
return cost
andadd
, where the first is pure, the second just executes the change without any checks.
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.
All in all, I'm fairly confident that this does not break existing code. Did you run it on any of the benchmarkers in prod at all? Or somewhere else.. ?
createDataGas := uint64(len(ret)) * params.CreateDataGas | ||
if contract.UseGas(createDataGas, evm.Config.Tracer, tracing.GasChangeCallCodeStorage) { | ||
evm.StateDB.SetCode(address, ret) | ||
if !evm.chainRules.IsEIP4762 { |
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.
IMO we should rename IsEIP4762
to IsVerkle
, to make things easier for a casual reader
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.
I could, but here's the thing: EIP4762 and verkle are two separate things, eip4762 pertains to statelessness while verkle is a specific implementation of statelessness (i.e. if for whatever reason we switch to another state tree later, eip4762 will still apply).
Co-authored-by: Martin HS <martin@swende.se>
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.
sgtm.
I think it should be safe to merge without affecting existing non-verkle part logic.
Re the EIP itself, it might need to be iterated a few rounds, but can be done in following prs.
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.
Seems sufficiently ok :)
|
} | ||
paddedCodeCopy, copyOffset, nonPaddedCopyLength := getDataAndAdjustedBounds(code, uint64CodeOffset, length.Uint64()) | ||
statelessGas := interpreter.evm.AccessEvents.CodeChunksRangeGas(addr, copyOffset, nonPaddedCopyLength, uint64(len(contract.Code)), false) | ||
if !scope.Contract.UseGas(statelessGas, interpreter.evm.Config.Tracer, tracing.GasChangeUnspecified) { |
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.
I'm late to this but there are some remainingtracing.GasChangeUnspecified
. Can you please assign a reason to them?
…reum#29338) Implements some of the changes required to charge and do gas accounting in verkle testnet.
…reum#29338) Implements some of the changes required to charge and do gas accounting in verkle testnet.
This is a rewrite of #29234 that uses the simplified approach suggested by @holiman