EVM version by chriseth · Pull Request #3569 · argotorg/solidity

chriseth · 2018-02-21T22:57:12Z

tests for metadata

axic · 2018-02-21T23:07:55Z

libsolidity/interface/EVMVersion.h

+	static EVMVersion homestead() { return {Version::Homestead}; }
+	static EVMVersion byzantium() { return {Version::Byzantium}; }
+
+	static boost::optional<EVMVersion> fromString(std::string const& _version)


Are we moving over to boost::optional? A lot of places started to use it now.

I just think it is very convenient in this situation.

chriseth · 2018-02-22T13:48:02Z

Ready for review.

chriseth · 2018-02-26T17:03:15Z

@axic ready for review.

chriseth · 2018-02-26T17:09:20Z

Actually I just realized we have a third test combination to run: contracts compiled for homestead, running on a byzantium VM...

axic · 2018-02-26T18:46:56Z

libsolidity/inlineasm/AsmAnalysis.cpp

-	if (futureInstructions.count(_instr))
+	// We assume that returndatacopy, returndatasize and staticcall are either all available
+	// or all not available.
+	solAssert(m_evmVersion.hasReturndatacopy() == m_evmVersion.hasStaticCall(), "");


Does hasReturndatacopy also means hasReturndatasize?

Yes, I can rename it to hasReturndatacopyAndReturndatasize or add a comment.

I think a comment is enough. Maybe hasReturndata was the generic name of the feature?

I will change it to supportsReturndata.

axic · 2018-02-26T18:48:34Z

test/RPCSession.cpp

 			"allowFutureBlocks": true,
 			"homesteadForkBlock": "0x00",
