Skip to content

Commit 85be92f

Browse files
committed
commitment: address review comments
- BranchCache.Get returns a copy to prevent callers mutating cached data - Remove duplicate BranchCache() accessor; GetBranchCache() satisfies Trie interface - NewBranchPrefetcher takes ctx so goroutines exit on parent cancellation - branchFromCacheOrDB: return data, nil (err guaranteed nil at final return) - Remove dead ExperimentalConcurrentCommitment flag and all its wiring; concurrent trie is now always enabled unconditionally - Add BranchPrefetcher tests covering Start/Stop lifecycle, cache population, context cancellation, and Submit-after-Stop safety
1 parent 34e65bc commit 85be92f

11 files changed

Lines changed: 138 additions & 42 deletions

File tree

cmd/integration/commands/commitment.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,6 @@ func init() {
109109
withReset(cmdCommitmentRebuild)
110110
withSqueeze(cmdCommitmentRebuild)
111111
withBlock(cmdCommitmentRebuild)
112-
withConcurrentCommitment(cmdCommitmentRebuild)
113112
withUnwind(cmdCommitmentRebuild)
114113
withPruneTo(cmdCommitmentRebuild)
115114
withIntegrityChecks(cmdCommitmentRebuild)

cmd/integration/commands/flags.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"github.com/spf13/cobra"
2121

2222
"github.com/erigontech/erigon/cmd/utils"
23-
"github.com/erigontech/erigon/db/state/statecfg"
2423
"github.com/erigontech/erigon/node/cli"
2524
"github.com/erigontech/erigon/node/ethconfig"
2625
)
@@ -149,10 +148,6 @@ func withDataDir(cmd *cobra.Command) {
149148
must(cmd.MarkFlagDirname("chaindata"))
150149
}
151150

152-
func withConcurrentCommitment(cmd *cobra.Command) {
153-
cmd.Flags().BoolVar(&statecfg.ExperimentalConcurrentCommitment, utils.ExperimentalConcurrentCommitmentFlag.Name, utils.ExperimentalConcurrentCommitmentFlag.Value, utils.ExperimentalConcurrentCommitmentFlag.Usage)
154-
}
155-
156151
func withBatchSize(cmd *cobra.Command) {
157152
cmd.Flags().StringVar(&batchSizeStr, "batchSize", cli.BatchSizeFlag.Value, cli.BatchSizeFlag.Usage)
158153
}

