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

ETH deposits all return status 0 (Revert) #497

Closed
K-Ho opened this issue Apr 19, 2021 · 6 comments · Fixed by #937
Closed

ETH deposits all return status 0 (Revert) #497

K-Ho opened this issue Apr 19, 2021 · 6 comments · Fixed by #937
Assignees
Labels
C-bug Category: bugs

Comments

@K-Ho
Copy link
Contributor

K-Ho commented Apr 19, 2021

Despite succeeding (ETH actually deposits and the correct events are emitted), ETH deposits show status: 0 when they are processed on L2.

Steps to reproduce the behavior:

  1. Run native eth integration tests
  2. Check getTransactionReceipt on the L2 tx hash of a completed ETH deposit.
@K-Ho K-Ho added A-geth C-bug Category: bugs labels Apr 19, 2021
@K-Ho
Copy link
Contributor Author

K-Ho commented Apr 19, 2021

This is blocking going live with fees. cc @ben-chain I remember you mentioned knowing the cause of this?

@ben-chain
Copy link
Collaborator

I am pretty sure that this behavior is because we treat calls to predeploys as "pre-execution" WRT the action the user wants to take. This is because we want e.g. the L2xDM's return status to be ignore and instead return the result of its subcall in relayMessage. However, this line in geth means that we do not correctly identify the OVM_ETH contract as the "intended target" of the finalizeDeposit transaction.

I believe that a quick fix would be modifying this condition so that it correctly sees the OVM_ETH.finalizeDeposit call as being the "intended target subcall" whose status should be returned. However, correctly identifying this is a little hacky. A more robust solution is to have the execution manager always return data, so that the common pattern is predeploys return the status of their target message, whatever that might be, as discussed here: #474

@snario
Copy link
Contributor

snario commented Apr 20, 2021

I've reproduced this here: #502

@ben-chain
Copy link
Collaborator

I thought through this a little bit more, and unfortunately the hotfix I laid out above seems insufficient/difficult to pull off. The reason is because the fee payment logic within OVM_ECDSAContractAccount always produces a call frame into the OVM_ETH contract, and it will be hard to differentiate between this frame and the "real" call to OVM_ETH. We could still solve the problem with deposits, but it could end up manifesting elsewhere. Therefore, I think the "EM returndata" approach is the right one here.

@snario snario removed the P-high label Apr 26, 2021
@tynes
Copy link
Contributor

tynes commented Apr 28, 2021

Is this fixed by #643 ?

@K-Ho
Copy link
Contributor Author

K-Ho commented May 17, 2021

Bump on this, causing confusion with using the gateway - cc @gigamesh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants