-
Notifications
You must be signed in to change notification settings - Fork 135
Instrument the CALL, CALLCODE, DELEGATECALL and STATICCALL opcodes #486
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
base: cbp-feature
Are you sure you want to change the base?
Conversation
gasCall
with multi gase66c1e6
to
3e5a9fd
Compare
core/vm/gas_table.go
Outdated
if evm.chainRules.IsEIP4762 && !contract.IsSystemCall { | ||
if transfersValue { | ||
gas, overflow = math.SafeAdd(gas, evm.AccessEvents.ValueTransferGas(contract.Address(), address)) | ||
valueTransferGas := evm.AccessEvents.ValueTransferGas(contract.Address(), address) | ||
overflow := multiGas.SafeIncrement(multigas.ResourceKindStorageAccess, valueTransferGas) |
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.
Note: In EIP-4762 (Verkle trees), value transfers require accessing the account's balance leaf in the tree, which is considered a storage access operation
e99cca4
to
487493b
Compare
core/vm/gas_table.go
Outdated
return multigas.ZeroGas(), 0, ErrGasUintOverflow | ||
} | ||
} | ||
} | ||
evm.callGasTemp, err = callGas(evm.chainRules.IsEIP150, contract.Gas, gas, stack.Back(0)) | ||
|
||
// NOTE(NIT-3483): using only the computation gas for callGas |
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.
Needs to be double-checked
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 don't really understand why.
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 think the third parameter to callGas
below (called base
in that function) should be the sum of all dimensions, not just the computation gas, and with my earlier comment on how CallNewAccountGas
needs to be storage growth, that will actually matter
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.
Thank you, now I figured out. Initially I thought that memoryGasCost
only makes sense for computation gas because I was getting inconsistent results in tests, which was due to the wrong type for CallNewAccountGas
. Fixed both issues with latest commit
@@ -52,7 +52,7 @@ func memoryGasCost(mem *Memory, newMemSize uint64) (*multigas.MultiGas, uint64, | |||
fee := newTotalFee - mem.lastGasCost | |||
mem.lastGasCost = newTotalFee | |||
|
|||
return multigas.ZeroGas(), fee, nil | |||
return multigas.ComputationGas(fee), fee, nil |
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.
How did you decide to allocate this to computation gas?
Is that what Tyler's tracer did?
I would have guessed (possibly, incorrectly) that gas for expanding memory would be state growth gas.
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.
My assumption was based on this statement, and here and in all following places I have treated memory operations as computation cost since there are no appeals on statedb. Please correct me if I'm wrong
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.
Okay. So, this is, at least, consistent with what Tyler has done in the tracer. Good.
Now, though, I do need to understand why that's the right behavior.
Maybe I don't understand what the difference is between memory expansion and the storage in the statedb.
By way of analogy, are we saying that the "memory" of the EVM is similar to RAM on a real machine. And, that entries in the statedb are more like "disk"? And, so, when we talk about "state growth" and "state access" we are only talking about the equivalent of disk growth and i/o? And that "memory" is only what has to be paged into the working memory of the VM to support the computations which are happening while processing the transactions and even though the values of the allocated memory are part of what makes up the global state root, it's not really part of the "state" we're talking about when we talk about "state growth" or "state access?"
@tsahee, @relyt29 and @magicxyyz am I understanding this right?
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.
yes that is exactly correct
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.
Okay. Thanks. I feel like we should put some abbreviated explanation of this reasoning in the inline documentation so that we remember why we're treating memory expansion gas as computation gas. Keep it brief, but, let's do explain it.
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 don't see the documentation for this. Did you decide not to add something?
core/vm/gas_table.go
Outdated
return multigas.ZeroGas(), 0, ErrGasUintOverflow | ||
} | ||
} | ||
} | ||
evm.callGasTemp, err = callGas(evm.chainRules.IsEIP150, contract.Gas, gas, stack.Back(0)) | ||
|
||
// NOTE(NIT-3483): using only the computation gas for callGas |
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 don't really understand why.
0f1ce7a
to
ca34cf3
Compare
ca34cf3
to
0665b84
Compare
@@ -52,7 +52,7 @@ func memoryGasCost(mem *Memory, newMemSize uint64) (*multigas.MultiGas, uint64, | |||
fee := newTotalFee - mem.lastGasCost | |||
mem.lastGasCost = newTotalFee | |||
|
|||
return multigas.ZeroGas(), fee, nil | |||
return multigas.ComputationGas(fee), fee, nil |
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 don't see the documentation for this. Did you decide not to add something?
For NIT-3483 and NIT-3480
Instrument
CALL
andCALLCODE
opcode with multigas calculations:gasCall
funcgasCallCode
funcmakeCallVariantGasCallEIP2929
for EIP-2929 (access lists)makeCallVariantGasEIP4762
for EIP-4762 (Verkle trees)makeCallVariantGasCallEIP7702
for EIP-7702 (set code transaction)gasDelegateCall
funcgasStaticCall
func