feat(dash-spv): anchor sync at wallet birth height checkpoint#848
Conversation
📝 WalkthroughWalkthroughThis PR adds a ChangesCheckpoint anchoring for SPV client startup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #848 +/- ##
==========================================
+ Coverage 73.43% 73.53% +0.09%
==========================================
Files 324 324
Lines 73150 73197 +47
==========================================
+ Hits 53721 53826 +105
+ Misses 19429 19371 -58
|
cd6c04e to
02850d4
Compare
When no explicit `start_from_height` is configured, `DashSpvClient::new` now derives the sync anchor from the wallet's `earliest_required_height` and starts from the nearest checkpoint at or before it. Previously the wallet birth height only short-circuited the compact-filter download, while block headers and filter headers always synced from genesis. Explicit `start_from_height` still takes precedence. This also fixes the checkpoint anchoring itself, which never worked. `initialize_genesis_block` rebuilt the checkpoint header with a hardcoded version `0x20000000` and verified the recomputed hash against the stored `block_hash`. Old blocks use version `2`, so the hash never matched, the code logged a warning and silently fell back to a full genesis sync, defeating `start_from_height` entirely. The `Checkpoint` does not store the block version, so the exact header bytes cannot be reconstructed. Since the checkpoint `block_hash` is a trusted constant and chain linkage only ever compares against the stored hash (never a recomputed one), we now store the anchor via the new `HashedBlockHeader::with_trusted_hash`, pairing the reconstructed header with the trusted hash. `time` and `bits` still come from the checkpoint for difficulty checks of later headers.
02850d4 to
efef0e9
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dash-spv/src/client/lifecycle.rs (1)
282-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
checkpoint.target.to_compact_lossy()directlyThe extra
CompactTarget::from_consensus(...to_consensus())round-trip is redundant here;Header::bitsalready takes aCompactTarget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/client/lifecycle.rs` around lines 282 - 284, `Header::bits` is being assigned through a redundant `CompactTarget::from_consensus(...to_consensus())` round-trip in `lifecycle.rs`. Simplify the `checkpoint.target` conversion in the `Header` construction by using `checkpoint.target.to_compact_lossy()` directly for the `bits` field, and remove the unnecessary consensus conversion chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dash-spv/src/client/lifecycle.rs`:
- Around line 282-284: `Header::bits` is being assigned through a redundant
`CompactTarget::from_consensus(...to_consensus())` round-trip in `lifecycle.rs`.
Simplify the `checkpoint.target` conversion in the `Header` construction by
using `checkpoint.target.to_compact_lossy()` directly for the `bits` field, and
remove the unnecessary consensus conversion chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e4970ae-da78-4460-8fea-0bcf32833503
📒 Files selected for processing (4)
dash-spv/src/chain/checkpoints.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mod.rsdash-spv/src/types.rs
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
looks good, sam said something about main net starting in 720.000, are you working on that in a different PR??
From the conversation in standup i was under the impression thats already being done in the app. If not we can follow up to this PR. I think it feels a bit more like something we should do in the app even though it would be centralized in rust-dashcore.. Its just, we could potentially add support for non-hd keys.. just no idea if there would be any demand or something we want at all. |
|
yeah, I understand. now that you mention it, I think I saw something a long time ago, but that was with the old sync pipeline, I don't know if it still there |
2 participants
When no explicit
start_from_heightis configured,DashSpvClient::newnow derives the sync anchor from the wallet'searliest_required_heightand starts from the nearest checkpoint at or before it. Previously the wallet birth height only short-circuited the compact-filter download, while block headers and filter headers always synced from genesis. Explicitstart_from_heightstill takes precedence.This also fixes the checkpoint anchoring itself, which never worked.
initialize_genesis_blockrebuilt the checkpoint header with a hardcoded version0x20000000and verified the recomputed hash against the storedblock_hash. Old blocks use version2, so the hash never matched, the code logged a warning and silently fell back to a full genesis sync, defeatingstart_from_heightentirely.The
Checkpointdoes not store the block version, so the exact header bytes cannot be reconstructed. Since the checkpointblock_hashis a trusted constant and chain linkage only ever compares against the stored hash (never a recomputed one), we now store the anchor via the newHashedBlockHeader::with_trusted_hash, pairing the reconstructed header with the trusted hash.timeandbitsstill come from the checkpoint for difficulty checks of later headers.Based on:
birth_height#693Summary by CodeRabbit
New Features
Bug Fixes
Tests