eth/filters, core/filtermaps: safe chain view update #31590
Conversation
zsfelfoldi
requested review from
fjl,
s1na,
lightclient,
holiman and
rjl493456442
as code owners
zsfelfoldi
force-pushed
the
filter-head-update
branch
from
fd0eae9 to
6c7951a
Compare
zsfelfoldi
changed the title
zsfelfoldi
changed the title
zsfelfoldi
force-pushed
the
filter-head-update
branch
2 times, most recently
from
888367a to
844e795
Compare
zsfelfoldi
changed the title
| @@ -434,6 +434,7 @@ func DeleteBlockLvPointers(db ethdb.KeyValueStore, blocks common.Range[uint64], | |||
| // FilterMapsRange is a storage representation of the block range covered by the | |||
| // filter maps structure and the corresponting log value index range. | |||
| type FilterMapsRange struct { | |||
| Version uint32 | |||
There was a problem hiding this comment.
This will invalidate the previously stored entry and reindex the stuff in next launch, just want to mention
There was a problem hiding this comment.
This is intentional as also mentioned in the commit title, see also the description of #31642. The index has to be regenerated as it might be corrupted in a some cases.
| @@ -125,7 +128,7 @@ func (fm *FilterMapsMatcherBackend) synced() { | |||
| indexedBlocks.SetAfterLast(indexedBlocks.Last()) // remove partially indexed last block | |||
| } | |||
| fm.syncCh <- SyncRange{ | |||
| HeadNumber: fm.f.targetView.headNumber, | |||
| IndexedView: fm.f.indexedView, | |||
There was a problem hiding this comment.
Generally I feel it's very wrong.
Theoretically whenever reorg occurs, the affected data should be discarded, either from the uncommitted memory or persisted state. And then resume.
For the ongoing matching, also, the affected matched results should be discarded and then resume.
The whole reorg management feels very wonky to me.
|
My general feeling is that the chain view management was Whenever a potential reorg occurs, the current implementation Instead, the new chain view is updated asynchronously, and This approach may introduce a number of issues while offering Ideally, it should be implemented that: once reorg occurs, immediately |
| @@ -257,11 +256,14 @@ func (f *FilterMaps) loadHeadSnapshot() error { | |||
|
|
|||
| // makeSnapshot creates a snapshot of the current state of the rendered map. | |||
| func (r *mapRenderer) makeSnapshot() { | |||
| r.f.renderSnapshots.Add(r.iterator.blockNumber, &renderedMap{ | |||
| if r.iterator.blockNumber != r.currentMap.lastBlock { | |||
| panic("iterator state inconsistent with last block") | |||
There was a problem hiding this comment.
Probably discard the snapshot instead of panic here?
There was a problem hiding this comment.
I'd prefer to panic here because it is something that absolutely should not and will not happen; currentMap and iterator are both needed here and should be in a consistent state which is currently trivially ensured by the caller. If a future change ever breaks this then I think the louder and harder it breaks, the better.
zsfelfoldi
force-pushed
the
filter-head-update
branch
from
844e795 to
e7f92a9
Compare
zsfelfoldi
changed the title
zsfelfoldi
force-pushed
the
filter-head-update
branch
from
e7f92a9 to
b5b32f5
Compare
This PR changes the chain view update mechanism of the log filter. Previously the head updates were all wired through the indexer, even in unindexed mode. This was both a bit weird and also unsafe as the indexer's chain view was updates asynchronously with some delay, making some log related tests flaky. Also, the reorg safety of the indexed search was integrated with unindexed search in a weird way, relying on
syncRange.ValidBlocksin the unindexed case too, with a special condition added to only consider the head of the valid range but not the tail in the unindexed case.In this PR the current chain view is directly accessible through the filter backend and unindexed search is also chain view based, making it inherently safe. The matcher sync mechanism is now only used for indexed search as originally intended, removing a few ugly special conditions.
The PR is currently based on top of #31642
Together they fix #31518 and replace #31542