-
Notifications
You must be signed in to change notification settings - Fork 70
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
Clarify the difference between code and recipient #286
Conversation
This issue was brought up some other time (cannot find it now): the single I think it is time to solve it on EVMC level by adding one more |
|
||
if (message.kind == EVMC_CALL) { | ||
if (message.flags & EVMC_STATIC) { | ||
// Match geth logic | ||
// https://github.com/ethereum/go-ethereum/blob/v1.9.25/core/vm/evm.go#L391 | ||
state_.touch(message.destination); | ||
state_.touch(recipient_address); |
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 seems to be fixing a consensus bug. Are there any tests failing?
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 seems to be fixing a consensus bug. Are there any tests failing?
Not really because this code is executed only when message.kind == EVMC_CALL
I'd suggest to split "destination" into "recipient" and "code", in line with Section 8 "Message Call" of the Yellow Paper. |
@@ -224,7 +225,22 @@ evmc::result EVM::call(const evmc_message& message) noexcept { | |||
return res; | |||
} | |||
|
|||
evmc_address EVM::recipient_of_call_message(const evmc_message& message) noexcept { |
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 is similar code for it in Aleth: https://github.com/ethereum/aleth/blob/master/libevm/ExtVMFace.cpp#L206-L208
I'm usually confused by this, but it seems that for the CALLCODE
the answer is also message.destination
.
Sounds good except I cannot name it "code" :). Then |
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
==========================================
+ Coverage 81.14% 81.18% +0.03%
==========================================
Files 82 82
Lines 7334 7349 +15
==========================================
+ Hits 5951 5966 +15
Misses 1383 1383
Continue to review full report at Codecov.
|
My preference would be |
@chfast I think a better idea would be not to change |
@chfast Oh well, extending I've created ethereum/evmc#611, ethereum/evmone#360, and #288 that add |
An
evmc_message
contains only two addresses (sender and "destination").However, in case of
DELEGATECALL
we need 3 addresses (sender, recipient, and code), so we recover the missing 3rd address from theaddress_stack_
.See Section 8 "Message Call" of the Yellow Paper.
destination
inevmc_message
can mean either code or recipient, depending on the context.