Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Append ABI-decoded revert reason to message field in error responses #5706

Merged
merged 10 commits into from
Aug 4, 2023

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Jul 14, 2023

PR description

Todo:

  • eth_call unit tests
  • eth_estimateGas unit tests
  • Changelog

This PR does the following:

  • Adds a helper function to JsonRpcErrorResponse to decode revert reasons
  • Adds a private member variable reason to JsonRpcError which can optionally be set
  • Appends the reason to the response from JsonRpcError.getMessage() so that whenever the message for a JSON RPC error is retrieved, any reason that is set is included e.g. Execution reverted: ERC20: transfer amount exceeds balance

Note: this is proposed as a change to existing behaviour. I have assumed (rightly or wrongly) that existing applications will most likely use startsWith("Execution reverted") or other language equivalent, to parse error responses today. The fact that Quorum returns errors in this format (and that there isn't an option to exclude the decoded reason) suggests that any application not using startsWith semantics for error parsing is already brittle to different ethereum clients. It could be made configurable in Besu (perhaps with a --exclude-decoded-reason or similar option) but my personal view is to treat this as a fix to existing behaviour and document it in the changelog accordingly. I'm open to other opinions on this from reviewers.

Example error response before this PR:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -32000,
        "message": "Execution reverted",
        "data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002645524332303a207472616e7366657220616d6f756e7420657863656564732062616c616e63650000000000000000000000000000000000000000000000000000"
    }
}

Example error response with this PR:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -32000,
        "message": "Execution reverted: ERC20: transfer amount exceeds balance",
        "data": "0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002645524332303a207472616e7366657220616d6f756e7420657863656564732062616c616e63650000000000000000000000000000000000000000000000000000"
    }
}

Fixed Issue(s)

Fixes #5705

@github-actions
Copy link

github-actions bot commented Jul 14, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@matthew1001 matthew1001 force-pushed the revert-reason branch 3 times, most recently from c967d1a to c86967a Compare July 17, 2023 14:23
@matthew1001 matthew1001 marked this pull request as ready for review July 17, 2023 14:25
@matthew1001 matthew1001 added the doc-change-required Indicates an issue or PR that requires doc to be updated label Jul 17, 2023
Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001
Copy link
Contributor Author

Working through a failing unit test that passes locally...

Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001 matthew1001 marked this pull request as draft July 28, 2023 14:00
@matthew1001 matthew1001 marked this pull request as ready for review July 28, 2023 14:36
@matthew1001 matthew1001 force-pushed the revert-reason branch 2 times, most recently from ee1fe1d to 48d97f6 Compare July 28, 2023 14:50
@matthew1001 matthew1001 marked this pull request as draft July 28, 2023 15:24
@matthew1001 matthew1001 force-pushed the revert-reason branch 2 times, most recently from bd8fbe4 to e769fc2 Compare July 28, 2023 16:29
Signed-off-by: Matthew Whitehead <[email protected]>
@matthew1001 matthew1001 marked this pull request as ready for review July 28, 2023 16:30
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for the detailed PR description 👍

@@ -29,6 +38,9 @@ public class JsonRpcErrorResponse implements JsonRpcResponse {
private final JsonRpcError error;
@JsonIgnore private final RpcErrorType errorType;

// Encoding of "Error(string)" to check for at the start of the revert reason
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "Error(string)" prefix Besu-specific or in a spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an ethereum standard for the way revert reasons are encoded.

@siladu siladu merged commit a46cf27 into hyperledger:main Aug 4, 2023
8 checks passed
@matthew1001 matthew1001 deleted the revert-reason branch August 14, 2023 09:10
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Aug 14, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
…yperledger#5706)

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Simon Dudley <[email protected]>
Signed-off-by: garyschulte <[email protected]>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eth_call and eth_estimateGas only return encoded revert reason
3 participants