Add code_address to evmc_message

[yperbasis]

Without code address evmc_message is not enough to implement evmc_call_fn for the DELEGATECALL case. For more detail please see erigontech/silkworm#286.

Warning: This is an API breaking change and requires a major version bump!

Related PRs: ethereum/evmone#360 and erigontech/silkworm#288.

[codecov]

Codecov Report

Merging #611 (ca82e32) into master (6711434) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head ca82e32 differs from pull request most recent head 8314761. Consider uploading reports for the commit 8314761 to get more accurate results

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
+ Coverage   95.71%   95.73%   +0.01%     
==========================================
  Files          24       24              
  Lines        3971     3987      +16     
==========================================
+ Hits         3801     3817      +16     
  Misses        170      170              

[chfast]

What was the problem placing it next to destination and sander?

[yperbasis]

What was the problem placing it next to destination and sander?

I was thinking about better backwards compatibility. But then again, code_address is mandatory for correct calls, and logically it belongs next to destination and sender. I can rework this PR and put it back there once PR #612 is merged.

[yperbasis]

@chfast Then again, one argument for adding code_address at the end of evmc_message rather than next to destination is that it's only required for evmc_call_fn, not evmc_execute_fn.

Also, an alternative is to remove code_address & create2_salt from evmc_message and instead pass them as arguments to evmc_call_fn.

[chfast]

Also, an alternative is to remove code_address & create2_salt from evmc_message and instead pass them as arguments to evmc_call_fn.

This is an indicator that using the same evmc_message for both execution and calls is not good idea. We may see this problem from this perspective and plan something for the future.

I will review this PR soon.

[jlokier]

A related issue that this maybe closes is #335.

[jlokier]

@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?

[chfast]

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

[yperbasis]

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

[jlokier]

@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.

[jlokier]

(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.

[chfast]

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?

[jlokier]

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.

[jlokier]

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.

[yperbasis]

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.

[chfast]

See #615.