New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from May 24, 2017

Conversation

Projects
None yet
5 participants
@obscuren
Member

obscuren commented Apr 16, 2017

This commit is a preparation for the upcoming metropolis hardfork. It
prepares the state, core and vm packages such that integration with
metropolis becomes less of a hassle.

  • Difficulty calculation requires header instead of individual
    parameters
  • statedb.StartRecord renamed to statedb.Prepare and added Finalise
    method required by metropolis, which removes unwanted accounts from
    the state (i.e. selfdestruct)
  • State keeps record of destructed objects (in addition to dirty
    objects)
  • core/vm pre-compiles may now return errors
  • core/vm pre-compiles gas check now take the full byte slice as argument
    instead of just the size
  • core/vm now keeps several hard-fork instruction tables instead of a
    single instruction table and removes the need for hard-fork checks in
    the instructions
  • core/vm contains a empty restruction function which is added in
    preparation of metropolis write-only mode operations
  • Adds the bn256 curve
  • Adds and sets the metropolis chain config block parameters (2^64-1)

Follow up PR which adds metropolis features may be found here #14337

Show outdated Hide outdated consensus/ethash/consensus.go Outdated
Show outdated Hide outdated core/types/transaction_signing.go Outdated
@@ -125,6 +131,7 @@ func (c *ChainConfig) String() string {
c.EIP150Block,
c.EIP155Block,
c.EIP158Block,
c.MetropolisBlock,

This comment has been minimized.

@holiman

holiman Apr 24, 2017

Contributor

You need to add it to the format-statement aswell

@holiman

holiman Apr 24, 2017

Contributor

You need to add it to the format-statement aswell

@@ -47,13 +47,32 @@ type operation struct {
// jumps indicates whether operation made a jump. This prevents the program
// counter from further incrementing.
jumps bool
// writes determines whether this a state modifying operation
writes bool

This comment has been minimized.

@holiman

holiman Apr 24, 2017

Contributor

I'm not sure what's the ultimate intention of writes , but you could argue that CALL is also a state modifying operation, since if you send ether to a non-existing account, a new account is created in the state. You have not added the writes-flag to CALL (and friends) though ?

@holiman

holiman Apr 24, 2017

Contributor

I'm not sure what's the ultimate intention of writes , but you could argue that CALL is also a state modifying operation, since if you send ether to a non-existing account, a new account is created in the state. You have not added the writes-flag to CALL (and friends) though ?

This comment has been minimized.

@obscuren

obscuren May 4, 2017

Member

Correct, because a call might not always write. This is checked here. I guess this could have been moved to the metro PR to make things more clear.

In any case a CALL which does not transfer value (including sub calls) may be regarded as a non-writing operation.

@obscuren

obscuren May 4, 2017

Member

Correct, because a call might not always write. This is checked here. I guess this could have been moved to the metro PR to make things more clear.

In any case a CALL which does not transfer value (including sub calls) may be regarded as a non-writing operation.

Show outdated Hide outdated core/chain_makers.go Outdated
RequiredGas(inputSize int) uint64 // RequiredPrice calculates the contract gas use
Run(input []byte) []byte // Run runs the precompiled contract
RequiredGas(input []byte) uint64 // RequiredPrice calculates the contract gas use
Run(input []byte) ([]byte, error) // Run runs the precompiled contract

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Why do you need to return an error? What issue can bubble up from the VM into the node?

@karalabe

karalabe May 7, 2017

Member

Why do you need to return an error? What issue can bubble up from the VM into the node?

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

It's required in the metro PR.

@obscuren

obscuren May 7, 2017

Member

It's required in the metro PR.

@@ -0,0 +1 @@
package vm

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

This is an empty file. Perhaps drop until the actual content is added.

@karalabe

karalabe May 7, 2017

Member

This is an empty file. Perhaps drop until the actual content is added.

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

Meh :)

@obscuren

obscuren May 7, 2017

Member

Meh :)

