-
Notifications
You must be signed in to change notification settings - Fork 748
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
Implement freezer database #508
Conversation
And fix a bug in the compact committees accessor.
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.
Self-review
I'm going to start a review on this. I expect to be done tomorrow! |
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 really like what you've done here! It's so exciting to see storage optimization!!
I've left a bunch of unimportant comments here, sorry for the noise. I think the only main thing I'd like to see is the RwLock
pushed away from the "user".
In summary, awesome!
} | ||
|
||
/// Fetch a state from the store. | ||
fn get_state<E: EthSpec>( |
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.
WRT needing additional additional fields in put/get, I was thinking we could split StoreItem
into get/put and do :
However, I'm not sure that's better than what you have here. I'm just mentioning it FYI.
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.
Why did you change your mind on this? I was thinking it might be quite nice, and would resolve your concern about phase1 as a first class citizen
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'm not sure why I changed my mind on this, but I've changed it back again. I'm still down to have separate Put
/Get
traits. Perhaps not for this PR.
This is ready for review now. The previous issue with non-zero genesis values has been resolved, with special handling for them in
|
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.
Super clean! Not a whole lot has changed since the last review :)
I'm not recommending any changes, happy to squerge!
} | ||
|
||
/// Fetch a state from the store. | ||
fn get_state<E: EthSpec>( |
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'm not sure why I changed my mind on this, but I've changed it back again. I'm still down to have separate Put
/Get
traits. Perhaps not for this PR.
|
||
/// Provides a wrapper for an iterator that returns a given `T` before it starts returning results of | ||
/// the `Iterator`. | ||
pub struct ReverseChainIterator<T, I> { |
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.
Just to confirm, state Iterators
aren't presently optimized for KPDB at the moment?
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.
No, not yet, but they're not obviously suboptimal AFAICT
@@ -51,6 +72,20 @@ pub trait Store: Sync + Send + Sized { | |||
I::db_delete(self, key) | |||
} | |||
|
|||
/// Store a state in the store. | |||
fn put_state<E: EthSpec>( |
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'm not suggesting any change here, but I was considering these functions and how the Store
trait alone is a bit inflexible.
I was thinking that if we need to keep adding functionality to this (I'm not saying we will, it just seems possible) that perhaps we could use a builder instead.
At this present moment, we have the following requirements:
- Ability to have type-specific store/put impls (we have this already)
- Ability to specify parameters of the request (we don't already have this).
E.g.,
let state: BeaconState<E> = RequestBuilder::new(store)
.slot(42)
.err_if_none() // <-- seems cool
.get()?;
Once again, I'm not suggesting a change to the PR, I'm just thinking out loud :)
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.
Yeah that does seem cool! Would be worth investigating for sure
Issue Addressed
#499
Proposed Changes
FixedVector<T, N>
fields into tables.Freezer database is opt-in via the--db disk
option. The old disk database remains available under--db simple-disk
.Shortcomings
Additional Info
BeaconState
that we could store efficiently, but I'll leave that for future work.