core/filtermaps: clone cached slices, fix tempRange #31680
Conversation
| @@ -610,7 +610,9 @@ func (f *FilterMaps) storeFilterMapRowsOfGroup(batch ethdb.Batch, mapIndices []u | |||
| if uint32(len(mapIndices)) != f.baseRowGroupLength { // skip base rows read if all rows are replaced | |||
| var ok bool | |||
| baseRows, ok = f.baseRowsCache.Get(baseMapRowIndex) | |||
| if !ok { | |||
| if ok { | |||
There was a problem hiding this comment.
Should we also clone the slice in getFilterMapRow?
There was a problem hiding this comment.
We do clone baseRow and if required then we append extRow which is always retrieved from the database so it's guaranteed that the returned result does not share an underlying array with anything.
zsfelfoldi
changed the title
|
The slice cloning, though definitely necessary to be safe, did not solve the issue so I added a lot of debug prints and finally conclusively traced the error back to its origin. When I add a new block and overwrite the last map with a new rendered one, I temporarily use |
|
I also changed |
| @@ -656,7 +659,7 @@ func (f *FilterMaps) mapRowIndex(mapIndex, rowIndex uint32) uint64 { | |||
| // called from outside the indexerLoop goroutine. | |||
| func (f *FilterMaps) getBlockLvPointer(blockNumber uint64) (uint64, error) { | |||
| if blockNumber >= f.indexedRange.blocks.AfterLast() && f.indexedRange.headIndexed { | |||
| return f.indexedRange.headDelimiter, nil | |||
| return f.indexedRange.headDelimiter + 1, nil | |||
There was a problem hiding this comment.
Should we error out if blockNumber > f.indexedRange.blocks.AfterLast()?
Theoretically, the returned log index is for the next block and it must be consecutive with the latest indexed one.
There was a problem hiding this comment.
Since the search works async (range might change any time during the process) the underlying functions of the matcher backend do not fail when they are queried out of range, they just return results so that the search will not find any additional items (the head part of the results will be invalidated anyways in this case). getLogByLvIndex also silently returns with no result end no errors.
getBlockLvPointer also just returns the beginning of the next future block, even if queried further in the future, so that the search will just search until the end of the indexed range. The point is that if it's a long search then the majority of results will still be valuable and the head part will be searched again anyways so just do something that lets the operation finish safely.
We could have a different mechanism but this seems to work safely for now and we have to do the fork release today so I would not change it fundamentally right now.
| @@ -536,6 +536,7 @@ func (r *mapRenderer) getTempRange() (filterMapsRange, error) { | |||
| } else { | |||
| tempRange.blocks.SetAfterLast(0) | |||
| } | |||
| tempRange.headIndexed = false | |||
There was a problem hiding this comment.
is it the real root cause?
This PR ensures that caching a slice or a slice of slices will never affect the original version by always cloning a slice fetched from cache if it is not used in a guaranteed read only way.
This PR fixes the out-of-range block number logic of `getBlockLvPointer` which 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 when `headIndexed` was true and `headDelimiter` pointed to the end of the indexed range. Now it also works for an unfinished index. This logic is also moved from `FilterMaps.getBlockLvPointer` to `FilterMapsMatcherBackend.GetBlockLvPointer` because it is only required by the search anyways. `FilterMaps.getBlockLvPointer` now 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 positive `headIndexed` no database read was triggered in `getBlockLvPointer`. Also, the unnecessary `indexLock` in `synced()` (fixed in #31708) usually did prevent the search seeing the temp range and therefore avoided noticeable issues.
This PR fixes the out-of-range block number logic of `getBlockLvPointer` which 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 when `headIndexed` was true and `headDelimiter` pointed to the end of the indexed range. Now it also works for an unfinished index. This logic is also moved from `FilterMaps.getBlockLvPointer` to `FilterMapsMatcherBackend.GetBlockLvPointer` because it is only required by the search anyways. `FilterMaps.getBlockLvPointer` now 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 ethereum/go-ethereum#31680), though caused other problems, prevented this one from being triggered as with a positive `headIndexed` no database read was triggered in `getBlockLvPointer`. Also, the unnecessary `indexLock` in `synced()` (fixed in ethereum/go-ethereum#31708) usually did prevent the search seeing the temp range and therefore avoided noticeable issues.
This PR ensures that caching a slice or a slice of slices will never affect the original version by always cloning a slice fetched from cache if it is not used in a guaranteed read only way.
This PR fixes the out-of-range block number logic of `getBlockLvPointer` which 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 when `headIndexed` was true and `headDelimiter` pointed to the end of the indexed range. Now it also works for an unfinished index. This logic is also moved from `FilterMaps.getBlockLvPointer` to `FilterMapsMatcherBackend.GetBlockLvPointer` because it is only required by the search anyways. `FilterMaps.getBlockLvPointer` now 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 ethereum#31680), though caused other problems, prevented this one from being triggered as with a positive `headIndexed` no database read was triggered in `getBlockLvPointer`. Also, the unnecessary `indexLock` in `synced()` (fixed in ethereum#31708) usually did prevent the search seeing the temp range and therefore avoided noticeable issues.
This PR ensures that caching a slice or a slice of slices will never affect the original version by always cloning a slice fetched from cache if it is not used in a guaranteed read only way.
This PR ensures that caching a slice or a slice of slices will never affect the original version by always cloning a slice fetched from cache if it is not used in a guaranteed read only way.