core/vm,eth/tracers/native: prevent panic for LOG edge-cases

[jwasinger]

Prevents tracer from crashing when logs are enabled and a LOG opcode would underflow the stack or access memory that hasn't been expanded yet.

Please let me know if this is the right approach. If so, I will apply it across all tracers and write unit tests.

fixes #26845

[holiman]

I don't think we can do that either, there's no upper bound here is there? The msize can be petabytes, so we need to set a limit here.

[holiman]

@s1na don't we already have a similar limit set somewhere already?

[s1na]

Yep we added a similar logic in #25213

[s1na]

We'll need this also in prestateTracer. Please make it into a function in the eth/tracers package which the js, call and prestate tracers import and use to get a copied slice of memory.

[jwasinger]

I decided to place a new function in the vm.Memory which is used by the native/Goja tracers, moving the memory padding limit/checks that were present in the goja tracer into this function.

[zzh1996]

1 MB limit is not enough. It's only 2M gas. Note that #25213 also has this issue

[s1na]

Is this needed? AFAIK CaptureState is called after the stack validation

[jwasinger]

it's called before stack validation.

CaptureState: https://github.com/ethereum/go-ethereum/blob/master/core/vm/interpreter.go#L159
stack validation: https://github.com/ethereum/go-ethereum/blob/master/core/vm/interpreter.go#L181-L185

[s1na]

That case is only when op execution failed. These 2 are the main invocations, if memory expansion is expected:

go-ethereum/core/vm/interpreter.go

Line 217 in 94ff721

in.evm.Config.Tracer.CaptureState(pc, op, gasCopy, cost, callContext, in.returnData, in.evm.depth, err)

Otherwise:

go-ethereum/core/vm/interpreter.go

Line 224 in 94ff721

in.evm.Config.Tracer.CaptureState(pc, op, gasCopy, cost, callContext, in.returnData, in.evm.depth, err)

[jwasinger]

Looks like what happens in the case of stack underflow when executing LOG, is that the "op execution failed" case is called after the stack underflow error occurs in the interpreter loop.

So afaict, adding a stack check seems necessary?

[s1na]

Ah pardon you're right. I was assuming callTracer aborts if the error passed to CaptureState is non-nil. IMO that should be the fix and we shouldn't process the logs at all if the interpreter passed in an error.

[jwasinger]

Have applied this "return early if err != nil" logic to the native call tracer. Should this be applied across all tracers?

[s1na]

Should this be applied across all tracers?

Prestate tracer also has this issue. Please apply there too

[holiman]

Prestate tracer also has this issue.

@s1na I found one case, via CREATE2, where the early return fixed it. Was there any other case?

[holiman]

I would prefer not to add a new public method in 'core/vm', for something that is just a utility for the tracers.

[jwasinger]

Okay. moved it out of the vm.Memory object and into a public function in the vm package.

[holiman]

Example how to test this (sidenote: when I investigate something like this, I always start by making a testcase of it, so I can experiment with solutions)

in calltrace_test.go:

func TestLogStackDepletion(t *testing.T) {
	var (
		to        = common.HexToAddress("0x00000000000000000000000000000000deadbeef")
		origin    = common.HexToAddress("0x00000000000000000000000000000000feed")
		txContext = vm.TxContext{
			Origin:   origin,
			GasPrice: big.NewInt(1),
		}
		context = vm.BlockContext{
			CanTransfer: core.CanTransfer,
			Transfer:    core.Transfer,
			Coinbase:    common.Address{},
			BlockNumber: new(big.Int).SetUint64(8000000),
			Time:        5,
			Difficulty:  big.NewInt(0x30000),
			GasLimit:    uint64(6000000),
		}
		alloc = core.GenesisAlloc{
			to: core.GenesisAccount{
				Code: []byte{byte(vm.LOG3)},
			},
			origin: core.GenesisAccount{
				Nonce:   0,
				Balance: big.NewInt(500000000000000),
			},
		}
	)
	_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(), alloc, false)
	// Create the tracer, the EVM environment and run it
	tracer, err := tracers.DefaultDirectory.New("callTracer", nil, json.RawMessage(`{ "withLog": true }`))
	if err != nil {
		t.Fatalf("failed to create call tracer: %v", err)
	}
	evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Debug: true, Tracer: tracer})
	msg := &core.Message{
		To:                &to,
		From:              origin,
		Value:             big.NewInt(0),
		GasLimit:          50000,
		GasPrice:          big.NewInt(0),
		GasFeeCap:         big.NewInt(0),
		GasTipCap:         big.NewInt(0),
		SkipAccountChecks: false,
	}
	st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(msg.GasLimit))
	if _, err = st.TransitionDb(); err != nil {
		t.Fatalf("failed to execute transaction: %v", err)
	}
	// Retrieve the trace result and compare against the etalon
	res, err := tracer.GetResult()
	if err != nil {
		t.Fatalf("failed to retrieve trace result: %v", err)
	}
	wantStr := `{"from":"0x000000000000000000000000000000000000feed","gas":"0x7148","gasUsed":"0xc350","to":"0x00000000000000000000000000000000deadbeef","input":"0x","error":"stack underflow (0 \u003c=\u003e 5)","value":"0x0","type":"CALL"}`
	if string(res) != wantStr {
		t.Fatalf("trace mismatch\n have: %v\n want: %v\n", string(res), wantStr)
	}
}

Another sidenote: With the new core.Message, we can avoid the whole signing-step in that file, and simplify a lot of them

[holiman]

And a test for the second issue

func TestMemExpansion(t *testing.T) {
	var (
		to        = common.HexToAddress("0x00000000000000000000000000000000deadbeef")
		origin    = common.HexToAddress("0x00000000000000000000000000000000feed")
		txContext = vm.TxContext{
			Origin:   origin,
			GasPrice: big.NewInt(1),
		}
		context = vm.BlockContext{
			CanTransfer: core.CanTransfer,
			Transfer:    core.Transfer,
			Coinbase:    common.Address{},
			BlockNumber: new(big.Int).SetUint64(8000000),
			Time:        5,
			Difficulty:  big.NewInt(0x30000),
			GasLimit:    uint64(6000000),
		}
		alloc = core.GenesisAlloc{
			to: core.GenesisAccount{
				Code: []byte{
					byte(vm.PUSH1), 0x1,
					byte(vm.PUSH1), 0x0,
					byte(vm.MSTORE),
					byte(vm.PUSH1), 0xff,
					byte(vm.PUSH1), 0x0,
					byte(vm.LOG0),
				},
			},
			origin: core.GenesisAccount{
				Nonce:   0,
				Balance: big.NewInt(500000000000000),
			},
		}
	)
	_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(), alloc, false)
	// Create the tracer, the EVM environment and run it
	tracer, err := tracers.DefaultDirectory.New("callTracer", nil, json.RawMessage(`{ "withLog": true }`))
	if err != nil {
		t.Fatalf("failed to create call tracer: %v", err)
	}
	evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Debug: true, Tracer: tracer})
	msg := &core.Message{
		To:                &to,
		From:              origin,
		Value:             big.NewInt(0),
		GasLimit:          50000,
		GasPrice:          big.NewInt(0),
		GasFeeCap:         big.NewInt(0),
		GasTipCap:         big.NewInt(0),
		SkipAccountChecks: false,
	}
	st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(msg.GasLimit))
	if _, err = st.TransitionDb(); err != nil {
		t.Fatalf("failed to execute transaction: %v", err)
	}
	// Retrieve the trace result and compare against the etalon
	res, err := tracer.GetResult()
	if err != nil {
		t.Fatalf("failed to retrieve trace result: %v", err)
	}
	wantStr := `TODO fix this`
	if string(res) != wantStr {
		t.Fatalf("trace mismatch\n have: %v\n want: %v\n", string(res), wantStr)
	}
}

[holiman]

I don't think this tracer-helper method belongs in core/vm. Why not put it in eth/tracers? Memory exposes Len(), so there's afaict nothing private that you need access to in order to replicate this functionality somewhere else.

[holiman]

Not sure "silent return" is the best. IMO either errror-return or just continue with empty data ?

[jwasinger]

The logs won't be emitted because execution of the op will fail. So I silent-return to prevent emitting a log step in the tracer.

[holiman]

even better

diff --git a/eth/tracers/internal/tracetest/calltrace_test.go b/eth/tracers/internal/tracetest/calltrace_test.go
index 62182e3a82..eafa0258b5 100644
--- a/eth/tracers/internal/tracetest/calltrace_test.go
+++ b/eth/tracers/internal/tracetest/calltrace_test.go
@@ -260,75 +260,102 @@ func benchTracer(tracerName string, test *callTracerTest, b *testing.B) {
 	}
 }
 
