Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe checkpoint subsystem is refactored from a multi-field public struct with HashMap-based storage to a compact structure using a Vec with simplified field access. CheckpointManager now accepts a Network parameter to load embedded network-specific checkpoint data. Related modules and test utilities are updated to use the new API. The test module is removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
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).
dash-spv-ffi/src/checkpoints.rs
Outdated
| 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 {}
dash-spv/src/chain/checkpoints.rs
Outdated
| 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.
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
6593eb2 to
288b085
Compare
github-actions
bot
added
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
288b085 to
c51d2b2
Compare
github-actions
bot
removed
the
merge-conflict
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
c51d2b2 to
45bd3e7
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/client/lifecycle.rs (1)
63-83:⚠️ Potential issue | 🟠 MajorRemove ChainLockManager initialization from the filters block to prevent panic when filters are enabled without masternodes.
The code initializes ChainLockManager at lines 76–82 inside the
if config.enable_filtersblock, but ChainLockManager requires aMasternodeListEngine, which is only created whenenable_masternodes=true. With filters enabled and masternodes disabled (a valid configuration),masternode_engineisNoneand.expect()panics. ChainLockManager is correctly initialized again at lines 90–101 inside theif config.enable_masternodesblock—this is the only place it should be created.Remove the duplicate ChainLockManager initialization from the filters block:
Suggested fix
if config.enable_filters { managers.filter_headers = Some(FilterHeadersManager::new(storage.block_headers(), storage.filter_headers())); managers.filters = Some(FiltersManager::new( wallet.clone(), storage.block_headers(), storage.filter_headers(), storage.filters(), )); - let masternode_list_engine = masternode_engine - .clone() - .expect("Masternode list engine must exist if masternodes are enabled"); - managers.chainlock = Some(ChainLockManager::new( - storage.block_headers(), - storage.metadata(), - masternode_list_engine.clone(), - )); managers.blocks = Some(BlocksManager::new(wallet.clone(), storage.block_headers(), storage.blocks())); }This also aligns with the coding guideline to avoid
expect()in library code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/client/lifecycle.rs` around lines 63 - 83, Remove the ChainLockManager creation from inside the if config.enable_filters block (delete the masternode_list_engine.expect(...) and managers.chainlock = Some(ChainLockManager::new(...)) there) so filters can be enabled without masternodes; only initialize ChainLockManager in the if config.enable_masternodes block where masternode_engine is guaranteed, and avoid using expect() on masternode_engine (handle the None case or gate the code behind enable_masternodes) so managers.chainlock is only set when a valid MasternodeListEngine exists.
♻️ Duplicate comments (2)
dash-spv/src/chain/checkpoints.rs (2)
89-107:⚠️ Potential issue | 🟡 MinorAvoid panicking on checkpoint decode failures.
unreachable!will still panic if embedded bytes are corrupted or incompatible; consider returning a proper error instead of aborting.#!/bin/bash # Locate call sites if you make CheckpointManager::new fallible. rg -n "CheckpointManager::new\s*\(" -C2As per coding guidelines: Use proper error types (thiserror) and propagate errors appropriately; avoid unwrap/expect in library code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/chain/checkpoints.rs` around lines 89 - 107, The loop in CheckpointManager::new (which calls checkpoints_bytes and Checkpoint::consensus_decode) uses unreachable! on any non-IO decode error and will panic if the embedded bytes are corrupted; make construction fallible instead: change CheckpointManager::new to return Result<Self, YourErrorType> (define an error enum with thiserror), map the non-EOF encode::Error variants into a decode error variant and propagate that Result up, and at call sites update usages discovered (e.g., CheckpointManager::new) to handle the Result rather than allowing a panic.
145-149:⚠️ Potential issue | 🟡 MinorGuard against Err(0) underflow in binary_search.
If height < first checkpoint (or the invariant is violated in release),index - 1will underflow.🛠️ Suggested fix
- match self.checkpoints.binary_search_by_key(&height, |checkpoint| checkpoint.height) { - Ok(index) => &self.checkpoints[index], - Err(index) => &self.checkpoints[index - 1], - } + match self.checkpoints.binary_search_by_key(&height, |checkpoint| checkpoint.height) { + Ok(index) => &self.checkpoints[index], + Err(0) => &self.checkpoints[0], + Err(index) => &self.checkpoints[index - 1], + }#!/bin/bash # Review call sites to confirm height is never below the first checkpoint. rg -n "last_checkpoint_before_height" -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/chain/checkpoints.rs` around lines 145 - 149, The binary_search result can produce Err(0) which makes index - 1 underflow in last_checkpoint_before_height; update last_checkpoint_before_height to guard the Err case: when Err(0) return a reference to the first checkpoint (e.g. &self.checkpoints[0] or self.checkpoints.first().expect(...)) instead of doing index - 1, and keep the existing Ok branch unchanged; locate the method named last_checkpoint_before_height and the Vec field checkpoints to apply this check (or alternatively change the signature to return Option<&Checkpoint> and return None for heights below the first checkpoint if call sites are updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/src/chain/checkpoints.rs`:
- Around line 152-168: The checkpoints_between function can panic when
start_height > end_height because start_index may be > end_index before slicing;
update checkpoints_between (and its callers such as BlockHeadersPipeline::init)
to validate or normalize inputs: check if start_height >= end_height and either
return an empty Vec or swap/adjust the heights, then compute
start_index/end_index and only slice when start_index <= end_index; ensure
start_index and end_index are clamped to checkpoints.len() to avoid
out-of-bounds slicing and document the chosen behavior.
---
Outside diff comments:
In `@dash-spv/src/client/lifecycle.rs`:
- Around line 63-83: Remove the ChainLockManager creation from inside the if
config.enable_filters block (delete the masternode_list_engine.expect(...) and
managers.chainlock = Some(ChainLockManager::new(...)) there) so filters can be
enabled without masternodes; only initialize ChainLockManager in the if
config.enable_masternodes block where masternode_engine is guaranteed, and avoid
using expect() on masternode_engine (handle the None case or gate the code
behind enable_masternodes) so managers.chainlock is only set when a valid
MasternodeListEngine exists.
---
Duplicate comments:
In `@dash-spv/src/chain/checkpoints.rs`:
- Around line 89-107: The loop in CheckpointManager::new (which calls
checkpoints_bytes and Checkpoint::consensus_decode) uses unreachable! on any
non-IO decode error and will panic if the embedded bytes are corrupted; make
construction fallible instead: change CheckpointManager::new to return
Result<Self, YourErrorType> (define an error enum with thiserror), map the
non-EOF encode::Error variants into a decode error variant and propagate that
Result up, and at call sites update usages discovered (e.g.,
CheckpointManager::new) to handle the Result rather than allowing a panic.
- Around line 145-149: The binary_search result can produce Err(0) which makes
index - 1 underflow in last_checkpoint_before_height; update
last_checkpoint_before_height to guard the Err case: when Err(0) return a
reference to the first checkpoint (e.g. &self.checkpoints[0] or
self.checkpoints.first().expect(...)) instead of doing index - 1, and keep the
existing Ok branch unchanged; locate the method named
last_checkpoint_before_height and the Vec field checkpoints to apply this check
(or alternatively change the signature to return Option<&Checkpoint> and return
None for heights below the first checkpoint if call sites are updated
accordingly).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
dash-spv/checkpoints/mainnet.checkpointsdash-spv/checkpoints/testnet.checkpointsdash-spv/src/chain/checkpoint_test.rsdash-spv/src/chain/checkpoints.rsdash-spv/src/chain/mod.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/block_headers/manager.rsdash-spv/src/sync/block_headers/pipeline.rsdash-spv/src/test_utils/checkpoint.rsdocs/implementation-notes/CHECKPOINT_IMPLEMENTATION.md
💤 Files with no reviewable changes (1)
- dash-spv/src/chain/checkpoint_test.rs
✅ Files skipped from review due to trivial changes (1)
- docs/implementation-notes/CHECKPOINT_IMPLEMENTATION.md
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/test_utils/checkpoint.rs
| // Returns a vector of checkpoints where checkpoint height is in the range (start_height, end_height] | ||
| pub fn checkpoints_between(&self, start_height: u32, end_height: u32) -> Vec<&Checkpoint> { | ||
| let start_index = match self | ||
| .checkpoints | ||
| .binary_search_by_key(&start_height, |checkpoint| checkpoint.height) | ||
| { | ||
| Ok(index) => index + 1, | ||
| Err(index) => index, | ||
| }; | ||
| let end_index = match self | ||
| .checkpoints | ||
| .binary_search_by_key(&end_height, |checkpoint| checkpoint.height) | ||
| { | ||
| Ok(index) => index + 1, | ||
| Err(index) => index, | ||
| }; | ||
| self.checkpoints[start_index..end_index].iter().collect() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the checkpoints.rs file
find . -name "checkpoints.rs" -type fRepository: dashpay/rust-dashcore
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Read the checkpoints.rs file to see the actual implementation
wc -l dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the relevant section of the file
sed -n '140,180p' dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 1351
🏁 Script executed:
#!/bin/bash
# Search for all callers of checkpoints_between
rg -n "checkpoints_between" --type rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the function signature and any documentation
sed -n '145,175p' dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 1165
🏁 Script executed:
#!/bin/bash
# Search for all calls to checkpoints_between
rg "checkpoints_between" -C 3Repository: dashpay/rust-dashcore
Length of output: 1748
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for checkpoints_between
sed -n '180,300p' dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 4686
🏁 Script executed:
#!/bin/bash
# Look for the struct definition and understand the context
sed -n '1,50p' dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 1650
🏁 Script executed:
#!/bin/bash
# Get more context around the call to checkpoints_between in pipeline.rs
rg -B 10 -A 10 "checkpoints_between" dash-spv/src/sync/block_headers/pipeline.rsRepository: dashpay/rust-dashcore
Length of output: 818
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about parameter constraints
sed -n '150,172p' dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 823
🏁 Script executed:
#!/bin/bash
# Look for any validation or assertions in the checkpoint module
rg -n "assert|panic" dash-spv/src/chain/checkpoints.rsRepository: dashpay/rust-dashcore
Length of output: 906
Add validation to prevent panic when start_height > end_height.
The checkpoints_between() function will panic if start_index > end_index due to invalid slice range. Guard against this by either validating the input parameters or checking the computed indices before slicing.
The function is called from BlockHeadersPipeline::init() with user-provided heights, and per coding guidelines, inputs from callers should be validated to prevent panics.
Suggested fix
pub fn checkpoints_between(&self, start_height: u32, end_height: u32) -> Vec<&Checkpoint> {
+ if start_height > end_height {
+ return Vec::new();
+ }
let start_index = match self
.checkpoints
.binary_search_by_key(&start_height, |checkpoint| checkpoint.height)
{
Ok(index) => index + 1,
Err(index) => index,
};
let end_index = match self
.checkpoints
.binary_search_by_key(&end_height, |checkpoint| checkpoint.height)
{
Ok(index) => index + 1,
Err(index) => index,
};
- self.checkpoints[start_index..end_index].iter().collect()
+ if start_index >= end_index {
+ return Vec::new();
+ }
+ self.checkpoints[start_index..end_index].iter().collect()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv/src/chain/checkpoints.rs` around lines 152 - 168, The
checkpoints_between function can panic when start_height > end_height because
start_index may be > end_index before slicing; update checkpoints_between (and
its callers such as BlockHeadersPipeline::init) to validate or normalize inputs:
check if start_height >= end_height and either return an empty Vec or
swap/adjust the heights, then compute start_index/end_index and only slice when
start_index <= end_index; ensure start_index and end_index are clamped to
checkpoints.len() to avoid out-of-bounds slicing and document the chosen
behavior.
ZocoLini
force-pushed
the
refactor/checkpoints
branch
from
45bd3e7 to
362f762
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
1 participant
I made this little rewrite of the CheckpointManager for the next reasons:
changes made include:
Summary by CodeRabbit
Refactor
Tests
Documentation