Skip to content

Commit 97e9e37

Browse files
PastaPastaPastadcgclaude
authored
fix: coderabbit review fixes (#104)
* fix(dash-spv): correct NetworkMessage enum variant name from GetMnListD to GetMnListDiff Fix compile error by using the correct enum variant GetMnListDiff instead of GetMnListD in masternodes sync code. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): add explicit error handling to find_chain_lock_quorum function Replace silent None returns with descriptive SyncError::Validation errors for: - Missing masternode list at or before height - Missing quorums for required LLMQ type - Missing quorum entries for LLMQ type This addresses CodeRabbitAI review comment to improve error visibility and debugging when chain lock validation fails due to missing data. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): align block message expectations with runtime behavior in MnList phase - Remove block message expectation during DownloadingMnList phase in is_message_expected_in_phase - Clarify comment to explicitly state blocks are intentionally ignored during MnList sync - Ensures expectation check matches actual runtime behavior where blocks are ignored 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): resolve storage handle shadowing in chainlock validation test Remove shadowing of storage handle by using the previously created mutable Box<DiskStorageManager> instead of calling client.storage(). This ensures proper borrowing and avoids variable shadowing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): use actual block height in MnListDiff validation errors Replace hardcoded placeholder height value of 0 with actual block height resolved from MnListDiff's block_hash via the MasternodeListEngine's block_container.get_height() method. This provides more accurate error reporting and improves debugging capabilities. Changes: - Resolve block height from diff.block_hash in validate_qr_info method - Update ValidationCacheKey to use actual block height instead of 0 - Update validation function comments to reflect height resolution capability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): replace MemoryStorage with MemoryStorageManager in validation tests Replace MemoryStorage::new() with MemoryStorageManager::new().await in validation_test.rs to use the correct async constructor and storage manager implementation that matches the expected StorageManager trait interface. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): correct test assertion for qr_info_extra_share default Update test in qrinfo_integration_test.rs to assert that config.qr_info_extra_share is false by default, matching ClientConfig::default() implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): add bounds checking to sync phase completion methods Add bounds checking to complete_qr_info_request and complete_mn_diff_request methods in phases.rs to prevent qr_info_completed and mn_diff_completed counters from exceeding their planned totals. The methods now only increment counters when below the planned limits and clamp requests_completed to not exceed requests_total. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(sml): use per-window mining_start for LLMQ type skip check Move the should_skip_quorum_type check inside the per-window loop and use window.mining_start instead of the outer range start. This prevents incorrectly skipping all windows when a range straddles activation height. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(docs): update API documentation to match actual dash-spv implementation Replace placeholder get_header_height with actual get_header_height_by_hash API name and update behavior description to match the real implementation in dash-spv/src/storage/mod.rs, memory.rs, and disk.rs. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): replace hardcoded quorum size with dynamic LLMQ size Replace hardcoded array size of 50 in smart_fetch_integration_test.rs with dynamic quorum size from LLMQ type parameters. This ensures the signers and valid_members vectors match the actual quorum size and prevent panics if LLMQ type parameters change. The fix extracts the quorum size using `llmq_type.size()` and uses it to create vectors of the correct length, making the test more robust and maintainable. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): update MockStorageManager to match current StorageManager trait Update MockStorageManager implementation in error_handling_test.rs to match the current StorageManager trait methods: - Replace get_headers_range -> load_headers - Replace get_header_by_hash -> get_header_height_by_hash (returns Option<u32>) - Replace store_filter_header -> store_filter_headers (&[FilterHeader]) - Replace store_header -> store_headers (&[BlockHeader]) - Replace get_stats -> stats (async) - Replace get_utxos_by_address -> get_utxos_for_address - Replace get_chain_state -> load_chain_state - Replace get_masternode_state -> load_masternode_state - Replace get_mempool_state -> load_mempool_state - Remove impl-only methods (compact_storage, get_utxo) - Add missing trait methods (clear, clear_mempool, clear_sync_state, get_headers_batch, get_all_mempool_transactions, get_chain_locks, load_filter*, store_filter, store_metadata, load_metadata, etc.) - Update remove_utxo signature to return () instead of Option<Utxo> 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Replace todo!() with real QRInfo response correlation implementation - Implement wait_for_qr_info_response with proper async channel handling - Add request/response correlation using QRInfoCorrelationManager - Include configurable timeout (30s) with proper error mapping - Add response validation to ensure tip_hash and base_hash match request - Implement cleanup_request method for resource management - Add global correlation manager accessor for dependency injection - Handle all error paths: timeout, channel closure, validation failures - Ensure cleanup occurs on both success and error paths 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(phase-3): correct processing_time to store elapsed duration instead of instant Fix processing_time field in QRInfoResult to properly capture and store elapsed duration from start to completion of QRInfo request processing. Changed field type from Instant to Duration and updated all usage sites to calculate elapsed time from captured start_time. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Replace todo!() with real QRInfo response correlation implementation - Implement wait_for_qr_info_response with proper async channel handling - Add request/response correlation using QRInfoCorrelationManager - Include configurable timeout (30s) with proper error mapping - Add response validation to ensure tip_hash and base_hash match request - Implement cleanup_request method and global manager accessor - Handle all error paths: timeout, channel closure, validation failures - Ensure cleanup occurs on both success and error paths - Fix QRInfoResult processing_time field to use Duration instead of Instant Addresses CodeRabbitAI review comment by replacing placeholder todo!() with complete implementation that correlates responses to requests and returns appropriate errors instead of panicking. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(qr_info_spv_plan): replace send_error with proper Result type handling Replace oneshot::Sender<QRInfo> with oneshot::Sender<Result<QRInfo, CorrelationError>> to enable proper error propagation in timeout cleanup. This fixes the invalid send_error method call by using standard Result error handling patterns. Changes: - Update PendingQRInfoRequest.response_sender type to Result<QRInfo, CorrelationError> - Update register_request return type to match - Replace send_error call with send(Err(CorrelationError::Timeout)) - Update successful response handling to send(Ok(qr_info)) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(qr_info_spv_plan): replace trait object Vec with enum-based recovery strategies - Replace Vec<Box<dyn RecoveryStrategy>> with Vec<RecoveryStrategyEnum> - Define RecoveryStrategyEnum with variants for all strategy types - Implement forwarding execute method on enum to avoid trait object issues - Add concrete implementations for all strategy types - Resolves async method safety problems with trait objects 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix QRInfoCorrelationManager pending_requests field synchronization - Wrap pending_requests HashMap in Mutex<HashMap<RequestId, PendingQRInfoRequest>> - Update all methods to acquire mutex lock for thread safety - Make methods async and change &mut self to &self where appropriate - Update test methods to use await for new async method signatures - Minimize locked sections to necessary operations only 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: integrate Phase 3 with existing dash-spv interfaces instead of duplicating Rewrite PHASE_3.md to follow integration rather than replacement approach: - Extend existing MasternodeSyncManager with feature-gated parallel QRInfo support - Enhance RequestController to add parallel scheduling while preserving validation - Augment RecoveryManager with parallel-aware strategies using existing patterns - Consolidate parallel config with existing ClientConfig structure and validation - Integrate QRInfo correlation into existing message handler routing - Reuse existing MockNetworkManager and storage for testing Key integration principles: - All parallel features behind #[cfg(feature = "parallel-qrinfo")] - Sequential sync remains unchanged and default - Existing APIs only extended, never replaced - Full backward compatibility maintained - Conservative defaults and adaptive concurrency limits 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(dash-spv): correct NetworkMessage enum variant to GetMnListD The NetworkMessage enum uses GetMnListD not GetMnListDiff as the variant name. This fixes the compilation error that was preventing the build from completing. * fix(build): resolve compilation errors in key-wallet-ffi and dash-spv - Fixed NetworkMessage enum variant from GetMnListDiff to GetMnListD - Removed non-existent address module import from key-wallet - Fixed AddressType conversion to use dashcore types - Commented out AddressGenerator as it doesn't exist in dashcore - Added missing Error enum pattern matches - Fixed InvalidAddress error variant usage The build now succeeds for all main packages except dash-fuzz and key-wallet-ffi which have deeper issues that need separate attention. * revert: undo extra changes to dash-spv/src/sync/sequential/mod.rs from commit 6be28eb Reverted the unintended changes to sequential/mod.rs that were included in commit 6be28eb. These changes modified: - Comment text about blocks during MnList phase - Removed the Block(_) pattern match for DownloadingMnList phase The original behavior has been restored where blocks are allowed during masternode sync phase. * revert: restore KeyWalletFFI targets in Package.swift from commit 5786b78 Reverted the unintended removal of KeyWalletFFI targets that were removed in commit 5786b78. Restored: - KeyWalletFFISwift product library - KeyWalletFFI target with its linker settings - KeyWalletFFISwift target These targets are needed for the key wallet FFI functionality. * chore: remove qr_info_spv_plan folder Removed the qr_info_spv_plan planning documentation folder as requested. This folder contained phase planning documents that are no longer needed. * revert: uncomment AddressGenerator interface in key_wallet.udl Restored the AddressGenerator interface definition that was commented out. This interface is needed for the FFI bindings to work properly. * revert: restore original key-wallet-ffi/src/lib.rs implementation Reverted all recent changes that replaced kw_address with dashcore types: - Restored 'address as kw_address' import - Reverted AddressType conversions back to kw_address types - Removed extra Error enum match cases (CoinJoinNotEnabled, Serialization, InvalidParameter) - Restored Address struct to use kw_address::Address - Uncommented AddressGenerator struct and all its methods - Restored generate() and generate_range() method implementations This brings the file back to its original working state with proper key-wallet address handling. * fix: restore working versions from v0.40-dev to fix build and test compilation - Restored key-wallet-ffi/src/lib.rs with correct imports (Address/AddressType from key_wallet) - Restored fuzz/Cargo.toml with required key-wallet dependency - Restored fuzz/fuzz_targets/dash/deserialize_psbt.rs to use key_wallet::psbt - Restored dash-spv-ffi test files with correct struct field initializations - Added sync_base_height and synced_from_checkpoint to ChainState tests - Added connected_peers, total_peers, header_height, filter_height to SpvStats tests These files were incorrectly modified in previous commits, causing build failures. Restoring them from origin/v0.40-dev resolves all compilation issues. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: add missing UnconfirmedTransaction import in error_handling_test.rs Added the missing import for UnconfirmedTransaction type which is used in the StorageManager trait implementation. This prevents compilation failure when the cfg gate is eventually removed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: dcg <dcg@dcgs-Mac-Studio.local> Co-authored-by: Claude <noreply@anthropic.com>
1 parent ab71a7a commit 97e9e37