-// TestZeroValueToNotExitCall tests the calltracer(s) on the following:
-// Tx to A, A calls B with zero value. B does not already exist.
-// Expected: that enter/exit is invoked and the inner call is shown in the result
-func TestZeroValueToNotExitCall(t *testing.T) {
-	var to = common.HexToAddress("0x00000000000000000000000000000000deadbeef")
-	privkey, err := crypto.HexToECDSA("0000000000000000deadbeef00000000000000000000000000000000deadbeef")
-	if err != nil {
-		t.Fatalf("err %v", err)
-	}
-	signer := types.NewEIP155Signer(big.NewInt(1))
-	tx, err := types.SignNewTx(privkey, signer, &types.LegacyTx{
-		GasPrice: big.NewInt(0),
-		Gas:      50000,
-		To:       &to,
-	})
-	if err != nil {
-		t.Fatalf("err %v", err)
-	}
-	origin, _ := signer.Sender(tx)
-	txContext := vm.TxContext{
-		Origin:   origin,
-		GasPrice: big.NewInt(1),
-	}
-	context := vm.BlockContext{
-		CanTransfer: core.CanTransfer,
-		Transfer:    core.Transfer,
-		Coinbase:    common.Address{},
-		BlockNumber: new(big.Int).SetUint64(8000000),
-		Time:        5,
-		Difficulty:  big.NewInt(0x30000),
-		GasLimit:    uint64(6000000),
-	}
-	var code = []byte{
-		byte(vm.PUSH1), 0x0, byte(vm.DUP1), byte(vm.DUP1), byte(vm.DUP1), // in and outs zero
-		byte(vm.DUP1), byte(vm.PUSH1), 0xff, byte(vm.GAS), // value=0,address=0xff, gas=GAS
-		byte(vm.CALL),
+func TestInternals(t *testing.T) {
+	var (
+		to        = common.HexToAddress("0x00000000000000000000000000000000deadbeef")
+		origin    = common.HexToAddress("0x00000000000000000000000000000000feed")
+		txContext = vm.TxContext{
+			Origin:   origin,
+			GasPrice: big.NewInt(1),
+		}
+		context = vm.BlockContext{
+			CanTransfer: core.CanTransfer,
+			Transfer:    core.Transfer,
+			Coinbase:    common.Address{},
+			BlockNumber: new(big.Int).SetUint64(8000000),
+			Time:        5,
+			Difficulty:  big.NewInt(0x30000),
+			GasLimit:    uint64(6000000),
+		}
+	)
+	mkTracer := func(name string, cfg json.RawMessage) tracers.Tracer {
+		tr, err := tracers.DefaultDirectory.New(name, nil, cfg)
+		if err != nil {
+			t.Fatalf("failed to create call tracer: %v", err)
+		}
+		return tr
 	}
-	var alloc = core.GenesisAlloc{
-		to: core.GenesisAccount{
-			Nonce: 1,
-			Code:  code,
+
+	for _, tc := range []struct {
+		name   string
+		code   []byte
+		tracer tracers.Tracer
+		want   string
+	}{
+		{
+			// TestZeroValueToNotExitCall tests the calltracer(s) on the following:
+			// Tx to A, A calls B with zero value. B does not already exist.
+			// Expected: that enter/exit is invoked and the inner call is shown in the result
+			name: "ZeroValueToNotExitCall",
+			code: []byte{
+				byte(vm.PUSH1), 0x0, byte(vm.DUP1), byte(vm.DUP1), byte(vm.DUP1), // in and outs zero
+				byte(vm.DUP1), byte(vm.PUSH1), 0xff, byte(vm.GAS), // value=0,address=0xff, gas=GAS
+				byte(vm.CALL),
+			},
+			tracer: mkTracer("callTracer", nil),
+			want:   `{"from":"0x000000000000000000000000000000000000feed","gas":"0x7148","gasUsed":"0x54d8","to":"0x00000000000000000000000000000000deadbeef","input":"0x","calls":[{"from":"0x00000000000000000000000000000000deadbeef","gas":"0x6cbf","gasUsed":"0x0","to":"0x00000000000000000000000000000000000000ff","input":"0x","value":"0x0","type":"CALL"}],"value":"0x0","type":"CALL"}`,
 		},
-		origin: core.GenesisAccount{
-			Nonce:   0,
-			Balance: big.NewInt(500000000000000),
+		{
+			name:   "Stack depletion in LOG0",
+			code:   []byte{byte(vm.LOG3)},
+			tracer: mkTracer("callTracer", json.RawMessage(`{ "withLog": true }`)),
+			want:   `{"from":"0x000000000000000000000000000000000000feed","gas":"0x7148","gasUsed":"0xc350","to":"0x00000000000000000000000000000000deadbeef","input":"0x","error":"stack underflow (0 \u003c=\u003e 5)","value":"0x0","type":"CALL"}`,
 		},
-	}
-	_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(), alloc, false)
-	// Create the tracer, the EVM environment and run it
-	tracer, err := tracers.DefaultDirectory.New("callTracer", nil, nil)
-	if err != nil {
-		t.Fatalf("failed to create call tracer: %v", err)
-	}
-	evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Debug: true, Tracer: tracer})
-	msg, err := core.TransactionToMessage(tx, signer, nil)
-	if err != nil {
-		t.Fatalf("failed to prepare transaction for tracing: %v", err)
-	}
-	st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(tx.Gas()))
-	if _, err = st.TransitionDb(); err != nil {
-		t.Fatalf("failed to execute transaction: %v", err)
-	}
-	// Retrieve the trace result and compare against the etalon
-	res, err := tracer.GetResult()
-	if err != nil {
-		t.Fatalf("failed to retrieve trace result: %v", err)
-	}
-	wantStr := `{"from":"0x682a80a6f560eec50d54e63cbeda1c324c5f8d1b","gas":"0x7148","gasUsed":"0x54d8","to":"0x00000000000000000000000000000000deadbeef","input":"0x","calls":[{"from":"0x00000000000000000000000000000000deadbeef","gas":"0x6cbf","gasUsed":"0x0","to":"0x00000000000000000000000000000000000000ff","input":"0x","value":"0x0","type":"CALL"}],"value":"0x0","type":"CALL"}`
-	if string(res) != wantStr {
-		t.Fatalf("trace mismatch\n have: %v\n want: %v\n", string(res), wantStr)
+		{
+			name: "Mem expansion in LOG0",
+			code: []byte{
+				byte(vm.PUSH1), 0x1,
+				byte(vm.PUSH1), 0x0,
+				byte(vm.MSTORE),
+				byte(vm.PUSH1), 0xff,
+				byte(vm.PUSH1), 0x0,
+				byte(vm.LOG0),
+			},
+			tracer: mkTracer("callTracer", json.RawMessage(`{ "withLog": true }`)),
+			want:   "`TODO fix this`",
+		},
+	} {
+		_, statedb := tests.MakePreState(rawdb.NewMemoryDatabase(),
+			core.GenesisAlloc{
+				to: core.GenesisAccount{
+					Code: tc.code,
+				},
+				origin: core.GenesisAccount{
+					Balance: big.NewInt(500000000000000),
+				},
+			}, false)
+		evm := vm.NewEVM(context, txContext, statedb, params.MainnetChainConfig, vm.Config{Debug: true, Tracer: tc.tracer})
+		msg := &core.Message{
+			To:                &to,
+			From:              origin,
+			Value:             big.NewInt(0),
+			GasLimit:          50000,
+			GasPrice:          big.NewInt(0),
+			GasFeeCap:         big.NewInt(0),
+			GasTipCap:         big.NewInt(0),
+			SkipAccountChecks: false,
+		}
+		st := core.NewStateTransition(evm, msg, new(core.GasPool).AddGas(msg.GasLimit))
+		if _, err := st.TransitionDb(); err != nil {
+			t.Fatalf("test %v: failed to execute transaction: %v", tc.name, err)
+		}
+		// Retrieve the trace result and compare against the expected
+		res, err := tc.tracer.GetResult()
+		if err != nil {
+			t.Fatalf("test %v: failed to retrieve trace result: %v", tc.name, err)
+		}
+		if string(res) != tc.want {
+			t.Fatalf("test %v: trace mismatch\n have: %v\n want: %v\n", tc.name, string(res), tc.want)
+		}
 	}
 }

[s1na]

We should let the JS code decide what to do in case of error IMO.

Suggested change

	if t.err != nil || err != nil {
	if t.err != nil {

[s1na]

Needs gofmt

[holiman]

@s1na PTAL?

[s1na]

LGTM!