core, core/vm: added gas price variance table (EIP #150) #3111

obscuren · 2016-10-09T15:42:27Z

This implements 1b of ethereum/EIPs#150 by adding a new GasTable which must be
returned from the RuleSet config method. This table is used to determine
the gas prices for the current epoch.

Please note that when the CreateBySuicide gas price is set it is assumed
that we're in the new epoch phase.

In addition this PR will serve as temporary basis while refactorisation
in being done in the EVM64 PR, which will substentially overhaul the gas
price code.

mention-bot · 2016-10-09T15:42:28Z

@obscuren, thanks for your PR! By analyzing the history of the files in this pull request, we identified @karalabe, @Gustav-Simonsson and @Arachnid to be potential reviewers.

obscuren · 2016-10-11T10:03:32Z

@fjl @karalabe @Arachnid PTAL. Left in progress because number needs to be determined.

karalabe

Review in progress

karalabe · 2016-10-12T10:17:48Z

core/config.go

@@ -35,6 +35,8 @@ type ChainConfig struct {
 	DAOForkBlock   *big.Int `json:"daoForkBlock"`   // TheDAO hard-fork switch block (nil = no fork)
 	DAOForkSupport bool     `json:"daoForkSupport"` // Whether the nodes supports or opposes the DAO hard-fork

+	HomesteadGasPriceBlock *big.Int `json:"homesteadGasPriceBlock"` // homestead gas price switch block


Since this is essentially an API, I'd suggest a slight name change to better reflect what's happening. I.e. homesteadGasRepriceBlock. This would make it clearer later on what happened here.

Also please add to the comment what the default (nil and 0) values mean.

karalabe · 2016-10-12T10:18:37Z

core/config.go

@@ -45,3 +47,37 @@ func (c *ChainConfig) IsHomestead(num *big.Int) bool {
 	}
 	return num.Cmp(c.HomesteadBlock) >= 0
 }
+
+func (c *ChainConfig) GasTable(num *big.Int) vm.GasTable {


Please add docs here and to the variable declarations below. Also please emphasize that the caller should not change any values as these are global ones.

karalabe · 2016-10-12T10:25:25Z

core/config.go

+}
+
+var (
+	GasChartHomestead = vm.GasTable{


Could you please move these "constants" to the params package. All other protocol parameters are there, including fork configs so I'd expect to find these there, not in the core package.

karalabe · 2016-10-12T10:26:36Z

core/config.go

+		CreateBySuicide: nil,
+	}
+
+	GasChartHomesteadGasPriceFork = vm.GasTable{


I'd recommend naming this similarly to my above suggestion, GasReprice opposed to simply Price. Also please add a link in a doc to the actual EIP with all the rationale so that anyone reading up on this and wondering where the values come from has an explanation.

karalabe · 2016-10-12T10:49:53Z

core/vm/environment.go

@@ -26,6 +26,8 @@ import (
 // execution of the EVM instructions (e.g. whether it's homestead)
 type RuleSet interface {
 	IsHomestead(*big.Int) bool
+	// GasChart returns the gas prices for this epoch


As agreed, please remove mentions of epoch as they are too easy to be confused by the Ethereum notion of 30K block epochs.

karalabe · 2016-10-12T10:57:00Z

core/vm/runtime/runtime.go

@@ -31,6 +31,18 @@ import (
 type ruleSet struct{}

 func (ruleSet) IsHomestead(*big.Int) bool { return true }
+func (ruleSet) GasTable(*big.Int) vm.GasTable {


If you move these constants from core to params as suggested somewhere above, then you could reuse the same global variable and not have to redeclare the params here (potentially causing issues later down the line).

karalabe · 2016-10-12T10:57:31Z

core/vm/util_test.go

@@ -23,3 +23,17 @@ type ruleSet struct {
 }

 func (r ruleSet) IsHomestead(n *big.Int) bool { return n.Cmp(r.hs) >= 0 }
+func (r ruleSet) GasTable(*big.Int) GasTable {


Same here, you could reuse these numbers from a global variable in params.

Arachnid · 2016-10-12T12:31:33Z

core/vm/gas.go

-	CALLCODE:     {7, params.CallGas, 1},
-	DELEGATECALL: {6, params.CallGas, 1},
-	JUMPDEST:     {0, params.JumpdestGas, 0},
+	//BALANCE:      {1, GasExtStep, 1},


Please don't include commented out lines in the patch!

That was an accident. Will remove.

Arachnid · 2016-10-12T12:32:07Z

core/vm/gas.go

-	DELEGATECALL: {6, params.CallGas, 1},
-	JUMPDEST:     {0, params.JumpdestGas, 0},
+	//BALANCE:      {1, GasExtStep, 1},
+	BALANCE: {1, Zero, 1},


Is it possible to use a placeholder other than Zero? nil seems more suitable.

Arachnid · 2016-10-12T12:33:03Z

core/vm/gas_table.go

+
+import "math/big"
+
+type GasTable struct {


Why not use a map keyed by opcode, and handle CreateBySuicide separately?

Arachnid · 2016-10-12T12:38:09Z

core/vm/vm.go

@@ -234,7 +236,7 @@ func (evm *EVM) Run(contract *Contract, input []byte) (ret []byte, err error) {

 // calculateGasAndSize calculates the required given the opcode and stack items calculates the new memorysize for
 // the operation. This does not reduce gas or resizes the memory.
-func calculateGasAndSize(env Environment, contract *Contract, caller ContractRef, op OpCode, statedb Database, mem *Memory, stack *Stack) (*big.Int, *big.Int, error) {
+func calculateGasAndSize(gasTable GasTable, env Environment, contract *Contract, caller ContractRef, op OpCode, statedb Database, mem *Memory, stack *Stack) (*big.Int, *big.Int, error) {


Since you're passing in env here, wouldn't it make more sense for it to fetch the gasTable itself?

Because that require us to call the GasTable method again which causes additional overhead and big int comparison.

Arachnid · 2016-10-12T12:38:54Z

core/vm/vm.go

+		// (availableGas - gas) * 63 / 64
+		// We replace the stack item so that it's available when the opCall instruction is
+		// called. This information is otherwise lost due to the dependency on *current*
+		// available cost.


s/cost/gas/

Gustav-Simonsson · 2016-10-12T17:09:23Z

core/vm/gas.go

+		g.Div(g, n64)
+		if g.Cmp(callCost) < 0 {
+			return g
+		}


EIP150 currently says:

Define "all but one 64th" of N as N - floor(N / 64)

This code implements (N * 63) // 64 which compared to the EIP150 formula is off-by-one for some values: https://play.golang.org/p/QUbnDxCcq_

Either this code or EIP150 needs to change.

The code needs to change. We changed it ~4h ago.

it being the spec :)

Updated the code now as well. PTAL

karalabe · 2016-10-13T08:39:16Z

params/util.go

-	MainNetHomesteadBlock = big.NewInt(1150000) // Mainnet homestead block
+	TestNetHomesteadBlock           = big.NewInt(494000)  // Testnet homestead block
+	MainNetHomesteadBlock           = big.NewInt(1150000) // Mainnet homestead block
+	TestNetHomesteadGasRepriceBlock = big.NewInt(2648000) // Main net gas reprice block


Doc -> Testnet

obscuren · 2016-10-14T13:28:11Z

core/vm/gas.go

+// The cost of gas was changed during the homestead price change HF. To allow for EIP150
+// to be implemented. The returned gas is gas - base * 63 / 64.
+func callGas(gasTable params.GasTable, availableGas, base, callCost *big.Int) *big.Int {
+	if gasTable.CreateBySuicide != nil {


add comment @karalabe

obscuren · 2016-10-14T13:34:07Z

core/vm/instructions.go

-	contract.UseGas(contract.Gas)
+	if env.RuleSet().GasTable(env.BlockNumber()).CreateBySuicide != nil {
+		gas.Div(gas, n64)
+		gas = gas.Sub(contract.Gas, gas)


remove assignment

obscuren · 2016-10-14T13:41:03Z

params/gas_table.go

+
+import "math/big"
+
+type GasTable struct {


obscuren · 2016-10-14T13:42:15Z

params/protocol_params.go

@@ -71,4 +71,5 @@ var (
 	SuicideRefundGas     = big.NewInt(24000)  // Refunded following a suicide operation.
 	MemoryGas            = big.NewInt(3)      // Times the address of the (highest referenced byte in memory + 1). NOTE: referencing happens on read, write and in instructions such as RETURN and CALL.
 	TxDataNonZeroGas     = big.NewInt(68)     // Per byte of data attached to a transaction that is not equal to zero. NOTE: Not payable on data of calls between transactions.
+


This implements 1b & 1c of EIP150 by adding a new GasTable which must be returned from the RuleSet config method. This table is used to determine the gas prices for the current epoch. Please note that when the CreateBySuicide gas price is set it is assumed that we're in the new epoch phase. In addition this PR will serve as temporary basis while refactorisation in being done in the EVM64 PR, which will substentially overhaul the gas price code.

karalabe

SGTM 👍

obscuren added the core label Oct 9, 2016

obscuren added the in progress label Oct 9, 2016

obscuren force-pushed the gas-price-fork branch 4 times, most recently from 7284c57 to a3e87ea Compare October 11, 2016 10:02

obscuren added the review label Oct 11, 2016

obscuren force-pushed the gas-price-fork branch 2 times, most recently from 80e4bdc to c65acc1 Compare October 12, 2016 09:39

karalabe reviewed Oct 12, 2016

View reviewed changes

Arachnid reviewed Oct 12, 2016

View reviewed changes

Gustav-Simonsson reviewed Oct 12, 2016

View reviewed changes

obscuren force-pushed the gas-price-fork branch 2 times, most recently from c329845 to 416d8d7 Compare October 12, 2016 19:34

karalabe reviewed Oct 13, 2016

View reviewed changes

obscuren force-pushed the gas-price-fork branch from b4a9d3b to 6a484b2 Compare October 13, 2016 15:37

pdaian mentioned this pull request Oct 13, 2016

Long-term gas cost changes for IO-heavy operations to mitigate transaction spam attacks ethereum/EIPs#150

Closed

obscuren commented Oct 14, 2016

View reviewed changes

params/gas_table.go

import "math/big"

type GasTable struct {

Copy link

Contributor Author

obscuren Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc it

obscuren commented Oct 14, 2016

View reviewed changes

obscuren force-pushed the gas-price-fork branch from b1d62fa to 82daefa Compare October 14, 2016 14:13

obscuren force-pushed the gas-price-fork branch from 82daefa to 64af2aa Compare October 14, 2016 16:09

karalabe approved these changes Oct 14, 2016

View reviewed changes

karalabe merged commit 81b01f1 into ethereum:develop Oct 14, 2016

obscuren removed in progress labels Oct 14, 2016

This was referenced Oct 18, 2016

Bad block #446189 ... gas used error... Synchronisation failed: peer is unknown or unhealthy #1117

Closed

Bad block #1422330 gas used error (2278520 / 2278360). #3183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core, core/vm: added gas price variance table (EIP #150) #3111

core, core/vm: added gas price variance table (EIP #150) #3111

obscuren commented Oct 9, 2016 •

edited

Loading

mention-bot commented Oct 9, 2016

obscuren commented Oct 11, 2016

karalabe left a comment

karalabe Oct 12, 2016

karalabe Oct 12, 2016 •

edited

Loading

karalabe Oct 12, 2016

karalabe Oct 12, 2016

karalabe Oct 12, 2016

karalabe Oct 12, 2016

karalabe Oct 12, 2016

Arachnid Oct 12, 2016

obscuren Oct 12, 2016

Arachnid Oct 12, 2016

Arachnid Oct 12, 2016

Arachnid Oct 12, 2016

obscuren Oct 12, 2016

Arachnid Oct 12, 2016

Gustav-Simonsson Oct 12, 2016 •

edited

Loading

obscuren Oct 12, 2016

obscuren Oct 12, 2016

obscuren Oct 12, 2016

karalabe Oct 13, 2016

obscuren Oct 14, 2016

obscuren Oct 14, 2016

obscuren Oct 14, 2016

obscuren Oct 14, 2016

karalabe left a comment

core, core/vm: added gas price variance table (EIP #150) #3111

core, core/vm: added gas price variance table (EIP #150) #3111

Conversation

obscuren commented Oct 9, 2016 • edited Loading

mention-bot commented Oct 9, 2016

obscuren commented Oct 11, 2016

karalabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe Oct 12, 2016 • 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

Gustav-Simonsson Oct 12, 2016 • 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

karalabe left a comment

Choose a reason for hiding this comment

obscuren commented Oct 9, 2016 •

edited

Loading

karalabe Oct 12, 2016 •

edited

Loading

Gustav-Simonsson Oct 12, 2016 •

edited

Loading