Upstream merge up to 1.9.25 #1671

gastonponti · 2021-08-17T20:31:32Z

Description

Merge from upstream, go-ethereum v1.9.25

Versions

Geth v1.9.20 is another maintenance release containing bug fixes and security enhancements. This update is recommended for all users. Please note that reverting to Geth v1.9.19 or prior after upgrading to v1.9.20 is not possible without a resync because the blockchain database layout has changed.
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.20)
Geth v1.9.21 is a regular maintenance release, the highlights being the removal of whisper, better call tracing and multiple memory stability fixes during fast sync to both stabilize usage as well as to fix a memory leak that lead to crashers before.
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.21)
Geth v1.9.22 is our usual maintenance release, fixing a few bugs and adding some minor features
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.22)
Geth v1.9.23 is a maintenance release containing security fixes. This update is recommended for all users.
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.23)
Geth v1.9.24 is a security release. It is built with Go v1.15.5, fixing CVE-2020-28362, which has a critical impact for Ethereum (already fixed in Celo). This release also contains a fix for a consensus issue related to mining, which would have triggered a chain split on January 1st 2021.
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.24)
Geth v1.9.25 is a maintenance release.
(https://github.com/ethereum/go-ethereum/releases/tag/v1.9.25)

Merge Notes

Commit: e7872729012a4871397307b12cc3f4772ffcbec6

Removed

whisper module
p2p/discover/v5_encoding.go
mobile/shhclient.go
les/balance.go

Merge Decisions

miner/miner.go: startCh leave it as struct{}{} (the miner data is set in the Start)
cmd/utils/flags.go remove new flag GpoMaxGasPriceFlag
consolecmd_test.go added "--light.maxpeers", "0" flag to the runMinimalGeth function
core/vm/contracts.go
- eip2565 added to bigModExp
- init PrecompiledContractsDonut
core/vm/evm.go
- create-> L484 remove (ephemeral chain)

	// We add this to the access list _before_ taking a snapshot. Even if the creation fails,
	// the access-list change should not be rolled back
	if evm.chainRules.IsYoloV2 {
		evm.StateDB.AddAddressToAccessList(address)
	}

core/vm/runtime/runtime.go
- Execute-> L111 remove (ephemeral chain)

	if cfg.ChainConfig.IsYoloV2(vmenv.Context.BlockNumber) {
		cfg.State.AddAddressToAccessList(cfg.Origin)
		cfg.State.AddAddressToAccessList(address)
		for _, addr := range vmenv.ActivePrecompiles() {
			cfg.State.AddAddressToAccessList(addr)
		}
	}

* Create-> L147 remove (ephemeral chain)

	if cfg.ChainConfig.IsYoloV2(vmenv.Context.BlockNumber) {
		cfg.State.AddAddressToAccessList(cfg.Origin)
		for _, addr := range vmenv.ActivePrecompiles() {
			cfg.State.AddAddressToAccessList(addr)
		}
	}

* Call-> L175 remove (ephemeral chain)

	if cfg.ChainConfig.IsYoloV2(vmenv.Context.BlockNumber) {
		cfg.State.AddAddressToAccessList(cfg.Origin)
		cfg.State.AddAddressToAccessList(address)
		for _, addr := range vmenv.ActivePrecompiles() {
			cfg.State.AddAddressToAccessList(addr)
		}
	}

core/state_processor.go
- applyTransaction-> L137 remove (ephemeral chain)

	// Add addresses to access list if applicable
	if config.IsYoloV2(header.Number) {
		statedb.AddAddressToAccessList(msg.From())
		if dst := msg.To(); dst != nil {
			statedb.AddAddressToAccessList(*dst)
			// If it's a create-tx, the destination will be added inside evm.create
		}
		for _, addr := range evm.ActivePrecompiles() {
			statedb.AddAddressToAccessList(addr)
		}
	}

core/headerchain.go
- writeHeaders-> newTD = number + 1
eth/downloader/downloader.go
- syncWithPeer
- processFastSyncContent

			if height := latest.Number.Uint64(); height >= pivot.Number.Uint64()+2*uint64(fsMinFullBlocks)-uint64(reorgProtHeaderDelay) {

->

			if height := latest.Number.Uint64(); height >= pivot.Number.Uint64()+2*max(d.epoch, fsMinFullBlocks)-uint64(reorgProtHeaderDelay) {

This pivot part should be checked, because the functionality of moving the pivot while syncing, is using a max of 64 blocks in ethereum, but as we require the epoch to calculate the uptime score, some of the changes in the pivot, should be forced to the epoch block

Tested

An explanation of how the changes were tested or an explanation as to why they don't need to be.

Related issues

Fixes Merge upstream 1.9.20 to 1.9.25 #1327

Backwards compatibility

Brief explanation of why these changes are/are not backwards compatible.

build: drop disco, enable groovy on Ubuntu PPAs

* t8ntool: add output basedir * t8ntool: add txhash to trace filename * t8ntool: don't default to '.' basedir, allow absolute paths

* core: define and test chain reparation cornercases * core: write up a variety of set-head tests * core, eth: unify chain rollbacks, handle all the cases * core: make linter smile * core: remove commented out legacy code * core, eth/downloader: fix review comments * core: revert a removed recovery mechanism

travis, dockerfile, appveyor, build: bump to Go 1.15

* metrics: zero temp variable in updateMeter Previously the temp variable was not updated properly after summing it to count. This meant we had astronomically high metrics, now we zero out the temp whenever we sum it onto the snapshot count * metrics: move temp variable to be aligned, unit tests Moves the temp variable in MeterSnapshot to be 64-bit aligned because of the atomic bug. Adds a unit test, that catches the previous bug.

eth/downloader: fix rollback issue on short chains

* eth/downloader, eth/handler: utilize sync bloom for getNodeData * trie: handle if bloom is nil * trie, downloader: check bloom nilness externally

core/state/snapshot: reduce disk layer depth during generation

…pty (#21396) This change improves discovery behavior in small networks. Very small networks would often fail to bootstrap because all member nodes were dropping table content due to findnode failure. The check is now changed to avoid dropping nodes on findnode failure when their bucket is almost empty. It also relaxes the liveness check requirement for FINDNODE/v4 response nodes, returning unverified nodes as results when there aren't any verified nodes yet. The "findnode failed" log now reports whether the node was dropped instead of the number of results. The value of the "results" was always zero by definition. Co-authored-by: Felix Lange <[email protected]>

core/rawdb: only complain loudly if truncating many items

params: update CHTs for v1.9.20 release

…(#21334) * accounts/abi/bind/backends: Disallow timeshift for non-empty blocks * accounts/abi/bind/backends: added tests for adjust time * accounts/abi/bind/simulated: added comments, fixed test for AdjustTime * accounts/abi/bind/backends: updated comment

* go.mod | goleveldb latest update * go.mod update * leveldb options * go.mod: double check Co-authored-by: Péter Szilágyi <[email protected]>

core/state, eth, trie: stabilize memory use, fix memory leak

… calls (#21387) * tests: add testdata of call tracer * eth/tracers: return revert reason in call_tracer * eth/tracers: regenerate assets * eth/tracers: add error message even if no exec occurrs, fixes #21438 Co-authored-by: Martin Holst Swende <[email protected]>

# Conflicts: # core/genesis.go # scripts/check_imports.sh # scripts/hooks/pre-commit

piersy · 2021-10-13T18:06:51Z

Hi @gastonponti I've had a look through and here are my thoughts so far. I'm currently looking at syncWithPeer, but that will take a while.

miner/miner.go startCh leave it as struct{}{} (the miner data is set in the Start)

leaving startCh as struct{}{} means that there is a data race that could occur when accessing the validator addresses (because the values are set from one go routine and accesse from another with no synchronisation). I would suggest removing the validator field in miner (because its also set in the worker) and then adding a read locked method to access the validator address from the worker.

Why are we not supporting the ephemeral chain?

core/headerchain.go
writeHeaders-> newTD = number + 1

I'm not really understanding the significance of the above change?

piersy · 2021-10-14T13:35:51Z

@gastonponti

With regards to the pivot selection I think there could be a problem.

I think pruning was introduced in this PR:

all: bloom-filter based pruning mechanism ethereum/go-ethereum#21724
Where IIUC it was decided to prune after the n-127 block.

Then there was this PR:

eth/downloader: dynamically move pivot even during chain sync ethereum/go-ethereum#21529
Where the intent is to allow changing the pivot because the state becomes unavailable after 128 blocks. So the line you called out was to ensure the pivot got recalculated when it was stale (2*fsMinSyncBlocks where fsMinSyncBlocks=64), and with your change it won't be recalculated for an extremely long time.
I don't really understand the syncing code enough to verify any of the rest of it, maybe we could schedule a call to go over it because I would not be confident in my current assessment.

…-1.9.25 # Conflicts: # internal/ethapi/api.go # les/client_handler.go

piersy · 2021-10-15T13:19:41Z

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit e725fb2

coverage: 44.5% of statements across all listed packages

coverage:  54.1% of statements in consensus/istanbul
coverage:  45.0% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  43.3% of statements in consensus/istanbul/backend/internal/db
coverage:  24.1% of statements in consensus/istanbul/backend/internal/enodes
coverage:  22.4% of statements in consensus/istanbul/backend/internal/replica
coverage:  63.3% of statements in consensus/istanbul/core
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  75.3% of statements in consensus/istanbul/uptime
coverage: 100.0% of statements in consensus/istanbul/uptime/store
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

CommentID: 0adf78fd58

…pstream-1.9.25

mcortesi · 2021-10-26T00:06:02Z

@gastonponti could you check that all changes to the bind pkg are ported to bind_v2. If they fixed things it would be nice to port those to the v2 version. I've seen severa changes on the files.

mcortesi

Leaving some comments while i'm doing the rest of the review

oss-fuzz.sh

mcortesi · 2021-10-26T00:18:19Z

.travis.yml

-        - go run build/ci.go install -arch 386
-        - go run build/ci.go archive -arch 386 -type tar -signer LINUX_SIGNING_KEY -upload gethstore/builds
+        - go run build/ci.go install -dlgo
+        - go run build/ci.go archive -type tar -signer LINUX_SIGNING_KEY -signify SIGNIFY_KEY -upload gethstore/builds


SIGNIFY_KEY is a variable name or a value? If a variable, where is it defined?

It's an environment variable

It's used by the archive command of the build/ci.go

mcortesi · 2021-10-26T19:17:33Z

accounts/usbwallet/wallet.go

 					break
 				}
-				// If the next account is empty, stop self-derivation, but add for the last base path
+				// We've just self-derived a new account, start tracking it locally


is usb wallet ledger? does this impact it in any way?

They basically missed the base derivation path, and they had to change m/44'/60'/0'/0 to m/44'/60'/0'/0/0.
For geth, they log the accounts legacy that they are not discovering (this is of course the case where the user is not specifying the path). For clef they are getting the first 5 of the legacy and 5 from the normal one.
We have the same legacy/normal path specification in the accounts/hd.go
What it seems, is that when we build/change our hw in celo, we added that differentiation. So, in our case, probably all the accounts were built using the new path, so if it's selecting legacy accounts just to log them as an error, we shouldn't have any.
TL;DR; won't impact, just log something that we probably avoided

mcortesi · 2021-10-26T19:35:19Z

cmd/geth/version_check_test.go

+	"testing"
+)
+
+func TestVerification(t *testing.T) {


does this work? is it tied to geth or celo blockchain versions?

Tied to geth. Added the issue #1728

mcortesi

Can't say that my review is any guarantee here. Too many lines of code, don't really know the context of many of them. But didn't find any red alerts. if e2e tests are good, and it's syncing. I would merge this

mcortesi · 2021-10-26T22:02:14Z

core/types/block.go


 var (
-	EmptyRootHash       = DeriveSha(Transactions{})
+	EmptyRootHash       = common.HexToHash("56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421")


where does this come from?

56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421 is the empty root. Check this similar one. ethereum/go-ethereum#19254
If you check the codebase, that emptyRootHash is al over the place

gastonponti · 2021-10-28T02:37:30Z

Just to add context in the PR for an offline discussion, the comment #1671 (comment) was dismissed, because the moving pivot requiring less that 127 blocks, is needed for the snap sync, which we have disabled.

We of course, will need to fix this, but to achieve this, we should (and will) need to change how we are calculating the uptimeScore of every validator in the the middle of an epoch.

Right now, as we are only calculating the uptimeScore when we are inserting a block, we changed the fast sync, to use the block of the actual epoch change as the pivot block. This way, the fast sync uses headers until the beginning of the epoch, and then, just sync inserting blocks. This calculates the uptimeScore with the block insertion. In the scenario of not having more of 127 blocks, will break how we calculate the uptimeScore, and that's why we need to change that.
The idea is to continue this work, change the uptimeScore calculation and enable the snap sync after this "upstream updates" to avoid adding functionalities in the middle of this massive PR. (cc @piersy)

karalabe and others added 30 commits August 19, 2020 10:28

build: drop disco, enable groovy on Ubuntu PPAs

32b078d

Merge pull request #21461 from karalabe/ppa-drop-disco

560d444

build: drop disco, enable groovy on Ubuntu PPAs

cmd/evm: statet8n output folder + tx hashes on trace filenames (#21406)

7ebc6c4

* t8ntool: add output basedir * t8ntool: add txhash to trace filename * t8ntool: don't default to '.' basedir, allow absolute paths

core: more detailed metering for reorgs (#21420)

0bdd295

travis, dockerfile, appveyor, build: bump to Go 1.15

15fdaf2

Merge pull request #21466 from karalabe/go1.15

a70a79b

travis, dockerfile, appveyor, build: bump to Go 1.15

eth/downloader: fix rollback issue on short chains

9f7b79a

Merge pull request #21472 from holiman/fix_dltest_fail

b68929c

eth/downloader: fix rollback issue on short chains

core, eth, les, trie: add a prefix to contract code (#21080)

87c0ba9

core: do less lookups when writing fast-sync block bodies (#21468)

1b5a867

eth: utilize sync bloom for getNodeData (#21445)

0f4e7c9

* eth/downloader, eth/handler: utilize sync bloom for getNodeData * trie: handle if bloom is nil * trie, downloader: check bloom nilness externally

core/state/snapshot: reduce disk layer depth during generation

3ee91b9

Merge pull request #21477 from karalabe/snapshotter-shallow-generator

bdde616

core/state/snapshot: reduce disk layer depth during generation

core/rawdb: only complain loudly if truncating many items

5655dce

Merge pull request #21483 from karalabe/freezer-truncate-silent

d13b8e5

core/rawdb: only complain loudly if truncating many items

graphql: add support for retrieving the chain id (#21451)

341f451

params: update CHTs for v1.9.20 release

ce5f949

Merge pull request #21486 from karalabe/cht-1.9.20

63a9d4b

params: update CHTs for v1.9.20 release

params: release Geth v1.9.20

979fc96

params: begin v1.9.21 release cycle

fc20680

core/state, eth, trie: stabilize memory use, fix memory leak

d8da0b3

eth: updated comments (#21490)

16d7eae

go.mod | goleveldb latest update (#21448)

856307d

* go.mod | goleveldb latest update * go.mod update * leveldb options * go.mod: double check Co-authored-by: Péter Szilágyi <[email protected]>

Merge pull request #21491 from karalabe/state-sync-leak-fix

d97e006

core/state, eth, trie: stabilize memory use, fix memory leak

rpc: fix issue with null JSON-RPC messages (#21497)

5883afb

gastonponti added 3 commits August 18, 2021 11:00

Merge branch 'master' into gastonponti/merge-upstream-1.9.25

4e88f86

# Conflicts: # core/genesis.go # scripts/check_imports.sh # scripts/hooks/pre-commit

Fix lint from master merge

ccebec1

Fix block coinbase

053e887

gastonponti force-pushed the gastonponti/merge-upstream-1.9.25 branch from 8fa236f to 053e887 Compare August 19, 2021 20:36

gastonponti added 3 commits October 14, 2021 15:59

Fix rlp chain for tests

f4ef138

Skip checkpoint tests

d5e8024

Merge remote-tracking branch 'master' into gastonponti/merge-upstream…

d32cd98

…-1.9.25 # Conflicts: # internal/ethapi/api.go # les/client_handler.go

gastonponti marked this pull request as ready for review October 15, 2021 16:08

gastonponti requested review from a team, hbandura, kobigurk, nategraf and trianglesphere as code owners October 15, 2021 16:08

Merge remote-tracking branch 'public/master' into gastonponti/merge-u…

549b84c

…pstream-1.9.25

mcortesi reviewed Oct 26, 2021

View reviewed changes

mcortesi approved these changes Oct 26, 2021

View reviewed changes

gastonponti added 2 commits October 27, 2021 11:41

Bring abi/bind changes to abi/bind_V2

14d246c

Fix oss-fuzz path

bb99f20

gastonponti force-pushed the gastonponti/merge-upstream-1.9.25 branch 2 times, most recently from 2361d42 to e70b7d5 Compare October 29, 2021 12:39

Downloader. Fix pivot for fast sync

89d34bb

gastonponti force-pushed the gastonponti/merge-upstream-1.9.25 branch from e70b7d5 to 89d34bb Compare October 29, 2021 12:59

Merge branch 'master' into gastonponti/merge-upstream-1.9.25

e725fb2

mcortesi merged commit 690f30f into master Nov 1, 2021

mcortesi deleted the gastonponti/merge-upstream-1.9.25 branch November 1, 2021 21:53

Upstream merge up to 1.9.25 #1671

Upstream merge up to 1.9.25 #1671

Uh oh!

Conversation

gastonponti commented Aug 17, 2021 • edited by mcortesi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Versions

Merge Notes

Removed

Merge Decisions

Tested

Related issues

Backwards compatibility

Uh oh!

piersy commented Oct 13, 2021

Uh oh!

piersy commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piersy commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcortesi commented Oct 26, 2021

Uh oh!

mcortesi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcortesi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gastonponti Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gastonponti commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

47 participants

gastonponti commented Aug 17, 2021 •

edited by mcortesi

Loading

piersy commented Oct 14, 2021 •

edited

Loading

piersy commented Oct 15, 2021 •

edited

Loading

gastonponti Oct 27, 2021 •

edited

Loading

gastonponti commented Oct 28, 2021 •

edited

Loading