Skip to content

Conversation

owanikin
Copy link

Issue Address

#8048

Started addressing the Slasher database migration requirement for Redb as discussed.

Proposed Changes

  • Added upgrade() support for Redb in slasher/src/database/redb_impl.rs.
  • Integrated upgrade() into slasher/src/database/interface.rs.
  • Incremented schema version for Redb backend.
  • Handled upgrade/downgrade logic in slasher/src/migrate.rs.
  • Fixed clippy issues

Additional Info

  • Tested locally with Holesky by running Lighthouse beacon node + Slasher using Redb. I am having issues here, as I think I am not testing it properly. Will request advice from @eserilev

@owanikin owanikin marked this pull request as ready for review September 27, 2025 04:05
Copy link
Member

@eserilev eserilev left a comment

Choose a reason for hiding this comment

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

Thanks! I took a brief look and requested some changes

also the match(schema_version, CURRENT_SCHEMA_VERSION) has some cases that look a bit weird to me, I'll take a deeper look once we've cleaned this PR up a bit.

Comment on lines 106 to 109
pub fn upgrade(&self) -> Result<(), Error> {
tracing::info!("Checking SlasherDB Redb internals for upgrade");
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

should this function be upgrading the db?

Copy link
Author

Choose a reason for hiding this comment

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

Right now, this upgrade() in redb_impl.rs is just a backend hook.
The actual schema version check and upgrade orchestration happens in slasher/src/migrate.rs (via SlasherDB::migrate), which calls self.env.upgrade() and then writes the new schema version into the metadata_db.

For Redb specifically, there aren’t any low-level internal migrations needed at the moment, so this is effectively a no-op.
I left the function in place so the Redb backend matches the Environment interface and can support backend-specific migrations in the future if needed.

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.

2 participants