Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Make peer and blob range queries robust to existence of later key prefixes #99

Merged
merged 5 commits into from
Jul 23, 2024

Conversation

black-puppydog
Copy link

The open range scan_peer_key.. we were passing into db.range() was
returning all keys that start with 4u8 or higher.

Simple fix: give 5u8 as the upper bound of the query.

It's worth noting that the same should apply to the blob range queries,
I'll try to make a commit for those next.

One commit with a breaking test, one commit to fix it 🙂

P.S. : @mycognosist hope you're doing okay!

The open range `scan_peer_key..` we were passing into `db.range()` was
returning all keys that start with `4u8` *or higher*.

Simple fix: give `5u8` as the upper bound of the query.

It's worth noting that the same should apply to the blob range queries,
I'll try to make a commit for those next.
@black-puppydog
Copy link
Author

Okay the clippy issue seems unrelated but I guess that's an easy fix as well

Blobs get inserted into sled with `3u8` as their first byte.
Peers have a prefix byte of `4u8`.
However, the `get_pending_blobs` function uses an open range to query the
store, just like `get_peers` used to do.

This means we can cause a deserialization error when querying pending blobs,
simply by inserting a peer into the store. The open range query will return
the peer as a byte string and try to deserialize it into a `Blob` struct.
@black-puppydog black-puppydog changed the title fix: Make peer range query robust to existence of 5u8 key prefixes fix: Make peer and blob range queries robust to existence of later key prefixes Jul 22, 2024
@black-puppydog
Copy link
Author

done, added two more commits for the blob range queries.

See this run for the failing test. It's fixed in the last commit. :)

@mycognosist
Copy link
Owner

Hey @black-puppydog, thanks so much for the PR! Good catch on these open ranges and thanks for including tests.

P.s. Great to hear from you! All good over here; enjoying the social media-free life ^_^ Hope you're keeping well!

@mycognosist mycognosist merged commit 9457d51 into mycognosist:main Jul 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants