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

Extract revert messages #6226

Merged
merged 31 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
ca399c7
Create unit test
emlautarom1 Oct 25, 2023
1fd7512
Initial implementation
emlautarom1 Oct 25, 2023
88101b9
Lift const to property
emlautarom1 Oct 25, 2023
adb2ce9
Remove unused suppress
emlautarom1 Oct 25, 2023
dea12cd
Extract test case
emlautarom1 Oct 25, 2023
72504b6
Add extra test case
emlautarom1 Oct 25, 2023
1239db4
Remove unused imports
emlautarom1 Oct 25, 2023
073e8ef
Prefix with `Reverted`
emlautarom1 Oct 25, 2023
c721e79
Implement `Reverted` prefix
emlautarom1 Oct 25, 2023
8ee2d7f
Formatting
emlautarom1 Oct 25, 2023
159f2a2
Make length check explicit
emlautarom1 Oct 26, 2023
4344ef0
Simplify logic, name parameters better for readability
LukaszRozmej Oct 26, 2023
277c92d
improve naming
LukaszRozmej Oct 26, 2023
d9b3e48
Remove redundant unsafe
emlautarom1 Oct 26, 2023
076bf5c
Use Keccak.Compute
emlautarom1 Oct 26, 2023
1aea8fa
Add `Panic(uint256)` test cases
emlautarom1 Oct 26, 2023
8d44192
Support `Panic(uint256)`
emlautarom1 Oct 26, 2023
2e2328f
Introduce `WordSize` const
emlautarom1 Oct 26, 2023
9246b40
Initial 'TryUnpackSpecialFunction'
emlautarom1 Oct 26, 2023
d85ec07
Subsume `TryUnpackPanicFunctionMessage` with `TryUnpackSpecialFunctio…
emlautarom1 Oct 26, 2023
af73bfc
Initial `SliceAndMove`
emlautarom1 Oct 26, 2023
bf32923
Replace with `TakeAndMove`
emlautarom1 Oct 26, 2023
c262a7f
Simplify `TryGetErrorMessage` (1)
emlautarom1 Oct 26, 2023
6561eb0
Revert `TryGetErrorMessage`
emlautarom1 Oct 26, 2023
b2a04bc
Inline method
emlautarom1 Oct 26, 2023
1803115
Reorder checks
emlautarom1 Oct 26, 2023
6aa5cfe
Rename
emlautarom1 Oct 26, 2023
91cdc4a
Lift prefix concat
emlautarom1 Oct 26, 2023
19d066f
Add checks
emlautarom1 Oct 26, 2023
6603120
Move Span extension
emlautarom1 Oct 26, 2023
056a460
Add invalid test cases
emlautarom1 Oct 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 51 additions & 4 deletions src/Nethermind/Nethermind.Evm.Test/TransactionSubstateTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using System.Collections.Generic;
using FluentAssertions;
using Nethermind.Core;
using Nethermind.Core.Extensions;
Expand All @@ -14,12 +15,15 @@ public class TransactionSubstateTests
[Test]
public void should_return_proper_revert_error_when_there_is_no_exception()
{
byte[] data = {0, 0, 0, 0,
byte[] data =
{
0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x20,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x5,
0x05, 0x06, 0x07, 0x08, 0x09};
0x05, 0x06, 0x07, 0x08, 0x09
};
ReadOnlyMemory<byte> readOnlyMemory = new(data);
TransactionSubstate transactionSubstate = new(readOnlyMemory,
0,
Expand Down Expand Up @@ -64,16 +68,59 @@ public void should_return_proper_revert_error_when_revert_custom_error_badly_imp
transactionSubstate.Error.Should().Be($"Reverted {hex}");
}


private static IEnumerable<(byte[], string)> ErrorFunctionTestCases()
{
yield return (
new byte[]
{
0x08, 0xc3, 0x79, 0xa0, // Function selector for Error(string)
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, // Data offset
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1a, // String length
0x4e, 0x6f, 0x74, 0x20, 0x65, 0x6e, 0x6f, 0x75, 0x67, 0x68, 0x20, 0x45, 0x74, 0x68, 0x65, 0x72, 0x20, 0x70, 0x72, 0x6f, 0x76, 0x69, 0x64, 0x65, 0x64, 0x2e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // String data
},
"Reverted Not enough Ether provided.");

yield return (
new byte[]
{
0x08, 0xc3, 0x79, 0xa0,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x12,
0x52, 0x65, 0x71, 0x3a, 0x3a, 0x55, 0x6e, 0x41, 0x75, 0x74, 0x68, 0x41, 0x75, 0x64, 0x69, 0x74, 0x6f, 0x72, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
},
"Reverted Req::UnAuthAuditor");
}

[Test]
[TestCaseSource(nameof(ErrorFunctionTestCases))]
public void should_return_proper_revert_error_when_using_Error_function((byte[] data, string expected) tc)
{
// See: https://docs.soliditylang.org/en/latest/control-structures.html#revert
ReadOnlyMemory<byte> readOnlyMemory = new(tc.data);
TransactionSubstate transactionSubstate = new(
readOnlyMemory,
0,
new ArraySegment<Address>(),
new LogEntry[] { },
true,
true);

transactionSubstate.Error.Should().Be(tc.expected);
}

[Test]
[Ignore("Badly implemented")]
public void should_return_proper_revert_error_when_revert_custom_error()
{
byte[] data = {
byte[] data =
{
0x22, 0x02, 0x66, 0xb6, // Keccak of `FailedOp(uint256,string)` == 0x220266b6
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x17,
0x41, 0x41, 0x32, 0x31, 0x20, 0x64, 0x69, 0x64, 0x6e, 0x27, 0x74, 0x20, 0x70, 0x61, 0x79, 0x20, 0x70, 0x72, 0x65, 0x66, 0x75, 0x6e, 0x64, // "AA21 didn't pay prefund"
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
};
ReadOnlyMemory<byte> readOnlyMemory = new(data);
TransactionSubstate transactionSubstate = new(
readOnlyMemory,
Expand Down
34 changes: 34 additions & 0 deletions src/Nethermind/Nethermind.Evm/TransactionSubstate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class TransactionSubstate
private const int RevertPrefix = 4;

private const string RevertedErrorMessagePrefix = "Reverted ";
Copy link

Choose a reason for hiding this comment

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

If there was an extra colon in the prefix, i.e. Reverted: {message} this would become compatible with Geth. I want to point it out since you're already making a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an ongoing discussion on standarization of error messages: ethereum/execution-apis#463.

private readonly byte[] ErrorFunctionSelector = { 0x08, 0xc3, 0x79, 0xa0 };

public bool IsError => Error is not null && !ShouldRevert;
public string? Error { get; }
Expand Down Expand Up @@ -68,6 +69,7 @@ public TransactionSubstate(EvmExceptionType exceptionType, bool isTracerConnecte

ReadOnlySpan<byte> span = Output.Span;
Error = TryGetErrorMessage(span)
?? TryUnpackRevertMessage(span)
?? DefaultErrorMessage(span);
}

Expand Down Expand Up @@ -104,4 +106,36 @@ private string DefaultErrorMessage(ReadOnlySpan<byte> span)
return null;
}
}

private unsafe string? TryUnpackRevertMessage(ReadOnlySpan<byte> span)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be unified with TryGetErrorMessage? Looks similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are similar but not the same:

  • A specific prefix is required.
  • The span has a required minimum length, not a specific length.
  • The returned error message is decoded as UTF8.

Still, maybe there's something to be done to reduce the amount of code. What do you think @benaadams?

{
if (span.Length < RevertPrefix + sizeof(UInt256) * 2)
{
return null;
}

if (!span[..RevertPrefix].SequenceEqual(ErrorFunctionSelector))
{
return null;
}

try
{
int start = (int)new UInt256(span.Slice(RevertPrefix, sizeof(UInt256)), isBigEndian: true);
if (start != sizeof(UInt256))
{
return null;
}

int length = (int)new UInt256(span.Slice(RevertPrefix + sizeof(UInt256), sizeof(UInt256)), isBigEndian: true);
ReadOnlySpan<byte> binaryMessage = span.Slice(RevertPrefix + sizeof(UInt256) + sizeof(UInt256), length);
string message = string.Concat(RevertedErrorMessagePrefix, System.Text.Encoding.UTF8.GetString(binaryMessage));

return message;
}
catch (Exception e) when (e is OverflowException or ArgumentOutOfRangeException)
{
return null;
}
}
}
Loading