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

Update rocksdb to 0.19 #724

Closed
wants to merge 1 commit into from

Conversation

afilini
Copy link
Member

@afilini afilini commented Aug 16, 2022

Description

This is making our cargo audit fail due to a vulnerability in older rocksdb versions.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

This is making our cargo audit fail due to a vulnerability in older
rocksdb versions.
@afilini afilini force-pushed the fix/update-rocksdb branch 2 times, most recently from dede9aa to d01ffa3 Compare August 17, 2022 11:32
@afilini
Copy link
Member Author

afilini commented Aug 17, 2022

Apparently rocksdb requires rust 1.60, not only to compile the code itself but even just to include it as a dependency. I don't understand why they couldn't just fix the vulnerability and make one release keeping the old MSRV before bumping it, i guess they don't really care about being stable.

I don't feel like raising our official MSRV just for compact filters which is experimental and being basically rewritten from scratch as far as I understand. I'd like to hear your opinion on this @rajarshimaitra

For the time being I'd just keep this PR here while we wait and see what to do

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Aug 17, 2022

Removing Rocksdb from our dependency was already a long listed todo.. I also feel rocksdb is too much for the task for compact filters which is mostly just bulk storage and doesn't require a lot of indexing.. So something as simple as sled could work here.. Its not super critical of data security too.. Can get deleted and doesn't take a lot of time to resync back..

nakamoto (the other cbf impl in rust out there) uses sled, where as neutrino uses just flat files for filter storage.. While flat files could be interesting and very low over head without any extra dependency, getting it working might be a bit more code (but could be an interesting experiment to do, that can be useful in other places too).. So for now I think the easiest is to switch to sled, which is already in our dep..

And on the CBF side, I don't really know if anyone uses our CBF module.. The plan with the new cbf crate is to make it compatible with bdk_core and expose it into bdk from there.. So it might not make too much sense to update MSRV just for this..

@notmandatory
Copy link
Member

I agree it doesn't make sense to bump the project MSRV to 1.60 just for this. Let's see how the new CBF blockchain work goes and if it can make this change unnecessary.

@afilini
Copy link
Member Author

afilini commented Sep 13, 2022

I tried with sled in the past but it was very slow and for some reason it tried to keep everything in memory which made it crash even with 10+ gb of ram when indexing mainnet.

Rocksdb instead only uses a low amount of memory and swaps in and out of disk as needed. I decided to use it for that reason, even though it's not pure rust and takes a ton of time to compile.

Maybe newer versions of sled have gotten better at handling memory, I did my testing more than a year ago

@rajarshimaitra
Copy link
Contributor

I am working on CBF using nakamoto here #751. Which uses flat files to store data.. If all goes well over there and we manage to pass all the tests we might be able to swap rocksdb fully from our dependency..

@cryptoquick
Copy link

You might appreciate redb, an example of it being used is in the ordinals project. @casey recommends it over sled, in both performance and design, and I'd tend to agree. It also seems better maintained.

@rajarshimaitra
Copy link
Contributor

Sounds like a good idea to have another default db impl with redb..

@danielabrozzoni
Copy link
Member

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants