core, light, params: implement eip2028 #19931
Conversation
rjl493456442
requested review from
holiman,
karalabe and
zsfelfoldi
as code owners
core/state_transition.go
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
It's really nasty to pass a batch of fork indicators. What about using a dynasty?
There was a problem hiding this comment.
We could pass the ChainConfig perhaps?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
core/tx_pool.go
Outdated
| pool.homestead = true | ||
| } else { | ||
| pool.homestead = pool.chainconfig.IsHomestead(next) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/tx_pool.go
Outdated
| @@ -217,6 +217,9 @@ type TxPool struct { | |||
| signer types.Signer | |||
| mu sync.RWMutex | |||
|
|
|||
| homestead bool // Fork indicator whether we are in the homestead stage. | |||
There was a problem hiding this comment.
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.
| @@ -217,6 +217,8 @@ type TxPool struct { | |||
| signer types.Signer | |||
| mu sync.RWMutex | |||
|
|
|||
| istanbul bool // Fork indicator whether we are in the istanbul stage. | |||
There was a problem hiding this comment.
Personal preference: eip2028Active
|
I pushed a commit on top to prevent transaction tests from failing. We need to re-enable that once the tests have been regenerated. |
core/state_transition.go
Outdated
| @@ -76,10 +76,10 @@ 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, eip155Active bool, eip2028Active bool) (uint64, error) { | |||
There was a problem hiding this comment.
You are going to hate me now, but ChainConfig has IsIstanbul, and chainRules have IsEIPXYZ. Lets keep it consistent and name these isEIP155 and isEIP2028 instead of starting a new style with Active. @holiman ?
There was a problem hiding this comment.
isEIP2028 and the like is fine with me
There was a problem hiding this comment.
I update the arguments to isEIP style.
This reverts commit 67825d8.
Revert "core, light, params: implement eip2028 (ethereum#19931)"
This reverts commit 67825d8.
Reown from liorgold2#1