cache: Handle missing tiles stricter #2406
Conversation
|
Interesting, there is some failing MySQL unit test. Looking. |
pav-kv
changed the title
pav-kv
changed the title
| if err := s.cacheSubtree(s.newEmptySubtree(id)); err != nil { | ||
| return err | ||
| } | ||
| if r := len(want); r != 0 { |
There was a problem hiding this comment.
I'm not 100% sure if there's an edge case where this happens. It might possibly be where the tree grows enough to require a new tile that wasn't previously written.
There was a problem hiding this comment.
The key point is in the PR description: we will not try to read this new tile with GetMerkleNodes (and hence SubtreeCache.preload). The latter method is used in 2 scenarios:
- Sequencer's transaction: read compact range of the current tree. These nodes (and hence tiles) always exist, unless someone removed them from the DB.
- Reading proofs. These "read" transactions also know the current tree size, and request the existing nodes.
There was a problem hiding this comment.
This is a leftover from maps, I believe. There we could read a leaf which previously did not exist, so we would need empty tiles silently added.
There was a problem hiding this comment.
Maybe it was added for the map. It's a long time ago now.
Codecov Report
@@ Coverage Diff @@
## master #2406 +/- ##
=======================================
Coverage 65.80% 65.80%
=======================================
Files 106 106
Lines 7670 7670
=======================================
Hits 5047 5047
+ Misses 2100 2097 -3
- Partials 523 526 +3
Continue to review full report at Codecov.
|
|
Fixed the tests. |
GetMerkleNodesis always called with existing nodes, thereforeSubtreeCache.preloadwill always try to read existing tiles. We should return an error if some tile is not found,
instead of silently creating an empty tile.
Addresses some comments in #2404.
Part of #2378.