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

Merge/foundation release/1.11 #531

Merged
merged 687 commits into from
Apr 14, 2023
Merged

Merge/foundation release/1.11 #531

merged 687 commits into from
Apr 14, 2023

Conversation

meowsbits
Copy link
Contributor

@meowsbits meowsbits commented Feb 26, 2023

Merges ethereum/go-ethereum branch release/1.11 to core-geth master.

Significant changes include

  • Ropsten is dead and gone; Sepolia replaces it as the canonical testnet.
  • Shangahai feature support is added;
    • Introducing time-based fork definitions
  • eth/68 is the new best-default protocol
  • 2 debug_ API methods gone, 4 new ones introduced.
  • Pebble DB.
  • personal API is hashly deprecated, requiring opt-back-in with --rpc.enabledeprecatedpersonal.
  • Removed iOS and Android builds

Things I'd like to consider for this or next release

  • Remove all Parity, Aleth, and Multigeth configuration support. I don't think many people use this. Being abandoned projects, their chain spec schemas have long since not supported configurations for features post-Byzantium (so they could not be well-supported).

rjl493456442 and others added 30 commits February 21, 2023 06:12
The EmptyRootHash and EmptyCodeHash are defined everywhere in the codebase, this PR replaces all of them with unified one defined in core/types package, and also defines constants for TxRoot, WithdrawalsRoot and UncleRoot
This install untested logic for time-based forks
alongside block-based forks in the config compatibility
check logic.

Block-based forks use *big.Int
Time-based forks use *uint64.

That's about the only real difference here.
The rest is basically just boilerplate.

Now that I've seen what this looks like,
I think that it may a bad, cumbersome solution to the
problem of supporting time-based forks.

Time-based forks are conceptually the exact same
as block-based forks; both check some maybe-nil
value against another maybe-nil value, and
return some boolean whether a is greater than or equal b.

Date: 2023-02-20 16:29:48-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:13:54-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:25:39-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:28:04-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:28:25-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:37:06-08:00
Signed-off-by: meows <b5c6@protonmail.com>
isBadCache was a hotfix patch for
bad cache generation logic that happened
during the ECIP1099 transition era.

It is no longer useful.

Date: 2023-02-21 05:39:35-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:41:01-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:41:27-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:42:08-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:43:23-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:48:59-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:58:34-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:51:15-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 05:51:47-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 06:00:05-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 06:00:59-08:00
Signed-off-by: meows <b5c6@protonmail.com>
the *Syncer.commitHealer method merged weird.

PTAL:
Like the whole chunk was just... old, I guess.
Why? Is this a git thing? Was there something
core-geth modified in there that caused the
ineffectual merge?
Or another chunk in the file that bumped
the git merge algo off by a paragraph?

Date: 2023-02-21 06:05:23-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 06:09:04-08:00
Signed-off-by: meows <b5c6@protonmail.com>
…Block returns 3 values

Date: 2023-02-21 06:11:14-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 06:28:12-08:00
Signed-off-by: meows <b5c6@protonmail.com>
This function clones chain configurators.

It will be used as part of a drop-in replacement
for logic at overrideConfig in eth/tracers/api.go,
which is a cloner-crusher function.

Still TODO here is to write the Crusher logic.

Date: 2023-02-21 07:03:27-08:00
Signed-off-by: meows <b5c6@protonmail.com>
The rest of the stureby configs were deleted
upstream. This finishes the job.

TODO: Get testdata for the confp tests.

Date: 2023-02-21 07:31:52-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 07:33:17-08:00
Signed-off-by: meows <b5c6@protonmail.com>
…/ctypes,params/types/goethereum,params/types/internal,params/types/multigeth,params/types/parity: ElasticityMultiplier and BaseFeeChangeDenominator getter/setter no pointers

confp/TestConvert failed with a panic because the
reflection use of getters/setters
was causing a *uint64 to be passed where
the arg should have been a uint64.
Getters and setters both need to accept
and return the same type of value.

This commit also add JSON genesis dumps
for foundation,classic::go-ethereum,core-geth
configs as testdata, replacing the stureby configs.