+			)" + enableByzantium + R"(
 			"EIP150ForkBlock": "0x00",


This is "tangerine whistle".

axic · 2018-02-26T18:48:43Z

test/RPCSession.cpp

 			"homesteadForkBlock": "0x00",
+			)" + enableByzantium + R"(
 			"EIP150ForkBlock": "0x00",
 			"EIP158ForkBlock": "0x00"


This is "spurious dragon".

axic · 2018-02-26T18:50:37Z

test/libsolidity/SolidityABIJSON.cpp

 		m_compilerStack.reset(false);
 		m_compilerStack.addSource("", "pragma solidity >=0.0;\n" + _code);
+		m_compilerStack.setEVMVersion(dev::test::Options::get().evmVersion());
+		m_compilerStack.setOptimiserSettings(dev::test::Options::get().optimize);


This was missed but shouldn't make any difference.

axic · 2018-02-26T18:51:09Z

test/libsolidity/SolidityNameAndTypeResolution.cpp

 	CHECK_ALLOW_MULTI(text, (std::vector<std::pair<Error::Type, std::string>>{
 		{Error::Type::Warning, "Variable is shadowed in inline assembly by an instruction of the same name"},
-		{Error::Type::Warning, "only available after the Metropolis"},
+		{Error::Type::Warning, "The \"create2\" instruction is not supported for the VM version"},


"for the"? Shouldn't that be "in the"?

Either is fine for me.

axic · 2018-02-26T18:54:10Z

libsolidity/interface/EVMVersion.h

+	}
+
+	bool operator==(EVMVersion const& _other) const { return m_version == _other.m_version; }
+	bool operator!=(EVMVersion const& _other) const { return !this->operator==(_other); }


Should also have comparison operators if we introduce tangerine whistle now.

axic · 2018-02-26T18:58:09Z

libsolidity/interface/EVMVersion.h

+	std::string name() const { return m_version == Version::Byzantium ? "byzantium" : "homestead"; }
+
+	bool hasReturndatacopy() const { return m_version == Version::Byzantium; }
+	bool hasStaticCall() const { return m_version == Version::Byzantium; }


I'm not sure if we should have such a granularity. One option is this, the other is comparison whether currently version >= byzantium, though that assumes features can't be removed.

axic · 2018-02-26T18:59:17Z

libsolidity/interface/EVMVersion.h

+	/// or whether we can just forward easily all remaining gas (true).
+	bool canOverchargeGasForCall() const
+	{
+		// @TODO when exactly was this introduced? Was together with the call stack fix.


Tangerine Whistle, see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-608.md

axic · 2018-02-26T19:06:31Z

circle.yml

-      - run:
-          name: Init submodules
-          command: |
-            git submodule update --init


Why were these removed?

I think we don't have any submodules anymore, do we? I can remove it from this PR.

axic · 2018-02-26T19:07:38Z

Actually I just realized we have a third test combination to run: contracts compiled for homestead, running on a byzantium VM...

Probably the simplest way is adding another commandline option to the tests for --runtime-evm-version which changes the version in the genesis.

Then we could have more combinations on the CI.

chriseth · 2018-02-26T19:21:51Z

Added the other versions.

chriseth · 2018-02-26T19:59:28Z

Updated. Will leave it at the current test combinations for now.

chriseth · 2018-02-26T20:00:59Z

Updated.

axic · 2018-02-27T13:12:55Z

I'll rebase this after #2541 and fix up the asm analyzer part.

It is only in conflict with tests.sh atm.

axic · 2018-02-27T13:48:45Z

scripts/tests.sh

+    "$REPO_ROOT"/build/test/soltest --show-progress $log -- "$optimize" --evm-version "$vm" --ipcpath /tmp/test/geth.ipc
+    THIS_ERR=$?
+    set -e
+    if [ $? -ne 0 ]; then ERRO_CODE=$?; fi


Typo, the variable is called ERROR_CODE

axic · 2018-03-02T16:00:16Z

libevmasm/Assembly.cpp

 }

-Assembly& Assembly::optimise(bool _enable, bool _isCreation, size_t _runs)
+Assembly& Assembly::optimise(bool _enable, EVMVersion _evmVersion, bool _isCreation, size_t _runs)


Woohoo, I should be using this for the bitwise shifting optimiser rules in #3580.

Yes, it was a really painful process. This setting is needed literally everywhere...

axic · 2018-03-02T16:12:07Z

test/liblll/EndToEndTest.cpp

-	BOOST_CHECK(balanceAt(Address(0xdead)) == 42);
+	// "send" does not retain enough gas to be able to pay for account creation.
+	// Disabling for non-tangerineWhistle VMs.
+	if (dev::test::Options::get().evmVersion().canOverchargeGasForCall())


Hm, can we output a boost message that these are skipped due to the vm?

Also, how about?

if (..) return;

I tried return, but the macro seems to create a function that returns something.

I'll look into telling boost to skip a test, it seems to have such a feature.

Changed it accordingly. This will probably show up in the xml test report.

axic · 2018-03-02T16:12:38Z

test/liblll/EndToEndTest.cpp

+	{
+		char const* sourceCode = R"(
+			(returnlll
+				(send 0xdead 42))


Eventually send and msg should be updated I think.

@benjaminion @zigguratt what do you think?

axic

I think it looks good. I wouldn't have added evmVersion to LLL, but it can actually be useful (will address that separately).

axic · 2018-03-03T16:16:17Z

It is failing:

Testing soljson via the fuzzer...
--> Running tests using  --evm-version homestead...
Commandline tests successful.
Boost.Test framework internal error: unknown reason
Exited with code 200

chriseth · 2018-03-05T10:24:45Z

It seems there is a bug in boost which causes skipped tests to be treated as errors: https://svn.boost.org/trac10/ticket/12095

chriseth · 2018-03-05T10:25:56Z

I will revert to the previous version for now (which just had a if (..) {...}. These LLL tests already cost me 1-2 hours, although LLL is out of scope.

chriseth requested a review from axic February 21, 2018 22:57

axic reviewed Feb 21, 2018

View reviewed changes

chriseth force-pushed the evmVersion branch 4 times, most recently from 72354bb to 8833711 Compare February 26, 2018 17:02

chriseth force-pushed the evmVersion branch from 8833711 to aa9a9af Compare February 26, 2018 17:08

chriseth force-pushed the evmVersion branch from aa9a9af to efb2028 Compare February 26, 2018 17:12

This was referenced Feb 26, 2018

Send all gas on byzantium #3599

Merged

Use returndatacopy for retrieving dynamically sized outputs. #3308

Merged

axic mentioned this pull request Feb 26, 2018

Add bit shifting opcodes (EIP145) #2541

Merged

axic reviewed Feb 26, 2018

View reviewed changes

chriseth force-pushed the evmVersion branch from a2cd43a to aec7528 Compare February 26, 2018 19:59

axic reviewed Feb 27, 2018

View reviewed changes

chriseth and others added 13 commits March 1, 2018 17:19

Test both EVM versions.

f2f61f1

Use EVM version in type checker.

a53d6b4

Provide EVM version to assembly analysis.

dc317a4

Activate byzantium for testing.

739533e

Correctly set evm version in tests

05cc5f2

Returndatasize and staticcall test fixes.

1e26011

CREATE2 test fix.

7b0272c

Add TangerineWhistle.

982476f

Simplify error handling in tests.sh

7171ac0

Add constantinople.

4ce0e77

Explain json-io setting.

1246917

Abort on the first failed end-to-end run.

7f8e573

Only warn for shift instructions if not using constantinople

5a54cd5

chriseth force-pushed the evmVersion branch 2 times, most recently from 2880ed4 to e3de9ea Compare March 2, 2018 11:23

axic reviewed Mar 2, 2018

View reviewed changes

chriseth mentioned this pull request Mar 2, 2018

Use STATICCALL for pure function calls. #2966

Merged

axic reviewed Mar 2, 2018

View reviewed changes

chriseth force-pushed the evmVersion branch from e3de9ea to 6d77b4e Compare March 2, 2018 16:37

axic approved these changes Mar 2, 2018

View reviewed changes

Use EVM version in gas meter and optimizer.

6ec4517

chriseth force-pushed the evmVersion branch from 19df28f to 6ec4517 Compare March 5, 2018 10:36

chriseth merged commit 6dc1373 into develop Mar 5, 2018

axic mentioned this pull request Mar 6, 2018

Make byzantium the default VM and remove warnings #3500

Closed

axic deleted the evmVersion branch March 7, 2018 12:55

axic mentioned this pull request Mar 12, 2018

Support constantinople in evm-version #3713

Merged

Conversation

chriseth commented Feb 21, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented Feb 22, 2018

Uh oh!

chriseth commented Feb 26, 2018

Uh oh!

chriseth commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Feb 26, 2018

Uh oh!

chriseth commented Feb 26, 2018

Uh oh!

chriseth commented Feb 26, 2018

Uh oh!

chriseth commented Feb 26, 2018

Uh oh!

axic commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

axic commented Mar 3, 2018

Uh oh!

chriseth commented Mar 5, 2018

Uh oh!

chriseth commented Mar 5, 2018

chriseth commented Feb 21, 2018 •

edited by axic

Loading