feat(dash-spv): add --birth-height CLI flag#849
Open
xdustinface wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe SPV CLI adds an optional ChangesBirth height CLI flow
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant resolve_birth_height
participant CheckpointManager
participant Wallet
CLI->>resolve_birth_height: birth-height argument and network
resolve_birth_height->>CheckpointManager: resolve now/latest
CheckpointManager-->>resolve_birth_height: latest checkpoint height
resolve_birth_height-->>CLI: concrete birth height
CLI->>Wallet: create wallet with birth height
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #849 +/- ##
==========================================
- Coverage 74.29% 74.29% -0.01%
==========================================
Files 325 325
Lines 73910 73938 +28
==========================================
+ Hits 54911 54931 +20
- Misses 18999 19007 +8
|
xdustinface
force-pushed
the
feat/anchor-to-checkpoint
branch
2 times, most recently
from
02850d4 to
efef0e9
Compare
xdustinface
force-pushed
the
feat/birth-height-cli
branch
from
3728bec to
72f89de
Compare
4 tasks
Let the SPV CLI set the wallet birth height so header, filter-header and filter sync anchor at the nearest checkpoint at or before it instead of syncing the whole chain from genesis. `now`/`latest` resolves to the highest bundled checkpoint, the right anchor for a brand-new wallet with no prior history. An unspecified value keeps the previous full-scan default of genesis.
xdustinface
force-pushed
the
feat/birth-height-cli
branch
from
72f89de to
9b1e709
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dash-spv/src/main.rs (2)
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
"now"semantics between--start-heightand--birth-height.
--start-height nowresolves tou32::MAX(lines 349-350) while--birth-height nowresolves to the latest checkpoint height. Additionally,--start-heightdoesn't accept"latest"while--birth-heightdoes. Users using both flags may expect the same keyword to behave consistently. Consider aligning the keyword handling or documenting the difference in the CLI help text.🤖 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/main.rs` around lines 118 - 128, Align the keyword handling for start_height and birth_height so “now” and “latest” have consistent semantics when both options are used. Update the parsing logic associated with start_height and birth_height to accept the same keywords and resolve them to the same latest-checkpoint behavior, or explicitly document any intentional difference in both CLI help descriptions.
472-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for checkpointless networks.
The test covers Mainnet only. For devnet/regtest,
CheckpointManager::for_networkhas no checkpoints, so"now"/"latest"resolve to0(or an error, if the suggested fix is applied). Adding this edge case would prevent regressions.🧪 Suggested test additions
// Non-numeric junk is rejected. assert!(resolve_birth_height(&Some("abc".to_string()), Network::Mainnet).is_err()); + + // Devnet has no bundled checkpoints, so "now" resolves to genesis. + assert_eq!( + resolve_birth_height(&Some("now".to_string()), Network::Devnet).unwrap(), + 0 + ); + assert_eq!( + resolve_birth_height(&None, Network::Regtest).unwrap(), + 0 + );🤖 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/main.rs` around lines 472 - 500, Extend birth_height_resolves_none_now_and_numeric to cover a checkpointless network such as Devnet or Regtest: verify how resolve_birth_height handles both "now" and "latest" when CheckpointManager::for_network(...).last_checkpoint() is absent, expecting the defined result of 0 or the appropriate error behavior. Keep the existing Mainnet assertions unchanged.
🤖 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.
Inline comments:
In `@dash-spv/src/main.rs`:
- Around line 304-308: Update the “now”/“latest” branch in the checkpoint-height
parsing logic to avoid silently converting a missing checkpoint into height 0.
When CheckpointManager::for_network(network).last_checkpoint() returns None,
return an explicit error for checkpointless networks; preserve the existing
checkpoint height result when a checkpoint is available.
---
Nitpick comments:
In `@dash-spv/src/main.rs`:
- Around line 118-128: Align the keyword handling for start_height and
birth_height so “now” and “latest” have consistent semantics when both options
are used. Update the parsing logic associated with start_height and birth_height
to accept the same keywords and resolve them to the same latest-checkpoint
behavior, or explicitly document any intentional difference in both CLI help
descriptions.
- Around line 472-500: Extend birth_height_resolves_none_now_and_numeric to
cover a checkpointless network such as Devnet or Regtest: verify how
resolve_birth_height handles both "now" and "latest" when
CheckpointManager::for_network(...).last_checkpoint() is absent, expecting the
defined result of 0 or the appropriate error behavior. Keep the existing Mainnet
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78c4a853-140c-410b-a577-91b194d565da
📒 Files selected for processing (1)
dash-spv/src/main.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Let the SPV CLI set the wallet birth height so header, filter-header and filter sync anchor at the nearest checkpoint at or before it instead of syncing the whole chain from genesis.
now/latestresolves to the highest bundled checkpoint, the right anchor for a brand-new wallet with no prior history. An unspecified value keeps the previous full-scan default of genesis.Based on:
Summary by CodeRabbit
--birth-heightCLI option to set the wallet’s initial anchoring height.