Proper Revert reasons #342

cburgdorf · 2021-03-26T17:52:07Z

What was wrong?

As described in #288 we do currently not support revert reason strings in assert statements.

How was it fixed?

Added a bit machinery to test solidity fixtures and added a new category of tests for such cases
Added tests to prove how solidity handles revert reason strings
Added a new runtime function revert_with_reason_string which follows the error encoding that solidity uses: https://docs.soliditylang.org/en/latest/control-structures.html#revert
Added tests for this new runtime method and as a byproduct refactored some helpers to allow detailed testing of reverts as well as setting up a runtime that exposes static strings
Hooked up the mapping of assert to use the new runtime method when a string reason is given
Added several test cases to prove the functionality of assert strings with static strings, longer strings, passed in strings

codecov-io · 2021-03-26T18:21:18Z

Codecov Report

Merging #342 (7626e6d) into master (dd017d8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   92.60%   92.60%           
=======================================
  Files          56       56           
  Lines        3882     3882           
=======================================
  Hits         3595     3595           
  Misses        287      287

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd017d8...7626e6d. Read the comment docs.

cburgdorf · 2021-04-15T09:12:11Z

compiler/tests/utils.rs

@@ -225,6 +280,36 @@ pub fn deploy_contract(
    panic!("Failed to create contract")
 }

