eth/tracers: return proper error from debug_TraceTransaction when tx not found

[mdehoog]

Currently calling debug_TraceTransaction with a transacation hash that doesn't exist returns a confusing error: {code=-32000, message=genesis is not traceable}. This is because the GetTransaction returns blockNumber=0 when the transaction doesn't exist.

This PR checks if the tx is nil and returns nil if so. This will match the behavior of eth_getTransactionByHash and eth_getTransactionReceipt for transactions that don't exist or aren't yet included in a block.

Fixes #22846.

Example RPC response:

> curl -d '{"id":0,"jsonrpc":"2.0","method":"debug_traceTransaction","params":["0x0000000000000000000000000000000000000000000000000000000000000000"]}' -H "Content-Type: application/json" http://localhost:8545
{"jsonrpc":"2.0","id":0,"result":null}

[mdehoog]

this was removed because the testBackend implementation of GetTransaction does not match the real backend:

go-ethereum/eth/api_backend.go

Lines 267 to 270 in c2e0abc

	func (b *EthAPIBackend) GetTransaction(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error) {
	tx, blockHash, blockNumber, index := rawdb.ReadTransaction(b.eth.ChainDb(), txHash)
	return tx, blockHash, blockNumber, index, nil
	}

[holiman]

Why not return the error errTransactionNotFound here instead of nil?

[holiman]

Changing this to

Suggested change

	return nil, nil
	return nil, errors.New("transaction not found")

We get

> debug.traceTransaction("0x0000000000000000000000000000000000000000000000000000000000000000")
WARN [11-18|10:38:20.230] Served debug_traceTransaction            reqid=9 duration="127.204µs" err="transaction not found"
Error: transaction not found
	at web3.js:6365:9(45)
	at send (web3.js:5099:62(34))
	at <eval>:1:23(4)

[holiman]

It can be written slightly more compact using a switch

	switch {
	case err != nil:
		return nil, err
	case tx == nil:
		return nil, errors.New("transaction not found")
	case blockNumber == 0:
		return nil, errors.New("genesis is not traceable")
	}

[s1na]

Why not return the error errTransactionNotFound here instead of nil?

I came here to say the same

[fjl]

We have an existing convention in other RPC APIs returning null when the requested item is not found.

[mdehoog]

That sounds fine to me too; I was trying to match the behavior of eth_getTransactionByHash and eth_getTransactionReceipt which both return nil for a not-found transaction. But happy to take that suggestion if you think it's better.

[mdehoog]

If we want to return errTransactionNotFound, we could also do this in the GetTransaction method:

go-ethereum/eth/api_backend.go

Lines 267 to 270 in c2e0abc

	func (b *EthAPIBackend) GetTransaction(ctx context.Context, txHash common.Hash) (*types.Transaction, common.Hash, uint64, uint64, error) {
	tx, blockHash, blockNumber, index := rawdb.ReadTransaction(b.eth.ChainDb(), txHash)
	return tx, blockHash, blockNumber, index, nil
	}

[holiman]

We have an existing convention in other RPC APIs returning null when the requested item is not found.

@fjl so you think the pr as is (return nil rather than error) is preferrable?

I don't really understand the motivation behind that, personally.

[fjl]

The motivation for the behavior of getTransactionByHash and getTransactionReceipt is that 'not found' is not an error. You could imagine that, with an RPC client that throws an exception for any error response, the case where the requested item is not found should not raise an exception.

Another reason might be that the people who designed these APIs didn't know that JSON-RPC has error codes. Returning null provides a way to recognize this specific condition.

Returning errors.New("transaction not found") here is not great because it will return a generic error code, and callers will not be able to know whether the error is related to a database/disk fault or simply means the item is not there.

[mdehoog]

FWIW it's already returning an error: genesis is not traceable. So returning transaction not found would retain the existing behavior, with an error message that makes more sense.

[holiman]

I can somewhat agree that for getTransaction, it might not be an error for it to be missing. There are two expected outcomes, it's there or it's not there. For traceTransaction, however, I think it's an error if the tx is not present. The call implies that the tx exists. It not existing is an error. 

[s1na]

LGTM