core, vm, common: define constantinople fork + shift #16045

holiman · 2018-02-09T08:07:18Z

This PR defines Constantinople hard fork in genesis and instruction set. Additionally, it implementes SHR, SHL and SAR, along with testcases defined in the EIP .

Benchmarks:

#go test -bench BenchmarkOp . -run -
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/core/vm
BenchmarkOpAdd64-8        	 5000000	       243 ns/op
BenchmarkOpAdd128-8       	 5000000	       243 ns/op
BenchmarkOpAdd256-8       	 5000000	       268 ns/op
BenchmarkOpSub64-8        	 5000000	       270 ns/op
BenchmarkOpSub128-8       	 5000000	       302 ns/op
BenchmarkOpSub256-8       	 5000000	       379 ns/op
BenchmarkOpMul-8          	 3000000	       479 ns/op
BenchmarkOpDiv256-8       	 2000000	       624 ns/op
BenchmarkOpDiv128-8       	 5000000	       316 ns/op
BenchmarkOpDiv64-8        	 5000000	       276 ns/op
BenchmarkOpSdiv-8         	 2000000	      1048 ns/op
BenchmarkOpMod-8          	 2000000	       618 ns/op
BenchmarkOpSmod-8         	 1000000	      1047 ns/op
BenchmarkOpExp-8          	   50000	     35194 ns/op
BenchmarkOpSignExtend-8   	 3000000	       440 ns/op
BenchmarkOpLt-8           	 3000000	       419 ns/op
BenchmarkOpGt-8           	 3000000	       420 ns/op
BenchmarkOpSlt-8          	 2000000	       732 ns/op
BenchmarkOpSgt-8          	 2000000	       746 ns/op
BenchmarkOpEq-8           	 5000000	       399 ns/op
BenchmarkOpAnd-8          	 5000000	       381 ns/op
BenchmarkOpOr-8           	 5000000	       369 ns/op
BenchmarkOpXor-8          	 3000000	       464 ns/op
BenchmarkOpByte-8         	 3000000	       475 ns/op
BenchmarkOpAddmod-8       	 2000000	      1096 ns/op
BenchmarkOpMulmod-8       	 1000000	      1210 ns/op
BenchmarkOpSHL-8          	 2000000	       642 ns/op
BenchmarkOpSHR-8          	 3000000	       412 ns/op
BenchmarkOpSAR-8          	 2000000	       676 ns/op
PASS
ok  	github.com/ethereum/go-ethereum/core/vm	56.064s

The new shifts are defined with gascost GasFastestStep, same as ADD, SUB, GT, LT, SLT, SGT, EQ, ISZERO, AND, XOR, OR, NOT and BYTE.
Selecting only those in the benchmark results, and ordering them by speed, we get:

BenchmarkOpAdd256-8       	 5000000	       268 ns/op
BenchmarkOpOr-8           	 5000000	       369 ns/op
BenchmarkOpSub256-8       	 5000000	       379 ns/op
BenchmarkOpAnd-8          	 5000000	       381 ns/op
BenchmarkOpEq-8           	 5000000	       399 ns/op
BenchmarkOpSHR-8          	 3000000	       412 ns/op
BenchmarkOpLt-8           	 3000000	       419 ns/op
BenchmarkOpGt-8           	 3000000	       420 ns/op
BenchmarkOpXor-8          	 3000000	       464 ns/op
BenchmarkOpByte-8         	 3000000	       475 ns/op
BenchmarkOpSHL-8          	 2000000	       642 ns/op
BenchmarkOpSAR-8          	 2000000	       676 ns/op
BenchmarkOpSlt-8          	 2000000	       732 ns/op
BenchmarkOpSgt-8          	 2000000	       746 ns/op

They weigh in around the average of the other ops (we could probably improve SLT and SGT by not allocating as many bigints.)

This should not go in 1.8.0, but does not need to be "constantinople-complete" to be merged to master: the sooner we get it in the better -- the only prerequisite should be that it will not interfere with pre-constantinople vm runtime. If it panic's when constantinople is enabled, that's fine -- the more testing we get the better, and gives the test-team the possibility to validate their testcases across clients.

axic · 2018-02-09T16:12:20Z

I wonder why is SHL >50% slower than SHR? Is the underlying Go bignum library external?

karalabe · 2018-02-09T20:22:33Z

@axic SHL does memory allocs as it needs to grow the bignum, and only afterwards is it clipped to 256 bits.

axic · 2018-02-09T21:21:45Z

Oh, that is a good point. Perhaps even worth noting in the EIP.

Is it possible to have it operate on a fixed 256 bit length?

holiman · 2018-02-12T13:47:45Z

Is it possible to have it operate on a fixed 256 bit length?

It is, but what we really should do is swap out the entire bignums-on-stack implementation with a 256-bit-fixedlength-on-stack. It's a pretty big undertaking (which may be worth doing)

axic · 2018-02-12T23:54:51Z

Actually isn't this quite a big problem with for example multiplying 0xff..ff with 0xff..ff? Or is it internally implemented using mulmod?

karalabe

Generally looks good to me, just disable it on all code paths for now imho. Also pls gofmt.

karalabe · 2018-02-22T18:04:45Z

core/vm/jump_table.go

