Implement bitwise shifting (EIP145) #4054

axic · 2017-04-23T09:59:12Z

codecov-io · 2017-04-23T11:33:14Z

Codecov Report

Merging #4054 into develop will decrease coverage by 0.08%.
The diff coverage is 24.39%.

@@             Coverage Diff             @@
##           develop    #4054      +/-   ##
===========================================
- Coverage    60.36%   60.28%   -0.09%     
===========================================
  Files          352      352              
  Lines        27891    27912      +21     
  Branches      2884     2894      +10     
===========================================
- Hits         16836    16826      -10     
- Misses       10060    10089      +29     
- Partials       995      997       +2

chfast · 2017-04-25T19:00:23Z

libevm/VM.cpp

+			ON_OP();
+			updateIOGas();
+
+			/// TODO: confirm shift >= 256 results in 0


We should add unit tests for edge cases in boost::multiprecision.

That is done in #4064. So we should add appropriate boundary checks here (and masking).

pirapira · 2017-06-01T08:52:10Z

libevm/VM.cpp

+			updateIOGas();
+
+			/// This workarounds a bug in Boost, ...
+			u256 mask = (u256(1) << (256 - m_SP[1])) - 1;


I don't know what happens when m_SP[1] is more than 256.

Is the underflown subtraction defined?

Isn't the shift very slow when 256 - m_SP[1] is a big number?

The EIP says If the shift amount (`arg2`) is greater or equal 256 the result is 0. so the above code should be updated accordingly.

axic · 2017-09-25T07:27:34Z

Need to swap operands as per latest changes.

gcolvin · 2017-10-03T06:35:02Z

@axic @chfast There is substantial overlap with my #4569. This one is the one that should be pulled. However, you will want libevm/Instruction.h and Instruction.cpp from #4569. Also the convention I am using for an experimental feature like this is to place

#ifndef EIP_145
	#define EIP_145 false
#endif

in libevm/VMConfig.h and wall off the code for the feature with #if EIP_145.

gcolvin · 2017-10-03T07:05:04Z

@axic @chfast Inspecting our code there are many differences. For starters. I am taking the item on the top of the stack SP[0] as the amount to shift by, which is how I read the spec. E.g.

The SHL instruction (shift left) pops 2 values from the stack, arg1 and arg2, and pushes on the stack the second popped value arg2 shifted to the left by the number of bits in the first popped value arg1.

You are shifting by SP[1], so I think that's wrong.

Otherwise our code is just different.

For SHL I'm not working around whatever Boost bug you are, but I don't know if my approach triggers the bug.
For SHR I explicitly test for a shift amount >= 256, which might not be needed, but I suspect the test is much cheaper than the shift.
For SAR I use two shifts and a mask rather than division and exponentiation.

So perhaps my code is wrong, but please look.

chfast · 2017-10-03T15:31:03Z

libevm/VM.cpp

+			{
+				uint64_t amount = uint64_t(m_SP[0]);
+			    m_SPP[0] = shiftee >> amount;
+			    m_SPP[0] |= allbits << (256 - amount);


I think you have to check shiftee & hibit here as well.

axic · 2017-10-03T16:17:27Z

libevm/VM.cpp

+			if (m_SP[0] >= 256)
+				m_SPP[0] = 0;
+			else
+				m_SPP[0] = m_SP[1] << uint64_t(m_SP[0]);


Could also just use unsigned and not the specific uint64_t since it always fits into 8 bits.

axic · 2017-10-03T16:21:14Z

libevm/VM.cpp

+			ON_OP();
+			updateIOGas();
+
+			static u256 const hibit = u256(1) << 255;


I think I'd prefer a slower, but definitely correct implementation for this case to ensure everything is working properly. Once multiple clients agree on tests, it can be optimised for speed.

Do you think the code I have now is mathematically incorrect? Or are you worried about bugs in boost's library?

axic · 2017-10-03T16:44:54Z

libevm/VM.cpp

+					m_SPP[0] = s2u(-1);
+			}
+			else
+				m_SPP[0] = s2u(divWorkaround(value, exp256(2, shift)));


This was broken due to EVM DIV not rounding the same way.

axic · 2017-10-04T08:46:26Z

I wonder if it is possible to do the RC5 changes in a separate pull request as it doesn't seem related to this PR?

axic · 2017-10-04T08:49:12Z

libevm/VMConfig.h

@@ -25,9 +25,13 @@ namespace eth
 //
 // interpreter configuration macros for development, optimizations and tracing
 //
+// EIP_145                - bitwise shifting


