Repository navigation
refactor(dash-spv)!: generate the checkpoints from the network - #375
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:
📝 Walkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly identifies the main change: refactoring dash-spv to generate checkpoints from the network. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
refactor/checkpoints
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
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).
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.
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.
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 {}
|
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
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. |
288b085 to
c51d2b2
Compare
github-actions
Bot
removed
the
merge-conflict
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
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
github-actions
Bot
added
the
waiting-bots
github-actions
Bot
removed
the
waiting-bots
362f762 to
9f3e136
Compare
github-actions
Bot
removed
the
merge-conflict
ZocoLini
changed the title
github-actions
Bot
added
bot-review-skipped
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
The checkpoints were written by hand, and the one at mainnet height 750000 had the wrong time and bits. The header of a checkpoint was rebuilt from those fields, so once the difficulty of the headers after it is checked, a wrong field rejects the real chain. A new `generate-checkpoints` binary syncs the headers of a network from genesis without a wallet and writes, every 50000 blocks, the checkpoint block and the 24 headers before it, validated, to `checkpoints/<network>.checkpoints`, which are embedded. A client starting from a checkpoint stores those 25 headers, so the difficulty of the blocks after it can be checked from the first one on. The generated blocks match the hand-written ones except for the time and bits at mainnet 750000. Genesis is no longer a checkpoint: it is still initialized on its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2921382 to
104e2c0
Compare
github-actions
Bot
removed
the
merge-conflict
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
`CheckpointManager::for_network` decoded the embedded checkpoints on every call, and its users kept the result in an `Arc`. The managers of mainnet and testnet are now decoded once into private statics, and `for_network` returns a `&'static` reference that the headers manager and pipeline keep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
|
@coderabbitai review No review for |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
the binary files are never gonna change, only new headers will be added,
New headers being added is exactly when the binary files will change and they also might change in other cases like. Like how you are adding the extra hashes now would change them as well unless i miss something.
Now those changes cause an unreadable binary diff.. of course, AI makes it easier after all.
readable JSON weights way more than binary, and for a library that matters in my opinion
Surely size matters this feels like a micro-optimisation. You could probably just convert them in build.rs even though the saving would be small anyway.
we already had a 'readable' version and we had the wrong values anyway.
Thats whats fixed when they get generated yeah. Kinda irrelevant to what format it was though, a manually crafted binary file could have had the same issue.
I would like to hear a stronger argument before deciding to go with JSON
At the end i don't want to keep arguing so if its that important to you feel free to merge it but imo having the trust anchors of the chain not readable to humans to save few kb seems as strange as the fact that i now have to get an agent going to convert me the files when i just want to have a look at them.
…ld.rs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
that was actually a good idea, implemented |
The checkpoints were written by hand, and the one at mainnet height 750000 had the wrong
timeandbits(1491953700/0x1a075a02instead of1507424630/0x1a027884). The client rebuilt the checkpoint header from those fields, so once the difficulty of the headers after it is checked (#1161), a wrong field rejects the real chain.Changes
generate-checkpointsbinary. It syncs the block headers of a network from genesis, without a wallet, filters or masternodes. Then it reads, throughDashSpvClient::header_at, the checkpoint block and the 24 headers before it every 50000 blocks (at least 1000 below the tip). It validates each set withBlockHeaderValidatorand writes them todash-spv/checkpoints/<network>.checkpoints.Checkpointis now its height and those 25 headers, the checkpoint block last, and the files are embedded withinclude_bytes!. The hand-written lists, and the fields and lookups only tests used (chain_work,masternode_list_name, timestamp lookup,validate_block,should_reject_fork), are gone.height - 24, so the difficulty of the blocks after it can be checked from the first one on. The rebuilt header with a zeroed version is gone.The generated blocks match the hand-written ones (prev hash, merkle root and nonce) at every height except for the
timeandbitsat mainnet 750000. Mainnet now has 51 checkpoints up to 2550000 (100 KB), and testnet 31 up to 1550000 (61 KB).To regenerate them:
Breaking:
CheckpointandCheckpointManagerhave a new API.Fixes the checkpoint review points on #1161.
🤖 Generated with Claude Code
PR Hygiene ·
da2f182/skip-botsproceeds without the ones not yet reported/self-revieweddash-spv(dash-spv/Cargo.toml,dash-spv/build.rs,dash-spv/checkpoints/mainnet.jsonand 10 more) — QuantumExplorer or xdustinfaceWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.