Add VM Version Paris and new built in function `prevrandao` #13531

Marenz · 2022-09-15T17:29:42Z

nikola-matic · 2022-09-15T17:40:27Z

I've done a bit of grepping, and it seems like you're missing a case in libevmasm/SemanticInformation.cpp in invalidInPureFunction(Instruction _instructioN). If DIFFICULTY was invalid there, then surely PREVRANDAO is?

Also in test/tools/yulInterpreter/EVMInstructionInterpreter.cpp, and test/tools/ossfuzz/protoToYul.cpp, but those are tools, so maybe not relevant to this PR.

Marenz · 2022-09-15T17:47:09Z

seems like you're missing a case in libevmasm/SemanticInformation.cpp

They both have the same value, so it can only contain either DIFFICULTY or PREVRANDAO, in that switch case i decided to leave the original one.

...

same with the other cases, all switches just use the number and the semantics didn't really change. Whether we want to update the yul interpreter m_state.difficulty part is a different question, but nothing breaks if we keep it like this.

Marenz · 2022-09-15T17:52:25Z

Not quiet sure why the grammar test fails:

[1907/2991] Testing /root/project/test/libsolidity/semanticTests/inlineAssembly/prevrandao_pre-paris_function.sol FAILED
line 4:16 no viable alternative at input 'letprevrandao'
line 4:27 mismatched input ':=' expecting YulLParen
line 6:8 mismatched input '}' expecting YulLParen
line 10:21 mismatched input 'prevrandao' expecting YulIdentifier
line 10:34 extraneous input '->' expecting {YulBreak, YulContinue, YulFor, YulFunction, YulIf, 'leave', 'let', 'switch', YulEVMBuiltin, YulLBrace, YulRBrace, YulIdentifier}

Exited with code exit status 1

Marenz · 2022-09-15T18:00:31Z

found an exception in the grammar test for let basefee... and added one as well for let prevrandao

ekpyron

Nice, that was quick :-)!

I'm missing two things:

evmasm output (so solc --asm and related outputs) will contain opcode names - in those 0x44 should appear as difficulty pre-paris and as prevrandao with post-paris set as evm-version - I don't see that part accounted for in code or tests yet... test/libyul/evmCodeTransform tests can be abused for this, and/or test/libevmasm/Assembler.cpp.

There might also be some related complication in solidity::evmasm::c_instructions and c_instructionInfo getting initialized with the same key twice. This will only keep one version, won't it? We'll probably have to decide for PREVRANDAO only there and replace accesses to those two maps by a function taking an evm-version as argument, which adjusts accordingly.

The second thing is:
Ideally, the use of difficulty() and prevrandao() in inline assembly and yul would also cause warnings similar to the block members on mismatching evm versions. We usually don't warn about such things on the Yul level, so there may be some complication there and we may have to weigh whether the warning is worth adding additional infrastructure there, if needed, but ideally those should be warnings.

axic · 2022-09-16T11:13:07Z

Changelog.md

@@ -1,10 +1,11 @@
 ### 0.8.18 (unreleased)

 Language Features:
+ * Add support for ``prevrandao()`` which was introduced in EVM version to "Paris", supplanting ``difficulty`` in the process. Both can be used interchangeably.


As mentioned on the issue I think this is a bad idea, because the two outputs are expected to be vastly different and hence expect different use cases.

Well, this is exactly implemented according to the summary after the discussion, so I'm not sure what to tell you here...
#13512 (comment)

The main point is that we should not suggest to use them interchangeably. Supporting both (with a warning on mismatch) is a compromise for not being breaking for hypothetical libraries that want to use it as a source of randomness in a way that can rely on either version - but in general you should still very much use the correct one and not use them interchangeably.

axic · 2022-09-16T11:15:51Z

libyul/backends/evm/EVMDialect.cpp

+			_instr == evmasm::Instruction::PREVRANDAO &&
+			_instrName == "prevrandao"
+		)
+			return _evmVersion.hasPrevRandAO();


What is this weird capitalisation, RandAO?

I dunno, I tried my best to make sense of the weird name...

It comes from

A DAO (decentralised autonomous organisation) that anyone can participate in, and the random number is generated by all participants together!

So the names used are RANDAO, randao, Randao -- in this order in the consensus-specs. Likely hasPrevRandao is the best choice here.

test/evmc/evmc.h

cameel · 2022-09-20T13:49:05Z

This issue solves #13517 as well, right? Should be added to the top comment.

axic · 2022-09-22T14:01:00Z

docs/yul.rst

