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.

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

Discrepancy with Yellow Paper wrt warming the to_address of a CALL #978

Closed
OlivierBBB opened this issue Jul 17, 2024 · 8 comments
Closed
Labels
A-spec Area: specification C-bug Category: this is a bug, deviation, or other problem.

Comments

@OlivierBBB
Copy link

OlivierBBB commented Jul 17, 2024

Metadata

  • Hardfork: London and Cancun (at least: I only checked those two)

What was wrong?

AFAICT there is a discrepancy between the Yellow Paper and the present implementation in how the accrued state is updated for CALL-type instructions. Specifically the issue is with the warming up of the to_address.

Note. By CALL-type instruction I mean any of the following opcodes: CALL, CALLCODE, DELEGATECALL or STATICCALL.

Assume that the EVM is processing a CALL-type instruction . Assume furthermore that this instruction raises no exception i.e. no stack underflow, no out of gas and, in the case of CALL's specifically, no static context violation.

At this point the CALL-type instruction may still be aborted if either

  • the instruction attempts to transfer more value than the current balance allows for
  • the current frame has call stack depth $\geq$ 1024

The Yellow paper suggests that

  • if the CALL-type instruction isn't aborted a new accrued state $A ^ *$ is created which is identical to $A$ in all respects except that the to_address is inserted into the set of warm addresses and we carry out the message call function $\Theta$ using, among other parameters, this updated $A ^ *$;
  • if the CALL-type instruction is aborted then the accrued state $A$ remains unchanged, in particular the to_address isn't added to the set of warm addresses; this is the "otherwise" case in the screenshot below
image

In particular the accrued state $A'$ that results from an unexceptional aborted CALL-type instruction is identical to the accrued state $A$ prior to processing the CALL-type instruction.


The present implementation suggests, on the contrary, that

  • if the CALL-type instruction isn't aborted: no disagreement;
  • if the CALL-type instruction is aborted the to_address is added to the set of warm addresses

in particular the accrued state $A'$ that results from an unexceptional but aborted CALL-type instruction is in the present implementation is what the Yellow Paper would call $A ^ *$.

    if to in evm.accessed_addresses:
        access_gas_cost = GAS_WARM_ACCESS
    else:
        evm.accessed_addresses.add(to)        # this will take effect in the 'unexceptional but aborted' case
        access_gas_cost = GAS_COLD_ACCOUNT_ACCESS

    # gas computations
    # static context violation check

    sender_balance = get_account(
        evm.env.state, evm.message.current_target
    ).balance
    if sender_balance < value:
        push(evm.stack, U256(0))
        evm.return_data = b""
        evm.gas_left += message_call_gas.stipend
    else:
        generic_call(...)

The relevant EIP supports the current implementation, see section Storage Read Changes and in particular the sentence

In all cases, the gas cost is charged and the map is updated at the time that the opcode is being called.

If the current implementation is found to be correct the Yellow paper should be amended to

Amendment

Sources

Additional Context

This observation was originally made by comparing the Yellow Paper to the Besu implementation. Besu also warms the to_address up even in the case of an unexceptional yet aborted CALL-type instruction. This contradicted my understanding of the Yellow Paper and so I decided to check the present repo where I found the same approach as in Besu.

I raised a sibling issue on the yellow paper repo: ethereum/yellowpaper#910.

@SamWilsn
Copy link
Collaborator

I've compared the behaviour of EELS to geth using this test.

This is the patch that I used to implement the behaviour from the Yellow Paper:

diff --git a/src/ethereum/berlin/vm/instructions/system.py b/src/ethereum/berlin/vm/instructions/system.py
index 168f6ef7b..d5514e989 100644
--- a/src/ethereum/berlin/vm/instructions/system.py
+++ b/src/ethereum/berlin/vm/instructions/system.py
@@ -335,7 +335,6 @@ def call(evm: Evm) -> None:
     if to in evm.accessed_addresses:
         access_gas_cost = GAS_WARM_ACCESS
     else:
-        evm.accessed_addresses.add(to)
         access_gas_cost = GAS_COLD_ACCOUNT_ACCESS
 
     create_gas_cost = (
@@ -363,6 +362,7 @@ def call(evm: Evm) -> None:
         evm.return_data = b""
         evm.gas_left += message_call_gas.stipend
     else:
+        evm.accessed_addresses.add(to)
         generic_call(
             evm,
             message_call_gas.stipend,

Unmodified EELS generates the same state root as Geth, while applying the above patch generates different state roots.

This leads me to believe that the Yellow Paper is incorrect, but I've been wrong many times 🤣

@OlivierBBB
Copy link
Author

OlivierBBB commented Jul 17, 2024

This leads me to believe that the Yellow Paper is incorrect

I've reached the same conclusion albeit by different means. AFAICT the original EIP, Besu and the execution specs agree on the fact that the to_address must be warmed even if the (unexceptional) CALL is aborted. And it makes sense in that the pricing of the CALL instruction includes an extra fee when the to_address is warm. This fee is typically (e.g. EXTCODECOPY, BALANCE, ...) associated with inserting an addresses in the set of warm addresses.

@SamWilsn
Copy link
Collaborator

Are we good to close this then?

@OlivierBBB
Copy link
Author

OlivierBBB commented Jul 23, 2024

Yeah. BTW: do you know if people are still monitoring the yellow paper repo ?

@SamWilsn
Copy link
Collaborator

I think someone got a grant to update it to the latest. Not sure if it's ongoing though.

@pldespaigne
Copy link

pldespaigne commented Aug 25, 2024

yup! that's me 😄
Currently working on Cancun update for the YP!
I'll probably need some eyes to review my PRs once it's done.

@pldespaigne
Copy link

@SamWilsn @OlivierBBB

I just opened a PR to translate EIP-1153 Transient Storage into the YP, and any feedback on it would be highly appreciated! 🙏

Sorry to bother you with that, but it's pretty hard to get people to review YP changes, and you guys seems to be pretty familiar with it.

@SamWilsn
Copy link
Collaborator

I am, unfortunately, not at all qualified to read the Yellow Paper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-spec Area: specification C-bug Category: this is a bug, deviation, or other problem.
Projects
None yet
Development

No branches or pull requests

3 participants