cmd/utils/flags.go

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,11 +1088,6 @@ var (
10881088
Name: "polygon.wit-protocol",
10891089
Usage: "Enable WIT protocol for stateless witness data exchange (auto-enabled for Bor chains)",
10901090
}
1091-
ExperimentalConcurrentCommitmentFlag = cli.BoolFlag{
1092-
Name: "experimental.concurrent-commitment",
1093-
Usage: "EXPERIMENTAL: enables concurrent trie for commitment",
1094-
Value: false,
1095-
}
10961091
GDBMeFlag = cli.BoolFlag{
10971092
Name: "gdbme",
10981093
Usage: "restart erigon under gdb for debug purposes",
@@ -1922,11 +1917,6 @@ func SetEthConfig(ctx *cli.Context, nodeConfig *nodecfg.Config, cfg *ethconfig.C
19221917
cfg.AllowAA = ctx.Bool(AAFlag.Name)
19231918
cfg.Ethstats = ctx.String(EthStatsURLFlag.Name)
19241919

1925-
if ctx.Bool(ExperimentalConcurrentCommitmentFlag.Name) {
1926-
// cfg.ExperimentalConcurrentCommitment = true
1927-
statecfg.ExperimentalConcurrentCommitment = true
1928-
}
1929-
19301920
cfg.FcuTimeout = ctx.Duration(FcuTimeoutFlag.Name)
19311921
cfg.FcuBackgroundPrune = ctx.Bool(FcuBackgroundPruneFlag.Name)
19321922
cfg.FcuBackgroundCommit = ctx.Bool(FcuBackgroundCommitFlag.Name)

db/state/statecfg/state_schema.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,6 @@ func (s *SchemaGen) GetBlockIdxFilesCfg(name string) BlockIdxFilesCfg {
187187
return v
188188
}
189189

190-
var ExperimentalConcurrentCommitment = false // set true to use concurrent commitment by default
191-
192190
var Schema = SchemaGen{
193191
AccountsDomain: DomainCfg{
194192
Name: kv.AccountsDomain, ValuesTable: kv.TblAccountVals,

execution/commitment/backtester/backtester.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ import (
3838
"github.com/erigontech/erigon/db/kv/rawdbv3"
3939
"github.com/erigontech/erigon/db/services"
4040
"github.com/erigontech/erigon/db/state/execctx"
41-
"github.com/erigontech/erigon/db/state/statecfg"
4241
"github.com/erigontech/erigon/execution/commitment"
4342
"github.com/erigontech/erigon/execution/commitment/commitmentdb"
4443
)
@@ -192,9 +191,6 @@ func (bt Backtester) backtestBlock(ctx context.Context, tx kv.TemporalTx, block
192191
}
193192
toTxNum := maxTxNum + 1
194193
bt.logger.Info("backtesting block commitment", "fromTxNum", fromTxNum, "toTxNum", toTxNum, "paraTrie", bt.paraTrie)
195-
if bt.paraTrie {
196-
statecfg.ExperimentalConcurrentCommitment = true
197-
}
198194
sd, err := execctx.NewSharedDomains(ctx, tx, bt.logger)
199195
if err != nil {
200196
return err

execution/commitment/branch_cache.go

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -91,28 +91,29 @@ func CompactKeyIsPinned(key []byte) bool {
9191
return false
9292
}
9393

94-
// Get retrieves branch data from the cache. Returns nil, false on miss.
94+
// Get retrieves a copy of branch data from the cache. Returns nil, false on miss.
95+
// A copy is returned so callers may not modify the cached entry.
9596
// Compact key format: byte[0] bit 4 (0x10) = odd nibble count, bits 0-3 = first nibble.
9697
func (c *BranchCache) Get(key []byte) ([]byte, bool) {
98+
var data []byte
99+
var found bool
100+
97101
if CompactKeyIsPinned(key) {
98102
c.mu.RLock()
99-
data, found := c.getPinned(key)
103+
data, found = c.getPinned(key)
100104
c.mu.RUnlock()
101-
if found {
102-
c.hits.Add(1)
103-
} else {
104-
c.misses.Add(1)
105-
}
106-
return data, found
105+
} else {
106+
data, found = c.branches.Get(key)
107107
}
108108

109-
data, found := c.branches.Get(key)
110109
if found {
111110
c.hits.Add(1)
112-
} else {
113-
c.misses.Add(1)
111+
cp := make([]byte, len(data))
112+
copy(cp, data)
113+
return cp, true
114114
}
115-
return data, found
115+
c.misses.Add(1)
116+
return nil, false
116117
}
117118

118119
// getPinned returns data from the pinned tier arrays. Caller must hold mu.RLock.

execution/commitment/branch_prefetcher.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,11 @@ type BranchPrefetcher struct {
2929
}
3030

3131
// NewBranchPrefetcher creates a prefetcher that populates the given BranchCache.
32+
// The prefetcher's goroutine lifecycle is tied to ctx: if ctx is cancelled the
33+
// workers exit even without an explicit Stop() call. Call Stop() for orderly drain.
3234
// Call Start() to begin processing, then Submit() hashed keys as they arrive.
33-
func NewBranchPrefetcher(cache *BranchCache, ctxFactory TrieContextFactory, numWorkers, maxDepth int) *BranchPrefetcher {
34-
ctx, cancel := context.WithCancel(context.Background())
35+
func NewBranchPrefetcher(ctx context.Context, cache *BranchCache, ctxFactory TrieContextFactory, numWorkers, maxDepth int) *BranchPrefetcher {
36+
ctx, cancel := context.WithCancel(ctx)
3537
return &BranchPrefetcher{
3638
cache: cache,
3739
ctxFactory: ctxFactory,
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
package commitment
2+
3+
import (
4+
"context"
5+
"testing"
6+
"time"
7+
8+
"github.com/stretchr/testify/require"
9+
10+
"github.com/erigontech/erigon/db/kv"
11+
)
12+
13+
// mockPatriciaContext implements PatriciaContext for testing.
14+
type mockPatriciaContext struct {
15+
branches map[string][]byte
16+
}
17+
18+
func (m *mockPatriciaContext) Branch(prefix []byte) ([]byte, kv.Step, error) {
19+
// Return stub branch data (>=4 bytes) so prefetchBranches doesn't stop early.
20+
// Use the map when present, otherwise return a generic stub.
21+
if data, ok := m.branches[string(prefix)]; ok {
22+
return data, 0, nil
23+
}
24+
return []byte("stub-branch-data"), 0, nil
25+
}
26+
27+
func (m *mockPatriciaContext) Account(plainKey []byte) (*Update, error) { return nil, nil }
28+
func (m *mockPatriciaContext) Storage(plainKey []byte) (*Update, error) { return nil, nil }
29+
func (m *mockPatriciaContext) PutBranch(prefix []byte, data []byte, prevData []byte) error {
30+
return nil
31+
}
32+
func (m *mockPatriciaContext) TxNum() uint64 { return 0 }
33+
func (m *mockPatriciaContext) Variant() TrieVariant { return VariantHexPatriciaTrie }
34+
35+
func TestBranchPrefetcher_StartStop(t *testing.T) {
36+
cache := NewBranchCache(1024)
37+
factory := func() (PatriciaContext, func()) {
38+
return &mockPatriciaContext{branches: map[string][]byte{}}, nil
39+
}
40+
41+
ctx := context.Background()
42+
p := NewBranchPrefetcher(ctx, cache, factory, 2, 4)
43+
p.Start()
44+
p.Stop()
45+
// Double-stop must be a no-op.
46+
p.Stop()
47+
}
48+
49+
func TestBranchPrefetcher_PopulatesCache(t *testing.T) {
50+
rootKey := HexNibblesToCompactBytes(nil) // depth-0 compact key
51+
depth1Key := HexNibblesToCompactBytes([]byte{3}) // nibble 3
52+
53+
branchData := []byte("some-branch-data-long-enough")
54+
55+
factory := func() (PatriciaContext, func()) {
56+
ctx := &mockPatriciaContext{
57+
branches: map[string][]byte{
58+
string(rootKey): branchData,
59+
string(depth1Key): branchData,
60+
},
61+
}
62+
return ctx, nil
63+
}
64+
65+
cache := NewBranchCache(1024)
66+
ctx := context.Background()
67+
p := NewBranchPrefetcher(ctx, cache, factory, 2, 4)
68+
p.Start()
69+
70+
// Submit a plain key — prefetcher will hash it and walk ancestor compact keys.
71+
// Use an all-zero 32-byte key whose first nibble is 0 → depth-1 compact key [0x10].
72+
plainKey := make([]byte, 32)
73+
p.SubmitPlainKey(plainKey)
74+
75+
// Give workers time to process.
76+
deadline := time.Now().Add(2 * time.Second)
77+
for time.Now().Before(deadline) {
78+
if p.Prefetched() > 0 {
79+
break
80+
}
81+
time.Sleep(10 * time.Millisecond)
82+
}
83+
84+
p.Stop()
85+
require.Positive(t, p.Prefetched(), "expected at least one branch prefetched")
86+
}
87+
88+
func TestBranchPrefetcher_ContextCancellation(t *testing.T) {
89+
factory := func() (PatriciaContext, func()) {
90+
return &mockPatriciaContext{branches: map[string][]byte{}}, nil
91+
}
92+
93+
ctx, cancel := context.WithCancel(context.Background())
94+
cache := NewBranchCache(1024)
95+
p := NewBranchPrefetcher(ctx, cache, factory, 2, 4)
96+
p.Start()
97+
98+
// Cancel parent context — workers should exit without an explicit Stop().
99+
cancel()
100+
101+
// Stop should still be safe to call after context cancellation.
102+
p.Stop()
103+
}
104+
105+
func TestBranchPrefetcher_SubmitAfterStop(t *testing.T) {
106+
factory := func() (PatriciaContext, func()) {
107+
return &mockPatriciaContext{branches: map[string][]byte{}}, nil
108+
}
109+
110+
cache := NewBranchCache(1024)
111+
p := NewBranchPrefetcher(context.Background(), cache, factory, 1, 4)
112+
p.Start()
113+
p.Stop()
114+
115+
// Submit after Stop must not panic.
116+
p.Submit(make([]byte, 8))
117+
p.SubmitPlainKey(make([]byte, 32))
118+
}

execution/commitment/commitmentdb/commitment_context.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ func (sdc *SharedDomainsCommitmentContext) StartBranchPrefetcher(ctx context.Con
9595

9696
txNum := sdc.sharedDomains.TxNum()
9797
factory := sdc.trieContextFactory(ctx, sdc.paraTrieDB, txNum)
98-
sdc.branchPrefetcher = commitment.NewBranchPrefetcher(branchCache, factory, 4, commitment.WarmupMaxDepth)
98+
sdc.branchPrefetcher = commitment.NewBranchPrefetcher(ctx, branchCache, factory, 4, commitment.WarmupMaxDepth)
9999
sdc.branchPrefetcher.Start()
100100

101101
// Wire the callback so TouchPlainKey feeds hashed keys to the prefetcher

execution/commitment/hex_patricia_hashed.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2734,7 +2734,6 @@ func (hph *HexPatriciaHashed) SetTrace(trace bool) { hph.trace = t
27342734
func (hph *HexPatriciaHashed) SetTraceDomain(trace bool) { hph.traceDomain = trace }
27352735
func (hph *HexPatriciaHashed) EnableWarmupCache(enable bool) { hph.enableWarmupCache = enable }
27362736
func (hph *HexPatriciaHashed) SetBranchCache(cache *BranchCache) { hph.branchCache = cache }
2737-
func (hph *HexPatriciaHashed) BranchCache() *BranchCache { return hph.branchCache }
27382737
func (hph *HexPatriciaHashed) GetBranchCache() *BranchCache { return hph.branchCache }
27392738

27402739
func (hph *HexPatriciaHashed) GetCapture(truncate bool) []string {
@@ -2825,7 +2824,7 @@ func (hph *HexPatriciaHashed) branchFromCacheOrDB(key []byte) ([]byte, error) {
28252824
}
28262825
return data, nil
28272826
}
2828-
}
2827+
}
28292828

28302829
// Level 2: persistent branch cache (survives across Process calls)
28312830
if hph.branchCache != nil {
@@ -2851,7 +2850,7 @@ func (hph *HexPatriciaHashed) branchFromCacheOrDB(key []byte) ([]byte, error) {
28512850
hph.branchCache.Put(key, data)
28522851
}
28532852

2854-
return data, err
2853+
return data, nil
28552854
}
28562855

28572856
// accountFromCacheOrDB reads account data from cache if available, otherwise from DB.

0 commit comments

Comments
 (0)