From 1013657a627c2a3bf81f4ba4154890caa68f11bf Mon Sep 17 00:00:00 2001 From: meows Date: Fri, 21 Feb 2020 08:08:50 -0500 Subject: [PATCH 1/2] Revert "core/vm,params/vars: implement missing SLOAD gas cost change" This reverts commit 9ad9394d1caeaa2631ac9359497734e53615c572. --- core/vm/eips.go | 3 +-- params/vars/protocol_params.go | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/core/vm/eips.go b/core/vm/eips.go index 005e4c431397..ba7dcd6cc5ca 100644 --- a/core/vm/eips.go +++ b/core/vm/eips.go @@ -56,9 +56,9 @@ func enableSelfBalance(jt *JumpTable) { // - Define SELFBALANCE, with cost GasFastStep (5) func enable1884(jt *JumpTable) { // Gas cost changes - jt[SLOAD].constantGas = vars.SloadGasEIP1884 jt[BALANCE].constantGas = vars.BalanceGasEIP1884 jt[EXTCODEHASH].constantGas = vars.ExtcodeHashGasEIP1884 + jt[SLOAD].constantGas = vars.SloadGasEIP1884 // New opcode enableSelfBalance(jt) @@ -101,6 +101,5 @@ func enable2200Sloppy(jt *JumpTable) { // enable2200 applies EIP-2200 (Rebalance net-metered SSTORE) func enable2200(jt *JumpTable) { - jt[SLOAD].constantGas = vars.SloadGasEIP2200 jt[SSTORE].dynamicGas = gasSStoreEIP2200 } diff --git a/params/vars/protocol_params.go b/params/vars/protocol_params.go index 94102650be4f..62a8c85f6170 100644 --- a/params/vars/protocol_params.go +++ b/params/vars/protocol_params.go @@ -104,7 +104,6 @@ var ( SloadGasFrontier uint64 = 50 SloadGasEIP150 uint64 = 200 SloadGasEIP1884 uint64 = 800 // Cost of SLOAD after EIP 1884 (part of Istanbul) - SloadGasEIP2200 uint64 = 800 // Cost of SLOAD after EIP 2200 (part of Istanbul) ExtcodeHashGasConstantinople uint64 = 400 // Cost of EXTCODEHASH (introduced in Constantinople) ExtcodeHashGasEIP1884 uint64 = 700 // Cost of EXTCODEHASH after EIP 1884 (part in Istanbul) SelfdestructGasEIP150 uint64 = 5000 // Cost of SELFDESTRUCT post EIP 150 (Tangerine) From db2e8c53ac71a0403ff6668ddefea83464e29161 Mon Sep 17 00:00:00 2001 From: meows Date: Fri, 21 Feb 2020 10:30:22 -0500 Subject: [PATCH 2/2] core/vm,params: revert ECIP1086 implementation This ECIP allowed an incorrect specification for EIP2200 which is no longer needed, since the incorrect spec was actually 'correct'. Signed-off-by: meows --- core/vm/eips.go | 9 -- core/vm/gas_table_test.go | 1 + core/vm/jump_table.go | 11 +-- core/vm/jump_table_test.go | 88 ------------------- params/config_kotti.go | 2 - params/config_mordor.go | 2 - params/types/ctypes/configurator_iface.go | 3 - params/types/genesisT/genesis.go | 8 -- .../goethereum/goethereum_configurator.go | 11 --- params/types/multigeth/chain_config.go | 2 - .../multigeth/chain_config_configurator.go | 9 -- .../multigethv0_chain_config_configurator.go | 11 --- params/types/parity/parity_configurator.go | 11 --- 13 files changed, 2 insertions(+), 166 deletions(-) delete mode 100644 core/vm/jump_table_test.go diff --git a/core/vm/eips.go b/core/vm/eips.go index ba7dcd6cc5ca..2daa44bf8715 100644 --- a/core/vm/eips.go +++ b/core/vm/eips.go @@ -90,15 +90,6 @@ func opChainID(pc *uint64, interpreter *EVMInterpreter, contract *Contract, memo return nil, nil } -// enable2200Sloppy applies EIP-2200 (Rebalance net-metered SSTORE) -// WITHOUT IMPLEMENTING THE GAS REPRICING FOR SLOAD OPCODE. -func enable2200Sloppy(jt *JumpTable) { - // This value is wrong on purpose; it makes the "sloppiness" explicit. - jt[SLOAD].constantGas = vars.SloadGasEIP150 // 200 - - jt[SSTORE].dynamicGas = gasSStoreEIP2200 -} - // enable2200 applies EIP-2200 (Rebalance net-metered SSTORE) func enable2200(jt *JumpTable) { jt[SSTORE].dynamicGas = gasSStoreEIP2200 diff --git a/core/vm/gas_table_test.go b/core/vm/gas_table_test.go index 5d443de0eae6..743bf5d9af5c 100644 --- a/core/vm/gas_table_test.go +++ b/core/vm/gas_table_test.go @@ -91,6 +91,7 @@ func TestEIP2200(t *testing.T) { CanTransfer: func(StateDB, common.Address, *big.Int) bool { return true }, Transfer: func(StateDB, common.Address, common.Address, *big.Int) {}, } + vmenv := NewEVM(vmctx, statedb, params.AllEthashProtocolChanges, Config{ExtraEips: []int{2200}}) _, gas, err := vmenv.Call(AccountRef(common.Address{}), address, nil, tt.gaspool, new(big.Int)) diff --git a/core/vm/jump_table.go b/core/vm/jump_table.go index 42edd9845217..c79b38ce1d58 100644 --- a/core/vm/jump_table.go +++ b/core/vm/jump_table.go @@ -190,16 +190,7 @@ func instructionSetForConfig(config ctypes.ChainConfigurator, bn *big.Int) JumpT enableSelfBalance(&instructionSet) } - // EIP2200 was originally implemented incorrectly (not meeting specifications) by ethereum/go-ethereum, multi-geth, and Parity - // clients. - // ECIP1086 is a specification to allow this "bad" implementation, which is useful for ETC testnets Kotti and Mordor. - // - is2200enabled := config.IsEnabled(config.GetEIP2200Transition, bn) && !config.IsEnabled(config.GetEIP2200DisableTransition, bn) - if is2200enabled && - config.IsEnabled(config.GetECIP1086Transition, bn) && - !config.IsEnabled(config.GetEIP1884Transition, bn) { - enable2200Sloppy(&instructionSet) - } else if is2200enabled { + if config.IsEnabled(config.GetEIP2200Transition, bn) && !config.IsEnabled(config.GetEIP2200DisableTransition, bn) { enable2200(&instructionSet) // Net metered SSTORE - https://eips.ethereum.org/EIPS/eip-2200 } return instructionSet diff --git a/core/vm/jump_table_test.go b/core/vm/jump_table_test.go deleted file mode 100644 index ce4624a0e707..000000000000 --- a/core/vm/jump_table_test.go +++ /dev/null @@ -1,88 +0,0 @@ -package vm - -import ( - "math/big" - "testing" - - "github.com/ethereum/go-ethereum/params" - "github.com/ethereum/go-ethereum/params/confp" - "github.com/ethereum/go-ethereum/params/vars" -) - -// TestECIP1086 shows that SLOAD constant gas is never bumped to 800 on Kotti the testnet config. -// Despite EIP2200 being installed and it's specification calling for 800 gas, this was incorrectly -// implemented in several clients, and thus, via ECIP1086, made into a "canonical mistake," which is tested -// here. -// This tests shows that although EIP2200 is implemented for a window (sloppyWindow), the gas cost for SLOAD does not change. -func TestECIP1086_Kotti(t *testing.T) { - kc := params.KottiChainConfig - - sloppyWindowStart := uint64(2_058_191) - sloppyWindowEnd := uint64(2_208_203) - - // Since "sloppy" EIP2200 did not actually modify SLOAD gas (and ECIP1086 makes this canonical), - // we want to show that SLOAD=200 gas is always used on Kotti whether EIP2200 is enabled or, afterwards, disabled. - wantSloadGasAlways := vars.SloadGasEIP150 - - if wantSloadGasAlways != vars.NetSstoreDirtyGas { - // https://corepaper.org/ethereum/fork/istanbul/#aztlan-fix - /* - Design Failure - - Aztlan decided to include EIP-2200 for SSTORE net gas metering. - However, it did not thoroughly consider the facts that EIP-2200 is designed with the full context - of Istanbul hard fork, whose pre-conditions that Aztlan does not fulfill. - - Net gas metering has a family of EIP specifications, including EIP-1283, EIP-1706 and EIP-2200. - The specifications contain a parameter of "dirty gas", which is charged when a storage value has - already been modified in the same transaction context. This value, as in the design intention, - is expected to always equal to the value of SLOAD gas cost. - In EIP-2200, this "dirty gas" is set to be 800, so as to accommodate EIP-1884’s gas cost change of SLOAD from 200 to 800. - However, Aztlan does not include EIP-1884 but only applied EIP-2200, - resulting in inconsistency in the EIP design intention, and may lead to unknown gas cost issues. - - This has also led to confusions in implementations. For example, a contributor previously merged an - incorrect version of Aztlan hard fork into Parity Ethereum. - This, if left unspotted, will lead to consensus split on the whole Ethereum Classic network. - - Recommendation: Remove EIP-2200 from the applied specification list, and add EIP-1283 with EIP-1706 instead. - */ - t.Fatal("'dirty gas' parameter should be same as SLOAD cost") - } - - for _, f := range confp.Forks(kc) { - for _, ft := range []uint64{f - 1, f, f + 1} { - - jt := instructionSetForConfig(kc, new(big.Int).SetUint64(ft)) - - // Yes, this logic could be simplified, but I want to show the window/post-window no-op verbosely. - if ft >= sloppyWindowStart && ft < sloppyWindowEnd { - if jt[SLOAD].constantGas != wantSloadGasAlways { - t.Error(ft, "bad gas", jt[SLOAD].constantGas) - } - - } else if ft >= sloppyWindowEnd { - // EIP1283 and EIP1706 are activated, but neither modify the SLOAD constant gas price. - if jt[SLOAD].constantGas != wantSloadGasAlways { - t.Error(ft, "bad gas 2", jt[SLOAD].constantGas) - } - } - } - } -} - -// TestECIP1086_ETCMainnet is a variant of TestECIP1086_Kotti, -// showing that SLOAD gas is never 800. -func TestECIP1086_ETCMainnet(t *testing.T) { - etc := params.ClassicChainConfig - - for _, f := range confp.Forks(etc) { - for _, ft := range []uint64{f - 1, f, f + 1} { - head := new(big.Int).SetUint64(ft) - jt := instructionSetForConfig(etc, head) - if jt[SLOAD].constantGas == vars.SloadGasEIP2200 && etc.IsEnabled(etc.GetEIP2200DisableTransition, head) { - t.Errorf("wrong gas") - } - } - } -} diff --git a/params/config_kotti.go b/params/config_kotti.go index d28b63e9c1c2..35a9abc12119 100644 --- a/params/config_kotti.go +++ b/params/config_kotti.go @@ -71,8 +71,6 @@ var ( EIP2028FBlock: big.NewInt(2058191), EIP2200FBlock: big.NewInt(2058191), // RePetersburg (== re-1283) - ECIP1086FBlock: big.NewInt(2058191), - // ECIP-1078, aka Phoenix Fix EIP2200DisableFBlock: big.NewInt(2_208_203), EIP1283FBlock: big.NewInt(2_208_203), diff --git a/params/config_mordor.go b/params/config_mordor.go index d19cb4d31b6f..a1104d6ae570 100644 --- a/params/config_mordor.go +++ b/params/config_mordor.go @@ -65,8 +65,6 @@ var ( EIP2028FBlock: big.NewInt(778507), EIP2200FBlock: big.NewInt(778507), // RePetersburg (== re-1283) - ECIP1086FBlock: big.NewInt(778507), - // ECIP-1078, aka Phoenix Fix EIP2200DisableFBlock: big.NewInt(976_231), EIP1283FBlock: big.NewInt(976_231), diff --git a/params/types/ctypes/configurator_iface.go b/params/types/ctypes/configurator_iface.go index 85d33ec152f6..809fee63c6f1 100644 --- a/params/types/ctypes/configurator_iface.go +++ b/params/types/ctypes/configurator_iface.go @@ -120,9 +120,6 @@ type ProtocolSpecifier interface { SetECIP1080Transition(n *uint64) error GetEIP1706Transition() *uint64 SetEIP1706Transition(n *uint64) error - - GetECIP1086Transition() *uint64 - SetECIP1086Transition(n *uint64) error } type Forker interface { diff --git a/params/types/genesisT/genesis.go b/params/types/genesisT/genesis.go index f19a3acf4e60..be8982f8c5c8 100644 --- a/params/types/genesisT/genesis.go +++ b/params/types/genesisT/genesis.go @@ -536,14 +536,6 @@ func (g Genesis) SetEIP1706Transition(n *uint64) error { return g.Config.SetEIP1706Transition(n) } -func (g Genesis) GetECIP1086Transition() *uint64 { - return g.Config.GetECIP1086Transition() -} - -func (g Genesis) SetECIP1086Transition(n *uint64) error { - return g.Config.SetECIP1086Transition(n) -} - func (g *Genesis) IsEnabled(fn func() *uint64, n *big.Int) bool { return g.Config.IsEnabled(fn, n) } diff --git a/params/types/goethereum/goethereum_configurator.go b/params/types/goethereum/goethereum_configurator.go index 55bb12870767..50ca0b7845c5 100644 --- a/params/types/goethereum/goethereum_configurator.go +++ b/params/types/goethereum/goethereum_configurator.go @@ -382,17 +382,6 @@ func (c *ChainConfig) SetEIP1706Transition(n *uint64) error { return nil } -func (c *ChainConfig) GetECIP1086Transition() *uint64 { - return nil -} - -func (c *ChainConfig) SetECIP1086Transition(n *uint64) error { - if n == nil { - return nil - } - return ctypes.ErrUnsupportedConfigFatal -} - func (c *ChainConfig) IsEnabled(fn func() *uint64, n *big.Int) bool { f := fn() if f == nil || n == nil { diff --git a/params/types/multigeth/chain_config.go b/params/types/multigeth/chain_config.go index daf8a40f5251..e4af0b1eb6da 100644 --- a/params/types/multigeth/chain_config.go +++ b/params/types/multigeth/chain_config.go @@ -170,8 +170,6 @@ type MultiGethChainConfig struct { ECIP1017EraRounds *big.Int `json:"ecip1017EraRounds,omitempty"` // ECIP1017 era rounds ECIP1080FBlock *big.Int `json:"ecip1080FBlock,omitempty"` - ECIP1086FBlock *big.Int `json:"ecip1086FBlock,omitempty"` - DisposalBlock *big.Int `json:"disposalBlock,omitempty"` // Bomb disposal HF block SocialBlock *big.Int `json:"socialBlock,omitempty"` // Ethereum Social Reward block EthersocialBlock *big.Int `json:"ethersocialBlock,omitempty"` // Ethersocial Reward block diff --git a/params/types/multigeth/chain_config_configurator.go b/params/types/multigeth/chain_config_configurator.go index 0a6efa517410..4c0d824b5058 100644 --- a/params/types/multigeth/chain_config_configurator.go +++ b/params/types/multigeth/chain_config_configurator.go @@ -372,15 +372,6 @@ func (c *MultiGethChainConfig) SetEIP1706Transition(n *uint64) error { return nil } -func (c *MultiGethChainConfig) GetECIP1086Transition() *uint64 { - return bigNewU64(c.ECIP1086FBlock) -} - -func (c *MultiGethChainConfig) SetECIP1086Transition(n *uint64) error { - c.ECIP1086FBlock = setBig(c.ECIP1086FBlock, n) - return nil -} - func (c *MultiGethChainConfig) IsEnabled(fn func() *uint64, n *big.Int) bool { f := fn() if f == nil || n == nil { diff --git a/params/types/multigethv0/multigethv0_chain_config_configurator.go b/params/types/multigethv0/multigethv0_chain_config_configurator.go index 948bd057562c..30b27aefa31b 100644 --- a/params/types/multigethv0/multigethv0_chain_config_configurator.go +++ b/params/types/multigethv0/multigethv0_chain_config_configurator.go @@ -385,17 +385,6 @@ func (c *ChainConfig) SetEIP1706Transition(n *uint64) error { return ctypes.ErrUnsupportedConfigFatal } -func (c *ChainConfig) GetECIP1086Transition() *uint64 { - return nil -} - -func (c *ChainConfig) SetECIP1086Transition(n *uint64) error { - if n == nil { - return nil - } - return ctypes.ErrUnsupportedConfigFatal -} - func (c *ChainConfig) IsEnabled(fn func() *uint64, n *big.Int) bool { f := fn() if f == nil || n == nil { diff --git a/params/types/parity/parity_configurator.go b/params/types/parity/parity_configurator.go index 6de3439f6b99..0ef50256a889 100644 --- a/params/types/parity/parity_configurator.go +++ b/params/types/parity/parity_configurator.go @@ -477,17 +477,6 @@ func (c *ParityChainSpec) SetEIP1706Transition(n *uint64) error { return nil } -func (c *ParityChainSpec) GetECIP1086Transition() *uint64 { - return nil -} - -func (c *ParityChainSpec) SetECIP1086Transition(n *uint64) error { - if n == nil { - return nil - } - return ctypes.ErrUnsupportedConfigFatal -} - func (spec *ParityChainSpec) IsEnabled(fn func() *uint64, n *big.Int) bool { f := fn() if f == nil || n == nil {