-
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 all commits
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(crate) use replay::is_historical_block; | ||
| 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,73 @@ | ||
| //! 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. | ||
| //! | ||
| //! Block age is a proxy for "catching up", not a measurement of it. If the | ||
| //! network itself has not produced a block for longer than the threshold, a node | ||
| //! that finishes syncing during the halt sees the tip as historical too, and the | ||
| //! work gated on this predicate waits for the next block. | ||
|
|
||
| /// 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(crate) 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); | ||
| is_historical_block_at(block_time_ms, now_ms) | ||
| } | ||
|
|
||
| /// The comparison behind [`is_historical_block`], taking the current time as an | ||
| /// argument so the boundary can be tested without racing the real clock. | ||
| fn is_historical_block_at(block_time_ms: u64, now_ms: u64) -> bool { | ||
| now_ms.saturating_sub(block_time_ms) > HISTORICAL_BLOCK_AGE_MS | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| const NOW_MS: u64 = 2_000_000_000_000; | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_year_ago_is_historical() { | ||
| assert!(is_historical_block_at( | ||
| NOW_MS - 365 * 24 * 60 * 60 * 1000, | ||
| NOW_MS | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_minute_ago_is_not_historical() { | ||
| assert!(!is_historical_block_at(NOW_MS - 60 * 1000, NOW_MS)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_at_the_threshold_is_not_yet_historical() { | ||
| assert!(!is_historical_block_at( | ||
| NOW_MS - HISTORICAL_BLOCK_AGE_MS, | ||
| NOW_MS | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_one_millisecond_past_the_threshold_is_historical() { | ||
| assert!(is_historical_block_at( | ||
| NOW_MS - HISTORICAL_BLOCK_AGE_MS - 1, | ||
| NOW_MS | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_timestamped_in_the_future_is_not_historical() { | ||
| assert!(!is_historical_block_at(NOW_MS + 60 * 1000, NOW_MS)); | ||
| } | ||
| } | ||
|
PastaPastaPasta marked this conversation as resolved.
|
||
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
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
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.