core/vm: fix system call detection in extcodecopy

[gballet]

The check for a system call in EXTCODECOPY was always true, since it was based on an ad-hoc contract with IsSystemCall defaulting to false.

Replace this with a proper call to isSystemCall(target addr) to figure out if the leaves of the target contract must be filtered out since it's a system contract.

[rjl493456442]

You can use scope.Contract.IsSystemCall instead

[gballet]

I got confused by the same thing at first, but this is actually incorrect: scope.Contract.IsSystemCall is the caller, whereas addr is the contract whose bytecode is copied.

Now, consider the following cases:

• a (future) system contract has an EXTCODECOPY in its bytecode, and assume that it can call any non-system call contract (e.g. as a factory or something). What should end up in the witness? Not the code of the system contract, as per 4762 system contracts code chunks should not be included in the witness. But the code chunks of that non-system contract that are copied should be in the witness, as it's not a system contract.
• a regular contract copies code from a system contract, as per 4762 that code should not end up in the witness.

[MariusVanDerWijden]

I'm not 100% sure, but I think this is not correct. What I think you are trying to achieve is to filter for calls to the predeployed contracts in order to remove them from the witness. Afaict IsSystemCall checks whether the address is the caller of the predeployed contracts, not if the contract at this address is a predeployed contract.
I might be totally wrong though

[gballet]

oops, yes, I was looking at the definition of isSystemContract in a different branch, which had the correct behavior. Need to fix that, thanks.

[MariusVanDerWijden]

LGTM

[fjl]

This change has now turned into something completely different. So now it's just about preventing code from system contracts to be added into the witness when another contract invokes EXTCODECOPY on them. I think it's an unnecessary change.

[gballet]

Well, if you consider following the spec is unnecessary, sure. But otherwise, it is.

[gballet]

Not sure why you say it's completely different though? This is what the original intent of adding isSytemCall was: making sure system contract code doesn't end up in the witness. What do you feel has changed?

[fjl]

The idea with filtering was just to ensure that the system call itself doesn't add anything. If we didn't do the filtering in the system call, all system contracts would be included in the witness of every block.

If a contract calls into a system contract during transaction processing, the filtering does not need to happen. It's just like any other call. The user transaction pays for the inclusion into the witness. Same is true for EXTCODECOPY.

It could be argued that we should generally exclude the code of system contracts from the witness, since the code is technically defined in the EIP. But it's an unrelated change, and one that needs to be discussed separately.

[gballet]

It could be argued that we should generally exclude the code of system contracts from the witness, since the code is technically defined in the EIP. But it's an unrelated change, and one that needs to be discussed separately.

That's exactly what the spec says, we exclude the code of system contracts from the witness. An example is BLOCKHASH, what will call the system contract under the hood, via a non-system call.

The idea with filtering was just to ensure that the system call itself doesn't add anything.

It's a bit more subtle than that: if a system call calls a non-system contract for whatever reason, that non-system contract needs to be added to the witness otherwise stateless clients wouldn't be able to verify the block. This is why we changed isSystemCall from checking the origin to checking the caller.

[fjl]

There are different things there. For BLOCKHASH, I agree it should not add the code since it is transparent to the user. For other cases, it's debatable. If a user adds a withdrawal request by calling the contract, the code can be added to the witness.

[fjl]

I'm arguing like this because I'm trying to reach a solution where we don't have to explicitly list the addresses. The system call distinction is always inferable from context. The system contract distinction is not. I'm hopeful we can find a definition that doesn't require the latter.

[gballet]

Now you understand why I didn't want to have EVM calls in the first place, and why I said that was going to be more complex. This is just the beginning.

But in any case, the alternatives you describe arent't part of the spec, and would require a spec change, and would likely not be accepted on grounds that this makes the spec even more complex than it already is.

[fjl]

I don't think it's more complex in the spec. Defining it purely based on call is actually more aligned with how ethereum works. Ethereum doesn't have a notion of system contracts, it only has a notion of system calls. The contracts are just normal contracts, they become special only because the system calls into them from a known address.

If wanted it to be filtered based on destination address, we could just always use that list, i.e. use isSystemContract instead of isSystemCall everywhere. But the downside is, we'd have to update the list all the time, track forks, and downstream systems would have to maintain their own list. It's messier.

