From 52a6bae8f62f72dccaa98fd408f1d2f1188143a3 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 26 Aug 2026 17:04:06 +0700 Subject: [PATCH 1/3] execution/commitment: let trie warmup read branches without copying them TrieContext.Branch copies every branch it reads, because the trie's own consumers retain the bytes past the read: Merge, the encoder and merger buffers, and the deferred-update queues. Warmup retains nothing -- it pulls the child bitmap, a byte of field bits per cell and an extension length, all scalars, to pick the next nibble -- yet it pays the same copy, and it issues several times more branch reads than the fold does. WarmupBranch hands back the read uncopied. The bytes stay valid for the life of the context's transaction: mdbx guarantees that for a read, mmapped .kv pages are pinned by it, and the mem batch and the branch/state caches all store heap-owned values. Callers that keep branch data stay on Branch. Selected once per warmup worker rather than per key, so the interface assertion and the method value stay off the per-key path. --- execution/commitment/commitment.go | 7 +++ .../commitmentdb/commitment_context.go | 21 +++++++- execution/commitment/warmuper.go | 14 ++++-- execution/commitment/warmuper_test.go | 50 +++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/execution/commitment/commitment.go b/execution/commitment/commitment.go index dce122acc6a..46273fb748d 100644 --- a/execution/commitment/commitment.go +++ b/execution/commitment/commitment.go @@ -121,6 +121,13 @@ type PatriciaContext interface { Storage(plainKey []byte) (*Update, error) } +// BranchWarmer reads a branch for trie warmup, which only needs the bytes to pick +// the next nibble and never keeps them. Unlike Branch it does not copy, so the +// result stays valid only as long as the context's transaction. +type BranchWarmer interface { + WarmupBranch(prefix []byte) ([]byte, kv.Step, error) +} + type TrieVariant string const ( diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 982fb0ab4c4..93436f75742 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -954,7 +954,7 @@ func NewTrieContextRo(reader StateReader, stepSize uint64) *TrieContext { } func (sdc *TrieContext) Branch(pref []byte) ([]byte, kv.Step, error) { - enc, step, err := sdc.readDomain(kv.CommitmentDomain, pref) + enc, step, err := sdc.branch(pref) if err != nil { return nil, 0, err } @@ -963,10 +963,27 @@ func (sdc *TrieContext) Branch(pref []byte) ([]byte, kv.Step, error) { // underlying state cache / getter aliases shared storage that another goroutine // (concurrent commitment workers) can recycle. Own the bytes at the trie-context // boundary so all downstream consumers are safe. + return bytes.Clone(enc), step, nil +} + +// WarmupBranch returns the branch bytes uncopied, for trie warmup. They stay +// valid for the life of this context's transaction -- mdbx guarantees that much +// for a read, mmapped .kv pages are pinned by it, and the mem batch and the +// branch/state caches all hand out heap-owned values -- but not past it, which is +// why anything that keeps branch data uses Branch. +func (sdc *TrieContext) WarmupBranch(pref []byte) ([]byte, kv.Step, error) { + return sdc.branch(pref) +} + +func (sdc *TrieContext) branch(pref []byte) ([]byte, kv.Step, error) { + enc, step, err := sdc.readDomain(kv.CommitmentDomain, pref) + if err != nil { + return nil, 0, err + } if sdc.traceW != nil { fmt.Fprintf(sdc.traceW, "[SDC] Branch read %x => %x\n", pref, enc) } - return bytes.Clone(enc), step, nil + return enc, step, nil } func (sdc *TrieContext) PutBranch(prefix []byte, data []byte, prevData []byte) error { diff --git a/execution/commitment/warmuper.go b/execution/commitment/warmuper.go index 4af7bd3eb82..d341881fa5f 100644 --- a/execution/commitment/warmuper.go +++ b/execution/commitment/warmuper.go @@ -29,6 +29,7 @@ import ( "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/log/v3" + "github.com/erigontech/erigon/db/kv" "github.com/erigontech/erigon/execution/commitment/nibbles" ) @@ -115,6 +116,11 @@ func (w *Warmuper) Start() { return errors.New("warmup trie context factory returned nil PatriciaContext") } + readBranch := trieCtx.Branch + if w, ok := trieCtx.(BranchWarmer); ok { + readBranch = w.WarmupBranch + } + for { select { case <-w.ctx.Done(): @@ -123,7 +129,7 @@ func (w *Warmuper) Start() { if !ok { return nil } - w.warmupKey(trieCtx, item.hashedKey, item.startDepth) + w.warmupKey(readBranch, item.hashedKey, item.startDepth) w.keysProcessed.Add(1) w.releaseGen(item.gen) } @@ -141,13 +147,15 @@ func (w *Warmuper) Start() { }) } -func (w *Warmuper) warmupKey(trieCtx PatriciaContext, hashedKey []byte, startDepth int) { +// warmupKey descends one key, parsing each branch only to pick the next nibble. +// It keeps nothing, so the caller can pass the non-copying read. +func (w *Warmuper) warmupKey(readBranch func([]byte) ([]byte, kv.Step, error), hashedKey []byte, startDepth int) { depth := startDepth var compactBuf [maxCompactKeyLen]byte for depth <= len(hashedKey) && depth <= w.maxDepth { prefix := nibbles.HexToCompactInto(compactBuf[:], hashedKey[:depth]) - branchData, _, err := trieCtx.Branch(prefix) + branchData, _, err := readBranch(prefix) if err != nil { log.Debug(fmt.Sprintf("[%s][warmup] failed to get branch", w.logPrefix), "prefix", common.Bytes2Hex(prefix), "error", err) diff --git a/execution/commitment/warmuper_test.go b/execution/commitment/warmuper_test.go index 4b1ab31b301..36a35d128a0 100644 --- a/execution/commitment/warmuper_test.go +++ b/execution/commitment/warmuper_test.go @@ -17,10 +17,16 @@ package commitment import ( + "bytes" "context" "sync" + "sync/atomic" "testing" "time" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/db/kv" ) func TestWarmuperFactoryMustNotOutliveCloseAndWait(t *testing.T) { @@ -255,3 +261,47 @@ func TestCloseLeavesWorkChannelOpen(t *testing.T) { default: } } + +// countingBranchCtx records which read path the warmuper took. Branch models the +// owning read (it copies); WarmupBranch hands back the source slice itself. +type countingBranchCtx struct { + src []byte + owned atomic.Int64 + borrowed atomic.Int64 +} + +func (c *countingBranchCtx) Branch(prefix []byte) ([]byte, kv.Step, error) { + c.owned.Add(1) + return bytes.Clone(c.src), 0, nil +} + +func (c *countingBranchCtx) WarmupBranch(prefix []byte) ([]byte, kv.Step, error) { + c.borrowed.Add(1) + return c.src, 0, nil +} + +func (c *countingBranchCtx) PutBranch(prefix, data, prevData []byte) error { return nil } +func (c *countingBranchCtx) Account(plainKey []byte) (*Update, error) { return nil, nil } +func (c *countingBranchCtx) Storage(plainKey []byte) (*Update, error) { return nil, nil } + +// TestWarmuperUsesWarmupBranch pins the warmuper on the non-copying read. It keeps +// nothing it reads, and it issues several times more branch reads than the fold +// does, so an owning read there is the bulk of the copying. +func TestWarmuperUsesWarmupBranch(t *testing.T) { + t.Parallel() + // touchMap, afterMap with nibble 0 set, then one cell with no fields. + ctx := &countingBranchCtx{src: []byte{0x00, 0x01, 0x00, 0x01, 0x00}} + w := NewWarmuper(context.Background(), WarmupConfig{ + Enabled: true, + CtxFactory: func(context.Context) (PatriciaContext, func()) { return ctx, nil }, + NumWorkers: 1, + MaxDepth: WarmupMaxDepth, + }) + w.Start() + w.WarmKey([]byte{0, 0, 0, 0}, 0, 0) + require.NoError(t, w.WaitBufferFree(0)) + w.CloseAndWait() + + require.Positive(t, ctx.borrowed.Load(), "warmup read branches through the copying path") + require.Zero(t, ctx.owned.Load(), "warmup still copies branch bytes it drops immediately") +} From f1d94692623767ecbf7f979cb449eec01586ddfe Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Thu, 27 Aug 2026 09:30:20 +0700 Subject: [PATCH 2/3] execution/commitment: rename WarmupBranch to BranchNoCopy The name now says what it does. The doc says what the caller owes: the result aliases the reader's memory and must not be retained or mutated. Branch copies because the trie's consumers do both, not because the source is short-lived. --- execution/commitment/commitment.go | 10 +++++----- .../commitment/commitmentdb/commitment_context.go | 10 ++++------ execution/commitment/warmuper.go | 4 ++-- execution/commitment/warmuper_test.go | 8 ++++---- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/execution/commitment/commitment.go b/execution/commitment/commitment.go index 46273fb748d..09ffcaeb030 100644 --- a/execution/commitment/commitment.go +++ b/execution/commitment/commitment.go @@ -121,11 +121,11 @@ type PatriciaContext interface { Storage(plainKey []byte) (*Update, error) } -// BranchWarmer reads a branch for trie warmup, which only needs the bytes to pick -// the next nibble and never keeps them. Unlike Branch it does not copy, so the -// result stays valid only as long as the context's transaction. -type BranchWarmer interface { - WarmupBranch(prefix []byte) ([]byte, kv.Step, error) +// BranchNoCopyReader reads a branch without copying it, for a caller that reads +// the bytes and keeps nothing. The result is not owned: it must not be retained +// or mutated. Branch copies because the trie's consumers do both. +type BranchNoCopyReader interface { + BranchNoCopy(prefix []byte) ([]byte, kv.Step, error) } type TrieVariant string diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 93436f75742..2d80d7329da 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -966,12 +966,10 @@ func (sdc *TrieContext) Branch(pref []byte) ([]byte, kv.Step, error) { return bytes.Clone(enc), step, nil } -// WarmupBranch returns the branch bytes uncopied, for trie warmup. They stay -// valid for the life of this context's transaction -- mdbx guarantees that much -// for a read, mmapped .kv pages are pinned by it, and the mem batch and the -// branch/state caches all hand out heap-owned values -- but not past it, which is -// why anything that keeps branch data uses Branch. -func (sdc *TrieContext) WarmupBranch(pref []byte) ([]byte, kv.Step, error) { +// BranchNoCopy returns the branch bytes uncopied, for a caller that reads them +// and keeps nothing -- the trie warmup. The result aliases the reader's memory +// and is not owned: it must not be retained or mutated. +func (sdc *TrieContext) BranchNoCopy(pref []byte) ([]byte, kv.Step, error) { return sdc.branch(pref) } diff --git a/execution/commitment/warmuper.go b/execution/commitment/warmuper.go index d341881fa5f..af1bd6431ce 100644 --- a/execution/commitment/warmuper.go +++ b/execution/commitment/warmuper.go @@ -117,8 +117,8 @@ func (w *Warmuper) Start() { } readBranch := trieCtx.Branch - if w, ok := trieCtx.(BranchWarmer); ok { - readBranch = w.WarmupBranch + if w, ok := trieCtx.(BranchNoCopyReader); ok { + readBranch = w.BranchNoCopy } for { diff --git a/execution/commitment/warmuper_test.go b/execution/commitment/warmuper_test.go index 36a35d128a0..e9145bd80fb 100644 --- a/execution/commitment/warmuper_test.go +++ b/execution/commitment/warmuper_test.go @@ -263,7 +263,7 @@ func TestCloseLeavesWorkChannelOpen(t *testing.T) { } // countingBranchCtx records which read path the warmuper took. Branch models the -// owning read (it copies); WarmupBranch hands back the source slice itself. +// owning read (it copies); BranchNoCopy hands back the source slice itself. type countingBranchCtx struct { src []byte owned atomic.Int64 @@ -275,7 +275,7 @@ func (c *countingBranchCtx) Branch(prefix []byte) ([]byte, kv.Step, error) { return bytes.Clone(c.src), 0, nil } -func (c *countingBranchCtx) WarmupBranch(prefix []byte) ([]byte, kv.Step, error) { +func (c *countingBranchCtx) BranchNoCopy(prefix []byte) ([]byte, kv.Step, error) { c.borrowed.Add(1) return c.src, 0, nil } @@ -284,10 +284,10 @@ func (c *countingBranchCtx) PutBranch(prefix, data, prevData []byte) error { ret func (c *countingBranchCtx) Account(plainKey []byte) (*Update, error) { return nil, nil } func (c *countingBranchCtx) Storage(plainKey []byte) (*Update, error) { return nil, nil } -// TestWarmuperUsesWarmupBranch pins the warmuper on the non-copying read. It keeps +// TestWarmuperUsesBranchNoCopy pins the warmuper on the non-copying read. It keeps // nothing it reads, and it issues several times more branch reads than the fold // does, so an owning read there is the bulk of the copying. -func TestWarmuperUsesWarmupBranch(t *testing.T) { +func TestWarmuperUsesBranchNoCopy(t *testing.T) { t.Parallel() // touchMap, afterMap with nibble 0 set, then one cell with no fields. ctx := &countingBranchCtx{src: []byte{0x00, 0x01, 0x00, 0x01, 0x00}} From 92614857e66a488e8be26ebefa494acdc90f7035 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Thu, 27 Aug 2026 10:28:35 +0700 Subject: [PATCH 3/3] execution/commitment: state BranchNoCopy's contract as valid-until-next-call The tighter promise leaves the reader free to hand back a reused buffer later. The fake now poisons what the previous call returned, so a caller that keeps the bytes across a read descends differently and the test fails. --- execution/commitment/commitment.go | 7 ++++--- .../commitmentdb/commitment_context.go | 7 ++++--- execution/commitment/warmuper_test.go | 16 ++++++++++++---- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/execution/commitment/commitment.go b/execution/commitment/commitment.go index 09ffcaeb030..102c3b01102 100644 --- a/execution/commitment/commitment.go +++ b/execution/commitment/commitment.go @@ -121,9 +121,10 @@ type PatriciaContext interface { Storage(plainKey []byte) (*Update, error) } -// BranchNoCopyReader reads a branch without copying it, for a caller that reads -// the bytes and keeps nothing. The result is not owned: it must not be retained -// or mutated. Branch copies because the trie's consumers do both. +// BranchNoCopyReader reads a branch without copying it. The result aliases the +// reader's memory: it is valid until the next call on the same context, and must +// not be mutated. Branch copies because the trie's consumers keep branch data +// past that. type BranchNoCopyReader interface { BranchNoCopy(prefix []byte) ([]byte, kv.Step, error) } diff --git a/execution/commitment/commitmentdb/commitment_context.go b/execution/commitment/commitmentdb/commitment_context.go index 2d80d7329da..9f393046626 100644 --- a/execution/commitment/commitmentdb/commitment_context.go +++ b/execution/commitment/commitmentdb/commitment_context.go @@ -966,9 +966,10 @@ func (sdc *TrieContext) Branch(pref []byte) ([]byte, kv.Step, error) { return bytes.Clone(enc), step, nil } -// BranchNoCopy returns the branch bytes uncopied, for a caller that reads them -// and keeps nothing -- the trie warmup. The result aliases the reader's memory -// and is not owned: it must not be retained or mutated. +// BranchNoCopy returns the branch bytes uncopied, for a caller that consumes them +// before its next read -- the trie warmup. The result is valid only until the +// next call on this context and must not be mutated; anything that keeps branch +// data uses Branch. func (sdc *TrieContext) BranchNoCopy(pref []byte) ([]byte, kv.Step, error) { return sdc.branch(pref) } diff --git a/execution/commitment/warmuper_test.go b/execution/commitment/warmuper_test.go index e9145bd80fb..2cf3b9001ff 100644 --- a/execution/commitment/warmuper_test.go +++ b/execution/commitment/warmuper_test.go @@ -262,10 +262,12 @@ func TestCloseLeavesWorkChannelOpen(t *testing.T) { } } -// countingBranchCtx records which read path the warmuper took. Branch models the -// owning read (it copies); BranchNoCopy hands back the source slice itself. +// countingBranchCtx records which read path the warmuper took, and holds +// BranchNoCopy to its contract: each call poisons what the previous one returned, +// so a caller that kept the bytes reads garbage and descends differently. type countingBranchCtx struct { src []byte + last []byte owned atomic.Int64 borrowed atomic.Int64 } @@ -277,7 +279,11 @@ func (c *countingBranchCtx) Branch(prefix []byte) ([]byte, kv.Step, error) { func (c *countingBranchCtx) BranchNoCopy(prefix []byte) ([]byte, kv.Step, error) { c.borrowed.Add(1) - return c.src, 0, nil + for i := range c.last { + c.last[i] = 0xff + } + c.last = bytes.Clone(c.src) + return c.last, 0, nil } func (c *countingBranchCtx) PutBranch(prefix, data, prevData []byte) error { return nil } @@ -302,6 +308,8 @@ func TestWarmuperUsesBranchNoCopy(t *testing.T) { require.NoError(t, w.WaitBufferFree(0)) w.CloseAndWait() - require.Positive(t, ctx.borrowed.Load(), "warmup read branches through the copying path") require.Zero(t, ctx.owned.Load(), "warmup still copies branch bytes it drops immediately") + // One read per nibble of the key, then one more that stops at its end. A + // caller holding a poisoned branch would read 0xff and stop short. + require.Equal(t, int64(5), ctx.borrowed.Load(), "warmup descent read the wrong number of branches") }