cnidarium: prefix queries over substores are hazardous #4653
Conversation
This file contains 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
The bug is caused by a layering violation: keys returned by prefix queries have their *substore prefix* truncated. However, `StateDelta` are unaware of this implementation detail and maintain a global namespace for all changes. This create an issue in the cache interleaving logic, where the search range that we construct to look for new writes/covering deletions between keys will build a nonsensical range, for example using the full key (incl. substore prefix) as a lower bound and a susbtore key (with a truncated prefix). The effect of this bug can vary from a panic, to skipping valid entries that should be returned by the prefix query.
erwanor
added
A-node
erwanor
changed the title
|
This should also change the migration in 78 for deleting empty ibc commitments right? |
|
That's right, we should update it once this gets merged |
cratelyn
added
the
consensus-breaking
|
added the |
cratelyn
reviewed
Jun 24, 2024
There was a problem hiding this comment.
✔️ this looks good. approved, pending #4653 (comment) being addressed
cratelyn
approved these changes
Jun 24, 2024
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.
Describe your changes
This PR contains a minimal reproduction for a bug in cnidarium's prefix query handling. It also contains a sketch for a fix that we can workshop. The bug was introduced in the original substore implementation PR (#3131).
Minimal reproduction of the prefix range cache bug.
Context
cnidarium, our storage layer, supports prefix storage.This allows users to configure independent storage units, each with
their own merkle tree, nonverifiable sidecar, and separate namespace.
Routing is done transparently without the user having to worry about
the details.
Overview
Prefix queries return tuples of (key, value)s, but instead of
returning the full key, they return the substore key. This is a layering
violation, and indeed causes a bug in the cache interleaving logic.
Terminology
full_key: a key that contains a substore prefix, a delimiter, and a substore key.substore_key: a key with a stripped prefix.Walkthrough
StateDeltaindex changes using full keys, as it is not aware of theparticular substore configuration that it is working against, by design.
As part of the cache interleaving logic, the
StateDetlawill try look fornew writes or covering deletions. However, since the base prefix implementation
returns substore keys, the cache will build an incoherence range and panic (or miss data).
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: