feat: Implement persistent storage

[SoarinSkySagar]

This PR adds a persistent libmdbx-backed database for client data.

[ArtiomTr]

looks like this is not finished? there is nothing implemented yet?

[bomanaps]

looks like this is not finished? there is nothing implemented yet?

This will take some thing as it won't be a one of pr

[ArtiomTr]

This will take some thing as it won't be a one of pr

Ok but there is absolutely zero functionality yet? We can do this in iterations, i.e. first we can persist only blocks, but there is no point of merging dead code

[SoarinSkySagar]

Ok but there is absolutely zero functionality yet? We can do this in iterations, i.e. first we can persist only blocks, but there is no point of merging dead code

yeah this is still a WIP, I was asking for review on the initial architecture (KV pairs and grandine database), if this is the correct direction

[bomanaps]

One more thing can we remove the OOMing framing, as that issue has been resolved?

[SoarinSkySagar]

@bomanaps but since all state data is being managed through memory right now, don't you think it is bound to OOM sometime when the node is running for a long time?

[SoarinSkySagar]

On a separate note, after implementation of persistent db I'm planning to research into LRU cache implementation in the crate itself which it will manage automatically such that the db functions can be used without worrying about caching. What do you think about this?

[bomanaps]

On a separate note, after implementation of persistent db I'm planning to research into LRU cache implementation in the crate itself which it will manage automatically such that the db functions can be used without worrying about caching. What do you think about this?

The best person to answer this is @ArtiomTr and also on the side have you tried running a node maybe 3 node setup or more depending on your laptop capacity as this should give you a better feel of how lean Ethereum runs?

[ArtiomTr]

yeah this is still a WIP, I was asking for review on the initial architecture (KV pairs and grandine database), if this is the correct direction

It is hard to tell what is going on, without seeing actual implementation :). Better to implement something first, see if it works & is performant enough, then proceed with review. To avoid wasting much time, start with smaller scope - like just saving the blocks first. The database must keep blocks, because if you have blocks, you can reconstruct any historical state, at the cost of cpu time. This way you can scaffold database structure, get early feedback on that, and then proceed on implementing everything else.

＠bomanaps but since all state data is being managed through memory right now, don't you think it is bound to OOM sometime when the node is running for a long time?

This is true only for some cases, e.g. during long non-finality periods - roughly speaking, validator has to track every "branch", to be able to properly converge into whatever branch eventually wins. However, even in those cases, I think there are clever algorithms to avoid keeping all unfinalized history in memory.

During normal operation, usually memory consumption won't grow indefinitely - node has to keep only last finalized state, maybe some older ones, but no more. Node also has to keep some historical blocks (I believe all blocks up to weak subjectivity period, although I don't remember exactly and may be wrong on this), but blocks usually take only a fraction of space comparing to states, so should be a non-issue. Grandine the beacon chain existed for quite some time without database at all, and worked really good.

On a separate note, after implementation of persistent db I'm planning to research into LRU cache implementation in the crate itself which it will manage automatically such that the db functions can be used without worrying about caching. What do you think about this?

