Skip to content

feat: add Metadata table and StorageSettings to ProviderFactory#19384

Merged
joshieDo merged 10 commits intomainfrom
joshie/storage-settings
Nov 6, 2025
Merged

feat: add Metadata table and StorageSettings to ProviderFactory#19384
joshieDo merged 10 commits intomainfrom
joshie/storage-settings

Conversation

@joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Oct 29, 2025

towards #18890 (comment)

  • add generic Metadata table.
  • add StorageSettings entry to above table.
  • Initialize StorageSettings on init_genesis
  • Cache storage settings on ProviderFactory and DatabaseProvider and after init_genesis

#19384 <- #19399 <- #19401

@joshieDo joshieDo self-assigned this Oct 29, 2025
@joshieDo joshieDo added C-enhancement New feature or request A-db Related to the database labels Oct 29, 2025
@github-project-automation github-project-automation bot moved this to Backlog in Reth Tracker Oct 29, 2025
@joshieDo joshieDo force-pushed the joshie/storage-settings branch from 3293ca2 to b4e32d6 Compare October 29, 2025 14:00
@joshieDo joshieDo marked this pull request as ready for review October 29, 2025 14:03
@joshieDo joshieDo force-pushed the joshie/storage-settings branch from 49ada86 to 62b4443 Compare October 29, 2025 21:12
Comment on lines +13 to +18
pub struct StorageSettings {
/// Whether this node always writes receipts to static files.
///
/// If this is set to FALSE AND receipt pruning IS ENABLED, all receipts should be written to DB. Otherwise, they should be written to static files. This ensures that older nodes do not need to migrate their current DB tables to static files. For more, read: <https://github.com/paradigmxyz/reth/issues/18890#issuecomment-3457760097>
pub receipts_on_static_files: bool,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make this so that we can easily extend this and store arbitrary info?

Copy link
Member

@Rjected Rjected Nov 3, 2025

Choose a reason for hiding this comment

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

yeah I think we will also want something for snap sync here, maybe we could change receipts_on_static_files to static_files_v2_enabled since I think I'll just reuse this value to determine whether or not to write changesets in #18882

maybe this should just be a map?

Copy link
Collaborator Author

@joshieDo joshieDo Nov 5, 2025

Choose a reason for hiding this comment

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

couldnt we just add new booleans here as we need them and make the sf roll out changes more gradually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fn write_storage_settings(&self, settings: StorageSettings) -> ProviderResult<()> {
self.write_metadata(
keys::STORAGE_SETTINGS,
serde_json::to_vec(&settings).map_err(ProviderError::other)?,
)

this is also being encoded as serde_json so extending this should be fine

Comment on lines +544 to +549
/// Stores generic node metadata as key-value pairs.
/// Can store feature flags, configuration markers, and other node-specific data.
table Metadata {
type Key = String;
type Value = Vec<u8>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see this also works because then we can store arbitrary metadata entries like key value storage

static_file_provider: StaticFileProvider<N::Primitives>,
prune_modes: PruneModes,
storage: Arc<N::Storage>,
storage_settings: Arc<RwLock<StorageSettings>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might want to extend this in the future with things like snapsync node or smth, but for now i think this is okay

Arc::new(RwLock::new(StorageSettings::default())),
)
.storage_settings()?
.unwrap_or_default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is correct here because this defaults to false, could we make this explicit with a constructor fn on StorageSettings

Comment on lines +93 to +95
Arc::new(RwLock::new(StorageSettings::default())),
)
.storage_settings()?
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels a bit strange, why does this need StorageSettings as input in order to return it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it feels weird because its a bit cyclical, open to suggestions.

The overall idea is that both ProviderFactory and DatabaseProvider are instantiated with a cached storage settings object. But to obtain it in the first place, ProviderFactory needs to create a DatabaseProvider to fetch it from storage. That's what's happening here.

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Oct 30, 2025
@joshieDo joshieDo force-pushed the joshie/storage-settings branch 4 times, most recently from e5d730d to eea160a Compare October 31, 2025 12:48
@joshieDo joshieDo force-pushed the joshie/storage-settings branch from eea160a to 6429250 Compare October 31, 2025 12:54
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

smol style suggestion

static_file_provider.clone(),
Default::default(),
Default::default(),
Arc::new(RwLock::new(legacy_settings)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, isn't this the same as creating Self first and then updating the storage_settings?

Comment on lines +87 to +88
let legacy_settings = StorageSettings::legacy();
let storage_settings = DatabaseProvider::<_, N>::new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

please document what's going on here, why are we doing this

@joshieDo joshieDo added this pull request to the merge queue Nov 6, 2025
Merged via the queue into main with commit e20e56b Nov 6, 2025
41 checks passed
@joshieDo joshieDo deleted the joshie/storage-settings branch November 6, 2025 00:56
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-db Related to the database C-enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants