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

feat(snapshots): write to snapshots during BodyStage #5733

Merged
merged 132 commits into from
Jan 26, 2024

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Dec 12, 2023

Writes TransactionSignedNoHash to static files instead of mdbx during BodyStage.

  • Includes this PR feat: remove tx_range from filename #5823
  • Each static file has a fixed range of 500k blocks.
  • SegmentHeader block range in the *.conf file is the actual block range (eg. the latest one will have less than 500k blocks), while the filename block range is fixed to the 500k interval. This allows for easier file consistency and querying.
  • Adds SnapshotProviderRW used by stages to append data. It can also unwind/truncate data when an inconsistency error occurs (eg. node shuts down after a static file commit, but before the database could commit).

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.

I coulf follow this quite well, though I'd like to see more docs in the bodies stage that explains in detail what is happening

crates/stages/src/error.rs Show resolved Hide resolved
let mut next_tx_num = tx_cursor.last()?.map(|(id, _)| id + 1).unwrap_or_default();
let mut next_tx_num = tx_block_cursor.last()?.map(|(id, _)| id + 1).unwrap_or_default();

let snapshot_provider = provider.snapshot_provider().expect("should exist");
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I don't really like this panic.

I guess this is an expect because unreachable?

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, stages [should] receive a DatabaseProvider with a SnapshotProvider

crates/storage/provider/src/providers/snapshot/writer.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

It looks ok and we tested it, so let's merge into a feature branch and fix bugs that will pop up later to unblock other PRs building on top of this branch. LGTM.

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.

lgtm,

pedantic last nits

@@ -53,7 +53,7 @@ impl Command {
let mut row_indexes = tx_range.clone().collect::<Vec<_>>();

let path: PathBuf = SnapshotSegment::Receipts
.filename_with_configuration(filters, compression, &block_range, &tx_range)
.filename_with_configuration(filters, compression, &block_range)
Copy link
Collaborator

Choose a reason for hiding this comment

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

fewer args good

crates/interfaces/src/provider.rs Outdated Show resolved Hide resolved
crates/primitives/src/snapshot/mod.rs Outdated Show resolved Hide resolved
crates/primitives/src/snapshot/segment.rs Outdated Show resolved Hide resolved
crates/primitives/src/snapshot/segment.rs Show resolved Hide resolved
[self.data_path().into(), self.index_path(), self.offsets_path(), self.config_path()]
{
if path.exists() {
std::fs::remove_file(path)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the primitives fs

path::Path,
};

/// Size of one offset in bytes.
const OFFSET_SIZE_BYTES: u64 = 8;

/// Holds a reference or an owned [`NippyJar`].
#[derive(Debug)]
enum CowJar<'a, H: NippyJarHeader = ()> {
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 Clone so the Cow prefix isn't accurate. though I'm fine this, because it conveys what does

joshieDo and others added 5 commits January 26, 2024 12:07
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: joshieDo <ranriver@protonmail.com>
@joshieDo joshieDo merged commit 3dcb835 into feat/static-files Jan 26, 2024
24 checks passed
@joshieDo joshieDo deleted the joshie/tx-snap-sync branch January 26, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-static-files Related to static files C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants