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

fix(engine): use provider to initialize persistence state #9785

Merged
merged 3 commits into from
Jul 27, 2024

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Jul 24, 2024

Previously, on node startup, these would be zero, making should_persist always true on startup, also causing the engine to send empty blocks to save_blocks.

Now these are initialized using values from the provider factory, which should correspond to the values that are actually on-disk.

@Rjected Rjected added C-bug An unexpected or incorrect behavior A-blockchain-tree Related to sidechains, reorgs and pending blocks labels Jul 24, 2024
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.

hmm, should these be initialized with the current on disk head?

but I guess we can make these an option first until we have an initial persistence run?

let best_block_number = provider.best_block_number().unwrap_or(0);
let header = provider.sealed_header(best_block_number).ok().flatten().unwrap_or_default();

@Rjected
Copy link
Member Author

Rjected commented Jul 25, 2024

hmm, should these be initialized with the current on disk head?

yeah actually that would be better

@Rjected Rjected force-pushed the dan/move-last-persisted-to-options branch from 6904cd0 to e82a424 Compare July 25, 2024 17:38
@Rjected Rjected changed the title fix(engine): use Options for last_persisted fields fix(engine): use provider to initialize persistence state Jul 25, 2024
@Rjected Rjected requested a review from mattsse July 25, 2024 17:41
Copy link
Member

@fgimenez fgimenez left a comment

Choose a reason for hiding this comment

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

lgtm!

@mattsse
Copy link
Collaborator

mattsse commented Jul 25, 2024

failing test

Previously, on node startup, these would be zero, making
`should_persist` always true, also causing the engine to send empty
blocks to `save_blocks`.

Now these are options, which are only set when a persistence task
finished. Otherwise, the start for any persistence distance measurement
will be the minimum block stored in the tree.
@Rjected Rjected force-pushed the dan/move-last-persisted-to-options branch from e82a424 to 885cdc7 Compare July 26, 2024 23:18
@Rjected Rjected added this pull request to the merge queue Jul 26, 2024
Merged via the queue into main with commit 800e247 Jul 27, 2024
33 checks passed
@Rjected Rjected deleted the dan/move-last-persisted-to-options branch July 27, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-tree Related to sidechains, reorgs and pending blocks C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants