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.

Sign up for GitHub

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

Add code_address to evmc_message #611

Merged
merged 1 commit into from
Sep 5, 2021
Merged

Add code_address to evmc_message #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bindings/go/evmc/evmc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ static struct evmc_result execute_wrapper(struct evmc_vm* vm,
input_size,
*value,
{{0}}, // create2_salt: not required for execution
{{0}}, // code_address: not required for execution
};

struct evmc_host_context* context = (struct evmc_host_context*)context_index;
Expand Down
5 changes: 3 additions & 2 deletions bindings/go/evmc/host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ type HostContext interface {
EmitLog(addr Address, topics []Hash, data []byte)
Call(kind CallKind,
destination Address, sender Address, value Hash, input []byte, gas int64, depth int,
static bool, salt Hash) (output []byte, gasLeft int64, createAddr Address, err error)
static bool, salt Hash, codeAddress Address) (output []byte, gasLeft int64, createAddr Address, err error)
AccessAccount(addr Address) AccessStatus
AccessStorage(addr Address, key Hash) AccessStatus
}
Expand Down Expand Up @@ -208,7 +208,8 @@ func call(pCtx unsafe.Pointer, msg *C.struct_evmc_message) C.struct_evmc_result

kind := CallKind(msg.kind)
output, gasLeft, createAddr, err := ctx.Call(kind, goAddress(msg.destination), goAddress(msg.sender), goHash(msg.value),
goByteSlice(msg.input_data, msg.input_size), int64(msg.gas), int(msg.depth), msg.flags != 0, goHash(msg.create2_salt))
goByteSlice(msg.input_data, msg.input_size), int64(msg.gas), int(msg.depth), msg.flags != 0, goHash(msg.create2_salt),
goAddress(msg.code_address))

