-
Notifications
You must be signed in to change notification settings - Fork 111
[storage/adb] introduce "keyless" adb #1426
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
try_join!( | ||
self.mmr.sync(&mut self.hasher).map_err(Error::Mmr), | ||
self.log.sync(section).map_err(Error::Journal), | ||
self.locations.sync().map_err(Error::Journal), |
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.
Not directly related to this PR but these should have been calls to close, even if it is all the same in the end.
b9b86f2
to
4aa0741
Compare
86570e9
to
cbd2c64
Compare
5842949
to
eb1b9ea
Compare
eb1b9ea
to
2dac0b6
Compare
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.
Pull Request Overview
This PR introduces a "keyless" authenticated database (adb) that provides append-only storage of arbitrary data that can be retrieved by location rather than by key. The implementation includes operations for appending values and committing changes, with support for proofs and pruning.
Key Changes:
- Added a new
Keyless
operation enum for append and commit operations - Implemented a complete keyless ADB with append, commit, pruning, and proof generation capabilities
- Fixed an unrelated bug in the variable ADB close method where sync was being called instead of close
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
storage/src/store/operation.rs | Adds new Keyless operation enum with append/commit variants and serialization support |
storage/src/adb/mod.rs | Exposes the new keyless module |
storage/src/adb/keyless.rs | Complete implementation of the keyless ADB with all core functionality |
storage/src/adb/any/variable/mod.rs | Bug fix: corrects close method to call close instead of sync on components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Small nits 🫡
} | ||
|
||
impl<E: Storage + Clock + Metrics, V: Codec, H: CHasher> Keyless<E, V, H> { | ||
/// Returns a [Keyless] adb initialized from `cfg`. Any uncommitted operations will be discarded |
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.
nit: On restart, any uncommitted...
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 is documenting what this function actually does, not what happens on restart, so I think the comment is more correct as-is?
storage/src/adb/keyless.rs
Outdated
/// Get the value at location `loc` in the database. Returns None if the location is valid but | ||
/// does not correspond to an append. | ||
pub async fn get(&self, loc: u64) -> Result<Option<V>, Error> { | ||
match self.locations.read(loc).await { |
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 think you could do .await?
to avoid the error case (and needing to match here)?
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.
yep fixed
|
||
self.size += 1; | ||
if section != self.current_section() { | ||
self.values.sync(section).await?; |
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.
Should we also sync self.locations
here (I think the current recovery pattern doesn't require it but figure I'd call out in case)?
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 don't think so, it would only hurt performance, and any unsync'ed items we do sync here would just be rolled back before the next commit anyway.
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.
Actually, I think journal::Fixed
will call sync automagically behind the scenes if configured to have the same items_per_section
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.
1 small nit
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1426 +/- ##
==========================================
+ Coverage 91.70% 91.74% +0.03%
==========================================
Files 275 276 +1
Lines 69443 69723 +280
==========================================
+ Hits 63685 63967 +282
+ Misses 5758 5756 -2
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Introduces the "keyless" adb which is an immutable-type store allowing appending of new values that become associated with a location within the database that can be used for retrieval.
Resolves: #1413