Upstream merge v1.14.10 #1433

lucca30 · 2025-02-04T11:01:36Z

Description

Usptream merge from Geth.

Releases Included: Aegis (v1.14.9) and Kopis (v1.14.10)

Points of attention:

core/blockchain.go is a critical file in this upmerge. There was a lot of changes on it. So reviewer, please double check on this
PR 29179
- This one I reverted since it removes forkChoicer which is an essential part of bor
PR 30125
- This one I reverted since it fixes a scenario for blob transactions (not supported by us) but also break some of our tests
Some eth/catalyst tests were also skipped following the behaviour we already did on past upstream merges
InterruptCtx got removed when doing conflict solving but then I re-inserted the feature back

Changes

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)
Changes only for a subset of nodes

Checklist

I have added at least 2 reviewer or the whole pos-v1 team
I have added sufficient documentation in code
I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
Includes RPC methods changes, and the Notion documentation has been updated

Cross repository changes

This PR requires changes to heimdall
- In case link the PR here:
This PR requires changes to matic-cli
- In case link the PR here:

Testing

I have added unit tests
I have added tests to CI
I have tested this code manually on local environment
I have tested this code manually on remote devnet using express-cli
I have tested this code manually on mumbai/amoy
I have created new e2e tests into express-cli

the validation process only checks for 'less than', which is inconsistent with the error output

Includes a fix for MIPS32 support. Pebble release: https://github.com/cockroachdb/pebble/releases/tag/v1.1.2 Key fix for mips32: cockroachdb/pebble@9f3904a (also the only change from v1.1.1.

This PR refactors the genesis initialization a bit, s.th. we only compute the blockhash once instead of twice as before (during hashAlloc and flushAlloc) This will significantly reduce the amount of memory allocated during genesis init --------- Co-authored-by: Gary Rong <[email protected]>

This pull request drops the legacy transaction retrieval support from before eth68, adding the restrictions that transaction metadata must be provided along with the transaction announment.

Add coinbase address to javascript tracer context. This PR adds the `coinbase` address to `jsTracer.ctx`, allowing access to the coinbase address (fee receipient) in custom JavaScript tracers. Example usage: ```javascript result: function(ctx) { return toAddress(ctx.coinbase); } ``` This change enables custom tracers to access coinbase address, previously unavailable, enhancing their capabilities to match built-in tracers.

Here I am adding a discv5 nodes source into the p2p dial iterator. It's an improved version of #29533. Unlike discv4, the discv5 random nodes iterator will always provide full ENRs. This means we can apply filtering to the results and will only try dialing nodes which explictly opt into the eth protocol with a matching chain. I have also removed the dial iterator from snap. We don't have an official DNS list for snap anymore, and I doubt anyone else is running one. While we could potentially filter for snap on discv5, there will be very few nodes announcing it, and the extra iterator would just stall the dialer. --------- Co-authored-by: lightclient <[email protected]>

blsync was failing if the light endpoint it was provided ended with a `/`. This change should handle the joining more gracefully.

The withdrawal length is already verified by the beacon consensus package, so the check in the state processor is a duplicate.

To allow all error paths in `vm.EVM.create()` to consume the necessary gas, there is currently a pattern of gating code on `if err == nil` instead of returning as soon as the error occurs. The same behaviour can be achieved by abstracting the gated code into a method that returns immediately on error, improving readability and thus making it easier to understand and maintain.

When we are building in detached head, we cannot easily obtain the same information as we can if we're in non-detached head. However, one thing we _can_ obtain is the git-hash and git-date. Currently, we omit to include the git-date into the build-info, which causes problem for reproducable builds which are on a detached head. This change fixes it to include the date-info always.

removes ppa-build for ubuntu `mantic`

Our `WriteArchive`, used by ci builder, creates files in the repo root,in order to upload. After we've built the amd64-builds, we create the uploads, and cause the repo to be flagged as dirty for the remaining builds. This change fixes it by adding the artefacts to gitignore. Closes #30324

…#30264) closes #29475, replaces #29657, #30104 Fixes two issues. First is a deadlock where the txpool attempts to reorg, but can't complete because there are no readers left for the new txs subscription. Second, resolves a problem with on demand mode where txs may be left pending when there are more pending txs than block space. Co-authored-by: Martin Holst Swende <[email protected]>

convert parameter of type contract to the basic `address` type --------- Co-authored-by: Martin HS <[email protected]>

These are the leftovers from #24028.

This PR adds a sync.Pool to reuse instances of Memory in EVMInterpreter.

