Skip to content
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

Redb slasher backend impl #4529

Merged
merged 51 commits into from
Jul 1, 2024
Merged

Conversation

eserilev
Copy link
Collaborator

Issue Addressed

#4424

Proposed Changes

add redb implementation to the slasher backend

Additional Info

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Jul 26, 2023
@eserilev
Copy link
Collaborator Author

eserilev commented Aug 2, 2023

hi @michaelsproul

so I got all the slasher test cases to pass for the redb impl EXCEPT surrounds_existing_multi_vals_single_chunk. This test just hangs and never completes. I get a log message saying "test has been running for more than 60 seconds". Not sure whats going on there, will need to spend more time figuring that part out. In general though, everything else seems to be working

The code I wrote probably has a lot of room for improvement. I'm still sort of new to Rust and am having lots of fun with the borrow checker :). In general theres a few things I wanted to mention:

  • unlike lmdb and mdbx, redb has a concpt of database tables. so with this redb impl, instead of spinning up 9 separate databases, I created one database and 9 tables.
  • For now I'm opening all the tables in "write" mode. This probably needs to change for functions like get, next_key etc. tables are opened in read mode when relevant
  • Theres some general cleanup that needs to happen here. the impl of Database is mostly commented out. I have some functions there I'd like to use, but they dont play nice with the borrow checker. I need to spend some time figuring this out. Once this is resolved the code should be much cleaner
  • theres a single panic! I need to remove. Theres also a small snippit of useless code there that needs to be changed
  • Do we really need a separate commit function? As of now I've included the database commit directly in all of the relevant write functions.

If you get the chance, I could use a review of my changes up to this point. It would be great to get some general feedback so that I can understand if my implementation is at least trending in the right direction

@eserilev eserilev changed the title [WIP] Redb slasher backend impl Redb slasher backend impl Aug 8, 2023
@eserilev
Copy link
Collaborator Author

eserilev commented Aug 8, 2023

ready for a review

@eserilev eserilev marked this pull request as ready for review August 8, 2023 14:13
@michaelsproul michaelsproul added enhancement New feature or request ready-for-review The code is ready for review optimization Something to make Lighthouse run more efficiently. slasher and removed work-in-progress PR is a work-in-progress labels Aug 8, 2023

impl Environment {
pub fn new(config: &Config) -> Result<Environment, Error> {
let db_path = match config.database_path.join(BASE_DB).as_path().to_str() {
Copy link
Member

Choose a reason for hiding this comment

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

do we definitely need to join the BASE_DB here? I think the caller takes care of that already when constructing the config.database_path

@michaelsproul
Copy link
Member

I just pushed a minor optimisation that avoids collecting into an intermediate Vec: 6b0a81f

@michaelsproul
Copy link
Member

Also just back-merged unstable so the version reads 5.2

@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 24, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Just pushed one change to tidy up the DB filename, which we would have trouble changing later.

@michaelsproul
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Jul 1, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 70bcba1

@mergify mergify bot merged commit 70bcba1 into sigp:unstable Jul 1, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request optimization Something to make Lighthouse run more efficiently. ready-for-review The code is ready for review slasher
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants