core/vm: Better handling of static mode violation (ErrWriteProtection)

[chfast]

Detection of static mode violation (ErrWriteProtection) should be handled by the affected instructions (operation.writes), not in the interpreter loop:

go-ethereum/core/vm/interpreter.go

Lines 207 to 217 in 66ee942

	// If the operation is valid, enforce write restrictions
	if in.readOnly && in.evm.chainRules.IsByzantium {
	// If the interpreter is operating in readonly mode, make sure no
	// state-modifying operation is performed. The 3rd stack item
	// for a call operation is the value. Transferring value from one
	// account to the others means the state is modified and should also
	// return with an error.
	if operation.writes || (op == CALL && stack.Back(2).Sign() != 0) {
	return nil, ErrWriteProtection
	}
	}

1. Please confirm that in.readOnly can be true only when in.evm.chainRules.IsByzantium, so the condition in.readOnly && in.evm.chainRules.IsByzantium can be simplified.
2. Move this check to implementations of the affected instructions (e.g. opCall(), opSstore()).
3. Remove the check from the interpreter loop.
4. Remove operation.writes field.

This should land after #23952.

[holiman]

1. in.readOnly is only ever set from StaticCall, which was introduced in byzantium. So ACK on that.

[holiman]

Related observation: at that point in the code, we have both readOnly from the stack, and in.readOnly via pointer. Would probably be better to use readOnly instead of in.readOnly when checking the condition

[chfast]

1. in.readOnly is only ever set from StaticCall, which was introduced in byzantium. So ACK on that.

I confirmed that with panic() on State Tests.

[chfast]

As @axic noted, this will change the trace because the "readOnly" will be checked after "memory".

[holiman]

There's one thing though... IIRC, parity had a consensus issue, related to the order in which they did certain things. A 'forbidden' op was invoked, but with too few stack items. Depending on whether they errored on 'shallow stack' or 'write protected', they reverted back to the wrong scope.
I don't recall the exact details, but might be something we need to have in mind when doing this.

As for trace changing, I'm not sure that matters. The differential fuzzers don't care about the wording of the error messages.

[holiman]

IIRC, parity had a consensus issue

It was this: https://github.com/openethereum/parity-ethereum/pull/6889/files

So if the balance was too low, the create/call simply failed. So remained in the same scope, but with a 0 on the stack. Whereas it should have dropped back one level due to write-protection. If we just move the check slightly into the ops themselves, that should be fine, as long as we check write-protection before balance.

[holiman]

write ops

• SSTORE
• LOG0
• LOG1
• LOG2
• LOG3
• LOG4
• CREATE
• CREATE2
• SELFDESTRUCT
• ...And the bastard child: CALL, but only if the value is non-zero.
  • But not callcode, even though it "transfers" money, and even fails if it exceeds the balance -- but since it only transfers it to itself, it's not considered a write op

[axic]

As @axic noted, this will change the trace because the "readOnly" will be checked after "memory".

My comment was also a bit more general, it changes the order of evaluation for these checks. We discussed order-of-evaluation questions as part of EIP-3860 (limit initcode), and found that for most of the cases state tests do not care about it.