Date: 2023-02-21 08:03:37-08:00
Signed-off-by: meows <b5c6@protonmail.com>
…or method for genesisT.Genesis

These are just pass-through methods,
but Genesis was missing them and causing
the TestConvert to fail because Genesis
didn't implement ChainConfigurator so
the target values were zero-values.

Test now passing.

Date: 2023-02-21 08:25:13-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Date: 2023-02-21 08:37:28-08:00
Signed-off-by: meows <b5c6@protonmail.com>
…st,params/confp/tconvert,params/types/tests,tests: confp.Crush: reverse params order: now (dest, source)

This better matches Go conventions (eg. copy)
and will make more sense as I continue this
refactor to rename Convert to Crush
and extract common logic for crushing
with zero-values (current logic) adjacent
to only crushing non-zero values (new logic)
.

Date: 2023-02-21 09:08:47-08:00
Signed-off-by: meows <b5c6@protonmail.com>
…st,params/confp/tconvert,params/types/tests,tests: rename confp.Convert to confp.Crush

Date: 2023-02-21 09:09:31-08:00
Signed-off-by: meows <b5c6@protonmail.com>
Copy link
Member

@ziogaschr ziogaschr left a comment

Choose a reason for hiding this comment

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

This review is not finished. I am going to submit it though, and continue with PR comments from now. I reviewed up to eth/downloader/beaconsync.go.

@@ -4,7 +4,7 @@ ARG VERSION=""
ARG BUILDNUM=""

# Build Geth in a stock Go builder container
FROM golang:1.18-alpine as builder
Copy link
Member

Choose a reason for hiding this comment

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

Not important, just a heads up. go.mod suggests go1.19, which I used for testing. Dockerfile's are using go1.20, which is probably fine, but good to give them a test as well as they might create a regression.

cmd/geth/dbcmd.go Show resolved Hide resolved
consensus/ethash/ethash.go Show resolved Hide resolved
core/blockchain.go Show resolved Hide resolved
core/forkid/forkid.go Show resolved Hide resolved
storageUpdatedMeter = metrics.NewRegisteredMeter("state/update/storage", nil)
accountDeletedMeter = metrics.NewRegisteredMeter("state/delete/account", nil)
storageDeletedMeter = metrics.NewRegisteredMeter("state/delete/storage", nil)
accountTrieCommittedMeter = metrics.NewRegisteredMeter("state/commit/accountnodes", nil)
Copy link
Member

Choose a reason for hiding this comment

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

Are we using those at our metrics dashboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, yes. Nice catch! So it looks like state/commit/accountnodes becomes state/update/accountnodes, and then 3 other gauges are added. That'll be a TODO over at the dashboards where applicable, but not a major change..

