From 40f69c13062db09d3810a84b863df7c416f166cd Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 4 Apr 2024 13:56:17 +0100 Subject: [PATCH 1/8] Check for the revert reason in the TX receipt before using debug_traceTransaction Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 71 ++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index 88d81c0..8495813 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -26,6 +26,7 @@ import ( "github.com/hyperledger/firefly-common/pkg/fftypes" "github.com/hyperledger/firefly-common/pkg/i18n" + "github.com/hyperledger/firefly-common/pkg/log" "github.com/hyperledger/firefly-evmconnect/internal/msgs" "github.com/hyperledger/firefly-signer/pkg/ethtypes" "github.com/hyperledger/firefly-transaction-manager/pkg/ffcapi" @@ -33,17 +34,18 @@ import ( // txReceiptJSONRPC is the receipt obtained over JSON/RPC from the ethereum client, with gas used, logs and contract address type txReceiptJSONRPC struct { - BlockHash ethtypes.HexBytes0xPrefix `json:"blockHash"` - BlockNumber *ethtypes.HexInteger `json:"blockNumber"` - ContractAddress *ethtypes.Address0xHex `json:"contractAddress"` - CumulativeGasUsed *ethtypes.HexInteger `json:"cumulativeGasUsed"` - From *ethtypes.Address0xHex `json:"from"` - GasUsed *ethtypes.HexInteger `json:"gasUsed"` - Logs []*logJSONRPC `json:"logs"` - Status *ethtypes.HexInteger `json:"status"` - To *ethtypes.Address0xHex `json:"to"` - TransactionHash ethtypes.HexBytes0xPrefix `json:"transactionHash"` - TransactionIndex *ethtypes.HexInteger `json:"transactionIndex"` + BlockHash ethtypes.HexBytes0xPrefix `json:"blockHash"` + BlockNumber *ethtypes.HexInteger `json:"blockNumber"` + ContractAddress *ethtypes.Address0xHex `json:"contractAddress"` + CumulativeGasUsed *ethtypes.HexInteger `json:"cumulativeGasUsed"` + From *ethtypes.Address0xHex `json:"from"` + GasUsed *ethtypes.HexInteger `json:"gasUsed"` + Logs []*logJSONRPC `json:"logs"` + Status *ethtypes.HexInteger `json:"status"` + To *ethtypes.Address0xHex `json:"to"` + TransactionHash ethtypes.HexBytes0xPrefix `json:"transactionHash"` + TransactionIndex *ethtypes.HexInteger `json:"transactionIndex"` + RevertReason *ethtypes.HexBytes0xPrefix `json:"revertReason"` } // receiptExtraInfo is the version of the receipt we store under the TX. @@ -119,30 +121,37 @@ func ProtocolIDForReceipt(blockNumber, transactionIndex *fftypes.FFBigInt) strin return "" } -func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string) (pReturnValue *string, pErrorMessage *string) { - - // Attempt to get the return value of the transaction - not possible on all RPC endpoints - var debugTrace *txDebugTrace - traceErr := c.backend.CallRPC(ctx, &debugTrace, "debug_traceTransaction", transactionHash) - if traceErr != nil { - msg := i18n.NewError(ctx, msgs.MsgUnableToCallDebug, traceErr).Error() - return nil, &msg - } +func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, revertFromReceipt *ethtypes.HexBytes0xPrefix) (pReturnValue *string, pErrorMessage *string) { + + var revertReason string + if revertFromReceipt == nil { + log.L(ctx).Debug("No revert reason for the failed transaction found in the receipt. Calling debug_traceTransaction to retrieve it.") + // Attempt to get the return value of the transaction - not possible on all RPC endpoints + var debugTrace *txDebugTrace + traceErr := c.backend.CallRPC(ctx, &debugTrace, "debug_traceTransaction", transactionHash) + if traceErr != nil { + msg := i18n.NewError(ctx, msgs.MsgUnableToCallDebug, traceErr).Error() + return nil, &msg + } - returnValue := debugTrace.ReturnValue - if returnValue == "" { - // some clients (e.g. Besu) include the error reason on the final struct log - if len(debugTrace.StructLogs) > 0 { - finalStructLog := debugTrace.StructLogs[len(debugTrace.StructLogs)-1] - if *finalStructLog.Op == "REVERT" && finalStructLog.Reason != nil { - returnValue = *finalStructLog.Reason + revertReason := debugTrace.ReturnValue + if revertReason == "" { + // some clients (e.g. Besu) include the error reason on the final struct log + if len(debugTrace.StructLogs) > 0 { + finalStructLog := debugTrace.StructLogs[len(debugTrace.StructLogs)-1] + if *finalStructLog.Op == "REVERT" && finalStructLog.Reason != nil { + revertReason = *finalStructLog.Reason + } } } + } else { + log.L(ctx).Debug("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") + revertReason = string(*revertFromReceipt) } // See if the return value is using the default error you get from "revert" var errorMessage string - returnDataBytes, _ := hex.DecodeString(strings.TrimPrefix(returnValue, "0x")) + returnDataBytes, _ := hex.DecodeString(strings.TrimPrefix(revertReason, "0x")) if len(returnDataBytes) > 4 && bytes.Equal(returnDataBytes[0:4], defaultErrorID) { value, err := defaultError.DecodeCallDataCtx(ctx, returnDataBytes) if err == nil { @@ -153,12 +162,12 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string) // Otherwise we can't decode it, so put it directly in the error if errorMessage == "" { if len(returnDataBytes) > 0 { - errorMessage = i18n.NewError(ctx, msgs.MsgReturnValueNotDecoded, returnValue).Error() + errorMessage = i18n.NewError(ctx, msgs.MsgReturnValueNotDecoded, revertReason).Error() } else { errorMessage = i18n.NewError(ctx, msgs.MsgReturnValueNotAvailable).Error() } } - return &returnValue, &errorMessage + return &revertReason, &errorMessage } func (c *ethConnector) TransactionReceipt(ctx context.Context, req *ffcapi.TransactionReceiptRequest) (*ffcapi.TransactionReceiptResponse, ffcapi.ErrorReason, error) { @@ -178,7 +187,7 @@ func (c *ethConnector) TransactionReceipt(ctx context.Context, req *ffcapi.Trans var transactionErrorMessage *string if !isSuccess { - returnDataString, transactionErrorMessage = c.getErrorInfo(ctx, req.TransactionHash) + returnDataString, transactionErrorMessage = c.getErrorInfo(ctx, req.TransactionHash, ethReceipt.RevertReason) } ethReceipt.Logs = nil From c136537b4fa147f88629440f082fb28427818a9f Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 4 Apr 2024 16:54:45 +0100 Subject: [PATCH 2/8] Restore padding of leading 0 for odd-length hex strings Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 11 ++++++++++- internal/ethereum/get_receipt_test.go | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index 8495813..6976c8c 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -121,6 +121,15 @@ func ProtocolIDForReceipt(blockNumber, transactionIndex *fftypes.FFBigInt) strin return "" } +func padHexData(hexString string) string { + hexString = strings.TrimPrefix(hexString, "0x") + if len(hexString)%2 == 1 { + hexString = "0" + hexString + } + + return hexString +} + func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, revertFromReceipt *ethtypes.HexBytes0xPrefix) (pReturnValue *string, pErrorMessage *string) { var revertReason string @@ -151,7 +160,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, // See if the return value is using the default error you get from "revert" var errorMessage string - returnDataBytes, _ := hex.DecodeString(strings.TrimPrefix(revertReason, "0x")) + returnDataBytes, _ := hex.DecodeString(padHexData(revertReason)) if len(returnDataBytes) > 4 && bytes.Equal(returnDataBytes[0:4], defaultErrorID) { value, err := defaultError.DecodeCallDataCtx(ctx, returnDataBytes) if err == nil { diff --git a/internal/ethereum/get_receipt_test.go b/internal/ethereum/get_receipt_test.go index a952833..4bc450d 100644 --- a/internal/ethereum/get_receipt_test.go +++ b/internal/ethereum/get_receipt_test.go @@ -174,7 +174,7 @@ const sampleTransactionTraceBesu = `{ "0000000000000000000000000000000000000000000000000000000000000000" ], "storage": {}, - "reason": "08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000114e6f7420656e6f75676820746f6b656e73000000000000000000000000000000" + "reason": "8c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000114e6f7420656e6f75676820746f6b656e73000000000000000000000000000000" } ] }` From 15cfefbea44cfad720dfe42a107bb61a1dd4e90a Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 4 Apr 2024 17:00:20 +0100 Subject: [PATCH 3/8] Typo Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index 6976c8c..f8d92ce 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -143,7 +143,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, return nil, &msg } - revertReason := debugTrace.ReturnValue + revertReason = debugTrace.ReturnValue if revertReason == "" { // some clients (e.g. Besu) include the error reason on the final struct log if len(debugTrace.StructLogs) > 0 { From 7a846c568e71e5e75ccb2888ebf37153e10d2e3f Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Mon, 8 Apr 2024 11:22:14 +0100 Subject: [PATCH 4/8] Add more logs Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index f8d92ce..e8bd46d 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -144,6 +144,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, } revertReason = debugTrace.ReturnValue + log.L(ctx).Debugf("Revert reason from debug_traceTransaction: '%v'", revertReason) if revertReason == "" { // some clients (e.g. Besu) include the error reason on the final struct log if len(debugTrace.StructLogs) > 0 { @@ -156,6 +157,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, } else { log.L(ctx).Debug("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") revertReason = string(*revertFromReceipt) + log.L(ctx).Debugf("Revert reason from the receipt: '%v'", revertReason) } // See if the return value is using the default error you get from "revert" From 7afad6bcac7c876b20df7ff41f5b397c1f0ce0f7 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Mon, 8 Apr 2024 11:52:16 +0100 Subject: [PATCH 5/8] Add more logs Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index e8bd46d..f2face7 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -132,6 +132,8 @@ func padHexData(hexString string) string { func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, revertFromReceipt *ethtypes.HexBytes0xPrefix) (pReturnValue *string, pErrorMessage *string) { + log.L(ctx).Debugf("Revert from receipt passed in: '%v'", revertFromReceipt) + var revertReason string if revertFromReceipt == nil { log.L(ctx).Debug("No revert reason for the failed transaction found in the receipt. Calling debug_traceTransaction to retrieve it.") @@ -156,7 +158,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, } } else { log.L(ctx).Debug("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") - revertReason = string(*revertFromReceipt) + revertReason = revertFromReceipt.String() log.L(ctx).Debugf("Revert reason from the receipt: '%v'", revertReason) } From 1facd1923a059b0f14820c67f54cea469aee0a79 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Mon, 8 Apr 2024 13:36:21 +0100 Subject: [PATCH 6/8] Remove debug logs, add unit test to retain 100% coverage Signed-off-by: Matthew Whitehead --- internal/ethereum/get_receipt.go | 3 -- internal/ethereum/get_receipt_test.go | 45 +++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index f2face7..959271c 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -132,8 +132,6 @@ func padHexData(hexString string) string { func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, revertFromReceipt *ethtypes.HexBytes0xPrefix) (pReturnValue *string, pErrorMessage *string) { - log.L(ctx).Debugf("Revert from receipt passed in: '%v'", revertFromReceipt) - var revertReason string if revertFromReceipt == nil { log.L(ctx).Debug("No revert reason for the failed transaction found in the receipt. Calling debug_traceTransaction to retrieve it.") @@ -159,7 +157,6 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, } else { log.L(ctx).Debug("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") revertReason = revertFromReceipt.String() - log.L(ctx).Debugf("Revert reason from the receipt: '%v'", revertReason) } // See if the return value is using the default error you get from "revert" diff --git a/internal/ethereum/get_receipt_test.go b/internal/ethereum/get_receipt_test.go index 4bc450d..5aaa2e4 100644 --- a/internal/ethereum/get_receipt_test.go +++ b/internal/ethereum/get_receipt_test.go @@ -84,6 +84,24 @@ const sampleJSONRPCReceiptFailed = `{ "type": "0x0" }` +const sampleJSONRPCReceiptFailedWithRevertReason = `{ + "blockHash": "0x6197ef1a58a2a592bb447efb651f0db7945de21aa8048801b250bd7b7431f9b6", + "blockNumber": "0x7b9", + "contractAddress": "0x87ae94ab290932c4e6269648bb47c86978af4436", + "cumulativeGasUsed": "0x8414", + "effectiveGasPrice": "0x0", + "from": "0x2b1c769ef5ad304a4889f2a07a6617cd935849ae", + "gasUsed": "0x8414", + "logs": [], + "logsBloom": "0x00000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000100000000000000000002000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000800000000000000", + "status": "0", + "to": "0x302259069aaa5b10dc6f29a9a3f72a8e52837cc3", + "transactionHash": "0x7d48ae971faf089878b57e3c28e3035540d34f38af395958d2c73c36c57c83a2", + "transactionIndex": "0x1e", + "revertReason": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000001d5468652073746f7265642076616c756520697320746f6f20736d616c6c000000", + "type": "0x0" +}` + const sampleTransactionTraceGeth = `{ "gas": 23512, "failed": true, @@ -391,7 +409,6 @@ func TestGetReceiptErrorReasonErrorFromHexDecode(t *testing.T) { assert.Empty(t, reason) assert.False(t, res.Success) - } func TestGetReceiptErrorReasonErrorFromTrace(t *testing.T) { @@ -428,7 +445,6 @@ func TestGetReceiptErrorReasonErrorFromTrace(t *testing.T) { assert.Empty(t, reason) assert.False(t, res.Success) - } func TestGetReceiptErrorReasonErrorFromDecodeCallData(t *testing.T) { @@ -464,7 +480,32 @@ func TestGetReceiptErrorReasonErrorFromDecodeCallData(t *testing.T) { assert.Empty(t, reason) assert.False(t, res.Success) +} + +func TestGetReceiptErrorReasonErrorFromReceiptRevert(t *testing.T) { + ctx, c, mRPC, done := newTestConnector(t) + defer done() + + mRPC.On("CallRPC", mock.Anything, mock.Anything, "eth_getTransactionReceipt", + mock.MatchedBy(func(txHash string) bool { + assert.Equal(t, "0x7d48ae971faf089878b57e3c28e3035540d34f38af395958d2c73c36c57c83a2", txHash) + return true + })). + Return(nil). + Run(func(args mock.Arguments) { + err := json.Unmarshal([]byte(sampleJSONRPCReceiptFailedWithRevertReason), args[1]) + assert.NoError(t, err) + }) + mRPC.AssertNotCalled(t, "CallRPC", mock.Anything, mock.Anything, "debug_traceTransaction", mock.Anything) + var req ffcapi.TransactionReceiptRequest + err := json.Unmarshal([]byte(sampleGetReceipt), &req) + assert.NoError(t, err) + res, reason, err := c.TransactionReceipt(ctx, &req) + assert.NoError(t, err) + assert.Empty(t, reason) + assert.Contains(t, res.ExtraInfo.String(), "The stored value is too small") // Check the decoded revert reason string is present in extra-info + assert.False(t, res.Success) } func TestProtocolIDForReceipt(t *testing.T) { From bb1f5ad1d2301d7c3faf3d756fe5ef40ee431b73 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Mon, 8 Apr 2024 16:34:22 +0100 Subject: [PATCH 7/8] Update internal/ethereum/get_receipt.go Co-authored-by: Chengxuan Xing Signed-off-by: Matt Whitehead --- internal/ethereum/get_receipt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index 959271c..c45cef0 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -155,7 +155,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, } } } else { - log.L(ctx).Debug("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") + log.L(ctx).Trace("Revert reason is set in the receipt. Skipping call to debug_traceTransaction.") revertReason = revertFromReceipt.String() } From 50b17d6ec52f719debdeb5845f727fe0693d6a7f Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Mon, 8 Apr 2024 16:34:27 +0100 Subject: [PATCH 8/8] Update internal/ethereum/get_receipt.go Co-authored-by: Chengxuan Xing Signed-off-by: Matt Whitehead --- internal/ethereum/get_receipt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/ethereum/get_receipt.go b/internal/ethereum/get_receipt.go index c45cef0..97a1334 100644 --- a/internal/ethereum/get_receipt.go +++ b/internal/ethereum/get_receipt.go @@ -134,7 +134,7 @@ func (c *ethConnector) getErrorInfo(ctx context.Context, transactionHash string, var revertReason string if revertFromReceipt == nil { - log.L(ctx).Debug("No revert reason for the failed transaction found in the receipt. Calling debug_traceTransaction to retrieve it.") + log.L(ctx).Trace("No revert reason for the failed transaction found in the receipt. Calling debug_traceTransaction to retrieve it.") // Attempt to get the return value of the transaction - not possible on all RPC endpoints var debugTrace *txDebugTrace traceErr := c.backend.CallRPC(ctx, &debugTrace, "debug_traceTransaction", transactionHash)