core/filtermaps: fix log value search range #31734
Merged
+16
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
core/filtermaps/filtermaps.go
Outdated
| if f.indexedRange.headIndexed { | ||
| return f.indexedRange.headDelimiter + 1, nil | ||
| } else { | ||
| return uint64(f.indexedRange.maps.AfterLast()) << f.logValuesPerMap, nil |
There was a problem hiding this comment.
Why do we return the starting position of log value of the next map, if the current chain is not fully indexed?
There was a problem hiding this comment.
Indeed it makes more sense to just return the start pointer of the last block.
zsfelfoldi
force-pushed
the
fix-lv-range-end
branch
from
6a5e40c to
17676e5
Compare
rjl493456442
reviewed
May 2, 2025
| if fm.f.indexedRange.blocks.Count() > 0 { | ||
| // return index at the beginning of the last, partially indexed | ||
| // block (after the last fully indexed one) | ||
| blockNumber = fm.f.indexedRange.blocks.Last() |
There was a problem hiding this comment.
If I understand correct, if the requested block beyonds the last indexed block,
it's only possible that:
- When the matcher backend is constructed, the search range is initialized with the old indexed range (e.g., 1->10)
- Chain reorg occurs, the indexer unwinds a part of indexed range (e.g., 1->8)
- Matcher backend is not aware of this chain reorg, and block 9, 10 is still requested for the logValue
- The matching result is returned within the range of 1->8
- The extra match range 9->10 will be trimmed by
// discard everything that might be invalid
trimRange := s.syncRange.ValidBlocks.Intersection(s.chainView.SharedRange(s.syncRange.IndexedView))
matchRange, matches := s.trimMatches(trimRange, r, results)- the extra range 9->10 will be searched again, either in indexed way or unindexed way later
rjl493456442
reviewed
May 2, 2025
| // GetBlockLvPointer implements MatcherBackend. | ||
| func (fm *FilterMapsMatcherBackend) GetBlockLvPointer(ctx context.Context, blockNumber uint64) (uint64, error) { | ||
| fm.f.indexLock.RLock() | ||
| defer fm.f.indexLock.RUnlock() | ||
|
|
||
| if blockNumber >= fm.f.indexedRange.blocks.AfterLast() { | ||
| if fm.f.indexedRange.headIndexed { |
There was a problem hiding this comment.
If the indexed range is fully indexed, the whole range will be available for searching;
If the indexed range is partially indexed (the last one is not fully indexed), then [first, last) will be availabel for searching;
rjl493456442
approved these changes
May 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the out-of-range block number logic of
getBlockLvPointerwhich sometimes caused searches to fail if the head was updated in the wrong moment. This logic ensures that querying the pointer of a future block returns the pointer after the last fully indexed block (instead of failing) and therefore an async range update will not cause the search to fail. Earier this behaviour only worked whenheadIndexedwas true andheadDelimiterpointed to the end of the indexed range. Now it also works for an unfinished index.This logic is also moved from
FilterMaps.getBlockLvPointertoFilterMapsMatcherBackend.GetBlockLvPointerbecause it is only required by the search anyways.FilterMaps.getBlockLvPointernow only returns a pointer for existing blocks, consistently with how it is used in the indexer/renderer.Note that this unhandled case has been present in the code for a long time but went unnoticed because either one of two previously fixed bugs did prevent it from being triggered; the incorrectly positive
tempRange.headIndexed(fixed in #31680), though caused other problems, prevented this one from being triggered as with a positiveheadIndexedno database read was triggered ingetBlockLvPointer. Also, the unnecessaryindexLockinsynced()(fixed in #31708) usually did prevent the search seeing the temp range and therefore avoided noticeable issues.