feat: add generic <S> to NullifierTree enabling LargeSmt support#2091
feat: add generic <S> to NullifierTree enabling LargeSmt support#2091
<S> to NullifierTree enabling LargeSmt support#2091Conversation
drahnr
force-pushed
the
bernhard-1353-largesmt-for-nullifier-tree
branch
from
1710b9b to
f6bf8ae
Compare
drahnr
changed the title
<S> to NullifierTree enablign LargeSmt support<S> to NullifierTree enabling LargeSmt support
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Minor naming/nits; I did check the impls but I'll defer approval to @PhilippGackstatter
drahnr
force-pushed
the
bernhard-1353-largesmt-for-nullifier-tree
branch
3 times, most recently
from
348dc16 to
1556b91
Compare
- Add NullifierTreeBackend trait to abstract over Smt and LargeSmt - Make NullifierTree generic over the SMT backend - Implement backend trait for both Smt and LargeSmt<Backend> - Add comprehensive tests for LargeSmt backend - Update partial_nullifier_tree to use free helper functions - Maintain backward compatibility with default Smt backend
drahnr
force-pushed
the
bernhard-1353-largesmt-for-nullifier-tree
branch
from
1556b91 to
7f0d754
Compare
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
I left a few comments. The two main ones I think are whether the free function must be public and the panicking in num_entries.
| #[cfg(feature = "std")] | ||
| impl<Backend> NullifierTreeBackend for LargeSmt<Backend> | ||
| where |
There was a problem hiding this comment.
Question: My impression was that we want to eventually move LargeSmt out of miden crypto into a separate crate, and then, I think it would be great to move the implementation impl<Backend> NullifierTreeBackend for LargeSmt<Backend> out of miden-base, to avoid having the rocksdb dependency here, where we don't actually need it. Is that still the plan?
There was a problem hiding this comment.
I think the priority is to move the RocksDB SmtStore implementation out of miden-crypto, moving LargeSmt is still an open discussion point iiuc.
However, we only incur a dependency on RocksDB iff we enable the feature rocksdb, which we do not by default (not here, not in the node (yet, see 0xMiden/node#1352 ) ).
There was a problem hiding this comment.
Ah, that makes sense. I erroneously assumed rocks db and large smt meant the same thing. Makes sense to move rocks DB out of crypto I think.
I wonder if it would still make sense to keep the backend implementation for large SMT closer to where it is used with rocks DB, primarily to lower the chances of the error handling assumptions going out of sync. E.g. we currently assume storage operations are panicky (and the issue #2010 is already closed - is that intentional?) and this seems to be with rocks DB in mind. But the impl here is more general, e.g. it implements NullifierTreeBackend for any SmtStorage. So, keeping rocks DB usage + large SMT backed impl closer together (e.g. node) would be better in my mind.
There was a problem hiding this comment.
#2010 re-opened and added some more context.
I am not quite sold on keeping the trait SmtStore, struct LargeSmt and the relevant impl SmtStores all in one place, the proximity might not be as useful, but it also depends on the path forward re multi-monorepo, which is still an open discussion point.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
- Rename generic parameter S to Backend for better clarity - Rename new() to new_unchecked() to indicate lack of invariant validation - Add SAFETY comments explaining unwrap() usage in LargeSmt implementation - Clarify trait documentation regarding Default requirement - Update all call sites to use new_unchecked() Addresses feedback from PR #2091
drahnr
force-pushed
the
bernhard-1353-largesmt-for-nullifier-tree
branch
from
81de654 to
0d6a53d
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small question inline.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you for updating the error panic and the docs!
Left some minor comments.
4 participants
Allows seting up the
NullifierTreewithLargeSmtby means of adding an additional trait alaAccountTreeBackendfor the nullifier tree:NullifierTreeBackend.Part of 0xMiden/node#1353