bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Update rocksdb to `0.19`

Open afilini opened this issue 3 years ago • 7 comments
trafficstars

Description

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

Checklists

All Submissions:

  • [x] I've signed all my commits
  • [x] I followed the contribution guidelines
  • [x] 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

afilini avatar Aug 16 '22 17:08 afilini

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

afilini avatar Aug 17 '22 11:08 afilini

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..

rajarshimaitra avatar Aug 17 '22 14:08 rajarshimaitra

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.

notmandatory avatar Aug 26 '22 01:08 notmandatory

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

afilini avatar Sep 13 '22 16:09 afilini

I am working on CBF using nakamoto here https://github.com/bitcoindevkit/bdk/pull/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..

rajarshimaitra avatar Sep 14 '22 05:09 rajarshimaitra

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.

cryptoquick avatar Sep 21 '22 00:09 cryptoquick

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

rajarshimaitra avatar Sep 21 '22 04:09 rajarshimaitra

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!

danielabrozzoni avatar Mar 16 '23 17:03 danielabrozzoni