Skip to content

Refactor chain configuration #19735

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

Merged
merged 7 commits into from
Aug 5, 2019
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jun 18, 2019

Our chain is configured a bit spread out across several different abstractions, one of them being a gastable. This PR ries to make the configuration more coherent, and remove the need to keep a separate gastable and instead encapsulate everything inside the instruction sets.

This PR makes use of a previously unused recent change, the division between constantGas and dynamicGas for an operation, and the fact that an operation can have both, which makes certain simplifications possible.

The end goal is to simplify for a future PR where we can more easily add features (EIPS) to an existing defined fork, to make it simpler to execute state tests which focus on a particular feature (e..g Constantinople+1884) even if the whole (future) fork is still not defined.

@holiman
Copy link
Contributor Author

holiman commented Jun 19, 2019

This unstable-326b20e1-20190619 is now running on mon08, versus Geth/v1.9.0-unstable-3271a5af-20190619 (master) on mon09. Speed improvements are not really to be expected, mainly to check that it can process the forks correctly.

@holiman
Copy link
Contributor Author

holiman commented Jun 20, 2019

This PR now made it through all forks including byzantium, will take some more time for it to reach Constantinople.

@holiman holiman changed the title [WIP] Refactor chain configuration Refactor chain configuration Jun 20, 2019
@holiman
Copy link
Contributor Author

holiman commented Jun 20, 2019

This is no longer considered 'wip', although we should wait with merging until the node has synced up to head.

@holiman
Copy link
Contributor Author

holiman commented Jun 25, 2019

This PR now made it past all forks, including Constantinople. It's about 3h ahead of master, but that's probably just due to differences on the machines (exp is usually a tiny bit faster)

// CreateBySuicide occurs when the refunded account is one that does
// not exist. This logic is similar to call.
// Introduced in Tangerine Whistle (Eip 150)
CreateBySuicideGas uint64 = 25000
Copy link
Member

Choose a reason for hiding this comment

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

Nit, lets rename Suicide everywhere to Selfdestruct. Now's a good time imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed it everywhere where I've already touched things, so param names and opcode internals. It's still in use within e.g. statedb and stateobject. I figure it's better to do that change in a PR which does not contain any functional changes.

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.

Generally LGTM. I've enumerated some insignificant nits. One thing that I'm pondering about is an introduced slight confusion in the gas method now.

Previously all opcodes either cost a constant, or if they were dynamic, they had a fully encompassing method. Now we have three combos: fully constant, fully dynamic, or constant + dynamic. This last class seems a bit error prone, because looking at the method name gasSStore vs gasSha3, both seem to convey that they are standalone, whereas the latter actually has a constant component outside.

Would it perhaps make sense to have gasXYZ be the naming convention if it's fully self contained, or dyngasXYZ if it's just part of the gas. That would make the code clearer that there's something funky going on there.

@holiman
Copy link
Contributor Author

holiman commented Jul 30, 2019

I've addressed the nitpicks. Regarding the other slight confusion, one thing to have in mind is that most of these contants are named to be (somewhat) consistent with the YP, which defines things like Gsha3, and Gsha3word which then became Sha3Gas and Sha3WordGas in go-ethereum, which were used in the method gasSha3. It all seems pretty random to me, and if we want to change it, we should have some good underlying logic for how to name these things, so that

  • it's not so confusing internally,
  • it still somewhat consistent with YP
  • The name should also contain what fork it is relevant from.

For reference btw, there aren't many cases where things have only dynamic gas any more; SLOAD and LOGX, EXP, RETURN,REVERT, and SELFDESTRUCT.

LOGX could be rewritten to have a static portion (params.LogGas).
EXP is also only dynamic as of now, but could also be split up (params.ExpGas being constant)

If you want to, I can change all the methods that are only the dynamic part into dynGasXX/dynGasXXFrontier/dynGasXXEIP150.

@karalabe karalabe added this to the 1.9.2 milestone Jul 31, 2019
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.

LGTM

@holiman
Copy link
Contributor Author

holiman commented Jul 31, 2019 via email

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman
Copy link
Contributor Author

holiman commented Jul 31, 2019

Nits fixed. Some follow-up work that might be worth doing in other PRs is to ...

  • Extend the same logic to precompiles, so EIPs that modify (add/reprice) precompiles can be treated in a similar fashion,
  • Split up LOGX + EXP (and possibly others) into static + dynamic parts
  • Use a block context, and reuse the context across all txs in a block (same instructionset, same chainrules, same blocktime, number etc)

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.

4 participants