This PR implements the conclusions from ethereum/go-ethereum#28987 (comment), that is: Building with `--strip-all` as a ld-flag to the cgo linker, to remove symbols. Without that, some spurious reference to a temporary file is included into the kzg-related library. Building with `--build-id=none`, to avoid putting a `build id` into the file.

This PR updates the version of go used in builds and docker to 1.23.0. Release notes: https://go.dev/doc/go1.23 More importantly, following our policy of maintaining the last two versions (which now becomes 1.23 and 1.22), we can now make use of the things that were introduced in 1.22: https://go.dev/doc/go1.22 Go 1.22 makes two changes to “for” loops. - each iteration creates new variables, - for loops may range over integers Other than that, some interesting library changes and other stuff.

Fixes #30156 This adds a repro of the linked issue. I fixed it by adding a timeout when issuing the call to unsubscribe.

…loy (#30326) This PR adds the `dns:read` and `dns:edit` permissions to the required set of permissions checked before deploying an ENR tree to Cloudflare. These permissions are necessary for a successful publish. **Background**: The current logic for `devp2p dns to-cloudflare` checks for `zone:edit` and `zone:read` permissions. However, when running the command with only these two permissions, the following error occurs: ``` wrong permissions on zone REMOVED-ZONE: map[#zone:edit:false #zone:read:true] ``` Adding `zone:read` and `zone:edit` to the API token led to a different error: ``` INFO [08-19|14:06:16.782] Retrieving existing TXT records on pos-nodes.hardfork.dev Authentication error (10000) ``` This suggested that additional permissions were required. I added `dns:read`, but encountered another error: ``` INFO [08-19|14:11:42.342] Retrieving existing TXT records on pos-nodes.hardfork.dev INFO [08-19|14:11:42.851] Updating DNS entries failed to publish REMOVED.pos-nodes.hardfork.dev: Authentication error (10000) ``` Finally, after adding both `dns:read` and `dns:edit` permissions, the command executed successfully with the following output: ``` INFO [08-19|14:13:07.677] Checking Permissions on zone REMOVED-ZONE INFO [08-19|14:13:08.014] Retrieving existing TXT records on pos-nodes.hardfork.dev INFO [08-19|14:13:08.440] Updating DNS entries INFO [08-19|14:13:08.440] "Updating pos-nodes.hardfork.dev from \"enrtree-root:v1 e=FSED3EDKEKRDDFMCLP746QY6CY l=FDXN3SN67NA5DKA4J2GOK7BVQI seq=1 sig=Glja2c9RviRqOpaaHR0MnHsQwU76nJXadJwFeiXpp8MRTVIhvL0LIireT0yE3ETZArGEmY5Ywz3FVHZ3LR5JTAE\" to \"enrtree-root:v1 e=AB66M4ULYD5OYN4XFFCPVZRLUM l=FDXN3SN67NA5DKA4J2GOK7BVQI seq=1 sig=H8cqDzu0FAzBplK4g3yudhSaNtszIebc2aj4oDm5a5ZE5PAg-xpCnQgVE_53CsgsqQpalD9byafx_FrUT61sagA\"" INFO [08-19|14:13:16.932] Updated DNS entries new=32 updated=1 untouched=100 INFO [08-19|14:13:16.932] Deleting stale DNS entries INFO [08-19|14:13:24.663] Deleted stale DNS entries count=31 ``` With this PR, the required permissions for deploying an ENR tree to Cloudflare now include `zone:read`, `zone:edit`, `dns:read`, and `dns:edit`. The initial check now includes all of the necessary permissions and indicates in the error message which permissions are missing: ``` INFO [08-19|14:17:20.339] Checking Permissions on zone REMOVED-ZONE wrong permissions on zone REMOVED-ZONE: map[#dns_records:edit:false #dns_records:read:false #zone:edit:false #zone:read:true] ```

Co-authored-by: lightclient <[email protected]>

… (#30242) This is a performance improvement on the account-creation rollback code required for the archive node to support verkle. It uses the utility function `DeleteAtStem` to remove code and account data per-group instead of doing it leaf by leaf. It also fixes an index bug, as code is chunked in 31-byte chunks, so comparing with the code size should use 31 as its stride. --------- Co-authored-by: Felix Lange <[email protected]>

Proper fix for handling `count=0` get header requests. https://en.wikipedia.org/wiki/Count_Zero

core/txpool/blobpool: return all reinject-addresses

…ue upon rollback" (#30521) Reverts ethereum/go-ethereum#30495 You are free to create a proper Clear method if that's the best way. But one that does a proper cleanup, not some hacky call to set gas which screws up logs, metrics and everything along the way. Also doesn't work for legacy pool local transactions. The current code had a hack in the simulated code, now we have a hack in live txpooling code. No, that's not acceptable. I want the live code to be proper, meaningful API, meaningful comments, meaningful implementation.

Release Geth v1.14.10

…rtins/upstream_merge_v1.14.10

…merge_v1.14.10

manav2401

Few minor nits. I am yet to review a few files.

core/blockchain.go

manav2401 · 2025-02-13T15:17:03Z

core/state_processor.go

-		if interruptCtx != nil {
-			select {
-			case <-interruptCtx.Done():
-				return nil, nil, 0, interruptCtx.Err()
-			default:
-			}
-		}


I think we'll need this for commit interrupt (i.e. interrupting transactions in middle of execution if we're beyond deadline). Although I recall that it's handled in miner and evm so idk if it's needed here but might be wrong.

Yeah, completely agreed. I'll include it back an validate against tests. Any issue found I'll let you know

manav2401 · 2025-02-13T15:20:26Z

core/types/block.go

@@ -103,6 +111,9 @@ type Header struct {

 	// ParentBeaconRoot was added by EIP-4788 and is ignored in legacy headers.
 	ParentBeaconRoot *common.Hash `json:"parentBeaconBlockRoot" rlp:"optional"`
+
+	// RequestsHash was added by EIP-7685 and is ignored in legacy headers.
+	RequestsHash *common.Hash `json:"requestsRoot" rlp:"optional"`


Let's ensure this field is not set anywhere for bor else it can lead to bad block. Maybe deploying a devnet with 1 validator on old version and 1 on this branch should help.

We did some extra checks for withdrawals in bor (just for reference). I can help in checking this.

That's a nice concern. I'll accept this help checking this.

…merge_v1.14.10

cffls · 2025-02-14T19:02:38Z

core/blockchain.go

 		// Retrieve the parent block and it's state to execute on top
 		start := time.Now()

 		parent := it.previous()
 		if parent == nil {
 			parent = bc.GetHeader(block.ParentHash(), block.NumberU64()-1)
 		}
+		statedb, err := state.New(parent.Root, bc.statedb)


As discussed, I think the code block from this line to 2358 could be deleted. statedb created here isn't used. Instead, it is created as part of ProcessBlock.

100% agreed. After removing these lines below:

bor/core/blockchain.go

Lines 2343 to 2358 in 0873515

// If we are past Byzantium, enable prefetching to pull in trie node paths

// while processing transactions. Before Byzantium the prefetcher is mostly

// useless due to the intermediate root hashing after each transaction.

if bc.chainConfig.IsByzantium(block.Number()) {

// Generate witnesses either if we're self-testing, or if it's the

// only block being inserted. A bit crude, but witnesses are huge,

// so we refuse to make an entire chain of them.

if bc.vmConfig.StatelessSelfValidation || (makeWitness && len(chain) == 1) {

witness, err = stateless.NewWitness(block.Header(), bc)

if err != nil {

return nil, it.index, err

}

}

statedb.StartPrefetcher("chain", witness)

}

activeState = statedb

We basically solver the routine/memory leak while syncing the solution. But I'll now also remove these ones, as suggested:

bor/core/blockchain.go

Lines 2337 to 2341 in 0873515

statedb, err := state.New(parent.Root, bc.statedb)

if err != nil {

return nil, it.index, err

}

statedb.SetLogger(bc.logger)

cffls · 2025-02-14T19:12:07Z

core/state_processor.go

+	var requests types.Requests
+	if p.config.IsPrague(block.Number()) {
+		requests, err = ParseDepositLogs(allLogs, p.config)
+		if err != nil {
+			return nil, err
+		}
 	}


Let's disable this. Unlike Ethereum, bor doesn't have deposit contract.

Ok, thanks. Disabling this.

After disabling I also needed to remove a test case. See commits below:

1193fc4

f19812f

cffls · 2025-02-14T19:14:48Z

core/state_processor.go

+	if p.config.IsPrague(block.Number()) {
+		ProcessParentBlockHash(block.ParentHash(), vmenv, statedb)
+	}


We might want to add the same logic to parallel_state_processor.go as well.

Yes, it makes sense. Just double checking:

Do you think I should apply this on all changes related to this Pull Request?

Yes, I think you should. Just applying the changes to parallel_state_processor.go is sufficient (and you can try to reuse the function if possible).

cffls · 2025-02-14T19:19:31Z

core/vm/contracts.go

@@ -114,7 +118,7 @@ var PrecompiledContractsCancun = map[common.Address]PrecompiledContract{

 // PrecompiledContractsPrague contains the set of pre-compiled Ethereum
 // contracts used in the Prague release.
-var PrecompiledContractsPrague = map[common.Address]PrecompiledContract{
+var PrecompiledContractsPrague = PrecompiledContracts{


We want to add common.BytesToAddress([]byte{0x01, 0x00}): &p256Verify{}, (bor-specific pre-compile) to Prague as well. If possible, add a unit test for it and make sure this precompile is always available in future HFs.

Hum, that's really interest. I included a test on it. My approach to protect us in new HFs was looking to an hypothetical "too far" block over BorMainnetChainConfig config. Would you suggest something else?

bor/core/vm/contracts_test.go

Lines 444 to 460 in 9b0ca36

func TestPrecompiledP256VerifyAlwaysAvailableInHFs(t *testing.T) {

latestHfRules := params.BorMainnetChainConfig.Rules(big.NewInt(math.MaxInt64), true, 0)

precompiledP256VerifyAddress := common.BytesToAddress([]byte{0x01, 0x00})

addresses := ActivePrecompiles(latestHfRules)

addressFound := false

for _, addr := range addresses {

if addr == precompiledP256VerifyAddress {

addressFound = true

break

}

}

assert.Equal(t, true, addressFound)

preCompiledContracts := ActivePrecompiledContracts(latestHfRules)

assert.Equal(t, &p256Verify{}, preCompiledContracts[precompiledP256VerifyAddress])

}

I like this approach, thanks!

…14.10

fjl and others added 30 commits August 12, 2024 14:19

params: begin v1.14.9 release cycle

1cf3b5d

go.mod: remove github.com/julienschmidt/httprouter (#30290)

ab03c57

build: run 'go mod tidy' check as part of lint (#30291)

bc95452

core/txpool/blobpool: fix error message (#30247)

bd57f35

the validation process only checks for 'less than', which is inconsistent with the error output

go.mod: upgrade to pebble v1.1.2 (#30297)

2b9d198

Includes a fix for MIPS32 support. Pebble release: https://github.com/cockroachdb/pebble/releases/tag/v1.1.2 Key fix for mips32: cockroachdb/pebble@9f3904a (also the only change from v1.1.1.

.golangci.yml: remove lint warning for TxLookupLimit

2f2e5b0

eth/fetcher: always expect transaction metadata in announcement (#30288)

c4b01d8

This pull request drops the legacy transaction retrieval support from before eth68, adding the restrictions that transaction metadata must be provided along with the transaction announment.

eth/ethconfig: remove LES server config (#30298)

c356847

beacon/light: handle endpoint URL more gracefully (#30306)

43640f1

blsync was failing if the light endpoint it was provided ended with a `/`. This change should handle the joining more gracefully.

core: remove withdrawal length check for state processor (#30286)

09d889d

The withdrawal length is already verified by the beacon consensus package, so the check in the state processor is a duplicate.

build: remove mantic from ppa builds (#30322)

8486722

removes ppa-build for ubuntu `mantic`

accounts/abi: handle ABIs with contract type parameter (#30315)

65aaf52

convert parameter of type contract to the basic `address` type --------- Co-authored-by: Martin HS <[email protected]>

core/rawdb: drop MigrateTable (#30331)

3b48b16

These are the leftovers from #24028.

core/vm: reuse Memory instances (#30137)

fc88cea

This PR adds a sync.Pool to reuse instances of Memory in EVMInterpreter.

rpc: add timeout to rpc client Unsubscribe (#30318)

15fb0dc

Fixes #30156 This adds a repro of the linked issue. I fixed it by adding a timeout when issuing the call to unsubscribe.

all: clean up goerli flag and config (#30289)

3c37db7

Co-authored-by: lightclient <[email protected]>

cmd/utils,p2p: enable discv5 by default (#30327)

00294e9

travis.yml: use focal for builds (#30319)

2a534ee

eth/protocols/eth: handle zero-count header requests (#30305)

733fcbb

Proper fix for handling `count=0` get header requests. https://en.wikipedia.org/wiki/Count_Zero

karalabe and others added 6 commits September 27, 2024 13:05

Merge pull request #30518 from holiman/blobpool_fix

52a9d89

core/txpool/blobpool: return all reinject-addresses

params: release Geth v1.14.10

2695518

Merge pull request #30522 from ethereum/master

1015a42

Release Geth v1.14.10

Merge tag 'v1.14.10' into lmartins/upstream_merge_v1.14.10

fc2198b

Merge branch 'develop' into lmartins/upstream_merge_v1.14.10

3926e96

lucca30 requested a review from a team February 4, 2025 11:01

lucca30 changed the base branch from develop to upstream_merge_v1.14.8 February 4, 2025 11:02

lucca30 added 2 commits February 4, 2025 10:35

Merge remote-tracking branch 'origin/upstream_merge_v1.14.8' into lma…

26ec762

…rtins/upstream_merge_v1.14.10

fix test-integration

1bf49e2

lucca30 changed the title ~~Lmartins/upstream merge v1.14.10~~ Upstream merge v1.14.10 Feb 4, 2025

lucca30 added 5 commits February 4, 2025 11:38

fix on golang image dockerfile

af4a29d

keep gp to call always max

5db5e5d

fix global gas cap

13d6ac9

Merge remote-tracking branch 'origin/upstream_merge_v1.14.8' into lma…

0873515

…rtins/upstream_merge_v1.14.10

Merge remote-tracking branch 'origin/develop' into lmartins/upstream_…

5687bba

…merge_v1.14.10

cffls added the do not squash and merge This PR will be NOT be squashed and merged label Feb 11, 2025

manav2401 reviewed Feb 13, 2025

View reviewed changes

lucca30 changed the base branch from upstream_merge_v1.14.8 to develop February 14, 2025 06:44

lucca30 added 2 commits February 14, 2025 15:39

try removing byzantine prefetecher to solve goroutine leak

7cb7653

Merge remote-tracking branch 'origin/develop' into lmartins/upstream_…

92b79be

…merge_v1.14.10

cffls reviewed Feb 14, 2025

View reviewed changes

lucca30 and others added 8 commits February 16, 2025 16:49

including interrupt context back

fcdefd9

remove byzantine routine prefetcher

9367a9e

disabling requests

1193fc4

small fix building

1de5835

ensure latest HF is always with p256 verify

9b0ca36

removing tests on eip-6110 because bor has no deposit contracts

f19812f

miner: wait for next block before running commit interrupt tests

baea4af

Merge branch 'manav/fix-worker-test' into lmartins/upstream_merge_v1.…

39968d1

…14.10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream merge v1.14.10 #1433

Upstream merge v1.14.10 #1433

lucca30 commented Feb 4, 2025 •

edited

Loading

manav2401 left a comment

manav2401 Feb 13, 2025

lucca30 Feb 16, 2025

manav2401 Feb 13, 2025

manav2401 Feb 13, 2025

lucca30 Feb 16, 2025

cffls Feb 14, 2025

lucca30 Feb 16, 2025

cffls Feb 14, 2025

lucca30 Feb 16, 2025

lucca30 Feb 16, 2025

cffls Feb 14, 2025

lucca30 Feb 16, 2025

cffls Feb 18, 2025

cffls Feb 14, 2025

lucca30 Feb 16, 2025

cffls Feb 18, 2025

	// If we are past Byzantium, enable prefetching to pull in trie node paths
	// while processing transactions. Before Byzantium the prefetcher is mostly
	// useless due to the intermediate root hashing after each transaction.
	if bc.chainConfig.IsByzantium(block.Number()) {
	// Generate witnesses either if we're self-testing, or if it's the
	// only block being inserted. A bit crude, but witnesses are huge,
	// so we refuse to make an entire chain of them.
	if bc.vmConfig.StatelessSelfValidation \|\| (makeWitness && len(chain) == 1) {
	witness, err = stateless.NewWitness(block.Header(), bc)
	if err != nil {
	return nil, it.index, err
	}
	}
	statedb.StartPrefetcher("chain", witness)
	}
	activeState = statedb

	statedb, err := state.New(parent.Root, bc.statedb)
	if err != nil {
	return nil, it.index, err
	}
	statedb.SetLogger(bc.logger)

	func TestPrecompiledP256VerifyAlwaysAvailableInHFs(t *testing.T) {
	latestHfRules := params.BorMainnetChainConfig.Rules(big.NewInt(math.MaxInt64), true, 0)
	precompiledP256VerifyAddress := common.BytesToAddress([]byte{0x01, 0x00})

	addresses := ActivePrecompiles(latestHfRules)
	addressFound := false
	for _, addr := range addresses {
	if addr == precompiledP256VerifyAddress {
	addressFound = true
	break
	}
	}
	assert.Equal(t, true, addressFound)

	preCompiledContracts := ActivePrecompiledContracts(latestHfRules)
	assert.Equal(t, &p256Verify{}, preCompiledContracts[precompiledP256VerifyAddress])
	}

Upstream merge v1.14.10 #1433

Are you sure you want to change the base?

Upstream merge v1.14.10 #1433

Conversation

lucca30 commented Feb 4, 2025 • edited Loading

Description

Points of attention:

Changes

Checklist

Cross repository changes

Testing

manav2401 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucca30 commented Feb 4, 2025 •

edited

Loading