Out of curiosity, why is EIP_145 better/more expressive than EVM_USE_BITSHIFT ?

It tells the reader where to find the spec for the feature. And it's consistent with the others.

I prefer using a meaning names, because I'm usually confused by numbers.
As a compromise, can it be EIP145_BITSHIFT or EVM_EIP145_BITSHIFT?

I much prefer the compromise.

I chose to use the EIP numbers for a reason--if you are going into the code looking for all configuration macros specific to an EIP all you need to know is the number. Conversely, when you come upon an unnumbered macro in the code you might know EIP it relates to.

If we must remove the numbers from the configuration macros, please add comments at the point of definition as to what EIP they configure.

axic · 2017-10-04T08:54:04Z

Where are the unit tests being run? What did change in jsontests submodule?

gcolvin · 2017-10-04T15:12:37Z

The relationship is the change to use the solidity shift operators rather than exponentiation. The shifts in rc5 are data-dependent, and will be a good stress test.

gcolvin · 2017-10-04T15:17:37Z

If you mean libevm/tests/unittests/performance the tests there are run manually with tests.mk - testing performance on virtualized machines is useless. shift-tests.asm doesn't really belong there so far as being a performance test, but I don't know where else to put it.

gcolvin · 2017-10-04T15:29:04Z

As far as I know nothing changed in jsontests; I just did git submodule update --init. But apparently git thinks something changed, and I'm guessing doing git commit -a ... might have caused the problem?

gcolvin · 2017-10-04T16:31:20Z

@axic It appears my attempt to remove jsontests from the PR failed, although it did allow for an automatic merge again. I will refrain from further attempts to back this out and, with apologies, leave it to you.

pirapira · 2017-10-16T10:47:16Z

Please rebase.

axic · 2018-01-05T00:31:19Z

@gumb0 @pirapira @chfast is it possible to include a genesis.json "enable option" field in this PR (e.g. EIP145ForkBlock)?

gumb0 · 2018-01-05T09:49:59Z

It should be possible, you'd need to add EIP145ForkBlok to ChainOperationParams, parse it in ChainParams.cpp, also add a flag to EVMSchedule and define EIP145Schedule and check the flag in EVM

axic · 2018-02-09T16:06:06Z

How about adding support for the constantinople schedule in this PR then? Shouldn't take too long.

axic · 2018-02-09T16:06:30Z

Or could merge this and help @tcsiwula finish #4838

axic · 2018-02-22T08:07:55Z

@gumb0 @chfast I think this is ready for merging now.

axic · 2018-02-22T11:11:27Z

Merged this as every test succeeded except the code coverage :)

axic · 2018-02-22T11:11:40Z

@gcolvin @tcsiwula thank you for your help finishing this!

axic · 2018-02-22T11:12:20Z

@winsvega @pirapira @holiman everything's ready for writing bitwise shifting state tests now 😉

gcolvin · 2018-02-22T20:36:19Z

@axic Sorry to be late to the game. I believe the runtime tests in VM.cpp can more efficiently be handled in VM::optimize by conditionally replacing the unimplemented instructions with Instruction::INVALID.
https://github.com/ethereum/cpp-ethereum/blob/a3d93a8fb80c2620449e4d399507dd5ac77ecaae/libevm/VMOpt.cpp#L86

axic · 2018-02-22T21:54:05Z

@gcolvin it seems that the optimize function is not even used for the byzantium opcodes. I think it may be a good idea to implement that in a separate PR.

gcolvin · 2018-02-22T23:19:45Z

@axic Unless jump destinations are no longer verified the loop I pointed at is needed, and conditionally invalid opcodes can be tested for there, whether or not my work on bytecode optimization is being abandoned. Once replaced with INVALID these opcodes will be detected by the opcode dispatch without incurring extra overhead on every opcode execution. It would be small change to this PR, or a small new PR, but I haven't time to make it right now.

axic · 2018-02-22T23:30:28Z

it seems that the optimize function is not even used for the byzantium opcodes.

... aka it doesn't even replace Byzantium opcodes with INVALID on non-Byzantium mode.

gcolvin · 2018-02-22T23:52:45Z

Also Known As what? I'm not understanding you, sorry.

axic · 2018-02-22T23:55:08Z

The code you have linked to does not replace any Byzantium opcode with the invalid opcode. Not sure how can I write this sentence differently.

gcolvin · 2018-02-23T00:51:52Z

