From d0ec28c5dbab0fd3e37bc944f5d82f300b2aa162 Mon Sep 17 00:00:00 2001 From: lupin012 <58134934+lupin012@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:14:03 +0200 Subject: [PATCH] db/state: don't clear domain RAM under a published SharedDomains (#23046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the high-severity finding from the #22987 review. **Problem.** RPC read views keep a `DomainReader` pointing at the published SD's in-memory domain maps. The background-commit teardown (`bgSD.Close()` → `mem.Close()` → `ClearRam()`) emptied those maps while readers were still using them. A receipt read of the in-flight block then missed silently, fell back to the request's pre-commit tx, and `ReceiptAsOf` zero-filled the miss: `GetReceiptsGasUsed` returned `GasUsed=0` for every tx of the head block, and `eth_feeHistory` reward percentiles were silently wrong. Latest-state reads could likewise fall back to the previous block's state mid-request. **Fix.** `TemporalMemBatch.Close` no longer clears the in-memory domain maps: they go to the GC once the last reference drops. With that, `ClearRam` had no production caller left and is removed entirely — the batch has a single lifetime (write, maybe publish, close-and-drop) and no API can clear the maps under readers. The one internal test that used clear-and-reuse now mirrors what `cmd/integration` actually does today: a fresh `SharedDomains` per batch. **Tests.** New `TestClose_KeepsDomainRamForReaders` (red before the fix, green after). One existing assert updated: a post-teardown view now keeps serving the published head instead of falling back to its own tx. #22987 (draft) depends on this PR: pinning the overlay across `Fork` makes this window easier to hit, so that PR stays a draft until this one is merged. --- db/kv/kv_interface.go | 1 - db/state/execctx/close_keeps_ram_test.go | 65 +++++++++++++++++ db/state/execctx/domain_shared.go | 13 +--- .../statecache_rpc_integration_test.go | 2 +- db/state/temporal_mem_batch.go | 29 +------- .../commitmentdb/commitment_context.go | 6 -- .../from0_genesis_internal_test.go | 71 +++++++++---------- execution/execmodule/forkchoice.go | 2 +- 8 files changed, 103 insertions(+), 86 deletions(-) create mode 100644 db/state/execctx/close_keeps_ram_test.go diff --git a/db/kv/kv_interface.go b/db/kv/kv_interface.go index 78c4fdd76ef..cc75136347a 100644 --- a/db/kv/kv_interface.go +++ b/db/kv/kv_interface.go @@ -584,7 +584,6 @@ type TemporalMemBatch interface { GetLatest(domain Domain, key []byte) (v []byte, step Step, ok bool) GetDiffset(tx RwTx, blockHash common.Hash, blockNumber uint64) ([DomainLen][]DomainEntryDiff, bool, error) Merge(other TemporalMemBatch) error - ClearRam() IndexAdd(table InvertedIdx, key []byte, txNum uint64) (err error) IteratePrefix(domain Domain, prefix []byte, roTx Tx, it func(k []byte, v []byte) (cont bool, err error)) error HasPrefix(domain Domain, prefix []byte, roTx Tx) ([]byte, []byte, bool, error) diff --git a/db/state/execctx/close_keeps_ram_test.go b/db/state/execctx/close_keeps_ram_test.go new file mode 100644 index 00000000000..353d3b73ae1 --- /dev/null +++ b/db/state/execctx/close_keeps_ram_test.go @@ -0,0 +1,65 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package execctx_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/common/log/v3" + "github.com/erigontech/erigon/db/rawdb/rawtemporaldb" + "github.com/erigontech/erigon/db/state/execctx" +) + +// TestClose_KeepsDomainRamForReaders pins the lifetime guarantee readers rely +// on: a read view keeps its DomainReader pointing at the SD's in-memory domain +// maps, so Close must release writer resources without clearing those maps — +// otherwise in-flight domain reads (e.g. the head block's receipts) silently +// miss and fall back to a tx that does not have the data yet. +func TestClose_KeepsDomainRamForReaders(t *testing.T) { + t.Parallel() + db := newTestDb(t, 16) + ctx := context.Background() + + tx, err := db.BeginTemporalRo(ctx) + require.NoError(t, err) + defer tx.Rollback() + + sd, err := execctx.NewSharedDomains(ctx, tx, log.New()) + require.NoError(t, err) + require.NoError(t, sd.InitBlockOverlay(tx, t.TempDir())) + + const txNum = 7 + const cumGas = 21000 + require.NoError(t, rawtemporaldb.AppendReceiptMetadata(sd.AsPutDel(tx), 0, cumGas, 0, txNum)) + + view := sd.BlockOverlayTemporalTx(tx) + require.NotNil(t, view) + + assertReceiptVisible := func(msg string) { + got, _, _, err := rawtemporaldb.ReceiptAsOf(view, txNum+1) + require.NoError(t, err) + require.Equal(t, uint64(cumGas), got, msg) + } + assertReceiptVisible("the in-flight receipt must be visible through the view before Close") + + sd.Close() + + assertReceiptVisible("Close must not clear the domain RAM readers still hold") +} diff --git a/db/state/execctx/domain_shared.go b/db/state/execctx/domain_shared.go index b670d5460e5..932cae9c18f 100644 --- a/db/state/execctx/domain_shared.go +++ b/db/state/execctx/domain_shared.go @@ -889,15 +889,6 @@ func (sd *SharedDomains) PrintCacheStats() { } } -func (sd *SharedDomains) ClearRam(resetCommitment bool) { - // When the commitment calculator goroutine owns the Updates buffer, - // skip ClearRam on the commitment context to avoid concurrent btree access. - if resetCommitment && sd.sdCtx != nil && !sd.disableInlineTouchKey { - sd.sdCtx.ClearRam() - } - sd.mem.ClearRam() -} - func (sd *SharedDomains) Size() uint64 { return sd.mem.SizeEstimate() } @@ -1800,8 +1791,8 @@ func (sd *SharedDomains) DomainDelPrefix(domain kv.Domain, roTx kv.TemporalTx, p return nil } -// DiscardWrites disables updates collection for further flushing into db. -// Instead, it keeps them temporarily available until .ClearRam/.Close will make them unavailable. +// DiscardWrites disables updates collection for further flushing into db; +// the values stay readable in memory. func (sd *SharedDomains) DiscardWrites(d kv.Domain) { // TODO: Deprecated - need convert this method to Constructor-Builder configuration if d >= kv.DomainLen { diff --git a/db/state/execctx/statecache_rpc_integration_test.go b/db/state/execctx/statecache_rpc_integration_test.go index 789bea870df..aea67d930c3 100644 --- a/db/state/execctx/statecache_rpc_integration_test.go +++ b/db/state/execctx/statecache_rpc_integration_test.go @@ -335,7 +335,7 @@ func TestEmbeddedRPCCacheViewDoesNotRefillCodeOfDeletedAccount(t *testing.T) { got, err := rpcView.GetCode(addr) require.NoError(t, err) - require.Equal(t, code, got, "the pre-deletion view still reads the code from its own tx") + require.Empty(t, got, "the view keeps serving the published SD's state after teardown, so the deletion stays visible") _, ok = stateCache.View(nil).Get(kv.CodeDomain, addr) require.False(t, ok, "a pre-deletion RPC view must not refill the deleted account's code") diff --git a/db/state/temporal_mem_batch.go b/db/state/temporal_mem_batch.go index a7aa5d2c3df..e8efb326da2 100644 --- a/db/state/temporal_mem_batch.go +++ b/db/state/temporal_mem_batch.go @@ -360,32 +360,6 @@ func (sd *TemporalMemBatch) SizeEstimate() uint64 { return uint64(sd.metrics.CachePutSize) } -func (sd *TemporalMemBatch) ClearRam() { - sd.latestStateLock.Lock() - defer sd.latestStateLock.Unlock() - for i := range sd.domains { - sd.domains[i] = map[string][]dataWithTxNum{} - } - - sd.storage = btree2.NewMap[string, []dataWithTxNum](128) - sd.unwindToTxNum = 0 - sd.unwindChangeset = nil - sd.unwindChangesetRaw = nil - - sd.metrics.Lock() - defer sd.metrics.Unlock() - sd.metrics.CachePutCount = 0 - sd.metrics.CachePutSize = 0 - sd.metrics.CachePutKeySize = 0 - sd.metrics.CachePutValueSize = 0 - for _, dm := range sd.metrics.Domains { - dm.CachePutCount = 0 - dm.CachePutSize = 0 - dm.CachePutKeySize = 0 - dm.CachePutValueSize = 0 - } -} - func (sd *TemporalMemBatch) IteratePrefix(domain kv.Domain, prefix []byte, roTx kv.Tx, it func(k []byte, v []byte) (cont bool, err error)) error { sd.latestStateLock.RLock() defer sd.latestStateLock.RUnlock() @@ -631,6 +605,8 @@ func (sd *TemporalMemBatch) IndexAdd(table kv.InvertedIdx, key []byte, txNum uin panic(fmt.Errorf("unknown index %s", table)) } +// Close releases writer resources but must not clear the in-memory maps: +// readers of a published SD may still hold them. func (sd *TemporalMemBatch) Close() { for _, d := range sd.domainWriters { if d != nil { @@ -648,7 +624,6 @@ func (sd *TemporalMemBatch) Close() { for _, iiWriter := range sd.pastIIWriters { iiWriter.close() } - sd.ClearRam() } func (sd *TemporalMemBatch) Merge(o kv.TemporalMemBatch) error { diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 0bae9353f09..6652339a70f 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -278,12 +278,6 @@ func (sdc *SharedDomainsCommitmentContext) Reset() { } } -func (sdc *SharedDomainsCommitmentContext) ClearRam() { - sdc.updates.Reset() - sdc.Reset() - sdc.stateReader = nil -} - func (sdc *SharedDomainsCommitmentContext) KeysCount() uint64 { return sdc.updates.Size() } diff --git a/execution/execmodule/execmoduletester/from0_genesis_internal_test.go b/execution/execmodule/execmoduletester/from0_genesis_internal_test.go index b15e6117014..c0759eb7983 100644 --- a/execution/execmodule/execmoduletester/from0_genesis_internal_test.go +++ b/execution/execmodule/execmoduletester/from0_genesis_internal_test.go @@ -135,10 +135,10 @@ func runFromZeroGenesisAllocPreservedAfterResetReExec(t *testing.T) { // Mimic `stage_exec --reset`: wipe domain tables and reset stage progress. require.NoError(t, rawdbreset.ResetExec(ctx, emt.DB)) - // Now drive execution the SAME way cmd/integration/commands/stages.go:802 - // does — direct SpawnExecuteBlocksStage in a loop, with Flush/ClearRam/ - // Commit between iterations. This is the path that fails in CI; the - // engine-API InsertChain path above succeeds. + // Now drive execution the SAME way cmd/integration/commands/stages.go + // does — direct SpawnExecuteBlocksStage in a loop, one rwtx and one + // SharedDomains per batch, each committed with doms.Commit. This is the + // path that fails in CI; the engine-API InsertChain path above succeeds. require.NoError(t, reExecViaIntegrationPath(t, ctx, emt, gen.TopBlock.NumberU64(), emt.cfg.BatchSize, false /*badBlockHalt*/, logger)) postReExec := checkBalance("after-reset-and-integration-reexec") @@ -176,24 +176,18 @@ func setupOfflineExec(emt *ExecModuleTester, batchSize datasize.ByteSize, badBlo } // reExecViaIntegrationPath drives execution the way cmd/integration/commands/ -// stages.go does: SpawnExecuteBlocksStage one batch per rwtx, committing each -// with doms.Commit + ClearRam and reusing the SharedDomains. This bypasses the -// engine API. doms.Commit (not Flush) is load-bearing: it refreshes the -// aggregator BranchCache to match committed state — Flush leaves it stale and -// corrupts the next batch's trie root. +// stages.go does: SpawnExecuteBlocksStage one batch per rwtx and per +// SharedDomains, each committed with doms.Commit. This bypasses the engine +// API. doms.Commit (not Flush) is load-bearing: it refreshes the aggregator +// BranchCache to match committed state — Flush leaves it stale and corrupts +// the next batch's trie root. func reExecViaIntegrationPath(t *testing.T, ctx context.Context, emt *ExecModuleTester, toBlock uint64, batchSize datasize.ByteSize, badBlockHalt bool, logger log.Logger) error { t.Helper() cfg := setupOfflineExec(emt, batchSize, badBlockHalt) - doms, err := newReusedDomains(ctx, emt, logger) - if err != nil { - return err - } - defer doms.Close() - for { - progress, err := execOneBatch(ctx, emt, doms, cfg, toBlock, logger) + progress, err := execOneBatch(ctx, emt, cfg, toBlock, logger) if err != nil { return err } @@ -203,34 +197,23 @@ func reExecViaIntegrationPath(t *testing.T, ctx context.Context, emt *ExecModule } } -// newReusedDomains opens a SharedDomains seeded from committed state. The seeding -// tx is rolled back right away: each batch re-seeks commitment under its own tx, -// and the SharedDomains keeps no reference to the tx it was built from. -func newReusedDomains(ctx context.Context, emt *ExecModuleTester, logger log.Logger) (*execctx.SharedDomains, error) { - tx, err := emt.DB.BeginTemporalRo(ctx) +// execOneBatch runs a single batch in its own rwtx and its own SharedDomains +// (a fresh one per call avoids reusing a committed, spent one — same as the +// integration tool). doms.Commit commits the tx and refreshes the BranchCache. +// Returns the Execution stage progress after the batch. +func execOneBatch(ctx context.Context, emt *ExecModuleTester, cfg stagedsync.ExecuteBlockCfg, toBlock uint64, logger log.Logger) (uint64, error) { + tx, err := emt.DB.BeginTemporalRw(ctx) if err != nil { - return nil, err + return 0, err } defer tx.Rollback() doms, err := execctx.NewSharedDomains(ctx, tx, logger) - if err != nil { - return nil, err - } - doms.SetInMemHistoryReads(false) - return doms, nil -} - -// execOneBatch runs a single batch in its own rwtx (begin/rollback-on-error/ -// commit), reusing doms. doms.Commit commits the tx and refreshes the BranchCache; -// ClearRam drops the flushed batch so doms is clean for the next call. Returns the -// Execution stage progress after the batch. -func execOneBatch(ctx context.Context, emt *ExecModuleTester, doms *execctx.SharedDomains, cfg stagedsync.ExecuteBlockCfg, toBlock uint64, logger log.Logger) (uint64, error) { - tx, err := emt.DB.BeginTemporalRw(ctx) if err != nil { return 0, err } - defer tx.Rollback() + defer doms.Close() + doms.SetInMemHistoryReads(false) s, err := emt.Sync.StageState(stages.Execution, tx, true, false) if err != nil { @@ -249,7 +232,6 @@ func execOneBatch(ctx context.Context, emt *ExecModuleTester, doms *execctx.Shar if err := doms.Commit(ctx, tx); err != nil { return 0, err } - doms.ClearRam(true) return progress, nil } @@ -360,13 +342,24 @@ func TestExec_RestoresCommitmentStateReader(t *testing.T) { cfg := setupOfflineExec(emt, emt.cfg.BatchSize, false /*badBlockHalt*/) - doms, err := newReusedDomains(ctx, emt, logger) + tx, err := emt.DB.BeginTemporalRw(ctx) + require.NoError(t, err) + defer tx.Rollback() + + doms, err := execctx.NewSharedDomains(ctx, tx, logger) require.NoError(t, err) defer doms.Close() + doms.SetInMemHistoryReads(false) readerBefore := doms.GetCommitmentContext().StateReader() - _, err = execOneBatch(ctx, emt, doms, cfg, gen.TopBlock.NumberU64(), logger) + + s, err := emt.Sync.StageState(stages.Execution, tx, true, false) require.NoError(t, err) + err = stagedsync.SpawnExecuteBlocksStage(s, emt.Sync, doms, tx, gen.TopBlock.NumberU64(), ctx, cfg, logger) + if err != nil && !errors.Is(err, &stagedsync.ErrLoopExhausted{}) { + require.NoError(t, err) + } + require.NoError(t, doms.Commit(ctx, tx)) require.Equal(t, readerBefore, doms.GetCommitmentContext().StateReader(), "exec must restore the commitment state reader it found; leaving the parallel calculator's asOfStateReader installed breaks a later foreground SeekCommitment with in-mem history reads disabled") diff --git a/execution/execmodule/forkchoice.go b/execution/execmodule/forkchoice.go index f67f0d8b20d..1fc6a033ccc 100644 --- a/execution/execmodule/forkchoice.go +++ b/execution/execmodule/forkchoice.go @@ -575,7 +575,7 @@ func (e *ExecModule) updateForkChoice(ctx context.Context, originalBlockHash, sa } defer commitRwTx.Rollback() // idempotent after a successful Commit // The committed sd is spent; RunLoop closes it and continues on the - // fresh SD built below (no ClearRam reuse). + // fresh SD built below (no reuse). if err := sd.Commit(ctx, commitRwTx); err != nil { return nil, nil, fmt.Errorf("updateForkChoice: flush+commit sd after hasMore: %w", err) }