Panic with error codes. #10013

chriseth · 2020-10-12T16:18:00Z

TODO:

document the codes and the behaviour (maybe in the checked PR)

chriseth · 2020-10-14T16:09:14Z

This actually uses revert also on EVM versions that do not support it which causes an "invalid opcode" exception. I guess this is fine?

ekpyron · 2020-10-14T16:10:23Z

This actually uses revert also on EVM versions that do not support it which causes an "invalid opcode" exception. I guess this is fine?

Probably?

leonardoalt · 2020-10-14T16:11:15Z

This actually uses revert also on EVM versions that do not support it which causes an "invalid opcode" exception. I guess this is fine?

Probably?

I guess?

chriseth · 2020-10-14T16:20:17Z

test/libsolidity/semanticTests/enums/enum_explicit_overflow_homestead.sol

+}
+
+// ====
+// EVMVersion: <byzantium


This test runs on pre-byzantium and shows that invalid enum conversions also revert in these cases, but do not provide an error message (because the revert opcode is not available).

chriseth · 2020-10-19T19:05:42Z

Should use hex codes in the yul representation of the panic function.

hrkrshnn

Started reviewing this yesterday; some old comments. Will review the rest soon.

docs/control-structures.rst

leonardoalt

Some comments about docs and code, have not checked tests yet.

leonardoalt · 2020-10-20T13:33:52Z

docs/control-structures.rst

+#. 0x51: If you call a zero-initialized variable of internal function type.
+
+The ``require`` function either creates an error of type ``Error(string)``
+or ar error without ary error data and it


Suggested change

or ar error without ary error data and it

or an error without any error data and it

docs/control-structures.rst

leonardoalt · 2020-10-20T13:47:47Z

libsolidity/codegen/YulUtilFunctions.cpp

+		if (_revertOnFailure)
+			templ("failure", "revert(0, 0)");
+		else
+			templ("failure", panicFunction(panicCode) + "()");


What exactly would trigger this? ABI decoding for example?

No, abi decoding failure results in revert. _revertOnFailure is currently only false for enum conversion checks.

leonardoalt · 2020-10-20T13:49:04Z

libsolidity/codegen/YulUtilFunctions.cpp

@@ -3411,7 +3415,7 @@ string YulUtilFunctions::storageSetToZeroFunction(Type const& _type)
 			)")
 			("functionName", functionName)
 			("clearStruct", clearStorageStructFunction(dynamic_cast<StructType const&>(_type)))
-			("panic", panicFunction())
+			("panic", panicFunction(PanicCode::Generic))


When would this and the above happen?

Only though a bug in the compiler. We hope that the check is optimized away.

leonardoalt · 2020-10-20T13:50:30Z

libsolutil/ErrorCodes.h

+namespace solidity::util
+{
+
+


Extra blank

leonardoalt · 2020-10-20T13:51:18Z

libyul/backends/evm/EVMDialect.cpp

@@ -378,6 +378,7 @@ EVMDialectTyped::EVMDialectTyped(langutil::EVMVersion _evmVersion, bool _objectA
 		BuiltinContext&,
 		std::function<void(Expression const&)> _visitExpression
 	) {
+		// TODO this should use a Panic.


In this PR?

No, this dialect is not really used yet.

test/ExecutionFramework.cpp

hrkrshnn · 2020-10-22T08:48:03Z

libsolidity/codegen/ArrayUtils.cpp

+				if iszero(length) {
+					mstore(0, <panicSelector>)
+					mstore(4, <emptyArrayPop>)
+					revert(0, 0x24)


Why not make a call to YulUtilFunctions::panicFunction(util::PanicCode::EmptyArrayPop)

hrkrshnn · 2020-10-22T08:52:10Z

libsolidity/codegen/CompilerUtils.cpp

@@ -810,7 +810,7 @@ void CompilerUtils::convertType(
 			if (_asPartOfArgumentDecoding)
 				m_context.appendConditionalRevert(false, "Enum out of range");
 			else
-				m_context.appendConditionalInvalid();
+				m_context.appendConditionalPanic(util::PanicCode::EnumConversionError);


In the line above, for "Enum out of range", shouldn't this also be a panic?

Or is this left for the future?

No, it is a revert ("Error()") and not a panic.

hrkrshnn · 2020-10-22T08:57:13Z

libsolidity/codegen/ContractCompiler.cpp

+			mstore8(codepos, 0x73)
+			return(codepos, subSize)
+		}
+		)")("panicSig", util::selectorFromSignature("Panic(uint256)").str()).render(),


Suggested change

)")("panicSig", util::selectorFromSignature("Panic(uint256)").str()).render(),

)")

("panicSig", util::selectorFromSignature("Panic(uint256)").str()).render(),

Not sure if that's the right coding style.

It's not the separator between the ()-elements that is wrapping, but the content inside a single ()...

hrkrshnn · 2020-10-22T08:57:39Z

libsolidity/codegen/ContractCompiler.cpp

+				mstore(0, <panicSig>)
+				mstore(4, 0)
+				revert(0, 0x24)
+			}


I agree. Is there a reason we are inlining it ourselves?

test/cmdlineTests/exp_base_literal/output