Something like this, and remove the tests in VM.cpp

	for (size_t pc = 0; pc < nBytes; ++pc)
	{
		Instruction op = Instruction(m_code[pc]);
		TRACE_OP(2, pc, op);

		if (
			op == Instruction:: SHL ||
			op == Instruction::SHR ||
			op == Instruction::SAR
		)
		{
			if (!m_schedule->haveBitwiseShifting)
				m_code[pc] = (byte)Instruction::INVALID;
		}
	. . .

axic · 2018-02-23T07:43:56Z

I understand how it can be added, all I am saying that it is not present for returndatacopy and returndatasize at the moment.

As a result, this isn't something which was missed in this PR, rather than it is a new improvement, which can be done separately.

gcolvin · 2018-02-24T11:43:06Z

Aha. I didn't understand what you didn't understand ;) I agree it can be done separately. I just wanted to make note of it. I haven't had much time for this code, but try to notice when changes go in.

axic added the in progress label Apr 23, 2017

axic force-pushed the bitshift branch from 14b8515 to 648cced Compare April 23, 2017 10:22

chfast reviewed Apr 25, 2017

View reviewed changes

axic force-pushed the bitshift branch 2 times, most recently from 9d11d0f to d5f342e Compare May 4, 2017 17:09

pirapira reviewed Jun 1, 2017

View reviewed changes

axic mentioned this pull request Oct 2, 2017

Shift opcodes #4569

Closed

axic force-pushed the bitshift branch from d5f342e to 6ff0052 Compare October 3, 2017 15:29

chfast reviewed Oct 3, 2017

View reviewed changes

axic force-pushed the bitshift branch 3 times, most recently from afafed4 to 7e4fbf5 Compare October 3, 2017 16:11

axic commented Oct 3, 2017

View reviewed changes

axic commented Oct 4, 2017

View reviewed changes

axic mentioned this pull request Jan 5, 2018

Add bit shifting opcodes (EIP145) ethereum/solidity#2541

Merged

gumb0 mentioned this pull request Feb 21, 2018

Conditionally enable bitshift on ConstantinopleForkBlock #4838

Merged

axic and others added 7 commits February 22, 2018 09:03

Add bitwise shift instructions

514c28a

Implement shift opcodes

213e0c8

Workaround bug in Boost

02d77f1

Rewrite shift implementation

1bd656d

Add back guard when shifts are disabled

8d3fcd0

Add option guard for turning shifts off by default

fcda859

Update shift test examples

843f97d

axic force-pushed the bitshift branch from 8c1291a to 7c3d650 Compare February 22, 2018 08:04

conditionally enable bitshift on ConstantinopleForkBlock

a3d93a8

axic force-pushed the bitshift branch from 7c3d650 to a3d93a8 Compare February 22, 2018 08:07

chfast approved these changes Feb 22, 2018

View reviewed changes

axic merged commit 0913b23 into develop Feb 22, 2018

axic deleted the bitshift branch February 22, 2018 11:11

Souptacular mentioned this pull request Jul 30, 2018

Constantinople Progress Tracker ethereum/pm#53

Closed

Implement bitwise shifting (EIP145) #4054

Implement bitwise shifting (EIP145) #4054

Conversation

axic commented Apr 23, 2017 • edited Loading

codecov-io commented Apr 23, 2017 • 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

axic Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

axic commented Sep 25, 2017

gcolvin commented Oct 3, 2017 • edited Loading

gcolvin commented Oct 3, 2017 • 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

axic commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcolvin Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

axic commented Oct 4, 2017

gcolvin commented Oct 4, 2017

gcolvin commented Oct 4, 2017

gcolvin commented Oct 4, 2017

gcolvin commented Oct 4, 2017

pirapira commented Oct 16, 2017

axic commented Jan 5, 2018 • edited Loading

gumb0 commented Jan 5, 2018

axic commented Feb 9, 2018

axic commented Feb 9, 2018

axic commented Feb 22, 2018

axic commented Feb 22, 2018

axic commented Feb 22, 2018

axic commented Feb 22, 2018

gcolvin commented Feb 22, 2018

axic commented Feb 22, 2018

gcolvin commented Feb 22, 2018

axic commented Feb 22, 2018

gcolvin commented Feb 22, 2018

axic commented Feb 22, 2018

gcolvin commented Feb 23, 2018

axic commented Feb 23, 2018

gcolvin commented Feb 24, 2018

axic commented Apr 23, 2017 •

edited

Loading

codecov-io commented Apr 23, 2017 •

edited

Loading

axic Jul 3, 2017 •

edited

Loading

gcolvin commented Oct 3, 2017 •

edited

Loading

gcolvin commented Oct 3, 2017 •

edited

Loading

gcolvin Jan 11, 2018 •

edited

Loading

axic commented Jan 5, 2018 •

edited

Loading