Add offset support to MemoryFreezer#1965
Conversation
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
LarryArnault45
force-pushed
the
memory-freezer-offset
branch
from
0fab17e to
6097d13
Compare
|
|
Hi @pratikspatil024, I fixed the CI issues. Could you approve the workflow runs when you get a chance? Thanks! |
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
@LarryArnault45 - could you push an empty commit? |
There was a problem hiding this comment.
Pull request overview
This PR adds pruning-offset support to the in-memory ancient store (MemoryFreezer) so it can represent an ancient DB that starts at a non-zero block number (e.g., after pruning older ancients), aligning behavior with the file-based freezer’s offset concept.
Changes:
- Extend
NewMemoryFreezerto accept anoffsetand initialize internal counters accordingly. - Apply the offset in memory freezer write sequencing and in
Ancient,TruncateHead, andTruncateTail. - Update call sites and tests to use the new
NewMemoryFreezersignature.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
core/rawdb/freezer_memory.go |
Adds offset parameter + uses it in read/write/truncation paths for the in-memory freezer. |
core/rawdb/freezer_memory_test.go |
Updates tests to the new constructor signature. |
core/rawdb/chain_freezer.go |
Passes the pruning offset into the in-memory freezer when running without a datadir. |
core/rawdb/ancient_scheme.go |
Updates state freezer’s in-memory path to pass offset 0. |
Comments suppressed due to low confidence (1)
core/rawdb/freezer_memory.go:268
Ancientnow subtractsf.offsetfromnumberwithout guardingnumber < offset. With unsigned arithmetic this underflows and relies on downstream bounds checks to fail; it also makes the error cause less clear. Consider explicitly returningerrOutOfBounds(or a dedicated error) whennumber < f.offset.Load(), and ensureAncientRangeapplies the same offset semantics for consistency (currently it passesstartthrough unchanged).
func (f *MemoryFreezer) Ancient(kind string, number uint64) ([]byte, error) {
f.lock.RLock()
defer f.lock.RUnlock()
t := f.tables[kind]
if t == nil {
return nil, errUnknownTable
}
data, err := t.retrieve(number-f.offset.Load(), 1, 0)
if err != nil {
return nil, err
}
return data[0], nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| return NewMemoryFreezer(false, tables) | ||
| return NewMemoryFreezer(false, 0, tables) | ||
| }) | ||
| } |
| // NewMemoryFreezer initializes an in-memory freezer instance. | ||
| func NewMemoryFreezer(readonly bool, tableName map[string]freezerTableConfig) *MemoryFreezer { | ||
| func NewMemoryFreezer(readonly bool, offset uint64, tableName map[string]freezerTableConfig) *MemoryFreezer { | ||
| tables := make(map[string]*memoryTable) | ||
| for name, cfg := range tableName { | ||
| tables[name] = newMemoryTable(name, cfg) | ||
| } | ||
| return &MemoryFreezer{ | ||
| freezer := &MemoryFreezer{ | ||
| writeBatch: newMemoryBatch(), | ||
| readonly: readonly, | ||
| tables: tables, | ||
| } | ||
|
|
||
| freezer.offset.Store(offset) | ||
|
|
||
| // Some blocks in ancientDB may have already been frozen and been pruned, so adding the offset to | ||
| // represent the absolute number of blocks already frozen. | ||
| freezer.items += offset | ||
|
|
||
| return freezer | ||
| } |
| return old, nil | ||
| } | ||
| for _, table := range f.tables { | ||
| if err := table.truncateHead(items); err != nil { | ||
| if err := table.truncateHead(items - f.offset.Load()); err != nil { | ||
| return 0, err | ||
| } | ||
| } |
| } | ||
| for _, table := range f.tables { | ||
| if table.config.prunable { | ||
| if err := table.truncateTail(tail); err != nil { | ||
| if err := table.truncateTail(tail - f.offset.Load()); err != nil { | ||
| return 0, err | ||
| } | ||
| } |
|
@pratikspatil024 , did it, sir. |
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
@LarryArnault45 - could you fix the build? |
|
@pratikspatil024 WIll do. |
|
|
Hi @LarryArnault45 - thanks for the fixes. The CI looks good now, but now there are some additional changes in github workflows, docker files, and go files which are not related. |
|
@pratikspatil024 Done. |
|
@claude review once |
|
Claude finished @pratikspatil024's task in 4m 18s —— View job Review of PR #1965: Add offset support to MemoryFreezer
SummaryThis PR adds pruning-offset support to the in-memory freezer ( Overall this is a clean, well-tested change. One bug, one minor issue, and a few nits below. Bug:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if t == nil { | ||
| return nil, errUnknownTable | ||
| } | ||
| data, err := t.retrieve(number, 1, 0) | ||
| offset := f.offset.Load() | ||
| if number < offset { | ||
| return nil, errOutOfBounds | ||
| } | ||
| data, err := t.retrieve(number-offset, 1, 0) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🔴 MemoryFreezer.AncientBytes (core/rawdb/freezer_memory.go:467-490) was not updated to be offset-aware, while this PR adds offset translation to Ancient and AncientRange. With offset > 0, AncientBytes silently disagrees with Ancient/AncientRange: e.g. AncientBytes("test", 12, 0, n) on a freezer with offset=10 and items at absolute ids 10..12 hits the t.items<=start guard and returns errOutOfBounds, while AncientBytes("test", 2, 0, n) — below the absolute start — would silently return data for what is actually item 12. Fix: subtract f.offset.Load() from id with the same underflow guard the PR adds for Ancient/AncientRange/TruncateHead/TruncateTail, and add a TestMemoryFreezerOffset case for AncientBytes.
Extended reasoning...
The bug
This PR makes MemoryFreezer offset-aware, updating Ancient, AncientRange, TruncateHead, TruncateTail, ModifyAncients, Reset and the write-batch initializer to translate between absolute item numbers and table-relative indices via f.offset.Load(). However, the parallel reader MemoryFreezer.AncientBytes (core/rawdb/freezer_memory.go:467-490) — which is not in the diff — was not updated. It still calls table.retrieve(id, 1, 0) with the absolute id.
After this PR, AncientBytes is the only AncientReaderOp method left non-offset-aware, so a MemoryFreezer constructed with offset > 0 produces inconsistent results across its reader methods.
Step-by-step proof
Construct f := NewMemoryFreezer(false, 10, {"test": …}) and append three items at absolute ids 10, 11, 12 (matching the new TestMemoryFreezerOffset). After commit, the underlying memoryTable has t.items = 3 and t.offset = 0 (the memoryTable.items field is incremented only by commit and starts at 0 — unlike the freezer-level f.items which now includes the offset).
f.Ancient("test", 12)→ updated path:number=12,offset=10, callst.retrieve(12-10, 1, 0) = t.retrieve(2, 1, 0)→ returns"twelve". ✓f.AncientBytes("test", 12, 0, n)→ unchanged path: callst.retrieve(12, 1, 0)→ trips thet.items <= startguard (3 <= 12) → returnserrOutOfBounds. ✗ (disagrees withAncient)f.AncientBytes("test", 2, 0, n)(an id below the freezer's absolute start of 10) → callst.retrieve(2, 1, 0)→ silently returns"twelve"(the entry at relative index 2, i.e. absolute id 12). ✗ (worse: wrong data, not even an error)
Why existing code does not prevent it
The new TestMemoryFreezerOffset exercises Ancient, AncientRange, TruncateHead, TruncateTail, and Reset against offset=10, but does not call AncientBytes, so this gap is not caught. chainFreezer.AncientBytes (core/rawdb/chain_freezer.go:451) is a direct pass-through, and newChainFreezer now wires a non-zero offset into NewMemoryFreezer for the in-memory chain freezer case, so the inconsistency is reachable through the public ethdb.AncientStore interface even if no current caller of AncientBytes happens to use an offset != 0 today (state/trienode constructors still pass 0; chain reads currently route through Ancient, not AncientBytes).
Impact
Latent today (no current production caller triggers it), but a real correctness/consistency bug introduced by this PR's offset refactor. Any future caller — or any future code path that routes chain-table reads through AncientBytes while the chain freezer is configured with a pruning offset — would either silently read the wrong item (when id < offset) or get a spurious errOutOfBounds (when id >= offset). The contract that all reader methods agree on absolute ids is broken on the same struct.
Suggested fix
Apply the same translation and underflow guard already used for Ancient and AncientRange:
func (f *MemoryFreezer) AncientBytes(kind string, id, offset, length uint64) ([]byte, error) {
f.lock.RLock()
defer f.lock.RUnlock()
table := f.tables[kind]
if table == nil {
return nil, errUnknownTable
}
tableOffset := f.offset.Load()
if id < tableOffset {
return nil, errOutOfBounds
}
entries, err := table.retrieve(id-tableOffset, 1, 0)
// … rest unchanged
}And extend TestMemoryFreezerOffset with AncientBytes("test", 12, …) succeeding and AncientBytes("test", 9, …) returning errOutOfBounds, mirroring the existing Ancient/AncientRange assertions.
|
Thanks @LarryArnault45 - I would like to have the 2 issues raised by claude addressed in the PR. Could you do that? |
|
@pratikspatil024 sure, will do. |
|
MemoryFreezer was missing offset handling, causing issues when used with ancient database pruning.