eth/tracers/native: panic on memory read in prestateTracer

[ryanschneider]

We've run into a panic mentioned in #26845 inside the prestate tracer, not that #26848 fixed the issue in callTracer but did not fixed the prestateTracer issue also mentioned in the original issue, this PR goes back and fixes that as well.

However, I'm not sure what I should do, if anything, with the returned error, since CaptureState doesn't return an error itself. Looks like tracers.GetMemoryCopyPadded only really fails if the arguments are wrong or the copy is more than 1024KB, in which sae I personally think it's ok to panic(err) but wanted to discuss before doing so.

Also unsure if there should have been a unit test to cover this, FWIW this is the payload we hit on goerli to get a panic:

curl -X "POST" http://0.0.0.0:8545 \
     -H 'Content-Type: application/json' \
     -d $'{
  "id": 1,
  "method": "debug_traceBlockByHash",
  "jsonrpc": "2.0",
  "params": [
    "0x81804b81d6aac4f72878f73c044cf49303944a856a5d8e572c0abb9a97893385",
    {
      "tracer": "prestateTracer"
    }
  ]
}'

With this panic log:

 ERROR[07-10|16:03:13.685] RPC method debug_traceBlockByHash crashed: runtime error: slice bounds out of range [:4448] with capacity 3072
 goroutine 248641109 [running]:
 github.com/ethereum/go-ethereum/rpc.(*callback).call.func1()
         github.com/ethereum/go-ethereum/rpc/service.go:199 +0x89
 panic({0x1880f60, 0xc044486390})
         runtime/panic.go:884 +0x213
 github.com/ethereum/go-ethereum/core/vm.(*Memory).GetCopy(...)
         github.com/ethereum/go-ethereum/core/vm/memory.go:76
 github.com/ethereum/go-ethereum/eth/tracers/native.(*prestateTracer).CaptureState(0xc0c087a000, 0xc0043fcef0?, 0xf5, 0xc00282e0d0?, 0x41359a?, 0xc0877c7fb0, {0x1c0?, 0x1a8?, 0xc00282e0d0>
         github.com/ethereum/go-ethereum/eth/tracers/native/prestate.go:168 +0x770
 github.com/ethereum/go-ethereum/core/vm.(*EVMInterpreter).Run(0xc0c087a300, 0xc02a91dec0, {0xc0420c9200, 0x8c4, 0x900}, 0x0)
         github.com/ethereum/go-ethereum/core/vm/interpreter.go:217 +0x83a
 github.com/ethereum/go-ethereum/core/vm.(*EVM).DelegateCall(0xc06d0a6e00, {0x1cb75e0, 0xc02a91de00?}, {0xb5, 0xf, 0x59, 0x20, 0x9f, 0xfc, 0x86,...}, ...)
         github.com/ethereum/go-ethereum/core/vm/evm.go:336 +0x5de
 github.com/ethereum/go-ethereum/core/vm.opDelegateCall(0x1?, 0xc0c087a300, 0xc0877c7500)
         github.com/ethereum/go-ethereum/core/vm/instructions.go:750 +0x47f
 github.com/ethereum/go-ethereum/core/vm.(*EVMInterpreter).Run(0xc0c087a300, 0xc02a91de00, {0xc0d45af600, 0x8c4, 0x8c4}, 0x0)
         github.com/ethereum/go-ethereum/core/vm/interpreter.go:228 +0xa39
 github.com/ethereum/go-ethereum/core/vm.(*EVM).Call(0xc06d0a6e00, {0x1cbb640, 0xc044486270}, {0x6e, 0x10, 0x4, 0x4d, 0x6, 0xa1, 0x0, ...}, ...)
         github.com/ethereum/go-ethereum/core/vm/evm.go:235 +0xb9e
 github.com/ethereum/go-ethereum/core.(*StateTransition).TransitionDb(0xc00282ee68)
         github.com/ethereum/go-ethereum/core/state_transition.go:375 +0x895
 github.com/ethereum/go-ethereum/core.ApplyMessage(0x1cc4ef8?, 0xc007f3b130?, 0xc0b23ecff0?)

[s1na]

#26848 was fixing multiple issues and it seems this one fell through the cracks. Thanks a lot for the detailed report and fix!

We do in fact have a test case which is supposed to cover memory expansion of CREATE2 for the prestateTracer, but checking now and CREATE2 is not being traced because the op goes OOG. That does cover the fix we added in #26848 that opcodes shouldn't be processed if interpreter returned an error but not memory expansion as the name suggests. I'll check what's the issue in a bit.

As for the error handling, I prefer log.Warn over panicing in this case.

[s1na]

So what was happening in that test is the memory expansion is so big the call was going OOG instead. I added a new test case which covers the memory padding case. Pushed directly to your PR.

[s1na]

LGTM

[ryanschneider]

@s1na thinking about this more personally I'd prefer this panic(err) instead of an early return, especially since the log warnings could get spammy under load since one line could be logged per RPC request triggering the issue.

[holiman]

A panic in an rpc call would not crash the server, just the method-handler, and the user would get some form of error message like "method handler crashed", if I recall correctly.

As I see it, we have these options for what to return to the user:

1. A mostly correct trace + server-side printout indicating error
2. No trace + "method handler crashed" error
3. No trace + specific error about what happened and when ("failed to copy create2 input")

Not sure what my preference is. But previously, we settled for 1.

[holiman]

LGTM