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.

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

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Aug 9, 2019

Reown from liorgold2#1

@@ -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).

core/tx_pool.go Outdated
pool.homestead = true
} else {
pool.homestead = pool.chainconfig.IsHomestead(next)
}
Copy link
Member Author

@rjl493456442 rjl493456442 Aug 9, 2019

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.

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.
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.

@karalabe karalabe added this to the 1.9.2 milestone Aug 9, 2019
@karalabe karalabe mentioned this pull request Aug 9, 2019
10 tasks
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments which you're free to disregard (@karalabe might not agree with me on those)

@@ -217,6 +217,8 @@ type TxPool struct {
signer types.Signer
mu sync.RWMutex

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

Choose a reason for hiding this comment

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

Personal preference: eip2028Active

@karalabe karalabe modified the milestones: 1.9.2, 1.9.3 Aug 13, 2019
@holiman
Copy link
Contributor

holiman commented Aug 14, 2019

I pushed a commit on top to prevent transaction tests from failing. We need to re-enable that once the tests have been regenerated.
cc @winsvega

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

isEIP2028 and the like is fine with me

Copy link
Member Author

Choose a reason for hiding this comment

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

I update the arguments to isEIP style.

@karalabe karalabe merged commit c2c4c9f into ethereum:master Aug 14, 2019
@gzliudan gzliudan mentioned this pull request May 10, 2024
19 tasks
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request May 10, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request May 10, 2024
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request May 11, 2024
Revert "core, light, params: implement eip2028 (ethereum#19931)"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants