Avoid unnecessary 100 gas cost in CALL & add missing write-event for CODEHASH #508
Conversation
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
| @@ -814,7 +814,7 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt | |||
| } | |||
| } | |||
|
|
|||
| if interpreter.evm.chainRules.IsEIP4762 && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
| if interpreter.evm.chainRules.IsEIP4762 && !transfersValue && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
There was a problem hiding this comment.
This is something that surfaced when I was helping Luis with some work on Besu. I've chatted about this with some people, but re-explaining here for completeness.
Imagine we have a value-bearing CALL (it can be for an existing address, so nothing weird). In this case, we'll execute L811 first which is adding the "value transfer" part to the witness -- this includes doing a write-event on the target BASIC_DATA.
Then in L817 we were (i.e: before this PR) doing the usuasl *CALL touch on BASIC_DATA for the target. In this case, we'd be charging 100 of gas... since the "value transfer" touching we did before already added BASIC_DATA to the witness -- so a "warm charge" is done.
This looks wrong? This extra 100 of gas doesn't make much sense, in the sense that could be easily removed (as I do in this PR).
There was a problem hiding this comment.
Yes, that's because we consider a write event to be a read event as well, so it should be reasonable to assume that the client has the read value in the runtime. If we already read then we don't need to do it again.
If I'm not mistaken you could have swapped this if check with the transfer one too, being that you add the warm charges before anything else and then check if you need to write.
There was a problem hiding this comment.
hmmm yeah we should at least merge the two checks for eip4762 into a single if, and have an else to call chargeCallVariantEIP4762.
And adding a comment would be nice, so that we remember it in 2 weeks: just explain why we don't charge the first 100 in this case.
There was a problem hiding this comment.
BTW, I think there is a bug here:what if we call the same address with values multiple times? the 2nd time, TouchAndChargeValueTransfer should charge 100 gas. I think the best way to do this would be to change TouchAndChargeValueTransfer to return 100 if nothing gets added to the witness.
There was a problem hiding this comment.
Agree! Apparently this is the only place we use TouchAndChargeValueTransfer so looks safe to solve it that way. I'll make the change.
core/vm/evm.go
Outdated
| if !evm.StateDB.Exist(addr) { | ||
| if !isPrecompile && evm.chainRules.IsEIP4762 { | ||
| // add proof of absence to witness | ||
| if !isPrecompile && evm.chainRules.IsEIP4762 && value.Sign() != 0 { | ||
| gc := gasConsumer{availableGas: gas} | ||
| ok := evm.Accesses.TouchFullAccount(addr.Bytes(), false, gc.consumeGas) | ||
| ok := evm.Accesses.TouchCodeHash(addr.Bytes(), true, gc.consumeGas) |
There was a problem hiding this comment.
In a CALL targeting an account that doesn't exist, we were doing this read-event on TouchFullAccount. This looks like it's missing charging the write we'll do on the created account CODE_HASH=emptyCodeHash.
But my code change is apparently a bit more weird, so let me explain.
There're two ways in which we can create an account via a call:
- We're processing a value-bearing tx targeting an account that doesn't exist. In this case, we already added the target ("for free") in the witness. So this line will be a "noop" reg adding things to the witness and not chargin any extra gas.
- We're coming from a
CALLinstruction. In this case, we have a value-bearingCALLwhich means thegasCallalready write-touched theBASIC_DATA. So, at this point the only missing thing seems to be write-touching theCODE_HASH, thus why I removed theFullAccounttoCodeHashin the touching.
Note also I added value.Sign() != 0 in the if since we only want to do this write-event on value-bearing contexts. If this "call" isn't value bearing, I don't see why we ned to do any new touching if !Exist(addr) since the gasCall already added everything needed in the witness to check for account existance check in stateless clients (if we agree stateless client doesn't need checking for codeHash for that).
This is also why the deleted line L204 I believe was incorrect -- nothing in this place of hte code is about adding "proof of absence" .. this is indirectly done in the gasCall.
| @@ -814,7 +814,7 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt | |||
| } | |||
| } | |||
|
|
|||
| if interpreter.evm.chainRules.IsEIP4762 && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
| if interpreter.evm.chainRules.IsEIP4762 && !transfersValue && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
There was a problem hiding this comment.
hmmm yeah we should at least merge the two checks for eip4762 into a single if, and have an else to call chargeCallVariantEIP4762.
And adding a comment would be nice, so that we remember it in 2 weeks: just explain why we don't charge the first 100 in this case.
| @@ -814,7 +814,7 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt | |||
| } | |||
| } | |||
|
|
|||
| if interpreter.evm.chainRules.IsEIP4762 && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
| if interpreter.evm.chainRules.IsEIP4762 && !transfersValue && !chargeCallVariantEIP4762(interpreter.evm, scope) { | |||
There was a problem hiding this comment.
BTW, I think there is a bug here:what if we call the same address with values multiple times? the 2nd time, TouchAndChargeValueTransfer should charge 100 gas. I think the best way to do this would be to change TouchAndChargeValueTransfer to return 100 if nothing gets added to the witness.
Signed-off-by: Ignacio Hagopian <[email protected]>
Co-authored-by: Guillaume Ballet <[email protected]>
No description provided.