+pub fn compile_solidity_contract(name: &str, solidity_src: &str) -> Result<(String, String), ()> {


@satyamakgec This is all just WIP code in this PR but I wanted to make you aware of it because it adds a way to run tests against solidity code (which I needed to prove some assumptions about revert reason encoding). Since the differential contract testing will also need to run solidity code I thought you might find it useful to look at.

You are a saviour @cburgdorf I was worried about this today when I was doing researching and planning over the differential testing.

Very useful tools!

codecov-commenter · 2021-04-19T21:17:04Z

Codecov Report

Merging #342 (90601cb) into master (32c3f00) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 90601cb differs from pull request most recent head cf736bf. Consider uploading reports for the commit cf736bf to get more accurate results

@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   92.79%   92.80%           
=======================================
  Files          59       59           
  Lines        4012     4015    +3     
=======================================
+ Hits         3723     3726    +3     
  Misses        289      289

Impacted Files	Coverage Δ
compiler/src/yul/mappers/functions.rs	`100.00% <ø> (ø)`
compiler/src/yul/runtime/functions/data.rs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32c3f00...cf736bf. Read the comment docs.

cburgdorf · 2021-04-20T10:02:05Z

compiler/src/yul/runtime/functions/data.rs

+        function revert_with_reason_string(reason) {
+            (let ptr := alloc(4))
+            // Function selector for Error(string)
+            (mstore(ptr, 3963877391197344453575983046348115674221700746820753546331534351508065746944))


I wonder why I can't do this instead:

(let ptr := alloc_mstoren(3963877391197344453575983046348115674221700746820753546331534351508065746944, 4))

Here's my understanding of what's happening here:

3963877391197344453575983046348115674221700746820753546331534351508065746944 is the decimal number equivalent to the selector (0x08C379A0) padded with 28 zero bytes on the right. We're allocating 4 bytes and then writing the decimal selector to 32 bytes of memory. Since the number in bytes is all zeros on the right side, we don't actually end up writing any bits past the highest allocated memory address, so things are technically fine.

What we want is:

let ptr := alloc_mstoren(0x08C379A0, 4)

Also,

let ptr := alloc_mstoren(3963877391197344453575983046348115674221700746820753546331534351508065746944, 4)

is equivalent to:

let ptr := alloc_mstoren(0x08C379A000000000000000000000000000000000000000000000000000000000, 4)

which will only write the 4 least-significant bytes to memory (zero).

Ooooh! Now it all makes sense. THANK YOU 🙏

cburgdorf · 2021-04-20T10:08:58Z

compiler/src/yul/runtime/functions/data.rs

        mcopys(),
+        mloadn(),
+        mstoren(),
+        revert_with_reason_string(),


That's the only new one. I just sorted them alphabetically

cburgdorf · 2021-04-20T10:16:53Z

compiler/src/yul/runtime/functions/data.rs

+            (mstore(ptr, 3963877391197344453575983046348115674221700746820753546331534351508065746944))
+
+            // Write the (fixed) data offset into the next 32 bytes of memory
+            (let __ := alloc_mstoren(0x0000000000000000000000000000000000000000000000000000000000000020, 32))


I have to confess that I don't fully grasp the rationale for this ☝️ I tried to follow the conversation in ethereum/EIPs#838 but it doesn't go into much detail (or not enough for me to properly understand it). I think this data offset of 32 is just meant to say that the actual reason string is found at an offset of 32 bytes simply because thats how ABI encoded strings are defined. But to me it seems pointless to have this. After all Error(string) returns a string so it should be clear that there's a 32 data offset but maybe I'm just misunderstanding the whole thing haha. Anyway, this is how the encoding works and the tests prove that it is working in the same way as solidity.

Yeah, just how ABI encoding works. It makes sense when there's more items being encoded.

It makes sense when there's more items being encoded.

Do you have an example where the dataoffset becomes useful? Let's say we have Error(AnyOtherABIType). Why would I need the dataoffset for anything? Wouldn't it work just as fine without it? Whatever follows the selector would be the data that is decoded as the ABI type that is specified with the selector.

If we're encoding something like (string, string), we need to provide the offsets to both pieces of dynamically sized data. This way we only need to read the offset, size, and the data when reading each string. If offsets were not provided, we would need to read each dynamic size and sum them together to get the offset. This would make implementations much more error prone.

For reference, the encoding would look roughly like this:

[offset_1, offset_2], [(size_1, data_1), (size_2, data_2)]

With a single dynamic element, it's not necessary to provide this offset, but it's best to make a few simple rules with encoding schemes and stick with them - even if it seems silly at times.

if offsets were not provided, we would need to read each dynamic size and sum them together to get the offset

But isn't that what I would also have to do if I'm reading the output of pub foo -> (string, string)? In that case, the returned data will also only be [(size_1, data_1), (size_2, data_2)] without providing me the offsets separately, no?

The encoding of (string, string) would include the offsets of each string item. Then to decode, we read the offset, size, and data for each string.

Is there something I'm missing?

The examples are quite helpful.

cburgdorf · 2021-04-20T10:18:00Z

compiler/tests/features.rs

-            evm::Capture::Exit((evm::ExitReason::Revert(_), _))
-        ));
+        match exit1 {
+            evm::Capture::Exit((evm::ExitReason::Revert(_), output)) => assert_eq!(output.len(), 0),


Without a revert string the data will simply be empty (NOT the same as having a revert string of "")

cburgdorf · 2021-04-20T10:23:00Z

compiler/tests/runtime.rs

+        test_runtime_functions_revert(
+            &mut executor,
+            Runtime::default()
+                .with_data(


I introduced the Runtime object with the builder pattern to not introduce a data parameter to every test that most of them would not end up using.

We could actually go a bit further and incorporate the testing functions on the Runtime object, too so that the code becomes something like:

test_runtime_functions_revert( Runtime::default() .with_data( vec![yul::Data { name: keccak::full(reason.as_bytes()), value: reason.to_owned() }] ) .with_test_statements( statements! { (let reason := load_data_string((dataoffset([literal_expression! { (reason_id) }])), (datasize([literal_expression! { (reason_id) }])))) (revert_with_reason_string(reason)) }) .execute(executor) .expect_revert_with(reason) );

I'm happy to create an issue / PR if you happen to like it @g-r-a-n-t

Seems like a more rusty way of doing things 👍 I'm open to refactoring this way.

cburgdorf · 2021-04-20T10:24:08Z

compiler/tests/solidity.rs

+
+        let exit = harness.capture_call(&mut executor, method, &[]);
+
+        let expected_reason = format!("0x{}", hex::encode(encode_error_reason(reason)));


This is proving that our encode_error_reason is producing the same encoding that solidity does

cburgdorf · 2021-04-20T10:26:15Z

@g-r-a-n-t Celebrating your return with this PR to review

🏄‍♂️

g-r-a-n-t

Looks good.

Added some clarification around the selector stuff and suggested using pop in a few places.

g-r-a-n-t · 2021-04-21T00:52:48Z

compiler/src/yul/runtime/functions/data.rs

+        function revert_with_reason_string(reason) {
+            (let ptr := alloc(4))
+            // Function selector for Error(string)
+            (mstore(ptr, 3963877391197344453575983046348115674221700746820753546331534351508065746944))


Here's my understanding of what's happening here:

3963877391197344453575983046348115674221700746820753546331534351508065746944 is the decimal number equivalent to the selector (0x08C379A0) padded with 28 zero bytes on the right. We're allocating 4 bytes and then writing the decimal selector to 32 bytes of memory. Since the number in bytes is all zeros on the right side, we don't actually end up writing any bits past the highest allocated memory address, so things are technically fine.

What we want is:

let ptr := alloc_mstoren(0x08C379A0, 4)

Also,

let ptr := alloc_mstoren(3963877391197344453575983046348115674221700746820753546331534351508065746944, 4)

is equivalent to:

let ptr := alloc_mstoren(0x08C379A000000000000000000000000000000000000000000000000000000000, 4)

which will only write the 4 least-significant bytes to memory (zero).

g-r-a-n-t · 2021-04-21T00:55:53Z

compiler/src/yul/runtime/functions/data.rs

+            (mstore(ptr, 3963877391197344453575983046348115674221700746820753546331534351508065746944))
+
+            // Write the (fixed) data offset into the next 32 bytes of memory
+            (let __ := alloc_mstoren(0x0000000000000000000000000000000000000000000000000000000000000020, 32))


Yeah, just how ABI encoding works. It makes sense when there's more items being encoded.

g-r-a-n-t · 2021-04-21T01:31:31Z

compiler/src/yul/runtime/functions/data.rs

+            (let reason_size := mloadn(reason, 32))
+
+            //Copy the whole reason string (length + data) to the current segment of memory
+            (__ := mcopym(reason , (add(reason_size, 32))))


We can also pop these values right off the stack.

(pop((mcopym(reason , (add(reason_size, 32))))))

All function arguments are matched as token trees, so expressions like mcopym(reason , (add(reason_size, 32))) need to be wrapped in parens. Biggest downside with these macros imo.

Ah, right, thanks. Does it make any practical difference whether to assign them to an unused variable or pop them?

The optimizer will probably convert these to pops anyway.

Even if we weren't running the optimizer, I don't think it would make any difference in terms of gas. We would just keep these values on the stack a bit longer.

g-r-a-n-t · 2021-04-21T01:35:43Z

compiler/tests/utils.rs

@@ -225,6 +280,36 @@ pub fn deploy_contract(
    panic!("Failed to create contract")
 }

+pub fn compile_solidity_contract(name: &str, solidity_src: &str) -> Result<(String, String), ()> {


Very useful tools!

g-r-a-n-t · 2021-04-21T01:36:34Z

compiler/tests/utils.rs

+}
+
+#[allow(dead_code)]
+impl Runtime {


Makes sense 👍

g-r-a-n-t · 2021-04-21T01:38:29Z

compiler/tests/runtime.rs

+        test_runtime_functions_revert(
+            &mut executor,
+            Runtime::default()
+                .with_data(


Seems like a more rusty way of doing things 👍 I'm open to refactoring this way.

Closes ethereum#288

cburgdorf force-pushed the christoph/feat/rever_reasons branch 2 times, most recently from 2dc9387 to 02c614e Compare April 15, 2021 09:04

cburgdorf commented Apr 15, 2021

View reviewed changes

Add tests that cover how solidity handles revert reason strings

b2f6347

cburgdorf force-pushed the christoph/feat/rever_reasons branch 2 times, most recently from 9a69ccf to 1864523 Compare April 19, 2021 20:42

cburgdorf force-pushed the christoph/feat/rever_reasons branch 3 times, most recently from f2b37f1 to 4e27a29 Compare April 20, 2021 10:01

cburgdorf commented Apr 20, 2021

View reviewed changes

cburgdorf marked this pull request as ready for review April 20, 2021 10:25

cburgdorf requested a review from g-r-a-n-t April 20, 2021 10:25

g-r-a-n-t approved these changes Apr 21, 2021

View reviewed changes

cburgdorf added 2 commits April 21, 2021 10:39

Add revert_with_reason_string runtime function

78518a5

Add support for revert reason strings in assert statement

cf736bf

Closes ethereum#288

cburgdorf force-pushed the christoph/feat/rever_reasons branch from 4e27a29 to cf736bf Compare April 21, 2021 08:39

cburgdorf merged commit 22771e7 into ethereum:master Apr 21, 2021

This was referenced Apr 21, 2021

Refactoring of runtime testing helpers #360

Merged

Implement revert reasons #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper Revert reasons #342

Proper Revert reasons #342

cburgdorf commented Mar 26, 2021 •

edited

Loading

codecov-io commented Mar 26, 2021

cburgdorf Apr 15, 2021

satyamakgec Apr 15, 2021

g-r-a-n-t Apr 21, 2021

codecov-commenter commented Apr 19, 2021 •

edited

Loading

cburgdorf Apr 20, 2021

g-r-a-n-t Apr 21, 2021

cburgdorf Apr 21, 2021

cburgdorf Apr 20, 2021

cburgdorf Apr 20, 2021 •

edited

Loading

g-r-a-n-t Apr 21, 2021

cburgdorf Apr 21, 2021

g-r-a-n-t Apr 21, 2021 •

edited

Loading

cburgdorf Apr 22, 2021

g-r-a-n-t Apr 26, 2021

g-r-a-n-t Apr 26, 2021

cburgdorf Apr 20, 2021

cburgdorf Apr 20, 2021

g-r-a-n-t Apr 21, 2021

cburgdorf Apr 20, 2021

cburgdorf commented Apr 20, 2021

g-r-a-n-t left a comment

g-r-a-n-t Apr 21, 2021

g-r-a-n-t Apr 21, 2021

g-r-a-n-t Apr 21, 2021

cburgdorf Apr 21, 2021

g-r-a-n-t Apr 21, 2021

g-r-a-n-t Apr 21, 2021

g-r-a-n-t Apr 21, 2021

g-r-a-n-t Apr 21, 2021


		let exit = harness.capture_call(&mut executor, method, &[]);

		let expected_reason = format!("0x{}", hex::encode(encode_error_reason(reason)));

Proper Revert reasons #342

Proper Revert reasons #342

Conversation

cburgdorf commented Mar 26, 2021 • edited Loading

What was wrong?

How was it fixed?

codecov-io commented Mar 26, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-r-a-n-t Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Apr 20, 2021

g-r-a-n-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Mar 26, 2021 •

edited

Loading

codecov-commenter commented Apr 19, 2021 •

edited

Loading

cburgdorf Apr 20, 2021 •

edited

Loading

g-r-a-n-t Apr 21, 2021 •

edited

Loading