statusCode := C.enum_evmc_status_code(0)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion bindings/go/evmc/host_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (host *testHostContext) EmitLog(addr Address, topics []Hash, data []byte) {

func (host *testHostContext) Call(kind CallKind,
destination Address, sender Address, value Hash, input []byte, gas int64, depth int,
static bool, salt Hash) (output []byte, gasLeft int64, createAddr Address, err error) {
static bool, salt Hash, codeAddress Address) (output []byte, gasLeft int64, createAddr Address, err error) {
output = []byte("output from testHostContext.Call()")
return output, gas, Address{}, nil
}
Expand Down
8 changes: 6 additions & 2 deletions bindings/java/java/src/test/java/org/ethereum/evmc/TestMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public class TestMessage {
long inputSize;
char[] value;
byte[] createSalt;
byte[] codeAddress;

public TestMessage(
int kind,
Expand All @@ -38,6 +39,7 @@ public TestMessage(
this.inputSize = (long) inputData.length;
this.value = value;
this.createSalt = new byte[32];
this.codeAddress = new byte[20];
}

public TestMessage(ByteBuffer msg) {
Expand All @@ -56,11 +58,12 @@ public TestMessage(ByteBuffer msg) {
tmpbuf = msg.get(new byte[32]);
this.value = StandardCharsets.ISO_8859_1.decode(tmpbuf).array();
this.createSalt = msg.get(new byte[32]).array();
this.codeAddress = msg.get(new byte[20]).array();
}

public ByteBuffer toByteBuffer() {

return ByteBuffer.allocateDirect(152)
return ByteBuffer.allocateDirect(172)
.order(ByteOrder.nativeOrder())
.putInt(kind) // 4
.putInt(flags) // 4
Expand All @@ -72,6 +75,7 @@ public ByteBuffer toByteBuffer() {
.put(StandardCharsets.ISO_8859_1.encode(CharBuffer.wrap(inputData))) // 8
.putLong(inputSize) // 8
.put(StandardCharsets.ISO_8859_1.encode(CharBuffer.wrap(value))) // 32
.put(createSalt); // 32
.put(createSalt) // 32
.put(codeAddress); // 20
}
}
1 change: 1 addition & 0 deletions bindings/rust/evmc-vm/src/container.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ mod tests {
input_size: 0,
value: ::evmc_sys::evmc_uint256be::default(),
create2_salt: ::evmc_sys::evmc_bytes32::default(),
code_address: ::evmc_sys::evmc_address::default(),
};
let message: ExecutionMessage = (&message).into();

Expand Down
21 changes: 21 additions & 0 deletions bindings/rust/evmc-vm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub struct ExecutionMessage {
input: Option<Vec<u8>>,
value: Uint256,
create2_salt: Bytes32,
code_address: Address,
}

/// EVMC transaction context structure.
Expand Down Expand Up @@ -126,6 +127,7 @@ impl ExecutionMessage {
input: Option<&[u8]>,
value: Uint256,
create2_salt: Bytes32,
code_address: Address,
) -> Self {
ExecutionMessage {
kind,
Expand All @@ -141,6 +143,7 @@ impl ExecutionMessage {
},
value,
create2_salt,
code_address,
}
}

Expand Down Expand Up @@ -188,6 +191,11 @@ impl ExecutionMessage {
pub fn create2_salt(&self) -> &Bytes32 {
&self.create2_salt
}

/// Read the code address of the message.
pub fn code_address(&self) -> &Address {
&self.code_address
}
}

impl<'a> ExecutionContext<'a> {
Expand Down Expand Up @@ -326,6 +334,7 @@ impl<'a> ExecutionContext<'a> {
input_size: input_size,
value: *message.value(),
create2_salt: *message.create2_salt(),
code_address: *message.code_address(),
};
unsafe {
assert!((*self.host).call.is_some());
Expand Down Expand Up @@ -495,6 +504,7 @@ impl From<&ffi::evmc_message> for ExecutionMessage {
},
value: message.value,
create2_salt: message.create2_salt,
code_address: message.code_address,
}
}
}
Expand Down Expand Up @@ -653,6 +663,7 @@ mod tests {
let sender = Address { bytes: [128u8; 20] };
let value = Uint256 { bytes: [0u8; 32] };
let create2_salt = Bytes32 { bytes: [255u8; 32] };
let code_address = Address { bytes: [64u8; 20] };

let ret = ExecutionMessage::new(
MessageKind::EVMC_CALL,
Expand All @@ -664,6 +675,7 @@ mod tests {
Some(&input),
value,
create2_salt,
code_address,
);

assert_eq!(ret.kind(), MessageKind::EVMC_CALL);
Expand All @@ -676,6 +688,7 @@ mod tests {
assert_eq!(*ret.input().unwrap(), input);
assert_eq!(*ret.value(), value);
assert_eq!(*ret.create2_salt(), create2_salt);
assert_eq!(*ret.code_address(), code_address);
}

#[test]
Expand All @@ -684,6 +697,7 @@ mod tests {
let sender = Address { bytes: [128u8; 20] };
let value = Uint256 { bytes: [0u8; 32] };
let create2_salt = Bytes32 { bytes: [255u8; 32] };
let code_address = Address { bytes: [64u8; 20] };

let msg = ffi::evmc_message {
kind: MessageKind::EVMC_CALL,
Expand All @@ -696,6 +710,7 @@ mod tests {
input_size: 0,
value: value,
create2_salt: create2_salt,
code_address: code_address,
};

let ret: ExecutionMessage = (&msg).into();
Expand All @@ -709,6 +724,7 @@ mod tests {
assert!(ret.input().is_none());
assert_eq!(*ret.value(), msg.value);
assert_eq!(*ret.create2_salt(), msg.create2_salt);
assert_eq!(*ret.code_address(), msg.code_address);
}

#[test]
Expand All @@ -718,6 +734,7 @@ mod tests {
let sender = Address { bytes: [128u8; 20] };
let value = Uint256 { bytes: [0u8; 32] };
let create2_salt = Bytes32 { bytes: [255u8; 32] };
let code_address = Address { bytes: [64u8; 20] };

let msg = ffi::evmc_message {
kind: MessageKind::EVMC_CALL,
Expand All @@ -730,6 +747,7 @@ mod tests {
input_size: input.len(),
value: value,
create2_salt: create2_salt,
code_address: code_address,
};

let ret: ExecutionMessage = (&msg).into();
Expand All @@ -744,6 +762,7 @@ mod tests {
assert_eq!(*ret.input().unwrap(), input);
assert_eq!(*ret.value(), msg.value);
assert_eq!(*ret.create2_salt(), msg.create2_salt);
assert_eq!(*ret.code_address(), msg.code_address);
}

unsafe extern "C" fn get_dummy_tx_context(
Expand Down Expand Up @@ -866,6 +885,7 @@ mod tests {
None,
Uint256::default(),
Bytes32::default(),
test_addr,
);

let b = exe_context.call(&message);
Expand Down Expand Up @@ -897,6 +917,7 @@ mod tests {
Some(&data),
Uint256::default(),
Bytes32::default(),
test_addr,
);

let b = exe_context.call(&message);
Expand Down
14 changes: 13 additions & 1 deletion include/evmc/evmc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ struct evmc_message
/** The amount of gas for message execution. */
int64_t gas;

/** The destination of the message. */
/** The destination (recipient) of the message. */
evmc_address destination;

/** The sender of the message. */
Expand Down Expand Up @@ -141,6 +141,18 @@ struct evmc_message
* Ignored in evmc_execute_fn().
*/
evmc_bytes32 create2_salt;

/**
* The address of the code to be executed.
*
* May be different from the evmc_message::destination (recipient) in case of
* ::EVMC_CALLCODE or ::EVMC_DELEGATECALL.
Comment on lines +148 to +149
Copy link

Choose a reason for hiding this comment

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

@yperbasis, When working on EVMC compatibility in Nimbus-eth1, I was surprised to find that correct handling of msg.destination received via evmc_call_fn required checking whether msg.destination corresponds to a valid precompile in the current block's fork. This is because DELEGATECALL and CALLCODE lose their special behaviour when calling a precompile.

Perhaps the EVMC API comment should mention that it depends on whether the destination is a precompile?

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is the case. What is so special about precompiles in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Precompiles are handled on the Host side, not VM side (in EVMC terms). After this change Hosts should use msg.code_address rather than msg.destination to determine which smart contract or precompile to execute. On the VM side DELEGATECALL and CALLCODE shouldn't have any special behaviour depending on whether the code address is a precompile or not. At least in EVMC terms, where VM is not responsible for things like maintaining EIP-2929 accessed_addresses.

For example, that's how the Host side looks in Silkworm after this change: https://github.com/torquem-ch/silkworm/blob/master/core/silkworm/execution/evm.cpp#L156

Copy link

@jlokier jlokier Sep 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@chfast, before this change,msg.destination given to evmc_call_fn always contained the address for looking up the contract bytecode for all types of call (obviously not for precompiles), and yes of course that's a host side operation.

Then for DELEGATECALL and CALLCODE, msg.destination had to be replaced with the recipient address before calling evmc_execute_fn, iff it was not a precompile. In other words, correct handling of msg.destination depended on whether it was a precompile. (Fork-specific too, not the rule in EIP-1352. Consensus tests fail otherwise.)

Silkworm calculated the recipient here, and then overwrote msg.destination here.

In Nimbus I simplified that to just overwrite msg.destination with the equivalent of address_stack_.top() if doing DELEGATECALL or CALLCODE to a non-precompile, which works out the same as Silkworm (because it is equal to msg.sender in the CALLCODE case).

Because Silkwork doesn't use EVMC for precompiles, it doesn't matter that it only overwrote msg.destination in the non-precompile branch. That's just the if being efficient.

But In Nimbus, precompiles are called via EVMC. In fact a different EVMC library can be loaded for precompiles than for non-precompiles (using EVMC_CAPABILITY_PRECOMPILES).

When EVMC is used to call precompiles, msg.destination passed to evmc_execute_fn tells the EVM which precompile to perform. This is why, before msg.code_address was added, storing the calculated recipient in msg.destination on nested calls for DELEGATECALL and CALLCODE before calling evmc_execute_fn had to be conditional on it not being a precompile destination. This does show up in the consensus tests, btw. That's how I know the EIP-1352 proposal can't be used on the host side to check for a precompile address.

Now, since the addition of msg.code_address, there's a new precompile issue:

  • evmc_execute_fn is not supposed to look at msg.code_address, and the host is not guaranteed to pass along the msg.code_address it received in evmc_call_fn.

  • Thererefore an EVM providing EVMC_CAPABILITY_PRECOMPILES must look at the msg.destination it receives via evmc_execute_fn to decide which precompile, same as before msg.code_address was added.

  • But the correct precompile is now in msg.code_address given to evmc_call_fn.

  • Therefore the host must copy msg.code_address into msg.destination iff the call is to a precompile when using the EVMC_CAPABILITY_PRECOMPILES EVM.

It is neat that msg can now be passed along unchanged by nested calls, even passed by const reference and saving a little stack space on deep recursions. Until now I thought it had been an intentional design hack to overload msg.destination in this clever way to save a field. That proved quite handy to make me think harder about the information flows in Nimbus and how the EVM isn't allowed to know the code address. Ironically, in practice it will now be passed the code address, though it's not supposed to use it.

I think probably the cleanest solution is:

  • For EVMC_CAPABILITIES_PRECOMPILES EVMs to switch to using msg.code_address to decide which precompile, and no guarantee that msg.destination contains the precompile address, because this is the solution which allows the const reference to be passed without change for these calls.

  • Change msg.destination's name to msg.recipient to ensure that no code accidently continues to read msg.destination without the code being updated, as this nearly always contains the precompile address but it shouldn't be used.

Copy link

Choose a reason for hiding this comment

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

(I've edited my comment above; if you read the old version and ignored it, please read again, there remains an unresolved issue with regard to the new msg.code_address change and EVMC_CAPABILITY_PRECOMPILES.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I can see the problem now.
I need to track Host implementations if they change msg.destination on the way. Even if they do, that's probably not recommended thing to do and EVMC precompiles implementation should just only consider msg.code_address for the precompile selection. Is this a problematic change?

What I plan to do as a follow up is to clarify the documentation and change EVMC examples. Might be also nice to change destination to recipient. How about we discuss the problem there?

Copy link

@jlokier jlokier Sep 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mind where the discussion takes place (though I tend to miss some GitHub notications without a poke).

Documentation on the flow from EVM caller to nested EVM callee would be a nice improvement. It might be enough just to expand the comments a bit. There were quite a few things I had to figure out. I don't need that documentation any more, but it would make EVMC clearer for the next person.

I think all host implementations must have changed msg.destination (before msg.code_address was added), for regular, non-precompiles, to get the correct answers from DELEGATECALL and CALLCODE. And since msg.code_address they must stop changing msg.destination to get the correct answers. So perhaps there's no need to track the implementations. If they run consensus tests, they'll notice.

That leaves only precompiles. I don't know if any implementations call precompiles via EVMC, or if any loadable EVM provides precompiles (please let me know if you find one). Google returns no interesting matches for EVMC_CAPABILITY_PRECOMPILES. Nimbus is on track to use it, though.

This change isn't a problem. As EVMC precompile calls are Nimbus to Nimbus only as far as I know, no problem changing it to msg.code_address. It would be good to have a clear resolution on what the API-correct thing is if only for "someday" compatibility with others. If nothing is really correct, we can switch to a vendor extension if that helps. The comment at EVMC_CAPABILITY_PRECOMPILES about EIP-1352 is wrong anyway, as the host must use the fork-specific threshold due to some consensus tests using the low address range.

Copy link

Choose a reason for hiding this comment

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

If they run consensus tests, they'll notice.

No, wait, who am I kidding. If both the host and EVM are doing the same wrong thing with msg.code_address added, they won't notice, and I'd guess cross-testing between different hosts and EVMs using the full consensus suite is not done much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nimbus has called precompiles via EVMC since it first supported any kind of EVMC a couple of years ago, and the flagEVMC_CAPABILITY_PRECOMPILES was added since then, but we didn't bother using it until recently.

If precompiles are required to be handled on the host side, that's surprising news.

My bad, I forgot about EVMC_CAPABILITY_PRECOMPILES.

Copy link
Member

Choose a reason for hiding this comment

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

See #615.

* See Section 8 "Message Call" of the Yellow Paper for detail.
*
* Not required when invoking evmc_execute_fn(), only when invoking evmc_call_fn().
* Ignored if kind is ::EVMC_CREATE or ::EVMC_CREATE2.
*/
evmc_address code_address;
};


Expand Down
28 changes: 22 additions & 6 deletions tools/vmtester/tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,17 @@ TEST_F(evmc_vm_test, execute_call)
TEST_F(evmc_vm_test, execute_create)
{
evmc::MockedHost mockedHost;
evmc_message msg{
EVMC_CREATE, 0, 0, 65536, evmc_address{}, evmc_address{}, nullptr, 0, evmc_uint256be{},
evmc_bytes32{}};
evmc_message msg{EVMC_CREATE,
0,
0,
65536,
evmc_address{},
evmc_address{},
nullptr,
0,
evmc_uint256be{},
evmc_bytes32{},
evmc_address{}};
std::array<uint8_t, 2> code = {{0xfe, 0x00}};

evmc_result result =
Expand Down Expand Up @@ -170,9 +178,17 @@ TEST_F(evmc_vm_test, precompile_test)
destination.bytes[18] = static_cast<uint8_t>(i >> 8);
destination.bytes[19] = static_cast<uint8_t>(i & 0xff);

evmc_message msg{
EVMC_CALL, 0, 0, 65536, destination, evmc_address{}, nullptr, 0, evmc_uint256be{},
evmc_bytes32{}};
evmc_message msg{EVMC_CALL,
0,
0,
65536,
destination,
evmc_address{},
nullptr,
0,
evmc_uint256be{},
evmc_bytes32{},
evmc_address{}};

evmc_result result = vm->execute(vm, nullptr, nullptr, EVMC_MAX_REVISION, &msg, nullptr, 0);

Expand Down