test/libsolidity/semanticTests/types/mapping_enum_key_library_encoderV2.sol

hrkrshnn · 2020-10-22T16:48:36Z

libsolidity/codegen/YulUtilFunctions.cpp

-				invalid()
+				mstore(0, <selector>)
+				mstore(4, <code>)
+				revert(0, 0x24)


Our selector is 4 byte and code is 2 byte? Can't we pack everything in 32 bytes?

We want to use ABI encoding. Furthermore, there is a missing feature in the RPC interface to the node implementations in that they do not return the success code of a transaction. Computing the length of the return value mod 32 is an indication of whether it succeeded or not.

chriseth · 2020-10-22T17:27:35Z

Rebased and squashed.

chriseth force-pushed the errorCodesPanic branch from d15ea0b to 2eb49f0 Compare October 13, 2020 12:43

chriseth changed the base branch from develop to breaking October 13, 2020 12:50

chriseth force-pushed the errorCodesPanic branch 3 times, most recently from 23e0f55 to 86b991f Compare October 13, 2020 16:50

chriseth marked this pull request as ready for review October 13, 2020 16:58

chriseth force-pushed the errorCodesPanic branch 4 times, most recently from 6d89d37 to 5a8df54 Compare October 14, 2020 15:42

chriseth force-pushed the errorCodesPanic branch from 5a8df54 to 53941f1 Compare October 14, 2020 16:19

chriseth commented Oct 14, 2020

View reviewed changes

chriseth mentioned this pull request Oct 19, 2020

Checked arithmetic by default. #9465

Merged

5 tasks

chriseth force-pushed the errorCodesPanic branch from 52aab54 to 107b564 Compare October 19, 2020 17:04

chriseth force-pushed the errorCodesPanic branch 2 times, most recently from 2713e74 to 3b99ed9 Compare October 20, 2020 13:11

hrkrshnn reviewed Oct 20, 2020

View reviewed changes

leonardoalt reviewed Oct 20, 2020

View reviewed changes

hrkrshnn reviewed Oct 22, 2020

View reviewed changes

test/cmdlineTests/exp_base_literal/output Show resolved Hide resolved

leonardoalt reviewed Oct 22, 2020

View reviewed changes

test/libsolidity/semanticTests/types/mapping_enum_key_library_encoderV2.sol Show resolved Hide resolved

hrkrshnn reviewed Oct 22, 2020

View reviewed changes

leonardoalt approved these changes Oct 22, 2020

View reviewed changes

chriseth added 3 commits October 22, 2020 19:25

Ignore return error data for pre-byzantium.

b559de1

Panic codes.

bfd2674

Update tests.

60af9d2

chriseth added 4 commits October 22, 2020 19:25

Update gas costs.

6d3528b

Enum panic test for pre-byzantium.

93642f9

Changelog entry.

fae1773

Document Panic.

70e7326

chriseth force-pushed the errorCodesPanic branch from 6e129e7 to 70e7326 Compare October 22, 2020 17:26

leonardoalt approved these changes Oct 22, 2020

View reviewed changes

chriseth merged commit f22cb64 into breaking Oct 22, 2020

chriseth deleted the errorCodesPanic branch October 22, 2020 17:38

chriseth mentioned this pull request Nov 2, 2020

Move away from INVALID and use custom error types instead. #9824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Panic with error codes. #10013

Panic with error codes. #10013

chriseth commented Oct 12, 2020 •

edited

Loading

chriseth commented Oct 14, 2020

ekpyron commented Oct 14, 2020

leonardoalt commented Oct 14, 2020

chriseth Oct 14, 2020

chriseth commented Oct 19, 2020

hrkrshnn left a comment

leonardoalt left a comment

leonardoalt Oct 20, 2020

leonardoalt Oct 20, 2020

chriseth Oct 21, 2020

leonardoalt Oct 20, 2020

chriseth Oct 21, 2020

leonardoalt Oct 21, 2020

leonardoalt Oct 20, 2020

leonardoalt Oct 20, 2020

chriseth Oct 21, 2020

leonardoalt Oct 21, 2020

hrkrshnn Oct 22, 2020

hrkrshnn Oct 22, 2020

hrkrshnn Oct 22, 2020

chriseth Oct 22, 2020

hrkrshnn Oct 22, 2020

chriseth Oct 22, 2020

hrkrshnn Oct 22, 2020

hrkrshnn Oct 22, 2020

chriseth Oct 22, 2020

chriseth commented Oct 22, 2020

	or ar error without ary error data and it
	or an error without any error data and it

	)")("panicSig", util::selectorFromSignature("Panic(uint256)").str()).render(),
	)")
	("panicSig", util::selectorFromSignature("Panic(uint256)").str()).render(),

		namespace solidity::util
		{

Panic with error codes. #10013

Panic with error codes. #10013

Conversation

chriseth commented Oct 12, 2020 • edited Loading

chriseth commented Oct 14, 2020

ekpyron commented Oct 14, 2020

leonardoalt commented Oct 14, 2020

Choose a reason for hiding this comment

chriseth commented Oct 19, 2020

hrkrshnn left a comment

Choose a reason for hiding this comment

leonardoalt 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Oct 22, 2020

chriseth commented Oct 12, 2020 •

edited

Loading