core/txpool/txpool.go Show resolved Hide resolved
@@ -201,12 +201,18 @@ func instructionSetForConfig(config ctypes.ChainConfigurator, isPostMerge bool,
maxStack: maxStack(0, 1),
}
}
if config.IsEnabledByTime(config.GetEIP3855TransitionTime, bt) {
Copy link
Member

Choose a reason for hiding this comment

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

For later, if we activate future EIPs in ETC, we have to add logic for checking with isEnabled as we don't use the time logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here...

  • ETC does not have any EIPs that require time-based activation (but ETH does and this patch supports that).
  • the configurator interface defines IsEnabledByTime adjacent to IsEnabled (by block);
    • configurator convention expects time-based activation configuration values (fields) to be named like EIP424242FTime (suffix=Time), vs. EIP2FBlock (suffix=Block).

What logic needs to be yet added?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not explaining better. I finished the first review of full PR an hour ago and I am going to check back comments and add commits for parts needed.

Based on this, my idea is that core-geth have to support both ByBlock and ByTime logic everywhere. Upstream is clear that will use ByTime from now on.

For future EIPs that ETC or other chains using core-geth will utilise on their network they might want to enable an EIP based on the current ByBlock logic.

My suggestion is to have something like: if config.IsEnabledByTime(config.GetEIP3855TransitionTime, bt) || config.IsEnabled(config.GetEIP3855TransitionNumber, bn) {


// Shanghai
// EIP-3651: Warm coinbase
eip3651f = cfg.ChainConfig.IsEnabledByTime(cfg.ChainConfig.GetEIP3651TransitionTime, &vmenv.Context.Time)
Copy link
Member

Choose a reason for hiding this comment

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

Check with isEnabled as well

Copy link
Member

Choose a reason for hiding this comment

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

Follow comments here (same logic)


// Shanghai
// EIP-3651: Warm coinbase
eip3651f = cfg.ChainConfig.IsEnabledByTime(cfg.ChainConfig.GetEIP3651TransitionTime, &vmenv.Context.Time)
Copy link
Member

Choose a reason for hiding this comment

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

Check with isEnabled as well

Copy link
Member

Choose a reason for hiding this comment

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

Follow comments here (same logic)

@ziogaschr
Copy link
Member

We will have to search globally forisEnabledByTime and modify accordingly.

eth/peer.go Show resolved Hide resolved
snap := chain.Snapshots().Snapshot(req.Root)
if snap == nil {
Copy link
Member

Choose a reason for hiding this comment

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

@meowsbits let's discuss in call if we want to keep this in

eth/tracers/js/goja.go Outdated Show resolved Hide resolved
@meowsbits meowsbits marked this pull request as ready for review April 13, 2023 17:28
meowsbits and others added 7 commits April 14, 2023 08:48
…ypes/genesisT,params/types/multigeth,tests: remove multigeth data type

Multi-geth has been disused and unimproved
for many a moon now. Though her
roots will be with us always, now
her name fades from the history books.
Thanks for the good idea.

Date: 2023-04-11 13:34:58-07:00
Signed-off-by: meows <b5c6@protonmail.com>
…onfp/internal,params/confp/tconvert,params/types/parity,params/types/tests,tests: remove openethereum (nee parity) data type

The OpenEthereum (nee Parity) client is
disused and maintenance for it continues to
to outweight the benefits.
It too shall go the way of the dodo.

Date: 2023-04-11 14:32:57-07:00
Signed-off-by: meows <b5c6@protonmail.com>
…aleth,pythereum data types

Like the previous two commits, this
removes the data type of disused/poorly
supported clients.
Not much interesting here since these were only
marginally supported in the first place.

Date: 2023-04-11 15:19:20-07:00
Signed-off-by: meows <b5c6@protonmail.com>
…ypes/genesisT: run 'go generate ./...'

Date: 2023-04-11 15:40:03-07:00
Signed-off-by: meows <b5c6@protonmail.com>
This only re-establishes the writes
to this file that are not supposed to
happen because its a generated file.

Date: 2023-04-11 16:16:48-07:00
Signed-off-by: meows <b5c6@protonmail.com>
This is ugly, but I don't see any other
way yet, and it doesn't seem that
different from what's going on
upstream.

Date: 2023-04-12 06:54:18-07:00
Signed-off-by: meows <b5c6@protonmail.com>
@meowsbits meowsbits force-pushed the merge/foundation-release/1.11 branch from 346f8b3 to 5adf7bd Compare April 14, 2023 15:48
The last commit edits a generated file.
This patch registers this last commit
to be reverted before the go-generate
CI test is run.

Date: 2023-04-13 07:52:52-07:00
Signed-off-by: meows <b5c6@protonmail.com>
@meowsbits meowsbits force-pushed the merge/foundation-release/1.11 branch from 5adf7bd to 4cbae0b Compare April 14, 2023 15:50
I rebased lately and broke the commit
hashes. This is fixing them.
This commit hash corresponds to a commit
which edits the gen_genesis.go file
.

Date: 2023-04-14 08:55:40-07:00
Signed-off-by: meows <b5c6@protonmail.com>
@meowsbits meowsbits merged commit 8a2fcc1 into master Apr 14, 2023
@meowsbits meowsbits deleted the merge/foundation-release/1.11 branch April 14, 2023 21:47
@ziogaschr ziogaschr mentioned this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.