From 4ac82c2cf0e60baddc6b00f9f7797fdcc892783e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Thu, 6 Jun 2024 15:43:07 +0300 Subject: [PATCH] cmd, core/state: fix nil panic, fix error handling, prefetch nosnap too --- cmd/geth/main.go | 2 +- cmd/utils/flags.go | 7 +++--- core/blockchain.go | 2 +- core/state/state_object.go | 13 +++++++---- core/state/statedb.go | 17 +++++++++----- core/state/trie_prefetcher.go | 36 +++++++++++++----------------- core/state/trie_prefetcher_test.go | 6 ++--- 7 files changed, 46 insertions(+), 37 deletions(-) diff --git a/cmd/geth/main.go b/cmd/geth/main.go index fab83ac929..f6bb09ee54 100644 --- a/cmd/geth/main.go +++ b/cmd/geth/main.go @@ -156,7 +156,7 @@ var ( utils.BeaconGenesisRootFlag, utils.BeaconGenesisTimeFlag, utils.BeaconCheckpointFlag, - utils.DebugCollectWitnessFlag, + utils.CollectWitnessFlag, }, utils.NetworkFlags, utils.DatabaseFlags) rpcFlags = []cli.Flag{ diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 81e40c1b41..ab917f42ca 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -604,8 +604,8 @@ var ( Usage: "Disables db compaction after import", Category: flags.LoggingCategory, } - DebugCollectWitnessFlag = &cli.BoolFlag{ - Name: "collectwitnesses", + CollectWitnessFlag = &cli.BoolFlag{ + Name: "collectwitness", Usage: "Enable state witness generation during block execution. Work in progress flag, don't use.", Category: flags.MiscCategory, } @@ -2209,7 +2209,8 @@ func MakeChain(ctx *cli.Context, stack *node.Node, readonly bool) (*core.BlockCh vmcfg.Tracer = t } } - vmcfg.EnableWitnessCollection = ctx.Bool(DebugCollectWitnessFlag.Name) + vmcfg.EnableWitnessCollection = ctx.Bool(CollectWitnessFlag.Name) + // Disable transaction indexing/unindexing by default. chain, err := core.NewBlockChain(chainDb, cache, gspec, nil, engine, vmcfg, nil, nil) if err != nil { diff --git a/core/blockchain.go b/core/blockchain.go index a9047619c1..ac4eb1c47e 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -1809,7 +1809,7 @@ func (bc *BlockChain) insertChain(chain types.Blocks, setHead bool) (int, error) // 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()) { - statedb.StartPrefetcher("chain", bc.vmConfig.EnableWitnessCollection) + statedb.StartPrefetcher("chain", !bc.vmConfig.EnableWitnessCollection) } activeState = statedb diff --git a/core/state/state_object.go b/core/state/state_object.go index bc5817ea51..16a5f3e5e2 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -212,9 +212,6 @@ func (s *stateObject) GetCommittedState(key common.Hash) common.Hash { } value.SetBytes(content) } - if s.data.Root != types.EmptyRootHash { - s.db.prefetcher.prefetchWitness(s.addrHash, s.origin.Root, s.address, [][]byte{key[:]}) - } } // If the snapshot is unavailable or reading from it fails, load from the database. if s.db.snap == nil || err != nil { @@ -233,6 +230,14 @@ func (s *stateObject) GetCommittedState(key common.Hash) common.Hash { } value.SetBytes(val) } + // Independent of where we loaded the data from, add it to the prefetcher. + // Whilst this would be a bit weird if snapshots are disabled, but we still + // want the trie nodes to end up in the prefetcher too, so just push through. + if s.db.prefetcher != nil && s.data.Root != types.EmptyRootHash { + if err = s.db.prefetcher.prefetch(s.addrHash, s.origin.Root, s.address, [][]byte{key[:]}, true); err != nil { + log.Error("Failed to prefetch storage slot", "addr", s.address, "key", key, "err", err) + } + } s.originStorage[key] = value return value } @@ -296,7 +301,7 @@ func (s *stateObject) finalise() { s.pendingStorage[key] = value } if s.db.prefetcher != nil && len(slotsToPrefetch) > 0 && s.data.Root != types.EmptyRootHash { - if err := s.db.prefetcher.prefetch(s.addrHash, s.data.Root, s.address, slotsToPrefetch); err != nil { + if err := s.db.prefetcher.prefetch(s.addrHash, s.data.Root, s.address, slotsToPrefetch, false); err != nil { log.Error("Failed to prefetch slots", "addr", s.address, "slots", len(slotsToPrefetch), "err", err) } } diff --git a/core/state/statedb.go b/core/state/statedb.go index 160a2a0782..b4ef0a6e47 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -200,14 +200,14 @@ func (s *StateDB) SetLogger(l *tracing.Hooks) { // StartPrefetcher initializes a new trie prefetcher to pull in nodes from the // state trie concurrently while the state is mutated so that when we reach the // commit phase, most of the needed data is already hot. -func (s *StateDB) StartPrefetcher(namespace string, collectWitnesses bool) { +func (s *StateDB) StartPrefetcher(namespace string, noreads bool) { if s.prefetcher != nil { s.prefetcher.terminate(false) s.prefetcher.report() s.prefetcher = nil } if s.snap != nil { - s.prefetcher = newTriePrefetcher(s.db, s.originalRoot, namespace, collectWitnesses) + s.prefetcher = newTriePrefetcher(s.db, s.originalRoot, namespace, noreads) // With the switch to the Proof-of-Stake consensus algorithm, block production // rewards are now handled at the consensus layer. Consequently, a block may @@ -218,7 +218,7 @@ func (s *StateDB) StartPrefetcher(namespace string, collectWitnesses bool) { // To prevent this, the account trie is always scheduled for prefetching once // the prefetcher is constructed. For more details, see: // https://github.com/ethereum/go-ethereum/issues/29880 - if err := s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, nil); err != nil { + if err := s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, nil, false); err != nil { log.Error("Failed to prefetch account trie", "root", s.originalRoot, "err", err) } } @@ -587,7 +587,6 @@ func (s *StateDB) getStateObject(addr common.Address) *stateObject { if acc == nil { return nil } - s.prefetcher.prefetchWitness(common.Hash{}, s.originalRoot, common.Address{}, [][]byte{addr[:]}) data = &types.StateAccount{ Nonce: acc.Nonce, Balance: acc.Balance, @@ -617,6 +616,14 @@ func (s *StateDB) getStateObject(addr common.Address) *stateObject { return nil } } + // Independent of where we loaded the data from, add it to the prefetcher. + // Whilst this would be a bit weird if snapshots are disabled, but we still + // want the trie nodes to end up in the prefetcher too, so just push through. + if s.prefetcher != nil { + if err := s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, [][]byte{addr[:]}, true); err != nil { + log.Error("Failed to prefetch account", "addr", addr, "err", err) + } + } // Insert into the live set obj := newObject(s, addr, data) s.setStateObject(obj) @@ -793,7 +800,7 @@ func (s *StateDB) Finalise(deleteEmptyObjects bool) { addressesToPrefetch = append(addressesToPrefetch, common.CopyBytes(addr[:])) // Copy needed for closure } if s.prefetcher != nil && len(addressesToPrefetch) > 0 { - if err := s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, addressesToPrefetch); err != nil { + if err := s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, addressesToPrefetch, false); err != nil { log.Error("Failed to prefetch addresses", "addresses", len(addressesToPrefetch), "err", err) } } diff --git a/core/state/trie_prefetcher.go b/core/state/trie_prefetcher.go index 07d13623b6..2fcf6a06b0 100644 --- a/core/state/trie_prefetcher.go +++ b/core/state/trie_prefetcher.go @@ -40,11 +40,11 @@ var ( // // Note, the prefetcher's API is not thread safe. type triePrefetcher struct { - db Database // Database to fetch trie nodes through - root common.Hash // Root hash of the account trie for metrics - fetchers map[string]*subfetcher // Subfetchers for each trie - term chan struct{} // Channel to signal interruption - collectWitnesses bool // whether to allow prefetch calls for witness collection + db Database // Database to fetch trie nodes through + root common.Hash // Root hash of the account trie for metrics + fetchers map[string]*subfetcher // Subfetchers for each trie + term chan struct{} // Channel to signal interruption + noreads bool // Whether to ignore state-read-only prefetch requests deliveryMissMeter metrics.Meter accountLoadMeter metrics.Meter @@ -55,14 +55,14 @@ type triePrefetcher struct { storageWasteMeter metrics.Meter } -func newTriePrefetcher(db Database, root common.Hash, namespace string, collectWitnesses bool) *triePrefetcher { +func newTriePrefetcher(db Database, root common.Hash, namespace string, noreads bool) *triePrefetcher { prefix := triePrefetchMetricsPrefix + namespace return &triePrefetcher{ - db: db, - root: root, - fetchers: make(map[string]*subfetcher), // Active prefetchers use the fetchers map - term: make(chan struct{}), - collectWitnesses: collectWitnesses, + db: db, + root: root, + fetchers: make(map[string]*subfetcher), // Active prefetchers use the fetchers map + term: make(chan struct{}), + noreads: noreads, deliveryMissMeter: metrics.GetOrRegisterMeter(prefix+"/deliverymiss", nil), accountLoadMeter: metrics.GetOrRegisterMeter(prefix+"/account/load", nil), @@ -128,7 +128,11 @@ func (p *triePrefetcher) report() { // upon the same contract, the parameters invoking this method may be // repeated. // 2. Finalize of the main account trie. This happens only once per block. -func (p *triePrefetcher) prefetch(owner common.Hash, root common.Hash, addr common.Address, keys [][]byte) error { +func (p *triePrefetcher) prefetch(owner common.Hash, root common.Hash, addr common.Address, keys [][]byte, read bool) error { + // If the state item is only being read, but reads are disabled, return + if read && p.noreads { + return nil + } // Ensure the subfetcher is still alive select { case <-p.term: @@ -144,14 +148,6 @@ func (p *triePrefetcher) prefetch(owner common.Hash, root common.Hash, addr comm return fetcher.schedule(keys) } -// prefetchWitness calls prefetch if witness collection is enabled. -func (p *triePrefetcher) prefetchWitness(owner common.Hash, root common.Hash, addr common.Address, keys [][]byte) error { - if p.collectWitnesses { - return p.prefetch(owner, root, addr, keys) - } - return nil -} - // trie returns the trie matching the root hash, blocking until the fetcher of // the given trie terminates. If no fetcher exists for the request, nil will be // returned. diff --git a/core/state/trie_prefetcher_test.go b/core/state/trie_prefetcher_test.go index 478407dfbb..8f01acd221 100644 --- a/core/state/trie_prefetcher_test.go +++ b/core/state/trie_prefetcher_test.go @@ -47,15 +47,15 @@ func filledStateDB() *StateDB { func TestUseAfterTerminate(t *testing.T) { db := filledStateDB() - prefetcher := newTriePrefetcher(db.db, db.originalRoot, "") + prefetcher := newTriePrefetcher(db.db, db.originalRoot, "", true) skey := common.HexToHash("aaa") - if err := prefetcher.prefetch(common.Hash{}, db.originalRoot, common.Address{}, [][]byte{skey.Bytes()}); err != nil { + if err := prefetcher.prefetch(common.Hash{}, db.originalRoot, common.Address{}, [][]byte{skey.Bytes()}, false); err != nil { t.Errorf("Prefetch failed before terminate: %v", err) } prefetcher.terminate(false) - if err := prefetcher.prefetch(common.Hash{}, db.originalRoot, common.Address{}, [][]byte{skey.Bytes()}); err == nil { + if err := prefetcher.prefetch(common.Hash{}, db.originalRoot, common.Address{}, [][]byte{skey.Bytes()}, false); err == nil { t.Errorf("Prefetch succeeded after terminate: %v", err) } if tr := prefetcher.trie(common.Hash{}, db.originalRoot); tr == nil {