File tree

18 files changed

+322
-4350
lines changed

18 files changed

+322
-4350
lines changed

dash-spv-ffi/src/platform_integration.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,9 @@ pub unsafe extern "C" fn ffi_dash_spv_get_quorum_public_key(
171171
);
172172
}
173173

174-
// Copy the public key directly from the global index
175-
let pubkey_ptr = public_key as *const _ as *const u8;
176-
std::ptr::copy_nonoverlapping(pubkey_ptr, out_pubkey, QUORUM_PUBKEY_SIZE);
174+
// Get the public key's canonical 48-byte representation safely
175+
let pubkey_bytes: &[u8; 48] = public_key.as_ref();
176+
std::ptr::copy_nonoverlapping(pubkey_bytes.as_ptr(), out_pubkey, QUORUM_PUBKEY_SIZE);
177177

178178
// Return success
179179
FFIResult {

dash-spv/src/network/message_handler.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,11 +92,11 @@ impl MessageHandler {
9292
self.stats.qrinfo_messages += 1;
9393
MessageHandleResult::QRInfo(qr_info)
9494
}
95-
NetworkMessage::GetQRInfo(_) => {
95+
NetworkMessage::GetQRInfo(req) => {
9696
// We don't serve QRInfo requests, only make them
9797
tracing::warn!("Received unexpected GetQRInfo request");
9898
self.stats.other_messages += 1;
99-
MessageHandleResult::Unhandled(message)
99+
MessageHandleResult::Unhandled(NetworkMessage::GetQRInfo(req))
100100
}
101101
other => {
102102
self.stats.other_messages += 1;

dash-spv/src/sync/chainlock_validation.rs

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -223,20 +223,38 @@ impl ChainLockValidator {
223223
// Get the masternode list at or before the height
224224
let mn_list_height = engine.masternode_lists.range(..=height).rev().next().map(|(h, _)| *h);
225225

226-
if let Some(list_height) = mn_list_height {
227-
if let Some(mn_list) = engine.masternode_lists.get(&list_height) {
228-
// Find the chain lock quorum
229-
if let Some(quorums) = mn_list.quorums.get(&self.config.required_llmq_type) {
230-
// Get the most recent quorum
231-
if let Some((quorum_hash, entry)) = quorums.iter().next() {
232-
// Get public key from the quorum entry
233-
return Ok(Some((*quorum_hash, entry.quorum_entry.quorum_public_key)));
234-
}
235-
}
236-
}
237-
}
238-
239-
Ok(None)
226+
let list_height = mn_list_height.ok_or_else(|| {
227+
SyncError::Validation(format!(
228+
"No masternode list found at or before height {}",
229+
height
230+
))
231+
})?;
232+
233+
let mn_list = engine.masternode_lists.get(&list_height).ok_or_else(|| {
234+
SyncError::Validation(format!(
235+
"Masternode list not found at height {}",
236+
list_height
237+
))
238+
})?;
239+
240+
// Find the chain lock quorum
241+
let quorums = mn_list.quorums.get(&self.config.required_llmq_type).ok_or_else(|| {
242+
SyncError::Validation(format!(
243+
"No quorums found for LLMQ type {:?} at masternode list height {}",
244+
self.config.required_llmq_type, list_height
245+
))
246+
})?;
247+
248+
// Get the most recent quorum
249+
let (quorum_hash, entry) = quorums.iter().next().ok_or_else(|| {
250+
SyncError::Validation(format!(
251+
"No quorum entries found for LLMQ type {:?}",
252+
self.config.required_llmq_type
253+
))
254+
})?;
255+
256+
// Get public key from the quorum entry
257+
Ok(Some((*quorum_hash, entry.quorum_entry.quorum_public_key)))
240258
}
241259

242260
/// Verify chain lock signature using the engine's built-in verification

dash-spv/src/sync/sequential/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1823,7 +1823,7 @@ impl SequentialSyncManager {
18231823
// If we have masternodes enabled, request masternode list updates for ChainLock validation
18241824
if self.config.enable_masternodes {
18251825
// For ChainLock validation, we need masternode lists at (block_height - CHAINLOCK_VALIDATION_MASTERNODE_OFFSET)
1826-
// So we request the masternode diff for this new block to maintain our rolling window
1826+
// We request the masternode diff for each new block (not just offset blocks) to maintain a complete rolling window
18271827
let base_block_hash = if height > 0 {
18281828
// Get the previous block hash
18291829
storage

dash-spv/src/sync/sequential/phases.rs

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -546,17 +546,22 @@ impl SyncPhase {
546546
if let SyncPhase::DownloadingMnList {
547547
sync_strategy:
548548
Some(HybridSyncStrategy::EngineDiscovery {
549+
qr_info_requests,
549550
qr_info_completed,
550551
..
551552
}),
552553
requests_completed,
554+
requests_total,
553555
last_progress,
554556
..
555557
} = self
556558
{
557-
*qr_info_completed += 1;
558-
*requests_completed += 1;
559-
*last_progress = Instant::now();
559+
// Only increment if we haven't reached the planned total
560+
if *qr_info_completed < *qr_info_requests {
561+
*qr_info_completed += 1;
562+
*requests_completed = (*requests_completed + 1).min(*requests_total);
563+
*last_progress = Instant::now();
564+
}
560565
}
561566
}
562567

@@ -565,19 +570,24 @@ impl SyncPhase {
565570
if let SyncPhase::DownloadingMnList {
566571
sync_strategy:
567572
Some(HybridSyncStrategy::EngineDiscovery {
573+
mn_diff_requests,
568574
mn_diff_completed,
569575
..
570576
}),
571577
requests_completed,
578+
requests_total,
572579
diffs_processed,
573580
last_progress,
574581
..
575582
} = self
576583
{
577-
*mn_diff_completed += 1;
578-
*requests_completed += 1;
579-
*diffs_processed += 1; // Backward compatibility
580-
*last_progress = Instant::now();
584+
// Only increment if we haven't reached the planned total
585+
if *mn_diff_completed < *mn_diff_requests {
586+
*mn_diff_completed += 1;
587+
*requests_completed = (*requests_completed + 1).min(*requests_total);
588+
*diffs_processed += 1; // Backward compatibility
589+
*last_progress = Instant::now();
590+
}
581591
}
582592
}
583593

dash-spv/src/sync/validation.rs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -233,14 +233,15 @@ impl ValidationEngine {
233233

234234
// Validate masternode list diffs
235235
for diff in &qr_info.mn_list_diff_list {
236+
let block_height = engine.block_container.get_height(&diff.block_hash).unwrap_or(0);
236237
match self.validate_mn_list_diff(diff, engine) {
237238
Ok(true) => items_validated += 1,
238239
Ok(false) => errors.push(ValidationError::InvalidMnListDiff(
239-
0, // We don't have block height in MnListDiff
240+
block_height,
240241
"Validation failed".to_string(),
241242
)),
242243
Err(e) => errors.push(ValidationError::InvalidMnListDiff(
243-
0, // We don't have block height in MnListDiff
244+
block_height,
244245
e.to_string(),
245246
)),
246247
}
@@ -277,7 +278,8 @@ impl ValidationEngine {
277278
diff: &MnListDiff,
278279
engine: &MasternodeListEngine,
279280
) -> SyncResult<bool> {
280-
let cache_key = ValidationCacheKey::MasternodeList(0); // Use 0 as we don't have block height
281+
let block_height = engine.block_container.get_height(&diff.block_hash).unwrap_or(0);
282+
let cache_key = ValidationCacheKey::MasternodeList(block_height);
281283

282284
// Check cache
283285
if let Some(cached) = self.get_cached_result(&cache_key) {
@@ -297,11 +299,11 @@ impl ValidationEngine {
297299
fn perform_mn_list_diff_validation(
298300
&self,
299301
diff: &MnListDiff,
300-
_engine: &MasternodeListEngine,
302+
engine: &MasternodeListEngine,
301303
) -> SyncResult<bool> {
302304
// Check if we have the base list
303-
// Note: We can't check by height as MnListDiff doesn't contain block height
304-
// We would need to look up the height from the block hash
305+
// We can resolve block height from the diff's block hash using the engine's block container
306+
let block_height = engine.block_container.get_height(&diff.block_hash).unwrap_or(0);
305307

306308
// Validate merkle root matches
307309
// TODO: Implement merkle root validation

dash-spv/src/sync/validation_test.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
#[cfg(test)]
44
mod tests {
55
use crate::client::ClientConfig;
6-
use crate::storage::MemoryStorage;
6+
use crate::storage::MemoryStorageManager;
77
use crate::sync::chainlock_validation::{ChainLockValidationConfig, ChainLockValidator};
88
use crate::sync::masternodes::MasternodeSyncManager;
99
use crate::sync::validation::{ValidationConfig, ValidationEngine};
@@ -136,7 +136,7 @@ mod tests {
136136
async fn test_qr_info_validation() {
137137
let config = create_test_config();
138138
let _sync_manager = MasternodeSyncManager::new(&config);
139-
let _storage = MemoryStorage::new();
139+
let _storage = MemoryStorageManager::new().await.expect("Failed to create MemoryStorageManager");
140140

141141
// Create mock QRInfo
142142
let _qr_info = create_mock_qr_info();

dash-spv/tests/chainlock_validation_test.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ async fn test_chainlock_queue_and_process_flow() {
288288
let storage_path = temp_dir.path().to_path_buf();
289289

290290
// Create storage
291-
let storage = Box::new(DiskStorageManager::new(storage_path).await.unwrap());
291+
let mut storage = Box::new(DiskStorageManager::new(storage_path).await.unwrap());
292292
let network = Box::new(MockNetworkManager::new());
293293

294294
// Create client config
@@ -325,7 +325,6 @@ async fn test_chainlock_queue_and_process_flow() {
325325

326326
// Process pending (will fail validation but clear the queue)
327327
let chain_state = ChainState::new();
328-
let storage = client.storage();
329328
let _ = chainlock_manager.validate_pending_chainlocks(&chain_state, &mut *storage).await;
330329

331330
// Verify queue is cleared
@@ -363,13 +362,12 @@ async fn test_chainlock_manager_cache_operations() {
363362

364363
// Add test headers
365364
let genesis = genesis_block(Network::Dash).header;
366-
let storage = client.storage();
365+
let mut storage = client.storage();
367366
// storage.store_header(&genesis, 0).await.unwrap();
368367

369368
// Create and process a ChainLock
370369
let chain_lock = create_test_chainlock(0, genesis.block_hash());
371370
let chain_state = ChainState::new();
372-
let storage = client.storage();
373371
let _ =
374372
chainlock_manager.process_chain_lock(chain_lock.clone(), &chain_state, &mut *storage).await;
375373

0 commit comments

Comments
 (0)