consensus, core/*: metropolis features #14337

obscuren · 2017-04-16T07:55:38Z

Todos:

96 Blockhash refactoring
- Peter will implement this
Felix will fix general state tests

Done:

98 Removal of intermediate state roots from receipts
100 Change difficulty adjustment
140 REVERT instruction
196 Precompiled contracts for addition and scalar multiplication on the elliptic curve
197 Precompiled contracts for optimal Ate pairing check on the elliptic curve alt_bn128 Buterin
198 Precompiled contract for bigint modular exponentiation
211 New opcodes: RETURNDATASIZE and RETURNDATACOPY
214 New opcode STATICCALL
86 Abstraction of transaction origin and signature

pirapira · 2017-04-19T14:26:18Z

core/types/transaction_signing.go

+
+func isAbstractSigner(tx *Transaction) bool {
+	v, r, s := tx.RawSignatureValues()
+	return v.BitLen() == 0 && r.BitLen() == 0 && s.BitLen() == 0


The EIP has changed and v here needs to be the chain ID.

pirapira · 2017-04-25T13:36:37Z

consensus/ethash/consensus.go

+		x = big.NewInt(2)
+	}
+	z := new(big.Int).Sub(bigTime, bigParentTime)
+	z.Div(x, big9)


Shouldn't this be z.Div(z, big9)?

pirapira · 2017-04-25T19:16:10Z

core/vm/interpreter.go

 	// may me left uninitialised and will be set the default
 	// table.
 	JumpTable [256]operation
 }

 // Interpreter is used to run Ethereum based contracts and will utilise the
-// passed environment to query external sources for state information.
+// passed evmironment to query external sources for state information.


That's a fancy word.

:-P screwed up find and replace

pirapira · 2017-05-11T15:18:33Z

core/state_processor.go

 	// Create a new receipt for the transaction, storing the intermediate root and gas used by the tx
 	// based on the eip phase, we're passing wether the root touch-delete accounts.
-	receipt := types.NewReceipt(statedb.IntermediateRoot(config.IsEIP158(header.Number)).Bytes(), usedGas)
+	receipt := types.NewReceipt(root, usedGas)


~~Since the option 1 of EIP98 was chosen, so it's not enough to make root the empty sequence. The receipt has one less elements in Metropolis. See a log of a coredev meeting.~~ Ah, no, this seems all fine.

pirapira · 2017-05-11T15:43:28Z

core/vm/contracts.go

+	common.BytesToAddress([]byte{4}): &dataCopy{},
+	common.BytesToAddress([]byte{5}): &bigModexp{},
+	common.BytesToAddress([]byte{6}): &bn256Add{},
+	common.BytesToAddress([]byte{6}): &bn256ScalarMul{},


Address 6 is used twice, it seems.

pirapira · 2017-05-11T16:14:32Z

core/vm/contracts.go

+	}
+	mod := new(big.Int).SetBytes(input[:modLen])
+
+	return base.Exp(base, exp, mod).Bytes(), nil


The length of the result needs to be the same as that of mod. See https://github.com/ethereum/EIPs/pull/198/files#diff-b02d15001884061861b0e92a735e7cefR11

pirapira · 2017-05-11T16:33:27Z

core/vm/evm.go

@@ -79,25 +104,72 @@ type EVM struct {
 	abort int32
 }

-// NewEVM retutrns a new EVM evmironment.
+// NewEVM retutrns a new EVM evmironment. The returned EVM is not thread safe


Now my brain thinks evmironment is a word.

pirapira · 2017-05-11T16:34:08Z

core/vm/evm.go

+	}
+
+	// initialise a new contract and set the code that is to be used by the
+	// E The contract is a scoped evmironment for this execution context


I guess this E is EVM.

pirapira · 2017-05-11T16:37:21Z

core/vm/evm.go

+	ret, err = evm.interpreter.Run(snapshot, contract, input)
+	// When an error was returned by the EVM or when setting the creation code
+	// above we revert to the snapshot and consume any gas remaining. Additionally
+	// when we're in homestead this also counts for code storage gas errors.


I guess we never hit here during Homestead.

pirapira · 2017-05-11T16:43:54Z

core/vm/interpreter.go

+			// if the interpreter is operating in readonly mode, make sure no
+			// state-modifying operation is performed.
+			if operation.writes ||
+				((op == CALL || op == CALLCODE) && stack.Back(3).BitLen() > 0) {


LOG0, LOG1, LOG2, LOG3, LOG4 should be banned here as well. Currently their .writes value is false.

This has been resolved. These instructions now have .writes == true.

pirapira · 2017-05-12T09:01:05Z

core/vm/contracts.go

+}
+
+func (c *bn256Add) Run(in []byte) ([]byte, error) {
+	if len(in) != 96 {


If the input is too short, it should be padded. I think this changed at some point.

I believe when I wrote this it was still up for debate, including changing the semantics of CALLDATALOAD (i.e. that it should throw)

pirapira · 2017-05-12T09:02:55Z

core/vm/contracts.go

+	return 0 // TODO
+}
+
+func (c *bn256Add) Run(in []byte) ([]byte, error) {


The type says bn256Add but this function calls ScalarMult.

pirapira · 2017-05-12T09:04:10Z

core/vm/contracts.go

+	return 0 // TODO
+}
+
+func (c *bn256ScalarMul) Run(in []byte) ([]byte, error) {


The type says bn256ScalarMul but this function performs addition.

pirapira · 2017-05-12T09:06:03Z

core/vm/contracts.go

+}
+
+func (c *bn256ScalarMul) Run(in []byte) ([]byte, error) {
+	if len(in) != 128 {


The input should be padded if it's too short.

obscuren · 2017-05-19T13:47:19Z

@pirapira thanks for the review. I'm awaiting review and approval on #14336 before I continue.

pirapira · 2017-05-19T13:57:59Z

@obscuren yes, I know. I've been just reading the interesting parts (which was a reasonable thing to do after doing YP).

obscuren · 2017-06-03T07:21:18Z

@pirapira I've addressed your comments. Thanks again.

holiman · 2017-06-07T15:28:28Z

I was trying to run in on Hive, but build fails:

# github.com/ethereum/go-ethereum/core/vm
core/vm/interpreter.go:76: undefined: metropolisInstructionSet

holiman · 2017-06-07T15:30:47Z

core/vm/instructions.go

@@ -729,7 +729,7 @@ func opReturnDataCopy(pc *uint64, evm *EVM, contract *Contract, memory *Memory,
 		cOff = stack.pop()
 		l    = stack.pop()
 	)
-	memory.Set(mOff.Uint64(), l.Uint64(), getData(memory.lastReturn, cOff, l))
+	memory.Set(mOff.Uint64(), l.Uint64(), getData(evm.interpreter.returnData, cOff, l))


This should generate an error if the operation tries to read out of bounds, as agreed on the last core dev call.

holiman · 2017-06-07T15:36:25Z

core/vm/interpreter.go

@@ -228,7 +234,7 @@ func (in *Interpreter) Run(snapshot int, contract *Contract, input []byte) (ret
 		// if the operation returned a value make sure that is also set
 		// the last return data.
 		if res != nil {


I'm not sure this is sufficient.

In the case of CALL, the res may be nil for various reasons, e.g. the balance being to low:

if !evm.Context.CanTransfer(evm.StateDB, caller.Address(), value) { return nil, gas, ErrInsufficientBalance }

The way it's written, I don't think RETURNDATA would be cleared. I believe the returndata should be cleared immediately when CALL is invoked.

Further, I believe that also CREATE should clear the RETURNDATA. But I'm not 100% certain in this case. @pirapira what's your take ?

I think it would be better with

if operations.clearsReturndata{ in.returnData = res }

References: ethereum/EIPs#211 (comment)

To summarize: Any opcode that attempts to create a new call stack frame resets the buffer of the current stack frame right before it executes, even if that opcode fails.

Also, from the EIP itself -
https://github.com/ethereum/EIPs/blob/aeaffcca26fa688f5015da7ce059c886e90ef374/EIPS/returndatacopy.md:

As an optimization, it is possible to share the return data buffer across call frames because only one will be non-empty at any time.

holiman · 2017-06-16T07:27:03Z

core/types/transaction_signing.go

+// to the chain identifier and r and s being 0.
+func isAbstractSigner(tx *Transaction, chainId *big.Int) bool {
+	v, r, s := tx.RawSignatureValues()
+	return v.Cmp(chainId) == 0 && r.BitLen() == 0 && s.BitLen() == 0


A requirement for it being an abstract-signer tx, is that gasprice = 0, nonce = 0, value = 0. Is this checked anywhere?

spec

holiman · 2017-06-16T07:28:02Z

core/vm/jump_table.go

+	}
+	instructionSet[RETURNDATASIZE] = operation{
+		execute:       opReturnDataSize,
+		gasCost:       constGasFunc(0), // TODO


According to https://github.com/ethereum/EIPs/blob/aeaffcca26fa688f5015da7ce059c886e90ef374/EIPS/returndatacopy.md#specification:

Gas costs: 2 (same as CALLDATASIZE)

Or rather, GasQuickStep.

holiman · 2017-06-19T19:40:12Z

core/vm/instructions.go

+		cOff = stack.pop()
+		l    = stack.pop()
+	)
+	memory.Set(mOff.Uint64(), l.Uint64(), getData(evm.interpreter.returnData, cOff, l))


Should raise error in case the requested data is out of bounds - getData pads with zeroes, afaik.

holiman · 2017-06-19T19:41:39Z

core/vm/interpreter.go

+			// if the interpreter is operating in readonly mode, make sure no
+			// state-modifying operation is performed.
+			if operation.writes ||
+				((op == CALL || op == CALLCODE) && stack.Back(3).BitLen() > 0) {


I think a comment would be in order here to shed some light on what magic is contained within stack.Back(3)

holiman · 2017-06-20T10:03:51Z

At https://github.com/obscuren/go-ethereum/blob/metropolis-finalisation/core/vm/evm.go#L195:

		if PrecompiledContracts[addr] == nil && evm.ChainConfig().IsEIP158(evm.BlockNumber) && value.Sign() == 0 {

You need to check for Metro, and if so, use PrecompiledContractsMetropolis instead of PrecompiledContracts.

I think. I'm actually not 100%.

Reimplemented RETURNDATA such that the returndata buffer is isolated inside the interpreter rather than the memory object.

* Right pad return data of native contract big mod exp to same length as the modulo input * Spelling * Make LOG{n} write operations so the throw during readonly * Right pad native contracts input with zeros * Fixed bn256{add, mul}

* Changed the gas for RETURNDATASIZE * Added comments for read only check * Fixed opReturnDataCopy

holiman · 2017-06-26T09:25:23Z

@obscuren there's also an error in bn256Add - see obscuren@ec8ff51

(but maybe ignore the gas calculation, I'm not sure it's correct)

karalabe · 2017-08-15T07:48:59Z

Superseded by #14726 (which itself is superseded already).

obscuren added the in progress label Apr 16, 2017

obscuren mentioned this pull request Apr 16, 2017

consensus, core/*, params: metropolis preparation refactor #14336

Merged

pirapira reviewed Apr 19, 2017

View reviewed changes

pirapira reviewed Apr 25, 2017

View reviewed changes

obscuren force-pushed the metropolis-finalisation branch from f5c98a4 to 8042bbe Compare May 6, 2017 11:49

pirapira reviewed May 11, 2017

View reviewed changes

pirapira reviewed May 12, 2017

View reviewed changes

obscuren force-pushed the metropolis-finalisation branch 2 times, most recently from 2cff572 to 046fc67 Compare May 29, 2017 07:11

fjl modified the milestone: 1.6.3 May 31, 2017

karalabe modified the milestones: 1.6.4, 1.6.3 Jun 1, 2017

obscuren force-pushed the metropolis-finalisation branch from 046fc67 to eab5213 Compare June 3, 2017 07:20

obscuren force-pushed the metropolis-finalisation branch from eab5213 to f90b410 Compare June 4, 2017 18:46

holiman reviewed Jun 7, 2017

View reviewed changes

Souptacular mentioned this pull request Jun 16, 2017

All Core Devs Meeting 18 Agenda ethereum/pm#16

Closed

holiman requested changes Jun 16, 2017

View reviewed changes

holiman reviewed Jun 19, 2017

View reviewed changes

Souptacular mentioned this pull request Jun 22, 2017

All Core Devs Meeting 19 Agenda ethereum/pm#17

Closed

fjl modified the milestones: 1.6.7, 1.6.4 Jun 23, 2017

obscuren and others added 8 commits June 24, 2017 09:26

consensus, core/*: metropolis features

1f7e091

core/types: make receipt PostState optional

f7212a4

core/vm: reimplemented RETURNDATA{SIZE, COPY}

ad0a31f

Reimplemented RETURNDATA such that the returndata buffer is isolated inside the interpreter rather than the memory object.

core/vm: added metropolis instructions

edf95ec

core/vm: fixed clearing issue on return data

797172b

core/vm: implemented create2

4eae3cc

core/vm: review changes (gas, comments)

43577cd

* Changed the gas for RETURNDATASIZE * Added comments for read only check * Fixed opReturnDataCopy

obscuren force-pushed the metropolis-finalisation branch from 9ec99ce to 43577cd Compare June 24, 2017 07:27

Souptacular mentioned this pull request Jul 12, 2017

All Core Devs Meeting 20 Agenda ethereum/pm#18

Closed

fjl modified the milestones: 1.6.7, 1.7.0 Jul 17, 2017

Souptacular mentioned this pull request Jul 26, 2017

All Core Devs Meeting 21 Agenda ethereum/pm#19

Closed

Souptacular mentioned this pull request Aug 6, 2017

Ethereum Core Devs Meeting 22 Agenda ethereum/pm#20

Closed

Souptacular mentioned this pull request Aug 14, 2017

Ethereum Core Devs Meeting 23 Agenda ethereum/pm#21

Closed

karalabe closed this Aug 15, 2017

adamschmideg removed in progress labels Dec 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consensus, core/*: metropolis features #14337

consensus, core/*: metropolis features #14337

obscuren commented Apr 16, 2017 •

edited

Loading

pirapira Apr 19, 2017

pirapira Apr 25, 2017

pirapira Apr 25, 2017

obscuren May 5, 2017

pirapira May 11, 2017 •

edited

Loading

pirapira May 11, 2017

pirapira May 11, 2017

pirapira May 11, 2017

pirapira May 11, 2017

pirapira May 11, 2017

pirapira May 11, 2017

pirapira Jun 9, 2017

pirapira May 12, 2017

obscuren Jun 3, 2017

pirapira May 12, 2017

pirapira May 12, 2017

pirapira May 12, 2017

obscuren commented May 19, 2017

pirapira commented May 19, 2017

obscuren commented Jun 3, 2017

holiman commented Jun 7, 2017

holiman Jun 7, 2017

holiman Jun 7, 2017 •

edited

Loading

holiman Jun 7, 2017

holiman Jun 8, 2017

holiman Jun 16, 2017

holiman Jun 16, 2017

holiman Jun 19, 2017

holiman Jun 19, 2017

holiman commented Jun 20, 2017 •

edited

Loading

holiman commented Jun 26, 2017 •

edited

Loading

karalabe commented Aug 15, 2017

consensus, core/*: metropolis features #14337

consensus, core/*: metropolis features #14337

Conversation

obscuren commented Apr 16, 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

pirapira May 11, 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

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

obscuren commented May 19, 2017

pirapira commented May 19, 2017

obscuren commented Jun 3, 2017

holiman commented Jun 7, 2017

Choose a reason for hiding this comment

holiman Jun 7, 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

Choose a reason for hiding this comment

holiman commented Jun 20, 2017 • edited Loading

holiman commented Jun 26, 2017 • edited Loading

karalabe commented Aug 15, 2017

obscuren commented Apr 16, 2017 •

edited

Loading

pirapira May 11, 2017 •

edited

Loading

holiman Jun 7, 2017 •

edited

Loading

holiman commented Jun 20, 2017 •

edited

Loading

holiman commented Jun 26, 2017 •

edited

Loading