[fjl]

BTW, where is the spec? I am reading EIP-7709 but it doesn't really mention this. It just says we're supposed to not add the code to the witness when blockhash is accessed. This can be implemented in many ways.

[gballet]

I don't think it's more complex in the spec. Defining it purely based on call is actually more aligned with how ethereum works. Ethereum doesn't have a notion of system contracts, it only has a notion of system calls. The contracts are just normal contracts, they become special only because the system calls into them from a known address.

If wanted it to be filtered based on destination address, we could just always use that list, i.e. use isSystemContract instead of isSystemCall everywhere. But the downside is, we'd have to update the list all the time, track forks, and downstream systems would have to maintain their own list. It's messier.

You still have the issue that you must detect if a contract is a system contract or not, and if it isn't, its code should be in the witness, otherwise no stateless client will be able to work with it. There's no going around the need to maintain a list of contracts. But now it would have to be called all over the place instead of just inside EXTCODEHASH.

BTW, where is the spec? I am reading EIP-7709 but it doesn't really mention this. It just says we're supposed to not add the code to the witness when blockhash is accessed. This can be implemented in many ways.

What you refer to is precisely designed to give the stateless clients the ability to not package the code of the system contracts.

Specs:

https://eips.ethereum.org/EIPS/eip-4762#access-events-for-account-headers and https://eips.ethereum.org/EIPS/eip-4762#system-contracts are what you asked for. As you can see, it says that when resolving an opcode (which calling EXTCODEHASH is) then you should not store the code in the witness.

[fjl]

Sorry but I don't think it's necessary to add this list even after considering everything you wrote. We have the same goal here: ensuring the witness doesn't contain unnecessary accesses in each block, while also ensuring a stateless client can verify the block transition.

You have mentioned for the second time the case where a system call performs a call to another contract. This is a hypothetical case which cannot occur with any of the system calls introduced into mainnet so far, because the contracts which we deployed do not perform any external CALL, not even to a precompile. I do think we need to define this correctly regardless, but the rule we added in #31036 should be good enough. we avoid adding the code of contracts when their CALLER is the system address. This rule covers all cases for now, you can add it to EIP-4762 and it will work for a long time.

The EIP-4762 spec that you linked needs to be adapted anyway. It mentions the word 'system contract' many times, but as I said before, that isn't a defined notion in the protocol right now. So either the EIP-4762 needs to (a) explicitly state which contract addresses are deemed 'system contracts', or (b) reference another EIP which states what a 'system contract' is, or (c) remove the statements about 'system contracts' and define everything in terms of 'system call' and 'precompile call'. I prefer option c because it's the easiest.

Under that option, calling EXTCODECOPY on an address which receives system calls will add the code to the witness, because these contracts are not special. I believe this is acceptable.

For the precompile/opcode like in EIP-7709 we could introduce a rule in EIP-4762 that states: if a precompile or EVM opcode is defined in terms of another EVM contract, the bytecode and account headers of that access do not appear in the block witness. Like the system call rule, this will work for a very long time. This case is even more hypothetical than the system call special case for ext calls because no EVM-based precompile exists right now. I highly doubt we will ever create an EVM-implemented precompile that calls into another contract in the state during its execution. It's a very unsafe thing to do in general. For EIP-7709, I don't think any client will ever call the EVM for the blockhash opcode, they will just read it from the state directly.

[gballet]

1. There are 4 clients implementing this eip, changing it takes time. It's not me just editing it nilly-willy.
2. The changes you propose do not conform to the spec (EXTCODECOPY-ing a system contract will add to the witness and no system contract code should be).

Those two things together mean I can't act on it fast enough for a release this week (nor wish to: the specs are complex enough as it is). I have removed the milestone and will keep my changes in a separate branch.

As for "system contract" not being part of Ethereum, it's been in use since at least 2018: here and here.

[fjl]

If we use https://eips.ethereum.org/EIPS/eip-1352 as the definition, the contracts added in Cancun and Prague are not system contracts because they do not have an address in that range. So it would only apply on precompiles, which have no code that can be copied.

[MariusVanDerWijden]

I agree with Felix that this is probably not the correct construction and we should probably change the spec, however I also agree with Guillaume that since this is currently the spec, we should merge this PR.

Lets discuss on triage