-
Notifications
You must be signed in to change notification settings - Fork 59
perf(drive-abci): don't create GroveDB checkpoints while replaying history #4570
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
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| mod replay; | ||
| mod serialization; | ||
| mod spawn; | ||
|
|
||
| pub use replay::is_historical_block; | ||
|
PastaPastaPasta marked this conversation as resolved.
Outdated
|
||
| pub use serialization::from_opt_str_or_number; | ||
| pub use serialization::from_str_or_number; | ||
| pub use spawn::spawn_blocking_task_with_name_if_supported; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| //! Telling a node that is replaying history from one that is following the tip. | ||
| //! | ||
| //! Some per-block work only earns its cost at the tip. Creating a GroveDB | ||
| //! checkpoint every ten minutes of chain time is useful on a running node and | ||
| //! pure waste while catching up, where ten minutes of chain time is a handful of | ||
| //! blocks and every checkpoint but the last few is deleted within the second. | ||
|
|
||
| /// A block older than this is not one the network just produced. Mainnet aims at | ||
| /// about 2.5 minutes a block, so this leaves several blocks of slack for a node | ||
| /// that is merely a little behind. | ||
| const HISTORICAL_BLOCK_AGE_MS: u64 = 10 * 60 * 1000; | ||
|
|
||
| /// True when a block with this timestamp is old enough that the node producing | ||
| /// it is clearly replaying history rather than following the tip. | ||
| pub fn is_historical_block(block_time_ms: u64) -> bool { | ||
| let now_ms = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|since_epoch| since_epoch.as_millis() as u64) | ||
| .unwrap_or(0); | ||
| now_ms.saturating_sub(block_time_ms) > HISTORICAL_BLOCK_AGE_MS | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn now_ms() -> u64 { | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .expect("system clock is before the unix epoch") | ||
| .as_millis() as u64 | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_year_ago_is_historical() { | ||
| assert!(is_historical_block( | ||
| now_ms() - 365 * 24 * 60 * 60 * 1000 | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_minute_ago_is_not_historical() { | ||
| assert!(!is_historical_block(now_ms() - 60 * 1000)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_at_the_threshold_is_not_yet_historical() { | ||
| assert!(!is_historical_block(now_ms() - HISTORICAL_BLOCK_AGE_MS)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_timestamped_in_the_future_is_not_historical() { | ||
| assert!(!is_historical_block(now_ms() + 60 * 1000)); | ||
| } | ||
| } | ||
|
PastaPastaPasta marked this conversation as resolved.
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Blocking: A stale chain tip can leave a synchronized node with no checkpoint
Block age is not equivalent to replay status. If a fresh node catches up while the network tip is more than ten minutes old, such as during a consensus halt, this branch skips every checkpoint including the actual tip. No additional block-finalization callback runs when catch-up completes, so the promised first real checkpoint is never created until the network produces another block. This leaves a fully synchronized node unable to serve address full-tree synchronization:
prove_address_funds_trunk_query_v0explicitly selectsGroveDBToUse::LatestCheckpoint, whose GroveDB query returnsNoCheckpointsAvailablewhen the registry is empty. Use an actual catch-up/tip signal or otherwise ensure completion creates a checkpoint instead of inferring synchronization state solely from the block timestamp.source: ['claude']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not changed in code. The limitation is documented in the
replay.rsmodule doc on head9dfffa61(lines 8-11): block age is a proxy for catching up, and a node that finishes syncing during a network halt will treat the tip as historical and skip the checkpoint until the next block arrives. Whether that trade-off is acceptable needs maintainer acceptance, so this thread is left open.🤖 Posted autonomously by Claude on behalf of pasta.