+// byzantium and contantinople instructions.
+func NewConstantinopleInstructionSet() [256]operation {
+	// instructions that can be executed during the byzantium phase.
+	instructionSet := NewHomesteadInstructionSet()


NewHomesteadInstructionSet -> NewByzantiumInstructionSet

karalabe · 2018-02-22T18:06:37Z

params/config.go


 	// AllCliqueProtocolChanges contains every protocol change (EIPs) introduced
 	// and accepted by the Ethereum core developers into the Clique consensus.
 	//
 	// This configuration is intentionally not using keyed fields to force anyone
 	// adding flags to the config to also have to set these fields.
-	AllCliqueProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, &CliqueConfig{Period: 0, Epoch: 30000}}
+	AllCliqueProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), big.NewInt(0), nil, &CliqueConfig{Period: 0, Epoch: 30000}}


I don't think we should enable it before it's actually accepted. --dev uses these and we don't really want users to start using alpha code.

karalabe · 2018-02-22T18:06:40Z

params/config.go

@@ -82,16 +82,16 @@ var (
 	//
 	// This configuration is intentionally not using keyed fields to force anyone
 	// adding flags to the config to also have to set these fields.
-	AllEthashProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), new(EthashConfig), nil}
+	AllEthashProtocolChanges = &ChainConfig{big.NewInt(1337), big.NewInt(0), nil, false, big.NewInt(0), common.Hash{}, big.NewInt(0), big.NewInt(0), big.NewInt(0), big.NewInt(0), new(EthashConfig), nil}


I don't think we should enable it before it's actually accepted. --dev uses these and we don't really want users to start using alpha code.

karalabe · 2018-02-22T18:07:35Z

cmd/puppeth/wizard_genesis.go

+		fmt.Println()
+		fmt.Printf("Which block should Constantinople come into effect? (default = %v)\n", w.conf.Genesis.Config.ConstantinopleBlock)
+		w.conf.Genesis.Config.ConstantinopleBlock = w.readDefaultBigInt(w.conf.Genesis.Config.ConstantinopleBlock)
+


I don't think puppeth should offer Constantinople until it's actually finalized. Otherwise people will end up with broken chains.

… shift instructions

holiman · 2018-02-23T08:53:53Z

Review concerns addressed, rebased to fix conflicts. PTAL

karalabe · 2018-02-23T09:45:05Z

core/vm/opcodes.go

@@ -63,6 +63,9 @@ const (
 	XOR
 	NOT
 	BYTE
+	SHL
+	SHR
+	SAR


Ah, missed this previously. You'll need to add these opcodes to the opcode -> string map in the same file a few hundred lines below this into opCodeToString.

holiman · 2018-02-23T10:21:55Z

Fixed, ptal

karalabe

LGTM

* core, vm, common: define constantinople fork, start implementation of shift instructions * vm: more testcases * vm: add tests for intpool erroneous intpool handling * core, vm, common: fix constantinople review concerns * vm: add string<->op definitions for new opcodes

holiman requested a review from karalabe as a code owner February 9, 2018 08:07

karalabe requested changes Feb 22, 2018

View reviewed changes

holiman added 4 commits February 23, 2018 09:51

core, vm, common: define constantinople fork, start implementation of…

50cecb8

… shift instructions

vm: more testcases

385fd13

vm: add tests for intpool erroneous intpool handling

853942f

core, vm, common: fix constantinople review concerns

26b8d50

holiman force-pushed the constantinople branch from b5c9446 to 26b8d50 Compare February 23, 2018 08:53

karalabe reviewed Feb 23, 2018

View reviewed changes

vm: add string<->op definitions for new opcodes

5f89802

karalabe approved these changes Feb 23, 2018

View reviewed changes

karalabe added this to the 1.8.2 milestone Feb 23, 2018

karalabe merged commit 44d40ff into ethereum:master Feb 23, 2018

karalabe mentioned this pull request Jul 24, 2018

Ethereum Core Devs Meeting 43 Agenda ethereum/pm#51

Closed

Souptacular mentioned this pull request Jul 30, 2018

Constantinople Progress Tracker ethereum/pm#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core, vm, common: define constantinople fork + shift #16045

core, vm, common: define constantinople fork + shift #16045

holiman commented Feb 9, 2018 •

edited

Loading

axic commented Feb 9, 2018

karalabe commented Feb 9, 2018

axic commented Feb 9, 2018

holiman commented Feb 12, 2018

axic commented Feb 12, 2018

karalabe left a comment

karalabe Feb 22, 2018

karalabe Feb 22, 2018

karalabe Feb 22, 2018

karalabe Feb 22, 2018

holiman commented Feb 23, 2018

karalabe Feb 23, 2018

holiman commented Feb 23, 2018

karalabe left a comment

@@ @@ -63,6 +63,9 @@ const ( @@
               	XOR
               	NOT
               	BYTE
+              	SHL
+              	SHR
+              	SAR

core, vm, common: define constantinople fork + shift #16045

core, vm, common: define constantinople fork + shift #16045

Conversation

holiman commented Feb 9, 2018 • edited Loading

axic commented Feb 9, 2018

karalabe commented Feb 9, 2018

axic commented Feb 9, 2018

holiman commented Feb 12, 2018

axic commented Feb 12, 2018

karalabe left a comment

Choose a reason for hiding this comment

karalabe Feb 22, 2018

Choose a reason for hiding this comment

karalabe Feb 22, 2018

Choose a reason for hiding this comment

karalabe Feb 22, 2018

Choose a reason for hiding this comment

karalabe Feb 22, 2018

Choose a reason for hiding this comment

holiman commented Feb 23, 2018

karalabe Feb 23, 2018

Choose a reason for hiding this comment

holiman commented Feb 23, 2018

karalabe left a comment

Choose a reason for hiding this comment

holiman commented Feb 9, 2018 •

edited

Loading