@@ -935,6 +935,8 @@ the ``dup`` and ``swap`` instructions as well as ``jump`` instructions, labels a
 +-------------------------+-----+---+-----------------------------------------------------------------+
 | difficulty()            |     | F | difficulty of the current block                                 |
 +-------------------------+-----+---+-----------------------------------------------------------------+
+| prevrandao()            |     | P | randomness provided by the beacon chain                         |


I think this documentation piece makes it look like that both opcodes exists and return a different value.

Hm what would be a good way to present it here, with the one letter column allowing very limited space

hrkrshnn

We also need a test for the view-pure checker.

hrkrshnn · 2022-09-22T15:46:13Z

test/libsolidity/syntaxTests/inlineAssembly/prevrandao_warn_pre-paris.sol

@@ -0,0 +1,7 @@
+function f() view returns (uint) {
+	return block.prevrandao;


Nit: indent and file name. Similarly, for other files as well. Somewhat

As far as I know we use both tabs/spaces in test files, just not mixed in the same file.
About the name.. you don't like the pre- part? I guess I could just remove it

As far as I know we use both tabs/spaces in test files

We do, but I think that's not on purpose. We should settle on one version.

test/libsolidity/syntaxTests/types/magic_block_istanbul.sol

hrkrshnn · 2022-09-22T16:25:22Z

libevmasm/Instruction.h

 	DIFFICULTY,			///< get the block's difficulty
-	GASLIMIT,			///< get the block's gas limit
+	PREVRANDAO = DIFFICULTY,	///< get randomness provided by the beacon chain
+	GASLIMIT = 0x45,	///< get the block's gas limit


Hmm, didn't know this works.

test/evmc/evmc.h

hrkrshnn · 2022-09-22T16:30:27Z

This issue solves #13517 as well, right? Should be added to the top comment.

It should. But just to note, this PR hasn't made Paris the default.

Marenz · 2022-09-22T16:48:06Z

It should. But just to note, this PR hasn't made Paris the default.

If that is desired I can change that, no problem :)

stackenbotten · 2022-09-27T15:00:36Z

There was an error when running chk_coding_style for commit 1228b043036d68149a94d1e2547f7cc5b09ad626:

Coding style error:
libevmasm/Instruction.h:295:    InstructionInfo(
libevmasm/Instruction.h:296:        std::string _name,
libevmasm/Instruction.h:297:        int _additional,
libevmasm/Instruction.h:298:        int _args,
libevmasm/Instruction.h:299:        int _ret,
libevmasm/Instruction.h:300:        bool _sideEffects,
libevmasm/Instruction.h:301:        Tier _gasPriceTier
libevmasm/Instruction.h:302:    ):
libevmasm/Instruction.h:303:        m_name(_name),
libevmasm/Instruction.h:304:        additional(_additional),
libevmasm/Instruction.h:305:        args(_args),
libevmasm/Instruction.h:306:        ret(_ret),
libevmasm/Instruction.h:307:        sideEffects(_sideEffects),
libevmasm/Instruction.h:308:        gasPriceTier(_gasPriceTier)
libevmasm/Instruction.h:309:    {}
libsolidity/interface/CompilerStack.cpp:227:    if (m_evmVersion < langutil::EVMVersion::paris())
libsolidity/interface/CompilerStack.cpp:228:        solidity::evmasm::usePreParisOpcodeNames();

Please check that your changes are working as intended.

libsolidity/interface/CompilerStack.cpp

aarlt · 2022-09-30T16:45:23Z

libsolidity/analysis/TypeChecker.cpp

+					"\"basefee\" is not supported by the VM version."
+				);
+			else if (memberName == "difficulty" && m_evmVersion.hasPrevRandao())
+				m_errorReporter.warning(


I would say this should be an error - because the semantic of difficulty vs. prevrandao is different. There is no case where difficulty in Paris means difficulty - that's why I would argue that compilation need to fail here.

A bit late to the discussion I'd say :)
I don't really mind either way, but as I understand it, this all already has been discussed

The current plan is to make it a warning, since making it an error right away is borderline breaking (even though one can argue about that). With 0.9 it should definitely become an error.

@ekpyron yeah that makes sense! @Marenz is that tracked somewhere somehow, so we will not forget it?

aarlt · 2022-09-30T16:45:38Z

libsolidity/analysis/TypeChecker.cpp

+					"\"difficulty\" was renamed and supplanted by \"prevrandao\" in the VM version paris."
+				);
+			else if (memberName == "prevrandao" && !m_evmVersion.hasPrevRandao())
+				m_errorReporter.warning(


Also this should be an error from my point of view.

aarlt · 2022-09-30T16:50:05Z

test/libsolidity/syntaxTests/inlineAssembly/difficulty_reserved.sol

+    }
+}
+// ----
+// ParserError 5568: (98-108): Cannot use builtin function name "difficulty" as identifier name.


Maybe also EVMVersion: <paris here? Is a difficulty identifier allowed again for >=paris?

If yes, I think the test for this is missing.

aarlt · 2022-09-30T16:50:49Z

test/libsolidity/syntaxTests/inlineAssembly/difficulty_warn_paris.sol

+// ====
+// EVMVersion: >=paris
+// ----
+// Warning 8417: (43-59): "difficulty" was renamed and supplanted by "prevrandao" in the VM version paris.


As written above, I think it's better to create an error instead of a warning.

aarlt · 2022-09-30T16:51:46Z

test/libsolidity/syntaxTests/inlineAssembly/prevrandao_warn_pre-paris.sol

+// ====
+// EVMVersion: <paris
+// ----
+// Warning 9432: (43-59): "prevrandao" is not supported by the VM version and will be treated like "difficulty".


aarlt · 2022-09-30T16:53:47Z

test/libsolidity/syntaxTests/viewPureChecker/inline_assembly_instructions_allowed_view.sol

@@ -73,7 +73,7 @@ contract C {
            pop(coinbase())
            pop(timestamp())
            pop(number())
-            pop(difficulty())
+            pop(prevrandao())


Maybe it's better to have two tests here, one <paris and one >= paris?

I introduced another test inline_assembly_instructions_allowed_view_london.sol for london that contains only the changed opcode

aarlt · 2022-09-30T16:59:49Z

libsolidity/analysis/TypeChecker.cpp

+				m_errorReporter.warning(
+					8417_error,
+					_memberAccess.location(),
+					"\"difficulty\" was renamed and supplanted by \"prevrandao\" in the VM version paris."


Why renamed and supplanted and not just replaced?

It's the terminology they used in the EIP and I believe makes it most clear that it's the same opcode behind it

Yeah, I saw that the EIP is using the exact same words - but I needed to check it's translation ;) that's why I thought it could make sense to use an easier wording

ekpyron · 2022-10-04T08:50:31Z

libevmasm/Instruction.cpp

+//	{ Instruction::PREVRANDAO,	{ "PREVRANDAO",		0, 0, 1, false, Tier::Base } },
+// TODO Replace with PREVRANDAO in the next breaking release
 	{ Instruction::DIFFICULTY,	{ "DIFFICULTY",		0, 0, 1, false, Tier::Base } },


IMHO, this doesn't make sense.
It's a question of the EVM version, not with source compatibility - with the next breaking release compiling to <paris still should result in DIFFICULTY and already now, it's weird to emit a non-existent opcode name for >=paris.

So should really do this right away, as already described in #13531 (review).

The EIP just changes the fact that there is a constant mapping from the binary opcode to instruction infos - we have to account for that in the code one way or another (and it's not like dragging an EVM version dependency through here is that much of a hassle).

…M Version Paris and new built in function ``prevrandao``

stackenbotten · 2022-10-04T17:26:59Z

There was an error when running chk_coding_style for commit 513d5a32658127eb4afa44b45cff526a36e87e30:

Coding style error:
libevmasm/Disassemble.cpp:19:#include "libevmasm/Instruction.h"
libevmasm/Disassemble.cpp:20:#include "liblangutil/EVMVersion.h"
libevmasm/Disassemble.h:21:#include "liblangutil/EVMVersion.h"
libsolidity/interface/StandardCompiler.cpp:24:#include "libevmasm/Instruction.h"
libyul/AsmAnalysis.cpp:22:#include "libevmasm/Instruction.h"

Please check that your changes are working as intended.

stackenbotten · 2022-10-04T17:27:00Z

libevmasm/Disassemble.cpp

@@ -16,6 +16,8 @@
 */
 // SPDX-License-Identifier: GPL-3.0

+#include "libevmasm/Instruction.h"


Coding style error

stackenbotten · 2022-10-04T17:27:01Z

libevmasm/Disassemble.cpp

@@ -16,6 +16,8 @@
 */
 // SPDX-License-Identifier: GPL-3.0

+#include "libevmasm/Instruction.h"
+#include "liblangutil/EVMVersion.h"


Coding style error

Marenz · 2022-10-04T17:28:00Z

I tried to keep my automatic Search & Replace in that commit.

Tests & style are not yet updated, probably some broken due to the rename.

ekpyron · 2022-10-04T17:40:41Z

libevmasm/Disassemble.cpp

 		if (!isValidInstruction(_instr))
 			ret << "0x" << std::uppercase << std::hex << static_cast<int>(_instr) << _delimiter;


This doesn't look right - in case the byte wasn't a valid instruction, it should output the byte directly, not the internal enum value... so both this and eachInstruction will probably need to change slightly.

ekpyron · 2022-10-04T17:45:21Z

libevmasm/Instruction.h

 /// Virtual machine bytecode instruction.
-enum class Instruction: uint8_t
+enum class InstructionOpCode: uint8_t


Do we really still need this one? The instruction info array can just contain the plain bytecode and the traits like inline bool isPushInstruction can all be easily implemented on top of the other enum, can't they? Not sure I've seen all remaining uses of this yet, though.

Oh, good thought. Yeah, it is very very rarely used, I think only in Disassembly.

The inline functions are already implemented on top of the internal one, true

nikola-matic · 2022-10-04T19:16:54Z

libyul/AsmAnalysis.cpp

+		});
+	};
+
+	// TODO remove this in 0.9.0. We allow creating functions or identifiers in Yul with the name


