Skip to content

Conversation

roberto-bayardo
Copy link
Collaborator

@roberto-bayardo roberto-bayardo commented Aug 19, 2025

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

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),
Copy link
Collaborator Author

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.

@roberto-bayardo roberto-bayardo force-pushed the keyless-store branch 2 times, most recently from 5842949 to eb1b9ea Compare August 19, 2025 16:21
@roberto-bayardo roberto-bayardo changed the title [storage/adb] initial keyless store work [storage/adb] introduce "keyless" adb Aug 19, 2025
@roberto-bayardo roberto-bayardo marked this pull request as ready for review August 19, 2025 16:36
Copy link
Contributor

@Copilot Copilot AI left a 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.

@commonwarexyz commonwarexyz deleted a comment from Copilot AI Aug 19, 2025
@commonwarexyz commonwarexyz deleted a comment from Copilot AI Aug 19, 2025
Copy link
Contributor

@patrick-ogrady patrick-ogrady left a 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
Copy link
Contributor

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

Copy link
Collaborator Author

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?

/// 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 {
Copy link
Contributor

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)?

Copy link
Collaborator Author

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?;
Copy link
Contributor

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)?

Copy link
Collaborator Author

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.

Copy link
Contributor

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

Copy link
Contributor

@patrick-ogrady patrick-ogrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 small nit

@patrick-ogrady patrick-ogrady merged commit 6226ac0 into main Aug 19, 2025
36 checks passed
@patrick-ogrady patrick-ogrady deleted the keyless-store branch August 19, 2025 19:50
@codecov
Copy link

codecov bot commented Aug 19, 2025

Codecov Report

❌ Patch coverage is 93.90476% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.74%. Comparing base (a7a4699) to head (94bf370).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
storage/src/adb/keyless.rs 93.23% 31 Missing ⚠️
storage/src/store/operation.rs 98.43% 1 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
storage/src/adb/any/variable/mod.rs 96.75% <100.00%> (-0.01%) ⬇️
storage/src/store/operation.rs 91.17% <98.43%> (+2.60%) ⬆️
storage/src/adb/keyless.rs 93.23% <93.23%> (ø)

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a4699...94bf370. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[storage/adb] Create a Key-Less Immutable ADB

2 participants