Skip to content
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.

Sign up for GitHub

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

core, light, params: implement eip2028 #19931

Merged
merged 5 commits into from
Aug 14, 2019
Merged

core, light, params: implement eip2028 #19931

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
acad147
core, light, params: implement eip2028
rjl493456442 Aug 9, 2019
80bb52c
core, light: address comments
rjl493456442 Aug 9, 2019
1b34f35
core: address comments
rjl493456442 Aug 13, 2019
4c7d373
tests: disable Istanbul tx tests (until updated)
holiman Aug 14, 2019
d94d5dd
core: address comment
rjl493456442 Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func genValueTx(nbytes int) func(int, *BlockGen) {
return func(i int, gen *BlockGen) {
toaddr := common.Address{}
data := make([]byte, nbytes)
gas, _ := IntrinsicGas(data, false, false)
gas, _ := IntrinsicGas(data, false, false, false)
tx, _ := types.SignTx(types.NewTransaction(gen.TxNonce(benchRootAddr), toaddr, big.NewInt(1), gas, nil, data), types.HomesteadSigner{}, benchRootKey)
gen.AddTx(tx)
}
Expand Down
13 changes: 9 additions & 4 deletions core/state_transition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ type Message interface {
}

// IntrinsicGas computes the 'intrinsic gas' for a message with the given data.
func IntrinsicGas(data []byte, contractCreation, homestead bool) (uint64, error) {
func IntrinsicGas(data []byte, contractCreation, homestead bool, istanbul bool) (uint64, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

It's really nasty to pass a batch of fork indicators. What about using a dynasty?

Copy link
Member

Choose a reason for hiding this comment

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

We could pass the ChainConfig perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

or the chainRules

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but we use this function in some tests, so it seems weird to construct the chainRule, maybe worse than homestead, istanbul. So what's your prefer? ChainRules can make normal path code nicer, but test is a bit dirty

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I agree we can have flags. But actually, I'd personally prefer to have not istanbull bool but instead eip2028Active bool (and possibly eip155Active bool).

// Set the starting gas for the raw transaction
var gas uint64
if contractCreation && homestead {
Expand All @@ -94,10 +94,14 @@ func IntrinsicGas(data []byte, contractCreation, homestead bool) (uint64, error)
}
}
// Make sure we don't exceed uint64 for all data combinations
if (math.MaxUint64-gas)/params.TxDataNonZeroGas < nz {
nonZeroGas := params.TxDataNonZeroGasFrontier
if istanbul {
nonZeroGas = params.TxDataNonZeroGasEIP2028
}
if (math.MaxUint64-gas)/nonZeroGas < nz {
return 0, vm.ErrOutOfGas
}
gas += nz * params.TxDataNonZeroGas
gas += nz * nonZeroGas

z := uint64(len(data)) - nz
if (math.MaxUint64-gas)/params.TxDataZeroGas < z {
Expand Down Expand Up @@ -187,10 +191,11 @@ func (st *StateTransition) TransitionDb() (ret []byte, usedGas uint64, failed bo
msg := st.msg
sender := vm.AccountRef(msg.From())
homestead := st.evm.ChainConfig().IsHomestead(st.evm.BlockNumber)
istanbul := st.evm.ChainConfig().IsIstanbul(st.evm.BlockNumber)
contractCreation := msg.To() == nil

// Pay intrinsic gas
gas, err := IntrinsicGas(st.data, contractCreation, homestead)
gas, err := IntrinsicGas(st.data, contractCreation, homestead, istanbul)
if err != nil {
return nil, 0, false, err
}
Expand Down
14 changes: 13 additions & 1 deletion core/tx_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ type TxPool struct {
signer types.Signer
mu sync.RWMutex

homestead bool // Fork indicator whether we are in the homestead stage.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't include homestead here. Strictly speaking this code is more correct, because if someone runs a Frontier chain, the pool will act weirdly. That said, I don't think we can afford to maintain all the quirks of all past forks indefinitely. The EVM must have the quirks of course because it's consensus, but we don't expect anyone to run a frontier txpool, so lets just not complicate things.

istanbul bool // Fork indicator whether we are in the istanbul stage.

currentState *state.StateDB // Current state in the blockchain head
pendingNonces *txNoncer // Pending state tracking virtual nonces
currentMaxGas uint64 // Current gas limit for transaction caps
Expand Down Expand Up @@ -540,7 +543,7 @@ func (pool *TxPool) validateTx(tx *types.Transaction, local bool) error {
return ErrInsufficientFunds
}
// Ensure the transaction has more gas than the basic tx fee.
intrGas, err := IntrinsicGas(tx.Data(), tx.To() == nil, true)
intrGas, err := IntrinsicGas(tx.Data(), tx.To() == nil, pool.homestead, pool.istanbul)
if err != nil {
return err
}
Expand Down Expand Up @@ -1118,6 +1121,15 @@ func (pool *TxPool) reset(oldHead, newHead *types.Header) {
log.Debug("Reinjecting stale transactions", "count", len(reinject))
senderCacher.recover(pool.signer, reinject)
pool.addTxsLocked(reinject, false)

// Update all fork indicators by next pending block number.
next := new(big.Int).Add(newHead.Number, big.NewInt(1))
pool.istanbul = pool.chainconfig.IsIstanbul(next)
if pool.istanbul {
pool.homestead = true
} else {
pool.homestead = pool.chainconfig.IsHomestead(next)
}
Copy link
Member Author

@rjl493456442 rjl493456442 Aug 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If the dynasty is changed, should we recheck all pending txs which we receive by old consensus rules?

Probably no, nonZeroDataGas is reduced, so gas checking is loose.

A corner case is: we reach the Istanbul but mini reorg happens and we switch to Petersburg again. During this tiny time window, we receive some transactions with lower NonZeroDataGas.

Copy link
Member

Choose a reason for hiding this comment

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

No, no need to recheck past ones. We only care that the intrinsic gas is lower than the total requested gas when inserting into the pool. As we're pushing the gas down, it will always hold true when transitioning into Istanbul.

If we have a reorg back from Istanbul into Homestead that might be a bit messy indeed, since we might accept some transactions that would become invalid, but I don't think this is a legit issue we need to concern ourselves with. At worse the pool might be shuffling junk until the chain progresses again into Istanbul.

}

// promoteExecutables moves transactions that have become processable from the
Expand Down
17 changes: 13 additions & 4 deletions light/txpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package light
import (
"context"
"fmt"
"math/big"
"sync"
"time"

Expand Down Expand Up @@ -67,7 +68,8 @@ type TxPool struct {
mined map[common.Hash][]*types.Transaction // mined transactions by block hash
clearIdx uint64 // earliest block nr that can contain mined tx info

homestead bool
homestead bool // Fork indicator whether we are in the homestead stage.
istanbul bool // Fork indicator whether we are in the istanbul stage.
}

// TxRelayBackend provides an interface to the mechanism that forwards transacions
Expand Down Expand Up @@ -309,8 +311,15 @@ func (pool *TxPool) setNewHead(head *types.Header) {
txc, _ := pool.reorgOnNewHead(ctx, head)
m, r := txc.getLists()
pool.relay.NewHead(pool.head, m, r)
pool.homestead = pool.config.IsHomestead(head.Number)
pool.signer = types.MakeSigner(pool.config, head.Number)

// Update fork indicators by next pending block number
next := new(big.Int).Add(head.Number, big.NewInt(1))
pool.istanbul = pool.config.IsIstanbul(next)
if pool.istanbul {
pool.homestead = true
} else {
pool.homestead = pool.config.IsHomestead(next)
}
}

// Stop stops the light transaction pool
Expand Down Expand Up @@ -378,7 +387,7 @@ func (pool *TxPool) validateTx(ctx context.Context, tx *types.Transaction) error
}

// Should supply enough intrinsic gas
gas, err := core.IntrinsicGas(tx.Data(), tx.To() == nil, pool.homestead)
gas, err := core.IntrinsicGas(tx.Data(), tx.To() == nil, pool.homestead, pool.istanbul)
if err != nil {
return err
}
Expand Down
27 changes: 14 additions & 13 deletions params/protocol_params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,20 @@ const (
JumpdestGas uint64 = 1 // Once per JUMPDEST operation.
EpochDuration uint64 = 30000 // Duration between proof-of-work epochs.

CreateDataGas uint64 = 200 //
CallCreateDepth uint64 = 1024 // Maximum depth of call/create stack.
ExpGas uint64 = 10 // Once per EXP instruction
LogGas uint64 = 375 // Per LOG* operation.
CopyGas uint64 = 3 //
StackLimit uint64 = 1024 // Maximum size of VM stack allowed.
TierStepGas uint64 = 0 // Once per operation, for a selection of them.
LogTopicGas uint64 = 375 // Multiplied by the * of the LOG*, per LOG transaction. e.g. LOG0 incurs 0 * c_txLogTopicGas, LOG4 incurs 4 * c_txLogTopicGas.
CreateGas uint64 = 32000 // Once per CREATE operation & contract-creation transaction.
Create2Gas uint64 = 32000 // Once per CREATE2 operation
SelfdestructRefundGas uint64 = 24000 // Refunded following a selfdestruct operation.
MemoryGas uint64 = 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 uint64 = 68 // Per byte of data attached to a transaction that is not equal to zero. NOTE: Not payable on data of calls between transactions.
CreateDataGas uint64 = 200 //
CallCreateDepth uint64 = 1024 // Maximum depth of call/create stack.
ExpGas uint64 = 10 // Once per EXP instruction
LogGas uint64 = 375 // Per LOG* operation.
CopyGas uint64 = 3 //
StackLimit uint64 = 1024 // Maximum size of VM stack allowed.
TierStepGas uint64 = 0 // Once per operation, for a selection of them.
LogTopicGas uint64 = 375 // Multiplied by the * of the LOG*, per LOG transaction. e.g. LOG0 incurs 0 * c_txLogTopicGas, LOG4 incurs 4 * c_txLogTopicGas.
CreateGas uint64 = 32000 // Once per CREATE operation & contract-creation transaction.
Create2Gas uint64 = 32000 // Once per CREATE2 operation
SelfdestructRefundGas uint64 = 24000 // Refunded following a selfdestruct operation.
MemoryGas uint64 = 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.
TxDataNonZeroGasFrontier uint64 = 68 // Per byte of data attached to a transaction that is not equal to zero. NOTE: Not payable on data of calls between transactions.
TxDataNonZeroGasEIP2028 uint64 = 16 // Per byte of non zero data attached to a transaction after EIP 2028 (part in Istanbul)

// These have been changed during the course of the chain
CallGasFrontier uint64 = 40 // Once per CALL operation & message call transaction.
Expand Down
21 changes: 12 additions & 9 deletions tests/transaction_test_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type TransactionTest struct {
RLP hexutil.Bytes `json:"rlp"`
Byzantium ttFork
Constantinople ttFork
Istanbul ttFork
EIP150 ttFork
EIP158 ttFork
Frontier ttFork
Expand All @@ -45,7 +46,7 @@ type ttFork struct {

func (tt *TransactionTest) Run(config *params.ChainConfig) error {

validateTx := func(rlpData hexutil.Bytes, signer types.Signer, isHomestead bool) (*common.Address, *common.Hash, error) {
validateTx := func(rlpData hexutil.Bytes, signer types.Signer, isHomestead bool, isIstanbul bool) (*common.Address, *common.Hash, error) {
tx := new(types.Transaction)
if err := rlp.DecodeBytes(rlpData, tx); err != nil {
return nil, nil, err
Expand All @@ -55,7 +56,7 @@ func (tt *TransactionTest) Run(config *params.ChainConfig) error {
return nil, nil, err
}
// Intrinsic gas
requiredGas, err := core.IntrinsicGas(tx.Data(), tx.To() == nil, isHomestead)
requiredGas, err := core.IntrinsicGas(tx.Data(), tx.To() == nil, isHomestead, isIstanbul)
if err != nil {
return nil, nil, err
}
Expand All @@ -71,15 +72,17 @@ func (tt *TransactionTest) Run(config *params.ChainConfig) error {
signer types.Signer
fork ttFork
isHomestead bool
isIstanbul bool
}{
{"Frontier", types.FrontierSigner{}, tt.Frontier, false},
{"Homestead", types.HomesteadSigner{}, tt.Homestead, true},
{"EIP150", types.HomesteadSigner{}, tt.EIP150, true},
{"EIP158", types.NewEIP155Signer(config.ChainID), tt.EIP158, true},
{"Byzantium", types.NewEIP155Signer(config.ChainID), tt.Byzantium, true},
{"Constantinople", types.NewEIP155Signer(config.ChainID), tt.Constantinople, true},
{"Frontier", types.FrontierSigner{}, tt.Frontier, false, false},
{"Homestead", types.HomesteadSigner{}, tt.Homestead, true, false},
{"EIP150", types.HomesteadSigner{}, tt.EIP150, true, false},
{"EIP158", types.NewEIP155Signer(config.ChainID), tt.EIP158, true, false},
{"Byzantium", types.NewEIP155Signer(config.ChainID), tt.Byzantium, true, false},
{"Constantinople", types.NewEIP155Signer(config.ChainID), tt.Constantinople, true, false},
{"Istanbul", types.NewEIP155Signer(config.ChainID), tt.Istanbul, true, true},
} {
sender, txhash, err := validateTx(tt.RLP, testcase.signer, testcase.isHomestead)
sender, txhash, err := validateTx(tt.RLP, testcase.signer, testcase.isHomestead, testcase.isIstanbul)

if testcase.fork.Sender == (common.UnprefixedAddress{}) {
if err == nil {
Expand Down