Do you think it would be a good idea to open an issue preemptively, so we don't forget, or is this something that will actively cause problems once we bump to 0.9.0. and will have to be removed anyway?

github-actions · 2022-10-25T12:11:15Z

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

.circleci/soltest_all.sh

ekpyron · 2022-12-01T14:45:22Z

Closing this as it will be superseded by #13759.
Will keep the branch around as reference for now.

Marenz force-pushed the paris-oh-paris branch from 1913d9c to 9e901fa Compare September 15, 2022 17:32

Marenz force-pushed the paris-oh-paris branch from 9e901fa to ca9605d Compare September 15, 2022 17:41

Marenz force-pushed the paris-oh-paris branch from ca9605d to 46edbbc Compare September 15, 2022 17:59

ekpyron reviewed Sep 15, 2022

View reviewed changes

axic reviewed Sep 16, 2022

View reviewed changes

test/evmc/evmc.h Show resolved Hide resolved

axic reviewed Sep 22, 2022

View reviewed changes

hrkrshnn reviewed Sep 22, 2022

View reviewed changes

Marenz force-pushed the paris-oh-paris branch 2 times, most recently from 7c87ced to 1228b04 Compare September 27, 2022 15:00

Marenz commented Sep 27, 2022

View reviewed changes

libsolidity/interface/CompilerStack.cpp Outdated Show resolved Hide resolved

aarlt reviewed Sep 30, 2022

View reviewed changes

Marenz force-pushed the paris-oh-paris branch from 1228b04 to 900952a Compare October 3, 2022 23:15

ekpyron requested changes Oct 4, 2022

View reviewed changes

wipAdd VM Version Paris and new built in function prevrandaoAdd V…

2dc5435

…M Version Paris and new built in function ``prevrandao``

Marenz added 3 commits October 4, 2022 19:25

wip

0f08a06

Asm output

b44b928

Search & Replace

513d5a3

Marenz force-pushed the paris-oh-paris branch from 900952a to 513d5a3 Compare October 4, 2022 17:26

stackenbotten reviewed Oct 4, 2022

View reviewed changes

ekpyron reviewed Oct 4, 2022

View reviewed changes

nikola-matic reviewed Oct 4, 2022

View reviewed changes

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Oct 25, 2022

cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Oct 25, 2022

ekpyron marked this pull request as draft November 1, 2022 14:06

axic reviewed Nov 9, 2022

View reviewed changes

.circleci/soltest_all.sh Show resolved Hide resolved

r0qs mentioned this pull request Nov 29, 2022

Add block.prevrandao as alias for block.difficulty #13759

Merged

ekpyron closed this Dec 1, 2022

		@@ -0,0 +1,7 @@
		function f() view returns (uint) {
		return block.prevrandao;

		if (!isValidInstruction(_instr))
		ret << "0x" << std::uppercase << std::hex << static_cast<int>(_instr) << _delimiter;

Add VM Version Paris and new built in function prevrandao #13531

Add VM Version Paris and new built in function prevrandao #13531

Conversation

Marenz commented Sep 15, 2022 • edited

nikola-matic commented Sep 15, 2022

Marenz commented Sep 15, 2022

Marenz commented Sep 15, 2022

Marenz commented Sep 15, 2022

ekpyron left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron Oct 5, 2022 • edited

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

cameel commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn 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

hrkrshnn commented Sep 22, 2022

Marenz commented Sep 22, 2022

stackenbotten commented Sep 27, 2022

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

stackenbotten commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Oct 4, 2022

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

github-actions bot commented Oct 25, 2022

ekpyron commented Dec 1, 2022

Add VM Version Paris and new built in function `prevrandao` #13531

Add VM Version Paris and new built in function `prevrandao` #13531

Marenz commented Sep 15, 2022 •

edited

ekpyron left a comment •

edited

ekpyron Oct 5, 2022 •

edited