core/vm: 64 bit gas instructions #3514

obscuren · 2017-01-04T19:18:24Z

This PR changes all gas instructions from *big.Int to uint64, a new theoretical limit.

It also implements a new *big.Int instruction pool, which contains reusable *big.Ints. Instructions found in core/vm/instructions.go do no longer initialise new big.Ints but instead call intPool.Get() returning either a reusable integer -- beware this may contain an old value -- or new(big.Int) if the pool is empty. I've decided to implement my own pool instead of using the sync.Pool because I do not require a thread safe pool and I do not want the typecasting overhead required with the `sync.Pool.

mention-bot · 2017-01-04T19:18:26Z

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

obscuren · 2017-01-13T11:40:05Z

PR vs master

benchmark                         old ns/op     new ns/op     delta
BenchmarkVmAckermann32Tests-4     3214842       2171613       -32.45%
BenchmarkVmFibonacci16Tests-4     19069610      13485337      -29.28%

benchmark                         old allocs     new allocs     delta
BenchmarkVmAckermann32Tests-4     39283          34590          -11.95%
BenchmarkVmFibonacci16Tests-4     228242         203283         -10.94%

benchmark                         old bytes     new bytes     delta
BenchmarkVmAckermann32Tests-4     1478684       1340153       -9.37%
BenchmarkVmFibonacci16Tests-4     8444250       7935278       -6.03%

PR vs 1.5.5

benchmark                         old ns/op     new ns/op     delta
BenchmarkVmAckermann32Tests-4     5832647       2171613       -62.77%
BenchmarkVmFibonacci16Tests-4     32728077      13485337      -58.80%

benchmark                         old allocs     new allocs     delta
BenchmarkVmAckermann32Tests-4     89996          34590          -61.56%
BenchmarkVmFibonacci16Tests-4     537210         203283         -62.16%

benchmark                         old bytes     new bytes     delta
BenchmarkVmAckermann32Tests-4     3355377       1340153       -60.06%
BenchmarkVmFibonacci16Tests-4     19962075      7935278       -60.25%

holiman · 2017-01-23T08:25:51Z

Regarding the intpool... It looks a bit 'dangerous', in that it seems relatively easy to make a mistake there. I feel it would be good to have some additional tests for coverage; not only to ensure that no consensus error occurs, but specifically it would be good to run the VM in 'checked' mode where it performs additional tests on the pool.

Examples of what could be done in checked mode:

Aggressively set a re-pooled bigint to e.g -42. Should make it more prone to fail in case of misuse.
Aggressively validate that all pooled bigints are -42, to detect outside modification (e.g. after each pc step or after each transaction).

I'm also concerned whether journal reverts can somehow hold a reference to a pooled bigint. And how we can ensure that such case does not occur in the future (because from what I've seen now, all journal entries appear to instantiate new bigints).

Third, it probably makes sense to have some rudimentary counter on the pool - it does not appear limited? If you run a node for several weeks, and there's a skewed balance (if there are more pushing than popping), couldn't that eventually become a problem.. ? The PUSH instruction, for example, creates new bigints (https://github.com/obscuren/go-ethereum/blob/gas64/core/vm/instructions.go#L709) instead of reusing existing ones.

holiman · 2017-01-23T08:35:00Z

common/math/integer.go

+	return x + y, y > gmath.MaxUint64-x
+}
+
+// IsAddSafe returns whether the multiplication is safe and does not overflow.


// IsAddSafe returns whether the multiplication is safe and does not overflow.

Should be

// SafeMul returns multiplication result and whether overflow occurred.

holiman · 2017-01-23T08:35:41Z

common/math/integer.go

+	return x - y, x < y
+}
+
+// IsAddSafe returns whether the addition is safe and does not overflow.


// IsAddSafe returns whether the addition is safe and does not overflow.

Should be
// SafeAdd returns the result and whether overflow occurred.

holiman · 2017-01-23T08:36:11Z

common/math/integer.go

+ * NOTE: The following methods need to be optimised using either bit checking or asm
+ */
+
+// IsMinSafe returns whether the subtraction is safe and does not overflow.


Should be

// SafeSub returns subtraction result and whether overflow occurred

holiman · 2017-01-23T08:39:04Z

core/state_transition.go

 	}
 }

 // ApplyMessage computes the new state by applying the given message
-// against the old state within the environment.
+// against the old state within the evmironment.


environment

holiman · 2017-01-23T08:39:45Z

core/state_transition.go

@@ -209,34 +212,40 @@ func (self *StateTransition) preCheck() (err error) {
 	return nil
 }

-// TransitionDb will move the state by applying the message against the given environment.
+// TransitionDb will move the state by applying the message against the given evmironment.


environment

obscuren · 2017-01-23T08:44:33Z

I feel it would be good to have some additional tests for coverage

I agree, I'll work on those.

I'm also concerned whether journal reverts can somehow hold a reference to a pooled bigint

If that were the case we'd already have issues (*). I'm going to assume it's not the case and that our current network would have already had ran in to any issues.

Third, it probably makes sense to have some rudimentary counter on the pool - it does not appear limited?

(*) The int pool is being recreated each time a interpreter is instantiated, which happens for each transaction. I'm not sure how the pool can grow very large either. Ints must come from either a PUSH instruction or the auxiliary instructions.

holiman · 2017-01-23T08:42:13Z

core/vm/contracts.go

-	n.Mul(n, params.Sha256WordGas)
-	return n.Add(n, params.Sha256Gas)
+func (c *sha256) RequiredGas(inputSize int) uint64 {
+	return uint64(inputSize+31)/32*params.Sha256WordGas + params.Sha256Gas


Shouldn't these use the SafeXXX-functions?

Technically yes, but you'd need a significant amount of gas to make this overfow. There simply isn't enough ether to make it significant. Even if you'd pass it zeros only, you'd still need an enormous amount amount of ether to make the initial call.

Right, because the memory expansion happens first... If there was a precompile which actually was calleable with e.g. a "num_rounds" of hashing, it could be an issue, but you're right.

That and the initial call (tx) you pay per byte for the transaction.

holiman · 2017-01-23T08:42:32Z

core/vm/contracts.go

-	n.Mul(n, params.Ripemd160WordGas)
-	return n.Add(n, params.Ripemd160Gas)
+func (c *ripemd160) RequiredGas(inputSize int) uint64 {
+	return uint64(inputSize+31)/32*params.Ripemd160WordGas + params.Ripemd160Gas


holiman · 2017-01-23T08:42:45Z

core/vm/contracts.go

-
-	return n.Add(n, params.IdentityGas)
+func (c *dataCopy) RequiredGas(inputSize int) uint64 {
+	return uint64(inputSize+31)/32*params.IdentityWordGas + params.IdentityGas


holiman · 2017-01-23T08:46:14Z

core/vm/gas.go

-	}
-	return nil
+func toWordSize(size uint64) uint64 {
+	return (size + 31) / 32


Is this function still used ? If so, should check for overflow... (?)

The same as above. The size would have to be very large for it to become an issue and there isn't enough ether for this. This number is always the results of the furthest memory byte, not the extra bytes added.

On a second thought, I think it might be worth it to call this unsafeToWordSize indicating caution.

holiman · 2017-01-23T08:48:06Z

core/vm/interpreter.go

+			//fmt.Printf("%04d: %8v    cost = %-8d stack = %-8d ERR = %v\n", pc, op, cost, stack.len(), err)
+			// TODO update the tracer
+			g, c := new(big.Int).SetUint64(contract.Gas), new(big.Int).SetUint64(cost)
+			evm.cfg.Tracer.CaptureState(evm.env, pc, op, g, c, mem, stack, contract, evm.env.depth, err)


You've added a comment to update the tracer, but as it is, the tracer could potentially hold on to bigints from the pool, no ? So that should go in before it is merged.

Update the tracer to use uint64, I meant. I'm not sure how the tracer can retain pooled integers? If anything, it could already hold on to integers by accessing the stack directly.

obscuren · 2017-01-25T20:17:34Z

EVM integer pool integrity verifier has been added @holiman: go build ./cmd/geth -tags VERIFY_EVM_INTEGER_POOL. Bear in mind, it's very slow as each and every integer in the pool needs to be verified (big int comparison) for each instruction.

holiman · 2017-01-26T08:07:15Z

core/vm/intpool.go

+// can be reused for all big.Int operations.
+type intPool struct {
+	pool  *Stack
+	debug bool


Maybe I'm blind.. but I don't see any usage of debug?

Ah, that needs to be removed. Thanks

obscuren · 2017-01-30T11:58:09Z

core/state_transition.go

+	// TODO convert to uint64
+	intrinsicGas := IntrinsicGas(self.data, contractCreation, homestead)
+	if intrinsicGas.BitLen() > 64 {
+		return nil, nil, nil, vm.ErrOutOfGas


Use InvalidTxError

obscuren · 2017-01-30T12:03:47Z

core/vm/contracts.go

-	n := big.NewInt(int64(inputSize+31) / 32)
-	n.Mul(n, params.Sha256WordGas)
-	return n.Add(n, params.Sha256Gas)
+func (c *sha256) RequiredGas(inputSize int) uint64 {


Leave in code comment about why no overflow checking is performed.

obscuren · 2017-01-30T12:13:04Z

core/vm/evm.go

 		evm.StateDB.RevertToSnapshot(snapshot)
+		contract.UseGas(contract.Gas)


Remove this line

obscuren · 2017-01-30T12:13:48Z

core/vm/gas.go

-	GasMidStep     = big.NewInt(8)
-	GasSlowStep    = big.NewInt(10)
-	GasExtStep     = big.NewInt(20)
+const (


Make these typed

obscuren · 2017-01-30T12:24:01Z

core/vm/gas.go

+	if gasTable.CreateBySuicide > 0 {
+		availableGas = availableGas - base
+		gas := availableGas - availableGas/64
+		if callCost.BitLen() > 64 || gas < callCost.Uint64() {


Add comment on the BitLen check

obscuren · 2017-01-30T14:00:58Z

core/vm/instructions.go

 		return nil, fmt.Errorf("invalid opcode %x", DELEGATECALL)
 	}

-	gas, to, inOffset, inSize, outOffset, outSize := stack.pop(), stack.pop(), stack.pop(), stack.pop(), stack.pop(), stack.pop()
+	gas, to, inOffset, inSize, outOffset, outSize := stack.pop().Uint64(), stack.pop(), stack.pop(), stack.pop(), stack.pop(), stack.pop()


Check 64bit unmetered

obscuren · 2017-01-30T14:15:51Z

core/vm/jump_table.go

 	"math/big"

 	"github.com/ethereum/go-ethereum/params"
 )

 type (
 	executionFunc       func(pc *uint64, env *EVM, contract *Contract, memory *Memory, stack *Stack) ([]byte, error)
-	gasFunc             func(params.GasTable, *EVM, *Contract, *Stack, *Memory, *big.Int) *big.Int
+	gasFunc             func(params.GasTable, *EVM, *Contract, *Stack, *Memory, uint64) (uint64, error)


add paramater name or comment

obscuren · 2017-01-30T14:18:37Z

core/vm/stack_table.go

@@ -12,8 +12,8 @@ func makeStackFunc(pop, push int) stackValidationFunc {
 			return err
 		}

-		if push > 0 && int64(stack.len()-pop+push) > params.StackLimit.Int64() {
-			return fmt.Errorf("stack limit reached %d (%d)", stack.len(), params.StackLimit.Int64())
+		if push > 0 && int64(stack.len()-pop+push) > int64(params.StackLimit) {


What was this again @fjl ...

obscuren · 2017-01-30T14:20:56Z

params/protocol_params.go


 	MaxCodeSize = 24576
 )
+
+var (
+	GasLimitBoundDivisor   *big.Int = big.NewInt(1024)                  // The bound divisor of the gas limit, used in update calculations.


remove *big.Int type

obscuren · 2017-01-30T14:30:17Z

core/vm/int_pool_verifier.go

+
+const verifyPool = true
+
+var checkVal = big.NewInt(-42)


Move to int_pool.go and use this variable for i.Set(checkVal)

obscuren · 2017-02-01T12:49:10Z

Before anyone merges, ping me. I want to clean up the commit message.

fjl · 2017-02-02T10:33:40Z

internal/ethapi/api.go

+	}()
+
+	// Setup the gas pool (also for unmetered requests)
+	// and apple the message.


apple -> apply

fjl

The code looks OK. It's hard to see whether all possible overflow situations are covered though.

I'm running a full sync in checked mode, will report back here when it's done.

Reworked the EVM gas instructions to use 64bit integers rather than arbitrary size big ints. All gas operations, be it additions, multiplications or divisions, are checked and guarded against 64 bit integer overflows. In additon, most of the protocol paramaters in the params package have been converted to uint64 and are now constants rather than variables. * common/math: added overflow check ops * core: vmenv, env renamed to evm * eth, internal/ethapi, les: unmetered eth_call and cancel methods * core/vm: implemented big.Int pool for evm instructions * core/vm: unexported intPool methods & verification methods * core/vm: added memoryGasCost overflow check and test

…m#3514)" This reverts commit 8b57c49.

Revert "params: core, core/vm, miner: 64bit gas instructions (#3514)"

Reworked the EVM gas instructions to use 64bit integers rather than arbitrary size big ints. All gas operations, be it additions, multiplications or divisions, are checked and guarded against 64 bit integer overflows. In additon, most of the protocol paramaters in the params package have been converted to uint64 and are now constants rather than variables. * common/math: added overflow check ops * core: vmenv, env renamed to evm * eth, internal/ethapi, les: unmetered eth_call and cancel methods * core/vm: implemented big.Int pool for evm instructions * core/vm: unexported intPool methods & verification methods * core/vm: added memoryGasCost overflow check and test

…m#3514)" This reverts commit 8b57c49.

obscuren added core labels Jan 4, 2017

obscuren added this to the 1.5.7 milestone Jan 4, 2017

obscuren force-pushed the gas64 branch 4 times, most recently from 7ff8ada to 4e4775a Compare January 9, 2017 23:00

obscuren force-pushed the gas64 branch from 6e15ceb to 8534153 Compare January 13, 2017 11:39

obscuren added review and removed in progress labels Jan 13, 2017

karalabe modified the milestones: 1.5.8, 1.5.7 Jan 16, 2017

obscuren force-pushed the gas64 branch from 8534153 to 1e0df90 Compare January 19, 2017 10:25

obscuren requested review from karalabe and fjl January 22, 2017 21:16

holiman reviewed Jan 23, 2017

View reviewed changes

obscuren force-pushed the gas64 branch 7 times, most recently from b70dc5c to 0e8c41e Compare January 25, 2017 13:57

obscuren force-pushed the gas64 branch from 023f918 to ae3fe15 Compare January 25, 2017 20:18

holiman reviewed Jan 26, 2017

View reviewed changes

obscuren modified the milestones: 1.5.9, 1.5.8 Jan 27, 2017

obscuren force-pushed the gas64 branch from 34985e2 to 7b8183c Compare January 30, 2017 11:42

obscuren commented Jan 30, 2017

View reviewed changes

obscuren force-pushed the gas64 branch 2 times, most recently from e4e93da to f83884e Compare January 31, 2017 15:49

fjl reviewed Feb 2, 2017

View reviewed changes

internal/ethapi/api.go Outdated

}()

// Setup the gas pool (also for unmetered requests)

// and apple the message.

Copy link

Contributor

fjl Feb 2, 2017

Choose a reason for hiding this comment

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

apple -> apply

fjl approved these changes Feb 2, 2017

View reviewed changes

obscuren force-pushed the gas64 branch from 00d0644 to d3c31b5 Compare February 2, 2017 12:10

obscuren merged commit 8b57c49 into ethereum:master Feb 2, 2017

obscuren removed the review label Feb 2, 2017

obscuren mentioned this pull request Feb 8, 2017

Revert "core/vm: 64 bit gas instructions" #3657

Closed

karalabe removed this from the 1.5.9 milestone Feb 13, 2017

obscuren added a commit to obscuren/go-ethereum that referenced this pull request Feb 13, 2017

Revert "params: core, core/vm, miner: 64bit gas instructions (ethereu…

57f4e90

…m#3514)" This reverts commit 8b57c49.

karalabe added a commit that referenced this pull request Feb 13, 2017

Merge pull request #3668 from obscuren/revert-gas64

0850f68

Revert "params: core, core/vm, miner: 64bit gas instructions (#3514)"

farazdagi pushed a commit to status-im/go-ethereum that referenced this pull request Feb 24, 2017

Revert "params: core, core/vm, miner: 64bit gas instructions (ethereu…

d73b601

…m#3514)" This reverts commit 8b57c49.

		evm.StateDB.RevertToSnapshot(snapshot)
		contract.UseGas(contract.Gas)

core/vm: 64 bit gas instructions #3514

core/vm: 64 bit gas instructions #3514

Conversation

obscuren commented Jan 4, 2017 • edited Loading

mention-bot commented Jan 4, 2017

obscuren commented Jan 13, 2017 • edited Loading

holiman commented Jan 23, 2017

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 Jan 23, 2017

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 Jan 25, 2017

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

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 Feb 1, 2017

Choose a reason for hiding this comment

fjl left a comment

Choose a reason for hiding this comment

obscuren commented Jan 4, 2017 •

edited

Loading

obscuren commented Jan 13, 2017 •

edited

Loading