// run runs the given contract and takes care of running precompiles with a fallback to the byte code interpreter.
func run(evm *EVM, snapshot int, contract *Contract, input []byte) ([]byte, error) {
if contract.CodeAddr != nil {
precompiledContracts := PrecompiledContracts

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Why copy this map into a local variable?

@karalabe

karalabe May 7, 2017

Member

Why copy this map into a local variable?

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

To make my life easier in the follow up PR. I literally split the PR in to two pieces and this was the easiest way.

@obscuren

obscuren May 7, 2017

Member

To make my life easier in the follow up PR. I literally split the PR in to two pieces and this was the easiest way.

@@ -275,10 +294,8 @@ func (evm *EVM) Create(caller ContractRef, code []byte, gas uint64, value *big.I
// when we're in homestead this also counts for code storage gas errors.
if maxCodeSizeExceeded ||
(err != nil && (evm.ChainConfig().IsHomestead(evm.BlockNumber) || err != ErrCodeStoreOutOfGas)) {
contract.UseGas(contract.Gas)

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Could you please expand on what's happening here? Why did you change the previous code to this?

@karalabe

karalabe May 7, 2017

Member

Could you please expand on what's happening here? Why did you change the previous code to this?

This comment has been minimized.

@obscuren

obscuren May 12, 2017

Member

Because it contains duplicate code. The return statement does the same thing as the return couple lines below.

@obscuren

obscuren May 12, 2017

Member

Because it contains duplicate code. The return statement does the same thing as the return couple lines below.

Show outdated Hide outdated core/vm/interpreter.go Outdated
// 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.

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Typo.

@karalabe

karalabe May 7, 2017

Member

Typo.

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

Fixed in the metro PR

@obscuren

obscuren May 7, 2017

Member

Fixed in the metro PR

defer func() { evm.env.depth-- }()
func (in *Interpreter) enforceRestrictions(op OpCode, operation operation, stack *Stack) error {
return nil
}

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Plop, what's this?

@karalabe

karalabe May 7, 2017

Member

Plop, what's this?

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

See metro PR. Merely to make my life easier in the follow up.

@obscuren

obscuren May 7, 2017

Member

See metro PR. Merely to make my life easier in the follow up.

Show outdated Hide outdated core/vm/interpreter.go Outdated
Show outdated Hide outdated core/vm/interpreter.go Outdated
Show outdated Hide outdated core/vm/interpreter.go Outdated
// the last return data.
if res != nil {
mem.lastReturn = ret
}

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Just to make sure, this doens't change the current behavior of the EVM, right? What happens if the last opcode in a current contract is CALL?

@karalabe

karalabe May 7, 2017

Member

Just to make sure, this doens't change the current behavior of the EVM, right? What happens if the last opcode in a current contract is CALL?

This comment has been minimized.

@obscuren

obscuren May 7, 2017

Member

No, this shouldn't change the behaviour of the EVM.

@obscuren

obscuren May 7, 2017

Member

No, this shouldn't change the behaviour of the EVM.

Show outdated Hide outdated core/vm/jump_table.go Outdated
Show outdated Hide outdated core/vm/jump_table.go Outdated
Show outdated Hide outdated core/vm/jump_table.go Outdated
EIP158Block: MainNetSpuriousDragon,
MetropolisBlock: MainNetMetropolisBlock,
Ethash: new(EthashConfig),

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

I'd recommend dropping all MainNetxxx constants and instead moving them into this struct. It's ambiguous from outside code that there's two way to retrieve the same constants and aso error prone if you add a constant but don't add it to the chain config.

@karalabe

karalabe May 7, 2017

Member

I'd recommend dropping all MainNetxxx constants and instead moving them into this struct. It's ambiguous from outside code that there's two way to retrieve the same constants and aso error prone if you add a constant but don't add it to the chain config.

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Similar to the TestnetChainConfig below.

@karalabe

karalabe May 7, 2017

Member

Similar to the TestnetChainConfig below.

EIP150Hash: common.HexToHash("0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d"),
EIP155Block: big.NewInt(10),
EIP158Block: big.NewInt(10),
MetropolisBlock: TestNetMetropolisBlock,

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

I'd recommend using the real value, not an intermediate constant here. It'd be cleaner.

@karalabe

karalabe May 7, 2017

Member

I'd recommend using the real value, not an intermediate constant here. It'd be cleaner.

EIP158Block: big.NewInt(10),
MetropolisBlock: TestNetMetropolisBlock,
Ethash: new(EthashConfig),
}
// RinkebyChainConfig contains the chain parameters to run a node on the Rinkeby test network.

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

Please add the Metropolis block field to RinkebyConfig too.

@karalabe

karalabe May 7, 2017

Member

Please add the Metropolis block field to RinkebyConfig too.

Show outdated Hide outdated params/config.go Outdated
Show outdated Hide outdated params/config.go Outdated
Show outdated Hide outdated params/config.go Outdated
MainNetMetropolisBlock = big.NewInt(math.MaxInt64)
TestNetChainID = big.NewInt(3) // Test net default chain ID
MainNetChainID = big.NewInt(1) // main net default chain ID

This comment has been minimized.

@karalabe

karalabe May 7, 2017

Member

I'd recommend dropping all these fields (,aybe the hash needs to stay and using the already existing ChainConfig constants instead.

@karalabe

karalabe May 7, 2017

Member

I'd recommend dropping all these fields (,aybe the hash needs to stay and using the already existing ChainConfig constants instead.

This comment has been minimized.

@obscuren

obscuren May 12, 2017

Member

We can do that. I'll make that change in the metro PR.

@obscuren

obscuren May 12, 2017

Member

We can do that. I'll make that change in the metro PR.

@holiman

This comment has been minimized.

Show comment
Hide comment
@holiman

holiman May 8, 2017

Contributor

I'd like to test it a bit in hive, but can't build it as it is now:

# github.com/ethereum/go-ethereum/consensus/ethash
consensus/ethash/consensus.go:318: cannot use time (type uint64) as type *big.Int in argument to new(big.Int).Set
Contributor

holiman commented May 8, 2017

I'd like to test it a bit in hive, but can't build it as it is now:

# github.com/ethereum/go-ethereum/consensus/ethash
consensus/ethash/consensus.go:318: cannot use time (type uint64) as type *big.Int in argument to new(big.Int).Set
@obscuren

This comment has been minimized.

Show comment
Hide comment
@obscuren
Member

obscuren commented May 12, 2017

@holiman: fixed

obscuren added some commits Feb 1, 2017

consensus, core/*, params: metropolis preparation refactor
This commit is a preparation for the upcoming metropolis hardfork. It
prepares the state, core and vm packages such that integration with
metropolis becomes less of a hassle.

* Difficulty calculation requires header instead of individual
  parameters
* statedb.StartRecord renamed to statedb.Prepare and added Finalise
  method required by metropolis, which removes unwanted accounts from
  the state (i.e. selfdestruct)
* State keeps record of destructed objects (in addition to dirty
  objects)
* core/vm pre-compiles may now return errors
* core/vm pre-compiles gas check now take the full byte slice as argument
  instead of just the size
* core/vm now keeps several hard-fork instruction tables instead of a
  single instruction table and removes the need for hard-fork checks in
  the instructions
* core/vm contains a empty restruction function which is added in
  preparation of metropolis write-only mode operations
* Adds the bn256 curve
* Adds and sets the metropolis chain config block parameters (2^64-1)
core/state: fixed (self)destructed objects
Add the object to the list of destructed objects during a selfdestruct /
suicide operation and also remove it from the list once the journal
reverts.

wuestholz and others added some commits May 22, 2017

core/vm: improved push instructions
Improved push instructions by removing unnecessary big int allocations
and by making it int instead of big.Int
core/vm: expose intpool to stack dup method
Improve the duplication method of the stack to reuse big ints by passing
in an existing integer pool.
common: fixed byte padding functions
Byte padding function should return the given slice if the length is
smaller or equal rather than *only* smaller than.

This fix improves almost all EVM push operations.

fjl added some commits May 24, 2017

crypto/bn256: fix go vet false positive
Also add the package to the license tool ignore list.
@fjl

fjl approved these changes May 24, 2017 edited

👍

@fjl fjl merged commit 261b3e2 into ethereum:master May 24, 2017

3 checks passed

commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@obscuren obscuren deleted the obscuren:metropolis-preparation branch May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment