core/vm: Move interpreter.ReadOnly check into the opcode implementations#23970
core/vm: Move interpreter.ReadOnly check into the opcode implementations#23970holiman merged 4 commits intoethereum:masterfrom
Conversation
There was a problem hiding this comment.
This is a new addition here, which I think may even make sense without the rest of the changes. Not sure about how well panic is perceived, but seen it used occasionally in similar context.
There was a problem hiding this comment.
We can leave this here if we want to do a test-run, but I think we should remove it from production code.
I'd also be fine with not having the panic here at all.
There was a problem hiding this comment.
Is there any option besides panic? I agree that panic is kind of risky as it may result in inconsistent shutdowns (and DoS), but if this is hit that is a potential consensus issue.
There was a problem hiding this comment.
We sometimes use panic, when something has failed beyond where we can do anything about it.
We don't usually actively look for failures to panic on, unless it's a piece of new code which we want to test during some benchmark runs.
So for prod code, we'll remove this. I'm personally pretty fine with believing that this will never happen, but if you want it for test-runs, then sure, we can keep it for a while
There was a problem hiding this comment.
Should we keep this panic for running a sync or just drop it now? I am fine either way. Should a comment be kept once the code is dropped?
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
LGTM aside from some nitpicks
There was a problem hiding this comment.
We can leave this here if we want to do a test-run, but I think we should remove it from production code.
I'd also be fine with not having the panic here at all.
There's not a lot of |
There was a problem hiding this comment.
So this is changing the order of operations a bit. Like one concrete change is the tracer's CaptureState will be called even on failure. Also memory will be resized, but I guess that's moot since the error will cause the call frame to fail. Another possibility is EVM returning a different error message if there are multiple failures (i.e. readonly violation AND not enough gas for memory expansion)
There was a problem hiding this comment.
Absolutely, this is one of my concerns, see the discussion on #23968. The order of evaluation for the most part is not covered by state tests, but in the long term I would hope it will be covered.
There was a problem hiding this comment.
Ah right missed that convo. Maybe time to revive the "trace after op execution" discussion
There was a problem hiding this comment.
The order of evaluation for the most part is not covered by state tests
It's not so much that it isn't covered -- the fact is that the order is opaque to state tests, in nearly all situations.
There was a problem hiding this comment.
We had ideated to expose error codes from execution and include those in the state tests. That would be a way to ensure some common order.
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
LGTM aside from a new nitpicks
There was a problem hiding this comment.
Please remove empty line. I've learnt (the hard way) that the golang purists on the team disapprove of empty lines after a closing bracket, with the reasoning that the bracket is enough of a divider.
Unless the empty line precedes a comment, in which case it might be ok, because it gives the comment more spotlight.
There was a problem hiding this comment.
This case, opCreate I think doesn't really follows the rules you mention, as it does have empty lines in seemingly irregular places. I'll remove the empty line as you suggested though.
|
Removed the empty lines, I hope to have removed the correct ones 😅 |
After Diff: < {"pc":4,"op":85,"gas":"0x96315f","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x1","0x1"],"returnData":"0x","depth":2,"refund":0,"opName":"SSTORE","error":"write protection"}
---
> {"pc":4,"op":85,"gas":"0x96315f","gasCost":"0x4e20","memory":"0x","memSize":0,"stack":["0x1","0x1"],"returnData":"0x","depth":2,"refund":0,"opName":"SSTORE","error":""}This may indeed be a bit problematic for the fuzzers to handle, since the error is never surfaced. I'll think about this a bit |
|
Ah, actually, the error is still sent to the logger, but the jsonlogger does not output it. So maybe we can fix it there, to give the fuzzers a better chance of handling this |
|
If we add this diff --git a/core/vm/logger_json.go b/core/vm/logger_json.go
index 364ce738a0..fac1c42dc8 100644
--- a/core/vm/logger_json.go
+++ b/core/vm/logger_json.go
@@ -46,7 +46,10 @@ func (l *JSONLogger) CaptureStart(env *EVM, from, to common.Address, create bool
l.env = env
}
-func (l *JSONLogger) CaptureFault(uint64, OpCode, uint64, uint64, *ScopeContext, int, error) {}
+func (l *JSONLogger) CaptureFault(pc uint64, op OpCode,gas, cos uint64,scope *ScopeContext, depth int, err error) {
+ // TODO: Add rData to this interface aswell
+ l.CaptureState(pc, op, gas, cos, scope, nil, depth,err)
+}
// CaptureState outputs state information on the logger.
func (l *JSONLogger) CaptureState(pc uint64, op OpCode, gas, cost uint64, scope *ScopeContext, rData []byte, depth int, err error) {Then the result is < {"pc":4,"op":85,"gas":"0x96315f","gasCost":"0x3","memory":"0x","memSize":0,"stack":["0x1","0x1"],"returnData":"0x","depth":2,"refund":0,"opName":"SSTORE","error":"write protection"}
---
> {"pc":4,"op":85,"gas":"0x96315f","gasCost":"0x4e20","memory":"0x","memSize":0,"stack":["0x1","0x1"],"returnData":"0x","depth":2,"refund":0,"opName":"SSTORE","error":""}
> {"pc":4,"op":85,"gas":"0x96315f","gasCost":"0x4e20","memory":"0x","memSize":0,"stack":["0x1","0x1"],"returnData":"0x","depth":2,"refund":0,"opName":"SSTORE","error":"write protection"}@MariusVanDerWijden we should be able to work with that, right? If the same 'pc' appears twice in a row, we can just ignore the first one. |
|
The gas discrepancy is still there, I wonder what gas value evmone outputs in most of these cases @chfast? |
|
Yes, but the discrepancy is such that it was previously wrong, using the previous value on error returns. And now it's more correct is all. I think we just didn't look too closely at the gas value whenever errors were involved. |
I don't output |
|
I'm getting these results from running evmone benchmarks with this against master. |
|
My benchmarks on a desktop CPU. |
|
@axic can you add the diff above to this PR? You might have to rebase, because I think Sina recently moved the loggers a different location. |
Not for the fuzzing engines. Not sure about other usecases -- I guess we can change that later if we have to |
|
Added the tracer change and rebased. |
|
@holiman rebased! |
|
I will benchmark it after #23952. |
|
New benchmarks after #23952 is merged. |
As @s1na pointed out during triage., this will most certainly affect the js legacy calltracer. The extra Then it will immediately ( on the second one, error-invocation) pop off the call again. However, it will still wind up one level too high. Previous:
Now:
This is somewhat fine -- the legacy calltracer is deprecated in favour of the scope-based calltracer. However, this might also cause problems for custom tracers. I kind of think that's fine too, but we should make a note of that in release notes. |
|
I think this should fix it but haven't tested it yet: diff --git a/eth/tracers/js/internal/tracers/call_tracer_legacy.js b/eth/tracers/js/internal/tracers/call_tracer_legacy.js
index 3ca737773..f3a756ecd 100644
--- a/eth/tracers/js/internal/tracers/call_tracer_legacy.js
+++ b/eth/tracers/js/internal/tracers/call_tracer_legacy.js
@@ -167,6 +167,19 @@
if (this.callstack[this.callstack.length - 1].error !== undefined) {
return;
}
+ // The write protection error "cancels" the last
+ // step event. We need to drop the extra calls
+ // that were potentially created and reset `descended`.
+ if (log.getError() === 'write protection') {
+ var op = log.op.toString();
+ // Pop the extra call
+ if (op === 'SELFDESTRUCT') {
+ this.callstack.pop();
+ } else if (op === 'CREATE' || op === 'CREATE2' || op === 'CALL') {
+ this.callstack.pop();
+ this.descended = false;
+ }
+ }
// Pop off the just failed call
var call = this.callstack.pop();
call.error = log.getError(); |
…ons (ethereum#23970) * core/vm: Move interpreter.ReadOnly check into the opcode implementations Also remove the same check from the interpreter inner loop. * core/vm: Remove obsolete operation.writes flag * core/vm: Capture fault states in logger Co-authored-by: Martin Holst Swende <martin@swende.se> * core/vm: Remove panic added for testing Co-authored-by: Martin Holst Swende <martin@swende.se>
…ons (#6396) Cherry-pick ethereum/go-ethereum#23970 Co-authored-by: Alex Beregszaszi <alex@rtfs.hu> Co-authored-by: Martin Holst Swende <martin@swende.se>
…ons (ethereum#23970) * core/vm: Move interpreter.ReadOnly check into the opcode implementations Also remove the same check from the interpreter inner loop. * core/vm: Remove obsolete operation.writes flag * core/vm: Capture fault states in logger Co-authored-by: Martin Holst Swende <martin@swende.se> * core/vm: Remove panic added for testing Co-authored-by: Martin Holst Swende <martin@swende.se>
6 participants
Fixes #23968.
I had trouble running tests locally, but
go test -v ./testspassed (and displayed a lot of state tests lines).make testruns into various failures regarding "too many files open" or networking issues. In short: this change may be broken as I couldn't 100% verify with tests.