refactor(dash-spv): checkpoint rewrite #375
Conversation
📝 WalkthroughWalkthroughCheckpoint types and loading were refactored to a compact, network-driven model; CheckpointManager now loads embedded checkpoint data and returns tuple-based checkpoint info. FFI removed Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ClientLifecycle
participant Sync as HeaderSyncManager
participant CP as CheckpointManager
participant Storage as Storage/DB
participant Peers as Peers/Network
rect rgba(200,230,201,0.5)
Client->>CP: new(network)
CP-->>Client: CheckpointManager
end
rect rgba(187,222,251,0.5)
Client->>CP: last_checkpoint_before_height(start_height)
CP-->>Client: (height, &HashedBlockHeader)
end
rect rgba(255,224,178,0.5)
Client->>Sync: init_from_checkpoint(height, state)
Sync->>Storage: store_chain_state(height, state)
Sync->>Peers: request_headers_from(height+1)
Peers-->>Sync: headers batch
Sync->>Storage: persist headers/cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
e9bd4cc to
c445e6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/src/checkpoints.rs`:
- Around line 44-50: Wrap calls to CheckpointManager::new in catch_unwind in the
FFI functions so panics from checkpoint decoding cannot unwind across the extern
"C" boundary; if catch_unwind catches a panic, call set_last_error with a
descriptive message and return a non-success FFIErrorCode, otherwise proceed to
call methods like CheckpointManager::last_checkpoint_before_height and return
FFIErrorCode::Success as before. Apply this change to both places where
CheckpointManager::new is invoked in this file (the two constructor call sites
that then call last_checkpoint_before_height and the later use at the other call
site), mapping panic results to an error code via set_last_error instead of
letting the panic propagate. Ensure you preserve the existing success path
behavior (setting out_height/out_hash) when no panic occurs.
In `@dash-spv/src/chain/checkpoints.rs`:
- Around line 109-111: The unreachable! in the decode error branch (inside the
bytes decode used in CheckpointManager::new) can panic on corrupted or
mismatched checkpoint data; change CheckpointManager::new to return Result<Self,
Error> (define or reuse an appropriate error type), replace the unreachable!
branch to return Err(with the decoding error wrapped) and update callers to
propagate/handle the Result; if you opt for the minimal change, include the
actual decode error in the panic message (e.g., panic!("failed to decode
checkpoint bytes: {}", err)) — but preferred is changing new() to return Result
and converting all places that call CheckpointManager::new to handle the error.
🧹 Nitpick comments (2)
dash-spv/src/test_utils/checkpoint.rs (1)
14-18: Make dummy input requirements explicit.
CheckpointManager::new_with_checkpointsexpects heights sorted and starting at genesis (0); adding explicit asserts here provides clearer test failures.♻️ Proposed guardrails
impl CheckpointManager { pub fn dummy(heights: &[u32]) -> CheckpointManager { + debug_assert!( + heights.first() == Some(&0), + "CheckpointManager::dummy expects heights to start with genesis (0)" + ); + debug_assert!( + heights.windows(2).all(|w| w[0] <= w[1]), + "CheckpointManager::dummy expects heights sorted ascending" + ); let checkpoints = heights.iter().map(|height| Checkpoint::dummy(*height)).collect::<Vec<_>>(); CheckpointManager::new_with_checkpoints(checkpoints) } }dash-spv/src/sync/headers/manager.rs (1)
200-200: Step 3 comment may be stale after removing checkpoint validation.The comment references "Step 3" but the checkpoint validation loop mentioned in the summary was removed. Consider renaming this to simply "Process the Entire Batch" or updating the step numbering to reflect the current flow (Step 1: Handle Empty Batch, Step 2: Validate Batch, Step 3: Process).
| let mgr = CheckpointManager::new(network.into()); | ||
|
|
||
| let (height, cp) = mgr.last_checkpoint_before_height(height); | ||
| *out_height = height; | ||
| let hash = cp.hash().to_byte_array(); | ||
| std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32); | ||
| FFIErrorCode::Success as i32 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the relevant files
echo "=== Locating checkpoint-related files ==="
fd -t f "checkpoints\.rs" --type f
echo -e "\n=== Checking dash-spv-ffi/src/checkpoints.rs existence ==="
test -f dash-spv-ffi/src/checkpoints.rs && echo "File exists" || echo "File not found"
echo -e "\n=== Checking dash-spv/src/chain/checkpoints.rs existence ==="
test -f dash-spv/src/chain/checkpoints.rs && echo "File exists" || echo "File not found"Repository: dashpay/rust-dashcore
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Read the FFI checkpoints.rs file to see the context around lines 44-50 and 70-76
echo "=== FFI checkpoints.rs content ==="
if [ -f dash-spv-ffi/src/checkpoints.rs ]; then
wc -l dash-spv-ffi/src/checkpoints.rs
echo ""
cat -n dash-spv-ffi/src/checkpoints.rs | head -100
fiRepository: dashpay/rust-dashcore
Length of output: 3152
🏁 Script executed:
#!/bin/bash
# Search for CheckpointManager::new and unreachable! in the dash-spv directory
echo "=== Searching for CheckpointManager in dash-spv ==="
rg -n "CheckpointManager|unreachable!" dash-spv/src/ -A 3 -B 1Repository: dashpay/rust-dashcore
Length of output: 6924
🏁 Script executed:
#!/bin/bash
# Read the full CheckpointManager::new implementation
echo "=== CheckpointManager::new implementation ==="
sed -n '92,120p' dash-spv/src/chain/checkpoints.rs | cat -nRepository: dashpay/rust-dashcore
Length of output: 1251
🏁 Script executed:
#!/bin/bash
# Check for other potential panic paths in CheckpointManager methods
echo "=== Methods called from FFI (last_checkpoint_before_height/timestamp) ==="
sed -n '130,210p' dash-spv/src/chain/checkpoints.rs | cat -nRepository: dashpay/rust-dashcore
Length of output: 3552
🏁 Script executed:
#!/bin/bash
# Verify the exact context and confirm there's no error handling pathway
echo "=== Checking FFI error handling for CheckpointManager::new ==="
sed -n '33,51p' dash-spv-ffi/src/checkpoints.rs | cat -n
echo -e "\n=== Second FFI function ==="
sed -n '59,77p' dash-spv-ffi/src/checkpoints.rs | cat -nRepository: dashpay/rust-dashcore
Length of output: 1728
🏁 Script executed:
#!/bin/bash
# Check if CheckpointManager provides a fallible constructor alternative
echo "=== All public CheckpointManager methods ==="
rg -n "pub.*fn" dash-spv/src/chain/checkpoints.rs | grep -i "checkpoint"Repository: dashpay/rust-dashcore
Length of output: 287
Prevent panics from crossing the FFI boundary.
CheckpointManager::new uses unreachable! when checkpoint decoding fails (dash-spv/src/chain/checkpoints.rs line 111). While the bytes are hardcoded, panics in library code will unwind across the extern "C" boundary and cause undefined behavior. Wrap the constructor call with catch_unwind to map panics to FFIErrorCode via set_last_error, or refactor CheckpointManager to support fallible construction.
This applies to both calls at lines 44 and 70 in this file.
🤖 Prompt for AI Agents
In `@dash-spv-ffi/src/checkpoints.rs` around lines 44 - 50, Wrap calls to
CheckpointManager::new in catch_unwind in the FFI functions so panics from
checkpoint decoding cannot unwind across the extern "C" boundary; if
catch_unwind catches a panic, call set_last_error with a descriptive message and
return a non-success FFIErrorCode, otherwise proceed to call methods like
CheckpointManager::last_checkpoint_before_height and return
FFIErrorCode::Success as before. Apply this change to both places where
CheckpointManager::new is invoked in this file (the two constructor call sites
that then call last_checkpoint_before_height and the later use at the other call
site), mapping panic results to an error code via set_last_error instead of
letting the panic propagate. Ensure you preserve the existing success path
behavior (setting out_height/out_hash) when no panic occurs.
| Err(_) => { | ||
| unreachable!("The bytes are hardcoded in the bin, decode cannot fail") | ||
| } |
There was a problem hiding this comment.
unreachable! on decode errors could panic with corrupted checkpoint files.
While the checkpoint bytes are indeed hardcoded at compile time, using unreachable! here means any corruption or version mismatch would cause a panic in release builds. Consider either:
- Returning a
Resultfromnew()to propagate the error, or - At minimum, adding the actual error to the panic message for debugging
As per coding guidelines, library code should avoid unwrap() and expect() in favor of explicit error types.
🛠️ Suggested improvement
Err(_) => {
- unreachable!("The bytes are hardcoded in the bin, decode cannot fail")
+ unreachable!("Checkpoint decode failed - checkpoint files may be corrupted or incompatible")
}Or preferably, make CheckpointManager::new return a Result<Self, Error>.
🤖 Prompt for AI Agents
In `@dash-spv/src/chain/checkpoints.rs` around lines 109 - 111, The unreachable!
in the decode error branch (inside the bytes decode used in
CheckpointManager::new) can panic on corrupted or mismatched checkpoint data;
change CheckpointManager::new to return Result<Self, Error> (define or reuse an
appropriate error type), replace the unreachable! branch to return Err(with the
decoding error wrapped) and update callers to propagate/handle the Result; if
you opt for the minimal change, include the actual decode error in the panic
message (e.g., panic!("failed to decode checkpoint bytes: {}", err)) — but
preferred is changing new() to return Result and converting all places that call
CheckpointManager::new to handle the error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dash-spv/src/client/lifecycle.rs`:
- Around line 254-256: Update the misleading comment that currently claims the
checkpoint header is not stored; instead reflect that the code does store the
checkpoint header via store_headers_at_height. Locate the comment next to the
call/use of store_headers_at_height in the lifecycle code (around the checkpoint
handling logic) and change the text to clearly state that the checkpoint header
is saved at that height and that subsequent headers will be requested from peers
starting after this stored checkpoint.
♻️ Duplicate comments (2)
dash-spv-ffi/src/checkpoints.rs (1)
44-50: Guard against panics crossing the FFI boundary.
CheckpointManager::newcan panic on decode errors (viaunreachable!in the loader), which would unwind acrossextern "C"here. Considercatch_unwind+set_last_error, or a fallible constructor that maps to anFFIErrorCode.Also applies to: 70-76
dash-spv/src/chain/checkpoints.rs (1)
101-111: Avoid panicking on decode failures.Even with embedded bytes, corruption or mismatched files can trigger
unreachable!and panic in library code. Consider a fallible constructor or error mapping instead. As per coding guidelines, avoid panics in library code.
🧹 Nitpick comments (1)
dash-spv/src/chain/checkpoints.rs (1)
12-22: Avoid hardcoding checkpoint heights.Consider generating the heights alongside the checkpoint bytes (or embedding a generated heights file) to prevent drift and keep network parameters out of source code. As per coding guidelines, avoid hardcoding network parameters.
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
c445e6c to
457e3b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/sync/headers/manager.rs (1)
351-360: Lazy-load checkpoint header to avoid blocking fresh syncs with empty storage.
prepare_syncunconditionally callsget_start_height()andget_header()at lines 351-360, which fails immediately withSyncError::Storagewhen storage is empty (sinceget_start_height()returnsNone). This error propagates before the genesis fallback logic at lines 375–400 can execute, breaking fresh syncs.Since
checkpoint_headeris only used within conditional blocks checkingis_synced_from_checkpoint()(lines 365, 528), only load it when actually syncing from a checkpoint:Suggested fix: Load checkpoint header conditionally
- let first_height = storage - .get_start_height() - .await - .ok_or(SyncError::Storage("Failed to get start height".to_string()))?; - let checkpoint_header = storage - .get_header(first_height) - .await - .map_err(|e| SyncError::Storage(format!("Failed to get first header: {}", e)))? - .ok_or(SyncError::Storage("Storage didn't return first header".to_string()))?; + let checkpoint_header = if self.is_synced_from_checkpoint() { + let first_height = storage + .get_start_height() + .await + .ok_or(SyncError::Storage("Failed to get start height".to_string()))?; + Some( + storage + .get_header(first_height) + .await + .map_err(|e| SyncError::Storage(format!("Failed to get first header: {}", e)))? + .ok_or(SyncError::Storage("Storage didn't return first header".to_string()))?, + ) + } else { + None + };Then guard all uses of
checkpoint_headerwithif let Some(ref checkpoint_header) = checkpoint_header { ... }.
🤖 Fix all issues with AI agents
In `@dash-spv/src/chain/checkpoints.rs`:
- Around line 187-191: The code currently uses expect when initializing
best_checkpoint, which can panic in release builds; make new_with_checkpoints
fallible by changing its signature to return Result<CheckpointManager,
YourErrorType>, replace the debug_assert! with an explicit runtime check that
returns Err if checkpoints.is_empty(), and in the initialization code (where
checkpoints.next().expect(...)) handle the None case by returning that Err
instead of panicking; update callers of new_with_checkpoints to propagate or
handle the Result accordingly and pick or define an appropriate error
enum/struct for empty checkpoints.
♻️ Duplicate comments (4)
dash-spv-ffi/src/checkpoints.rs (2)
44-50: Prevent panics from crossing the FFI boundary.
CheckpointManager::newusesunreachable!when checkpoint decoding fails. While the bytes are hardcoded, panics in library code will unwind across theextern "C"boundary and cause undefined behavior. Wrap the constructor call withcatch_unwindto map panics toFFIErrorCodeviaset_last_error, or refactorCheckpointManagerto support fallible construction.🔒 Recommended fix using catch_unwind
+use std::panic::catch_unwind; + pub unsafe extern "C" fn dash_spv_ffi_checkpoint_before_height( network: FFINetwork, height: u32, out_height: *mut u32, out_hash: *mut u8, ) -> i32 { if out_height.is_null() || out_hash.is_null() { set_last_error("Null output pointer provided"); return FFIErrorCode::NullPointer as i32; } - let mgr = CheckpointManager::new(network.into()); + let mgr = match catch_unwind(|| CheckpointManager::new(network.into())) { + Ok(m) => m, + Err(_) => { + set_last_error("Failed to initialize checkpoint manager"); + return FFIErrorCode::InternalError as i32; + } + }; let (height, cp) = mgr.last_checkpoint_before_height(height); *out_height = height; let hash = cp.hash().to_byte_array(); std::ptr::copy_nonoverlapping(hash.as_ptr(), out_hash, 32); FFIErrorCode::Success as i32 }Based on learnings, in Rust FFI code, unwrap() must not be used on operations that can panic, as it can cause undefined behavior when panics cross FFI boundaries.
70-76: Same panic risk applies here.The
CheckpointManager::newcall at line 70 has the same panic risk as identified indash_spv_ffi_checkpoint_before_height. Apply the samecatch_unwindpattern to prevent undefined behavior.dash-spv/src/client/lifecycle.rs (1)
229-268: Fix contradictory comment about checkpoint header storage.The code at line 247 explicitly stores the checkpoint header via
store_headers_at_height, but the comment at lines 254-255 states "Don't store the checkpoint header itself." This contradiction was previously flagged and marked as addressed, but the inconsistency remains.💡 Suggested fix
- // Don't store the checkpoint header itself - we'll request headers from peers - // starting from this checkpoint + // The checkpoint header is stored above; we'll request subsequent headers from peersdash-spv/src/chain/checkpoints.rs (1)
101-111: Decode failures still panic viaunreachable!.If embedded bytes are corrupted or incompatible,
unreachable!will panic in release. Prefer returning aResultfromCheckpointManager::new, or at least include the decode error for diagnosability. As per coding guidelines, avoid panics in library code.
🧹 Nitpick comments (2)
dash-spv/src/test_utils/checkpoint.rs (1)
13-19: Consider documenting the height 0 requirement.The
CheckpointManager::new_with_checkpointshas adebug_assertrequiring the first checkpoint to be at height 0. Test callers usingdummy(heights)must include 0 in the heights slice; otherwise, tests will panic in debug builds.📝 Suggested documentation
impl CheckpointManager { + /// Creates a dummy CheckpointManager for testing. + /// + /// # Panics (debug builds) + /// Panics if `heights` is empty or does not start with 0. pub fn dummy(heights: &[u32]) -> CheckpointManager { let checkpoints = heights.iter().map(|height| Checkpoint::dummy(*height)).collect::<Vec<_>>(); CheckpointManager::new_with_checkpoints(checkpoints) } }dash-spv/src/sync/headers/manager.rs (1)
579-582: Wire a real timestamp into checkpoint selection.Using
last_checkpoint_before_timestamp(0)will always return the earliest checkpoint (likely genesis), which removes most of the checkpoint benefit. Consider threading wallet creation time or a config-supplied timestamp into this call once available.
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
457e3b1 to
6593eb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dash-spv/src/chain/checkpoints.rs`:
- Around line 175-178: The Err(index) branch uses self.checkpoints[index - 1]
which underflows if index == 0; upgrade the debug-only invariant in
new_with_checkpoints to a runtime check (replace the debug_assert that ensures
the first checkpoint's height is 0 with assert! or explicitly return Err) so the
function enforces the first checkpoint's height == 0 in release builds and
prevents the underflow when Err(index) is taken; update new_with_checkpoints
(and its error handling) to either assert! that checkpoints.first().height == 0
or handle index == 0 explicitly before accessing index - 1.
In `@dash-spv/src/test_utils/checkpoint.rs`:
- Around line 14-18: The dummy helper currently forwards the provided heights
slice directly to CheckpointManager::new_with_checkpoints which debug-asserts
non-empty, sorted order, and starting at 0; normalize the input in dummy by
ensuring there is at least one height (insert 0 if empty), ensuring 0 is present
(insert 0 if missing), sorting the heights ascending, and removing duplicates
before mapping to Checkpoint::dummy and calling
CheckpointManager::new_with_checkpoints so the invariants (non-empty, sorted,
starts at 0) are always satisfied; update the dummy function accordingly.
♻️ Duplicate comments (4)
dash-spv-ffi/src/checkpoints.rs (1)
44-46: Guard against panics crossing the FFI boundary.
CheckpointManager::newcan panic on decode errors (viaunreachable!), which would unwind acrossextern "C". Wrap the construction withcatch_unwindand map failures toset_last_error+ a non-successFFIErrorCodeinstead of panicking.Also applies to: 70-72
dash-spv/src/client/lifecycle.rs (1)
254-255: Update the misleading checkpoint storage comment.The code stores the checkpoint header via
store_headers_at_height, so the comment currently states the opposite.✏️ Suggested comment update
- // Don't store the checkpoint header itself - we'll request headers from peers - // starting from this checkpoint + // Store the checkpoint header and request subsequent headers from peers + // starting from this checkpointdash-spv/src/chain/checkpoints.rs (2)
109-111:unreachable!on decode errors could panic with corrupted checkpoint files.This was flagged in a previous review but remains unaddressed. While checkpoint bytes are compile-time constants,
unreachable!provides no useful diagnostic if corruption occurs. Per coding guidelines, library code should avoid panic-inducing constructs.Consider making
new()returnResult<Self, Error>or at minimum include the error in the panic message.
189-190:expect()in library code violates coding guidelines.The past review marked this as addressed, but
expect()is still present. Per coding guidelines: "Avoidunwrap()andexpect()in library code; use explicit error types."Given the internal invariant that
CheckpointManageris never empty, this is unlikely to fail at runtime, but the guideline violation remains. Consider either:
- Changing the return type to
Option<(CoreBlockHeight, &HashedBlockHeader)>, or- Using indexing with a comment documenting the invariant:
&self.checkpoints[0]
🧹 Nitpick comments (2)
dash-spv/src/chain/checkpoints.rs (2)
192-197: Redundant height comparison in timestamp search loop.The condition
checkpoint.height >= best_checkpoint.heighton line 194 is always true because checkpoints are iterated in ascending height order. This check can be removed to simplify the logic.♻️ Suggested simplification
for checkpoint in checkpoints { - if checkpoint.hashed_block_header.header().time <= timestamp - && checkpoint.height >= best_checkpoint.height - { + if checkpoint.hashed_block_header.header().time <= timestamp { best_checkpoint = checkpoint; } }
49-53: Consider implementingOrdalongsidePartialOrd.The
PartialOrdimplementation always returnsSome(...), indicating a total ordering. ImplementingOrdwould be more semantically correct and enable use of methods that requireOrd. This is a minor improvement.♻️ Suggested addition
impl PartialOrd for Checkpoint { fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { Some(self.height.cmp(&other.height)) } } + +impl Ord for Checkpoint { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.height.cmp(&other.height) + } +} + +impl Eq for Checkpoint {}
| Err(index) => ( | ||
| self.checkpoints[index - 1].height, | ||
| &self.checkpoints[index - 1].hashed_block_header, | ||
| ), |
There was a problem hiding this comment.
Potential index underflow if checkpoint invariant is violated in release builds.
The Err(index) branch accesses checkpoints[index - 1], which would underflow if index == 0. This can only happen if the first checkpoint's height is > 0, which violates the invariant checked by debug_assert at line 154. However, debug_assert is stripped in release builds.
Since new_with_checkpoints is pub(crate), consider upgrading the height-0 check to a runtime assert! to prevent silent UB in release:
🛠️ Suggested improvement
- debug_assert_eq!(checkpoints[0].height, 0, "The first checkpoint must be at height 0");
+ assert_eq!(checkpoints[0].height, 0, "The first checkpoint must be at height 0");🤖 Prompt for AI Agents
In `@dash-spv/src/chain/checkpoints.rs` around lines 175 - 178, The Err(index)
branch uses self.checkpoints[index - 1] which underflows if index == 0; upgrade
the debug-only invariant in new_with_checkpoints to a runtime check (replace the
debug_assert that ensures the first checkpoint's height is 0 with assert! or
explicitly return Err) so the function enforces the first checkpoint's height ==
0 in release builds and prevents the underflow when Err(index) is taken; update
new_with_checkpoints (and its error handling) to either assert! that
checkpoints.first().height == 0 or handle index == 0 explicitly before accessing
index - 1.
I made this little rewrite of the CheckpointManager for the next reasons:
changes made include:
Summary by CodeRabbit
Removed Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.