ethereum-optimism / op-geth

GNU Lesser General Public License v3.0
255 stars 653 forks source link

Implement a call stack in EVM and expose through a precompile #256

Open canercidam opened 4 months ago

canercidam commented 4 months ago

Reference implementation of https://github.com/ethereum/RIPs/pull/10

tynes commented 4 months ago

I am not in favor of this feature. It couples solidity concepts like 4byte selectors to consensus. Just because there is an open PR in the RIPs repo, it doesn't mean that we will merge the feature. It needs to be proven to be actually useful and desired by the community.

canercidam commented 4 months ago

Hi @tynes, thanks for the feedback! We just wanted to present the reference implementation we have been working on - not suggesting that these changes should be merged right away. While we are continuing to discuss with the community, we wanted to get the OP Labs's perspective on these changes, as it is very helpful to adoption of this proposal. This is very useful in leveling protocols' call stack visibility with attackers and closing gaps in on-chain screening solutions (by exposing DELEGATECALLed logic contracts, other addresses from the call stack, function call patterns).

Your suggestion about the 4-byte selector makes sense. We can change the precompile to support a custom amount of bytes to peek at, instead of just limiting to 4, to solve the coupling problem. What do you think? We are also curious about your thoughts about the changes in general. We tried to keep it concise enough in terms of how it touches the EVM implementation.

tynes commented 4 months ago

We are also curious about your thoughts about the changes in general.

My personal opinion is that this is the wrong approach. This will end up being used for censorship which goes against Ethereum's core value of censorship resistance. Feel free to build censorship tools at the application layer, but I will never support in the core protocol itself.

canercidam commented 4 months ago

@tynes I absolutely understand where you are coming from and thanks for sharing the reference to the roadmap. I agree that any solution should be non-censorship at its best and we do not intend to propose a new tool for censorship. Like you suggested, it's already possible for anyone to build tooling at the application layer to refuse calls based on senders.

What we want to do with the proposal are just two things:

These changes are not equipping a central authority to have more control on the transactions. We are only suggesting that every protocol has its own right to do risk management if they want to do so by electing to not interact with some parties on-chain. If a protocol does this to block others, then it limits its own operation and potentially does some footshooting, which is again already currently possible.

In addition, all off-chain actors are equipped to carefully craft and simulate every execution and execute while on-chain protocols have very low visibility to what is happening. Wouldn't you agree that this is unfair to protocols? The incidents are concerning from Web3 adoption and development perspective and we want to make only a bit of visibility difference in core protocol.

Hoping to hear your thoughts, thanks!

tynes commented 4 months ago

Wouldn't you agree that this is unfair to protocols?

I do not agree with this claim. Your language attempts to make it sound like its universally accepted that its unfair to protocols that offchain simulation exists.

These changes are not equipping a central authority to have more control on the transactions

When the central authority is relative to an individual protocol, yes they are.

Like I said previously, I am not a fan of this proposal and I cannot think of any modifications to it that would make me think its a good idea. Maybe you can build up additional support through the RIP process and get feedback there. Good luck :)

protolambda commented 4 months ago

In this state I am very opposed to introducing this feature.

canercidam commented 4 months ago

Hi @protolambda!

The concerns about contracts behaving differently based on the call-stack is very error-prone. Many programming languages actively avoid exposing the callstack to a function to avoid side-effects.

It seems possible to get a backtrace/call-stack in Go and Rust and use for whatever purpose. Of course I agree that it could bring some complexity and this should require a very exceptional case. But our use case with EVM call stack is very different than the high level general purpose languages and the complexity is much lower.

The abstraction-leak comes with very real censorship concerns.

While we have heard some general concerns about this, we haven't heard of a real example. How can a contract censor the entire ecosystem based on this visibility improvement? It looks like a contract would only isolate itself by choosing to not participate and this is already possible to do based on msg.sender and tx.origin.

The feature lacks implementation in other execution-layer clients. This is essential: it helps harden the specification, is critical for OP-Stack to be multi-client, and is important for EVM tooling also (TS and Rust implementations in particular).

Fair but I think this is something that belongs to all RIPs. RIP-7212 has been accepted and have been implemented by multiple L2s. It is likely that we are not going to see the same change in all L2s or L1 or the tooling. The contract implementations can have fallback logic in case the precompile doesn't exist or interface with a substitute during development.

The feedback on the eth-magicians includes real concerns from (ex-)L1 devs that have not been properly addressed. In particular I believe the ZK proving complexity concern is not properly addressed.

This is because the implementation details will need to be discovered by ZK experts who can reason deeply about this. Creating a reference implementation is not as easy as modifying geth. The call stack or the precompile are not unpredictable in nature. The chains which use zkevm-circuits are providing EVM execution trace as the input to the prover, which is more than enough to construct the call stack. We are speaking to a couple of ZK rollups about the adoption of this.

Curious to hear more of your thoughts - thanks!