Need to document the FF1.3 revert error handling, and make the default safer

[matthew1001]

Firefly evmconnect automatically attempts to retrieve and decode revert error reasons for failed transactions.

This happens in the following cases:

1. When eth_estimateGas returns a failed transaction result
2. When a transaction that has specified its own gas limit retrieves the receipt for the transaction

We don't currently document the different mechanisms we use to retrieve error information, which are:

1. Get the revert reason from the receipt (if the EVM node has provided it)
2. Issue debug_traceTranasction to the node if the receipt doesn't contain it

The latter is a costly operation that could have a very negative impact on the stability and performance of an EVM chain under medium-heavy transaction workload. It would be safe to default to not issuing debug_traceTransaction, and allow the user to enable it manually.

[nguyer]

Thanks @matthew1001 for raising this. Trying to understand what is needed for this one. It looks like hyperledger/firefly-evmconnect#130 only includes new unit tests. Are there additional code changes in EVMConnect require for this?

[matthew1001]

Yeah I started by checking the current behaviour and adding a test to cover a custom-error case. I'm going to look at a config setting tomorrow to make debug_traceTransaction optional and non-default. Then I'll raise a separate PR to doc the current behaviour and possible side effects of enabling trace transaction

[matthew1001]

@nguyer @peterbroadhurst following the discussion on custom errors I've double checked the current behaviour and since Peter's PR hyperledger/firefly-evmconnect#112 the extra info looks like:

{
  "contractAddress":"0x87ae94ab290932c4e6269648bb47c86978af4436",
  "cumulativeGasUsed":"33812",
  "from":"0x2b1c769ef5ad304a4889f2a07a6617cd935849ae",
  "to":"0x302259069aaa5b10dc6f29a9a3f72a8e52837cc3",
  "gasUsed":"33812",
  "status":"0",
  "errorMessage":"FF23053: Error return value for custom error: 0x1320fa6a00000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000010", 
"returnValue":"0x1320fa6a00000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000010"
}

Since FFTM doesn't have access to the error signature/ABI I think the consensus from other discussions I've seen is to look at updates to FF core to decode the returnValue itself as it does have access to the ABI.

So for FF 1.3 my intention is to:

• Document the various revert error behaviours (as per the issue description and the new returnValue field)
• Make the use of debug_traceTransaction configurable but non-default

Let me know if there's anything else you think I've missed for the FF 1.3 must-do work.

[matthew1001]

@peterbroadhurst I've raised PR hyperledger/firefly-evmconnect#131 for making debug_traceTransaction configurable, defaulting to off if you could take a look?

[matthew1001]

@peterbroadhurst @nguyer I've raised #1493 to cover the documentation task

[matthew1001]

All of the PRs related to this have been merged so closing as complete

[peterbroadhurst]

@matthew1001 - seems I missed the chance to review.

Location of the docs

I see they got tucked all the way down in the depths of the architecture section, as a "how it works" section in the connector framework, whereas this is all EVM specific information about how one particular implementation of that works:
https://github.com/kaleido-io/firefly/blob/e8bbcc8b88819ed3a39d71f6ef9ac9ee3adce8b5/docs/architecture/blockchain_connector_framework.md#format-of-a-firefly-evmconnect-error-message

So I think the placement is wrong on a few levels.

Wonder if you'd mind moving it to a suitable section. Could be:

• FAQ / EVM Revert Errors
• Reference / EVM Revert errors

Missing TL/DR

I actually don't even know the answer reading through the information...

If I configure Hyperledger FireFly with Hyperledger Besu, using the default configuration (which I assert in FF 1.3 must include setting --revert-reason-enabled)... will I get all my errors decoded? or just some of them?

Missing description of the gap we know we're shipping v1.3 without closing

I also couldn't see the gap we I know to be true related to debug_traceTransaction not handling custom errors.

e.g. if you're not using --revert-reason-enabled (maybe if you are... ref above unanswered question), and you're not estimating gas on every transaction, you will not get useful errors from async receipts with customer errors

Should we link to #1466 from the docs for this?

[matthew1001]

Sorry I should have pinged you for a review separate to Nicko's before merging @peterbroadhurst I'll have a look at refactoring into a different structure along the lines you've suggested and try to clarify what behaviour you do and don't get with 1.3.

[matthew1001]

I've updated the docs as follows:

• Moved the topic under "Reference/Blockchain operation errors"
• Added a section to the topic which more clearly describes how FF 1.3 + Besu 24.3.0 works, both with and without revert-reason-enabled set
• Added a reference to the git issue that describes the custom error limitation

@peterbroadhurst let me know what you think of the latest shape of the docs

[EnriqueL8]

@matthew1001 @peterbroadhurst PR has been merged, I think we can close this one?

[matthew1001]

I think so @EnriqueL8 Everything we intended to complete for 1.3 has been done I believe. We can discuss the follow on work to expand custom errors in FF core as a possible 1.3.1 item?

[EnriqueL8]

Awesome, closing