It is kinda complicated topic. Ideally, the node should operate without depending on database at all. So in this sense, caching just wastes cpu/memory. However, sometimes you actually do want to have caches, for instance there may be cases when you need to load some state that is a bit older than finalized point on a hot path, so loading it quickly may be desirable. However, caches wont magically make loading quicker -- instead, you will pay some small performance cost once, for being able to query the same thing instantly next time. If you take straightforward approach, and cache every database query, then such caches are pointless -- it is very rare that same object is queried from database twice. But if you make them smart, by somehow, caching intermediate values that may be needed for both querying objects A and B, then such caches will be very useful. This is the approach I take when implementing new database layout for grandine beacon chain (https://github.com/ArtiomTr/grandine/blob/4ec3964cf42b04b8d1ac93791a6a14ff788b2d18/fork_choice_control/src/storage.rs#L907). Although this requires careful benchmarking & profiling first, so probably better to think about caches after you have working database.

Also, let me give you some advice on using libmdbx:

1. Try to do sequential writes, they will be much quicker, as libmdbx is a B+ tree internally (there is a good blog post explaining why sequential writes are more performant for B+ trees https://planetscale.com/blog/btrees-and-database-indexes)
2. Libmdbx allows to do range queries, so you can extract values only by key prefixes. Database also permits having large keys, up to 2022 bytes for default 4kb page size. This means you can put more information into key, allowing to drop indexes for example. In case of blocks, you can, for example, save slot + block_root + state_root in key. This way you can achieve three goals at the same time:
   • slot + block_root gives you a unique key per block, even for unfinalized chain, where proposers may differ (this is irrelevant for lean chain currently, as validators cannot enter/exit, and proposer is chosen via round-robin, though this will likely change when going to mainnet)
   • slot being at the beginning makes all writes to database sequential, except for backsync - although this shouldn't be a problem, as those writes are still probably gonna get into the same B-tree branch.
   • state_root in key allows to find which state corresponds to this block, without even reading/decompressing/decoding the block itself.
3. Keep in mind that keys in libmdbx are not necessarily UTF-8 encoded strings, so you can use any byte sequence you want to.
4. Use separate libmdbx databases for different types. This will allow to parallelize writes into different databases, although remember that by keeping values in different databases you lose atomicity guarantees -- so probably it is better to write "value" first, only then proceed with writing its indices, so you don't have dangling references. For deleting values, process should be reversed -- deleting indices first, values next. Or, you can just write/delete in any order you want to, while being careful and handle all edge cases, where write/delete into one database fails, and succeeds in other, and where index may point to non-existing value.

[ArtiomTr]

also, don't forget to change target branch to devnet-5, instead of main - latest changes are on devnet-5 branch.

[SoarinSkySagar]

rebased and changed target branch to devnet-5. continuing work now, setting up lean's own database.

[SoarinSkySagar]

implementation done, now moving to testing phase through local devnets.

[SoarinSkySagar]

I tested the implementation and it works, so marking it ready for review. I tried to keep the PR scope short like just block persistence for now, but then it couldn't be tested after restart since all states would have to be synced anyways. Hence I wired everything needed to be persisted, and after testing using lean-quickstart I noticed that sync time has reduced by almost 50% after 3 minutes of downtime. Would like a review to see if anything needs to be changed and/or any scope for performance improvements.

cc @ArtiomTr @bomanaps

[ArtiomTr]

This PR covers too much, while being very shallow. I would suggest focusing on building strong foundation, before implementing every single table needed.

I tried to keep the PR scope short like just block persistence for now, but then it couldn't be tested after restart since all states would have to be synced anyways.

You don't need anything else than genesis state & blocks - everything else is derived. Of course, you can save additional information for convenience, but again, I recommend keeping states out of this version - any state still can be reconstructed by applying blocks on top of genesis state. Consider this: when initializing, you can construct forkchoice Store from genesis state, then load all blocks from disk, and feed them via on_block. Then start node as usual - you will continue from last synced state.

About database implementation - your implementation contains a ton of boilerplate. Boilerplate isn't necessarily bad, but in your case it became too messy - it is really complicated to find where database operations end, and specialized methods (like get_network_id) start. It clearly misses abstraction layer, that would allow to easier understand, and more clearly share logic between all tables. Something, that could look like a proper KV interface, maybe even generic, to enforce single key/value type.

Next thing, why didn't you use compound key for blocks (slot + block_root)? This would make writes sequential, also enabled doing range queries, dramatically simplifying your pruning/"load_since" then would be straightforward and performant, instead of current random-access + index lookups.

Also, zstd compression level is not configurable - it should be. It is really convenient, specifically because zstd can automatically understand and decompress file with any compression level - so you don't have to keep track of it, just ask user which compression level to use for saving new data.

Finally, please read my previous comment. It contains useful information about how to make database implementation performant.

[ArtiomTr]

that's an antipattern - you panic in perfectly normal situations, where there is no write access to "data" folder in current directory. Moreover, the Default implementation of Storage doesn't make any sense - unless, of course, you have something like "in-memory" mode, that may look like a default storage engine. I would suggest not having default implementation at all, and instead use explicit constructors to create Storage.

[SoarinSkySagar]

Store struct has #[derive(Default)], so while adding storage as a field to Store I was facing an error because Storage does not have a default. Hence I added it

[ArtiomTr]

Then default from store should be removed too - it may be convenient before, but now that's just creating unnecessary complications.

[SoarinSkySagar]

that makes things simpler then, Default can be removed from Storage now

[ArtiomTr]

Not a big fan of such abstraction, as it essentially allows to lock database for indefinite amount of time:

storage.batch_write(|| {
    let state = slow_operation(); // for example, it takes 20s
    storage.put_state(state);
});

And what's the point of this method? I think you can achieve almost the same thing with list of key/value pairs to write. This way, implementation will lock database only for necessary amount of time - just to write data, not prepare it/then write.

[SoarinSkySagar]

This method was defined in the lean specs to allow for atomic writes. If we are providing a list of KV pairs then isn't only one function enough to make db writes? Then we wouldn't need any separate functions just to write, just one function to pass a list of KV pairs

[ArtiomTr]

This method was defined in the lean specs to allow for atomic writes.

Got it, but remember - spec just describes how client should work externally, but internal implementation will (and should!) differ. The primary goal of spec is to make it more simple/readable, not performant.

If we are providing a list of KV pairs then isn't only one function enough to make db writes?

Yes, precisely. Although it is sometimes convenient to have just single put method, that is just alias for single kv pair insert:

fn put(&self, key: K, value: V) {
    self.put_all(vec![(key, value)]);
}

[ArtiomTr]

This also looks like poor abstraction - you rarely need to iterate through every database entry, while decompressing every value