From 004526762b51f4270ce496f66a4dc2c7ac7cfa3b Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 28 Apr 2025 08:26:27 +0200 Subject: [PATCH 1/4] core/txpool/legacypool: refactor truncatePending (#31715) TruncatePending shows up bright red on our nodes, because it computes the length of a map multiple times. I don't know why this is so expensive, but around 20% of our time is spent on this, which is super weird. ``` //PR: BenchmarkTruncatePending-24 17498 69397 ns/op 32872 B/op 3 allocs/op //Master: BenchmarkTruncatePending-24 9960 123954 ns/op 32872 B/op 3 allocs/op ``` ``` benchmark old ns/op new ns/op delta BenchmarkTruncatePending-24 123954 69397 -44.01% benchmark old allocs new allocs delta BenchmarkTruncatePending-24 3 3 +0.00% benchmark old bytes new bytes delta BenchmarkTruncatePending-24 32872 32872 +0.00% ``` This simple PR is a 44% improvement over the old state ``` OUTINE ======================== github.com/ethereum/go-ethereum/core/txpool/legacypool.(*LegacyPool).truncatePending in github.com/ethereum/go-ethereum/core/txpool/legacypool/legacypool.go 1.96s 18.02s (flat, cum) 19.57% of Total . . 1495:func (pool *LegacyPool) truncatePending() { . . 1496: pending := uint64(0) 60ms 2.99s 1497: for _, list := range pool.pending { 250ms 5.48s 1498: pending += uint64(list.Len()) . . 1499: } . . 1500: if pending <= pool.config.GlobalSlots { . . 1501: return . . 1502: } . . 1503: . . 1504: pendingBeforeCap := pending . . 1505: // Assemble a spam order to penalize large transactors first . 510ms 1506: spammers := prque.New[int64, common.Address](nil) 140ms 2.50s 1507: for addr, list := range pool.pending { . . 1508: // Only evict transactions from high rollers 50ms 5.08s 1509: if uint64(list.Len()) > pool.config.AccountSlots { . . 1510: spammers.Push(addr, int64(list.Len())) . . 1511: } . . 1512: } . . 1513: // Gradually drop transactions from offenders . . 1514: offenders := []common.Address{} ``` ```go // Benchmarks the speed of batch transaction insertion in case of multiple accounts. func BenchmarkTruncatePending(b *testing.B) { // Generate a batch of transactions to enqueue into the pool pool, _ := setupPool() defer pool.Close() b.ReportAllocs() batches := make(types.Transactions, 4096+1024+1) for i := range len(batches) { key, _ := crypto.GenerateKey() account := crypto.PubkeyToAddress(key.PublicKey) pool.currentState.AddBalance(account, uint256.NewInt(1000000), tracing.BalanceChangeUnspecified) tx := transaction(uint64(0), 100000, key) batches[i] = tx } for _, tx := range batches { pool.addRemotesSync([]*types.Transaction{tx}) } b.ResetTimer() // benchmark truncating the pending for range b.N { pool.truncatePending() } } ``` --- core/txpool/legacypool/legacypool.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/core/txpool/legacypool/legacypool.go b/core/txpool/legacypool/legacypool.go index 7bf360ff65..affe44cf06 100644 --- a/core/txpool/legacypool/legacypool.go +++ b/core/txpool/legacypool/legacypool.go @@ -1494,22 +1494,22 @@ func (pool *LegacyPool) promoteExecutables(accounts []common.Address) []*types.T // equal number for all for accounts with many pending transactions. func (pool *LegacyPool) truncatePending() { pending := uint64(0) - for _, list := range pool.pending { - pending += uint64(list.Len()) + + // Assemble a spam order to penalize large transactors first + spammers := prque.New[uint64, common.Address](nil) + for addr, list := range pool.pending { + // Only evict transactions from high rollers + length := uint64(list.Len()) + pending += length + if length > pool.config.AccountSlots { + spammers.Push(addr, length) + } } if pending <= pool.config.GlobalSlots { return } - pendingBeforeCap := pending - // Assemble a spam order to penalize large transactors first - spammers := prque.New[int64, common.Address](nil) - for addr, list := range pool.pending { - // Only evict transactions from high rollers - if uint64(list.Len()) > pool.config.AccountSlots { - spammers.Push(addr, int64(list.Len())) - } - } + // Gradually drop transactions from offenders offenders := []common.Address{} for pending > pool.config.GlobalSlots && !spammers.Empty() { From c8c8d6c4039a476478dfcfb732ba83822b648288 Mon Sep 17 00:00:00 2001 From: Martin HS Date: Mon, 28 Apr 2025 08:37:02 +0200 Subject: [PATCH 2/4] trie: add edgecase for rangeproof correctness (#31667) This PR adds checking for an edgecase which theoretically can happen in the range-prover. Right now, we check that a key does not overwrite a previous one by checking that the key is increasing. However, if keys are of different lengths, it is possible to create a key which is increasing _and_ overwrites the previous key. Example: `0xaabbcc` followed by `0xaabbccdd`. This can not happen in go-ethereum, which always uses fixed-size paths for accounts and storage slot paths in the trie, but it might happen if the range prover is used without guaranteed fixed-size keys. This PR also adds some testcases for the errors that are expected. --- trie/proof.go | 14 +++++++++++--- trie/proof_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/trie/proof.go b/trie/proof.go index 2e527348bf..751d6f620f 100644 --- a/trie/proof.go +++ b/trie/proof.go @@ -485,10 +485,18 @@ func VerifyRangeProof(rootHash common.Hash, firstKey []byte, keys [][]byte, valu if len(keys) != len(values) { return false, fmt.Errorf("inconsistent proof data, keys: %d, values: %d", len(keys), len(values)) } - // Ensure the received batch is monotonic increasing and contains no deletions + // Ensure the received batch is + // - monotonically increasing, + // - not expanding down prefix-paths + // - and contains no deletions for i := 0; i < len(keys); i++ { - if i < len(keys)-1 && bytes.Compare(keys[i], keys[i+1]) >= 0 { - return false, errors.New("range is not monotonically increasing") + if i < len(keys)-1 { + if bytes.Compare(keys[i], keys[i+1]) >= 0 { + return false, errors.New("range is not monotonically increasing") + } + if bytes.HasPrefix(keys[i+1], keys[i]) { + return false, errors.New("range contains path prefixes") + } } if len(values[i]) == 0 { return false, errors.New("range contains deletion") diff --git a/trie/proof_test.go b/trie/proof_test.go index fab3a97650..b3c9dd753c 100644 --- a/trie/proof_test.go +++ b/trie/proof_test.go @@ -1000,3 +1000,35 @@ func TestRangeProofKeysWithSharedPrefix(t *testing.T) { t.Error("expected more to be false") } } + +// TestRangeProofErrors tests a few cases where the prover is supposed +// to exit with errors +func TestRangeProofErrors(t *testing.T) { + // Different number of keys to values + _, err := VerifyRangeProof((common.Hash{}), []byte{}, make([][]byte, 5), make([][]byte, 4), nil) + if have, want := err.Error(), "inconsistent proof data, keys: 5, values: 4"; have != want { + t.Fatalf("wrong error, have %q, want %q", err.Error(), want) + } + // Non-increasing paths + _, err = VerifyRangeProof((common.Hash{}), []byte{}, + [][]byte{[]byte{2, 1}, []byte{2, 1}}, make([][]byte, 2), nil) + if have, want := err.Error(), "range is not monotonically increasing"; have != want { + t.Fatalf("wrong error, have %q, want %q", err.Error(), want) + } + // A prefixed path is never motivated. Inserting the second element will + // require rewriting/overwriting the previous value-node, thus can only + // happen if the data is corrupt. + _, err = VerifyRangeProof((common.Hash{}), []byte{}, + [][]byte{[]byte{2, 1}, []byte{2, 1, 2}}, + [][]byte{[]byte{1}, []byte{1}}, nil) + if have, want := err.Error(), "range contains path prefixes"; have != want { + t.Fatalf("wrong error, have %q, want %q", err.Error(), want) + } + // Empty values (deletions) + _, err = VerifyRangeProof((common.Hash{}), []byte{}, + [][]byte{[]byte{2, 1}, []byte{2, 2}}, + [][]byte{[]byte{1}, []byte{}}, nil) + if have, want := err.Error(), "range contains deletion"; have != want { + t.Fatalf("wrong error, have %q, want %q", err.Error(), want) + } +} From a511553e448c947a0fe8f34acf7bb6f9818c2b49 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Tue, 29 Apr 2025 08:29:56 +0200 Subject: [PATCH 3/4] core: apply overrides to mainnet chainconfig (#31733) This PR applies the config overrides to the new config as well, otherwise they will not be applied to defined configs, making shadowforks impossible. To test: ``` > ./build/bin/geth --override.prague 123 --dev --datadir /tmp/geth INFO [04-28|21:20:47.009] - Prague: @123 > ./build/bin/geth --override.prague 321 --dev --datadir /tmp/geth INFO [04-28|21:23:59.760] - Prague: @321 `` --- core/genesis.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/genesis.go b/core/genesis.go index 95782a827a..080f8a3c5f 100644 --- a/core/genesis.go +++ b/core/genesis.go @@ -365,6 +365,9 @@ func SetupGenesisBlockWithOverride(db ethdb.Database, triedb *triedb.Database, g return nil, common.Hash{}, nil, errors.New("missing head header") } newCfg := genesis.chainConfigOrDefault(ghash, storedCfg) + if err := overrides.apply(newCfg); err != nil { + return nil, common.Hash{}, nil, err + } // Sanity-check the new configuration. if err := newCfg.CheckConfigForkOrder(); err != nil { From 0ac4a84a1fdb9e54d27d0814179856a90e0c989f Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Tue, 29 Apr 2025 19:21:18 +0800 Subject: [PATCH 4/4] beacon/engine: omit empty witness in payload response (#31739) Fixes https://github.com/ethereum/go-ethereum/issues/31737 --- beacon/engine/types.go | 2 +- eth/catalyst/api.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/beacon/engine/types.go b/beacon/engine/types.go index 984090ef89..3e52933a90 100644 --- a/beacon/engine/types.go +++ b/beacon/engine/types.go @@ -131,7 +131,7 @@ type executionPayloadEnvelopeMarshaling struct { type PayloadStatusV1 struct { Status string `json:"status"` - Witness *hexutil.Bytes `json:"witness"` + Witness *hexutil.Bytes `json:"witness,omitempty"` LatestValidHash *common.Hash `json:"latestValidHash"` ValidationError *string `json:"validationError"` } diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index e6f29c970b..1e6981a76a 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -373,8 +373,11 @@ func (api *ConsensusAPI) forkchoiceUpdated(update engine.ForkchoiceStateV1, payl } valid := func(id *engine.PayloadID) engine.ForkChoiceResponse { return engine.ForkChoiceResponse{ - PayloadStatus: engine.PayloadStatusV1{Status: engine.VALID, LatestValidHash: &update.HeadBlockHash}, - PayloadID: id, + PayloadStatus: engine.PayloadStatusV1{ + Status: engine.VALID, + LatestValidHash: &update.HeadBlockHash, + }, + PayloadID: id, } } if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash {