-
Notifications
You must be signed in to change notification settings - Fork 95
feat: rocksdb storage for AccountTree and NullifierTree by default
#1326
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
Conversation
|
Requires 0xMiden/crypto#613 to be merged, otherwise we might get a panic. |
a06e53f to
397aaa9
Compare
Makefile
Outdated
|
|
||
| WARNINGS=RUSTDOCFLAGS="-D warnings" | ||
| BUILD_PROTO=BUILD_PROTO=1 | ||
| ALL_FEATURES="--features=tracing-forest,concurrent,tracing-forest,tx-prover,batch-prover,block-prover,std" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment on why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plays into CI and not compiling rocksdb by default on CI (see absence of it in the list), but only if relevant files changed, i.e. anything in accounts. That was the rough idea to limit the CI roundtrip time to those PRs where it matters. I am aware this is far from optimal and I'd prefer to do it differently (always compile).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not loving the CI complexity.
Is the goal not to always have this enabled? When would we run a node without this once its working? I would imagine that CI caching should keep the compile times in check or no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate the CI crap that's part of the PR.
The CI caching should keep the CI times in check, but right now rocksdb also compiles it's C/CXX dependency, which takes a while.
The rationale: It takes a while to compile rocksdb. If it's cached, it re-claims a good chunk of cache size. I don't necessarily want to cache this at all. I'd preferably link statically against host libraries (at least in CI), but this has proven itself to be fragile (llvm compiler version fun).
I am very open to suggestions how to handle this.
Also: Not a fan of GHA, it's a constrained dumpsterfire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified it to use host libraries. As said earlier, no good solutions, only trade-offs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Host libraries are too old on debian 🫠
crates/store/src/state.rs
Outdated
| /// The storage backend for account trees. | ||
| #[cfg(feature = "rocksdb")] | ||
| pub type AccountTreeStorage = RocksDbStorage; | ||
| #[cfg(not(feature = "rocksdb"))] | ||
| pub type AccountTreeStorage = MemoryStorage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a roadmap or point-in-time when we would always have this enabled?
e42fc7e to
164f712
Compare
AccountTree and NullifierTree
Co-authored-by: Mirko <[email protected]>
07c4f17 to
a29ed14
Compare
AccountTree and NullifierTreeAccountTree and NullifierTree by default
Currently we only use
LargeSmt<Memory>with theMemorybackend. This is not very useful in practice, the entire purpose ofLargeSmtis having a disk backedSmtimplementation.This PR will ensure we have the
rocksdbfeature enabled inmiden-cryptountil it's extracted per #1218 .How it's done
We use the dependency feature unification of cargo to ensure we depend on
miden-cryptowith therocksdbfeature enabled. However, this incurs both a hefty penalty on compile times by default, compilingrocksdb-sysand additional CI dependencies, which leads to disabling it by default.CI
Since
rocksdbcomes with some overhead / CI time / cache we only want to run it whenever we change OR link against system level libraries statically.NOT part of this PR since I am unsure of the best path.
Extras
Some additional type aliases are used to avoid lengthy types.