feat(storage) L1BlockManager

[Zk2u]

Description

This PR adds a L1BlockManager struct that handles caching of L1 block data.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactor
• New or updated tests
• Dependency Update

Notes to Reviewers

I'm not using inst_ops! because it's kinda manky 🦨

• open an additional ticket to clean the whole storage crate up lol

Checklist

• I have performed a self-review of my code.
• I have commented my code where necessary.
• I have updated the documentation if needed.
• My changes do not introduce new warnings.
• I have added tests that prove my changes are effective or that my feature works.
• New and existing tests pass with my changes.

Related Issues

STR-797

[codecov]

Codecov Report

Attention: Patch coverage is 33.66337% with 67 lines in your changes missing coverage. Please review.

Project coverage is 56.67%. Comparing base (5008f3d) to head (c75a6f2).

Files with missing lines	Patch %	Lines
crates/storage/src/managers/l1.rs	0.00%	66 Missing ⚠️
crates/macros/src/lib.rs	97.14%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
- Coverage   56.76%   56.67%   -0.10%     
==========================================
  Files         306      308       +2     
  Lines       31263    31364     +101     
==========================================
+ Hits        17747    17774      +27     
- Misses      13516    13590      +74     

Files with missing lines	Coverage Δ
crates/db/src/traits.rs	0.00% <ø> (ø)
crates/macros/src/lib.rs	97.14% <97.14%> (ø)
crates/storage/src/managers/l1.rs	0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes

[delbonis]

I'm not using inst_ops! because it's kinda manky 🦨

But now you're introducing new paradigms and consumers have to import additional traits to use it in addition to using the type itself. Why?

Can we discuss things in #eng or in meetings before we start changing strategy here so that we consider all the needs across the team?

[delbonis]

This also still retains the L1Database type parameter. Hiding that baggage through the inst_ops! macro and then building shared logic around it is the whole goal of having the ops primitive and storage managers in the first place. Yes there's a tiny perf hit because of the indirect call, but it's calling IO and it's only a single indirect call per database call so it's fine (and it's not in a BoxFuture that would need an indirect call each time it's polled).

[bewakes]

One of the major reasons to have managers was to be free from these Db generics. The Ops construct was to facilitate this.

[Zk2u]

What issues would arise from having this generic here if the compiler is going to infer it anyway?

[delbonis]

You have to pass it down through everything that wants to interact with the database / manager, which becomes cumbersome.

[Zk2u]

But now you're introducing new paradigms and consumers have to import additional traits to use it in addition to using the type itself. Why?

This is not really a new paradigm - there's no need for a inst_ops macro and the current implementation is very convoluted and jumps through many hoops to achieve the same thing. I'm not sure I understand how importing a trait could be problematic either

[delbonis]

The need is that it cleanly wraps up all the machinery to implement async and blocking calls over a simpler blocking database interface and give some extra space for future indirection in the Context type.

[Zk2u]

give some extra space for future indirection in the Context type

I'm not sure what you mean by this, but the async version is just a generated version of the original sync trait. "The machinery" is just spawning it on a thread pool and waiting for a oneshot which is implemented via a trait. Synchronously we just implement the original sync trait and the caller can just decides what works best.

[delbonis]

The main issue right now is that this doesn't remove the need to pass around the generic type parameter as in the current strategy. We don't want the manager to just transparently implement the database traits but with additional caching, we want it to be the high level interface that can hide the implementation details. This is more apparent in the intention for the chainstate manager, which we want to be the main thing handling reconstructing arbitrary states from the checkpointed states and writes. The architecture with the other managers/ops also gives a lot of flexibility for transparently adding things like metrics collection to improve visibility.

The generics are great for performance sensitive code (because of monomorphization helping with automatic inlining) or logic that's highly abstracted (with associated types). But this isn't that, we're just using them for indirection between different underlying database impls.