-
Notifications
You must be signed in to change notification settings - Fork 10
Clean/remove memory storage #275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
25f5700
8b2f417
6ce3a96
33c0eac
19a4b80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,7 @@ | |
| mod tests { | ||
| use crate::client::block_processor::{BlockProcessingTask, BlockProcessor}; | ||
|
|
||
| use crate::storage::memory::MemoryStorageManager; | ||
| use crate::storage::StorageManager; | ||
| use crate::storage::DiskStorageManager; | ||
| use crate::types::{SpvEvent, SpvStats}; | ||
| use dashcore::{blockdata::constants::genesis_block, Block, Network, Transaction}; | ||
|
|
||
|
|
@@ -92,17 +91,19 @@ mod tests { | |
| } | ||
|
|
||
| async fn setup_processor() -> ( | ||
| BlockProcessor<MockWallet, MemoryStorageManager>, | ||
| BlockProcessor<MockWallet, DiskStorageManager>, | ||
| mpsc::UnboundedSender<BlockProcessingTask>, | ||
| mpsc::UnboundedReceiver<SpvEvent>, | ||
| Arc<RwLock<MockWallet>>, | ||
| Arc<Mutex<MemoryStorageManager>>, | ||
| Arc<Mutex<DiskStorageManager>>, | ||
| ) { | ||
| let (task_tx, task_rx) = mpsc::unbounded_channel(); | ||
| let (event_tx, event_rx) = mpsc::unbounded_channel(); | ||
| let stats = Arc::new(RwLock::new(SpvStats::default())); | ||
| let wallet = Arc::new(RwLock::new(MockWallet::new())); | ||
| let storage = Arc::new(Mutex::new(MemoryStorageManager::new().await.unwrap())); | ||
| let storage = Arc::new(Mutex::new( | ||
| DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage"), | ||
| )); | ||
|
Comment on lines
+94
to
+106
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cd /tmp && find . -name "block_processor_test.rs" 2>/dev/null | head -5Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: cd /tmp && find . -name "manager.rs" -path "*/storage/*" 2>/dev/null | head -5Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Search for block_processor_test.rs from repo root
git ls-files | grep -E "block_processor.*test"Repository: dashpay/rust-dashcore Length of output: 108 🏁 Script executed: # Search for DiskStorageManager and with_temp_dir implementation
rg "with_temp_dir" --type rust -A 10Repository: dashpay/rust-dashcore Length of output: 8533 🏁 Script executed: # Also search for DiskStorageManager struct definition
rg "struct DiskStorageManager" --type rust -A 5Repository: dashpay/rust-dashcore Length of output: 498 🏁 Script executed: cat -n dash-spv/src/client/block_processor_test.rs | head -120Repository: dashpay/rust-dashcore Length of output: 5000 🏁 Script executed: cat -n dash-spv/src/storage/manager.rs | head -80Repository: dashpay/rust-dashcore Length of output: 3300 🏁 Script executed: # Look at the full with_temp_dir implementation and surrounding code
rg "pub async fn with_temp_dir" -A 20 dash-spv/src/storage/manager.rsRepository: dashpay/rust-dashcore Length of output: 834 🏁 Script executed: # Check if TempDir is stored anywhere in DiskStorageManager
rg "TempDir" --type rust dash-spv/src/storage/Repository: dashpay/rust-dashcore Length of output: 2135 🏁 Script executed: cd dash-spv && cargo test --lib client::block_processor_test::tests --no-fail-fast 2>&1 | head -100Repository: dashpay/rust-dashcore Length of output: 174 🏁 Script executed: # Also try running the specific test with more details
cd dash-spv && cargo test --lib client::block_processor_test::tests::test_process_block -- --nocapture 2>&1Repository: dashpay/rust-dashcore Length of output: 174 🏁 Script executed: # Check if tests pass overall
cd dash-spv && cargo test --lib client::block_processor_test 2>&1 | tail -50Repository: dashpay/rust-dashcore Length of output: 174 🏁 Script executed: # Search for any TODO or comments about the temp_dir bug
rg "TODO|FIXME|BUG" dash-spv/src/storage/ --type rust -B 2 -A 2Repository: dashpay/rust-dashcore Length of output: 698 🏁 Script executed: # Look at the complete with_temp_dir implementation and what happens to TempDir
cat -n dash-spv/src/storage/manager.rs | sed -n '100,140p'Repository: dashpay/rust-dashcore Length of output: 2112 🏁 Script executed: # Check if TempDir is persisted anywhere or if there's a comment about the bug
rg "TempDir" -B 3 -A 3 dash-spv/src/storage/manager.rsRepository: dashpay/rust-dashcore Length of output: 267 🏁 Script executed: # Search for any recent changes or concerns about with_temp_dir
git log --oneline --all -- "dash-spv/src/storage/manager.rs" | head -10Repository: dashpay/rust-dashcore Length of output: 115 🏁 Script executed: # Look at DiskStorageManager struct definition completely to see all fields
rg "pub struct DiskStorageManager" -A 25 dash-spv/src/storage/manager.rsRepository: dashpay/rust-dashcore Length of output: 1023 🏁 Script executed: # Check if TempDir might be stored in LockFile or any Arc that persists
rg "struct LockFile" -A 10 dash-spv/src/storage/lockfile.rsRepository: dashpay/rust-dashcore Length of output: 400 🏁 Script executed: # Verify the complete chain of what happens after with_temp_dir returns
cat -n dash-spv/src/storage/manager.rs | sed -n '127,155p'Repository: dashpay/rust-dashcore Length of output: 1308 🏁 Script executed: # Check if there are any comments about this being intentional
rg "with_temp_dir\|TempDir" -B 5 -A 5 dash-spv/src/storage/manager.rs | head -40Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Look at how storage is used in actual tests after setup_processor
cat -n dash-spv/src/client/block_processor_test.rs | sed -n '119,160p'Repository: dashpay/rust-dashcore Length of output: 1947 🏁 Script executed: # Check if storage operations are deferred or happen immediately
rg "load_or_new\|create_dir" dash-spv/src/storage/manager.rs -B 2 -A 2 | head -50Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Look at what SegmentCache::load_or_new does during initialization
rg "pub async fn load_or_new" dash-spv/src/storage/segments.rs -A 15Repository: dashpay/rust-dashcore Length of output: 582 🏁 Script executed: # Check when files are actually accessed - during new() or lazily
cat -n dash-spv/src/storage/manager.rs | sed -n '64,90p'Repository: dashpay/rust-dashcore Length of output: 1488 🏁 Script executed: # See if there's documentation about the temp behavior
grep -r "with_temp_dir" dash-spv/ --include="*.rs" -B 2 -A 2 | grep -E "///|//.*temp|//.*dir" | head -20Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Check the complete implementation of with_temp_dir and what happens to the temp_dir variable
cat -n dash-spv/src/storage/manager.rs | sed -n '152,160p'Repository: dashpay/rust-dashcore Length of output: 365 🏁 Script executed: # Verify that DiskStorageManager only stores base_path, not the TempDir
rg "pub struct DiskStorageManager" -A 30 dash-spv/src/storage/manager.rs | grep -E "TempDir|_temp"Repository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Check if there's any async behavior that might defer the TempDir drop
rg "with_temp_dir" -B 2 -A 10 dash-spv/src/client/block_processor_test.rs | head -25Repository: dashpay/rust-dashcore Length of output: 934 🏁 Script executed: # Look for any comments in tests indicating workarounds or known issues
rg "TODO|FIXME|BUG|HACK" dash-spv/src/client/block_processor_test.rsRepository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Check if there's a wrapper or if TempDir is stored elsewhere in Arc
rg "TempDir\|temp_dir" dash-spv/src/storage/ --type rustRepository: dashpay/rust-dashcore Length of output: 47 🏁 Script executed: # Look at how tempfile::TempDir behaves - specifically if there's any way it could persist
rg "tempfile" dash-spv/Cargo.toml -A 2Repository: dashpay/rust-dashcore Length of output: 120 🏁 Script executed: # Check if there are any other usages of with_temp_dir that might give hints
rg "with_temp_dir" --type rust -B 3 -A 8Repository: dashpay/rust-dashcore Length of output: 9290 🏁 Script executed: # See if maybe TempDir is stored in some Arc somewhere up the chain
cat -n dash-spv/src/client/block_processor.rs | head -50Repository: dashpay/rust-dashcore Length of output: 2028 🏁 Script executed: # Verify the complete lifecycle - let's trace if TempDir is somehow stored or if the path persists
cat -n dash-spv/src/storage/manager.rs | sed -n '153,158p'Repository: dashpay/rust-dashcore Length of output: 290 🏁 Script executed: # Check tempfile documentation behavior
python3 << 'EOF'
# Explain tempfile::TempDir behavior
print("tempfile::TempDir behavior:")
print("- TempDir::new() creates a temporary directory")
print("- When TempDir is dropped, it deletes the directory automatically")
print("- TempDir is NOT automatically persisted")
print("- Passing path via .into() creates a PathBuf (just a path string)")
print("- After with_temp_dir() returns, TempDir goes out of scope and directory is deleted")
EOFRepository: dashpay/rust-dashcore Length of output: 151 Store The Add a 🤖 Prompt for AI Agents |
||
| let processor = BlockProcessor::new( | ||
| task_rx, | ||
| wallet.clone(), | ||
|
|
@@ -305,7 +306,9 @@ mod tests { | |
| let (event_tx, mut event_rx) = mpsc::unbounded_channel(); | ||
| let stats = Arc::new(RwLock::new(SpvStats::default())); | ||
| let wallet = Arc::new(RwLock::new(NonMatchingWallet {})); | ||
| let storage = Arc::new(Mutex::new(MemoryStorageManager::new().await.unwrap())); | ||
| let storage = Arc::new(Mutex::new( | ||
| DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage"), | ||
| )); | ||
|
|
||
| let processor = | ||
| BlockProcessor::new(task_rx, wallet, storage, stats, event_tx, Network::Dash); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,65 +307,29 @@ async fn run() -> Result<(), Box<dyn std::error::Error>> { | |
| } | ||
| }; | ||
|
|
||
| // Create and start the client based on storage type | ||
| if config.enable_persistence { | ||
| if let Some(path) = &config.storage_path { | ||
| let storage_manager = | ||
| match dash_spv::storage::DiskStorageManager::new(path.clone()).await { | ||
| Ok(sm) => sm, | ||
| Err(e) => { | ||
| eprintln!("Failed to create disk storage manager: {}", e); | ||
| process::exit(1); | ||
| } | ||
| }; | ||
| run_client( | ||
| config, | ||
| network_manager, | ||
| storage_manager, | ||
| wallet, | ||
| enable_terminal_ui, | ||
| &matches, | ||
| wallet_id, | ||
| ) | ||
| .await?; | ||
| } else { | ||
| let storage_manager = match dash_spv::storage::MemoryStorageManager::new().await { | ||
| Ok(sm) => sm, | ||
| Err(e) => { | ||
| eprintln!("Failed to create memory storage manager: {}", e); | ||
| process::exit(1); | ||
| } | ||
| }; | ||
| run_client( | ||
| config, | ||
| network_manager, | ||
| storage_manager, | ||
| wallet, | ||
| enable_terminal_ui, | ||
| &matches, | ||
| wallet_id, | ||
| ) | ||
| .await?; | ||
| } | ||
| let path = if let Some(path) = &config.storage_path { | ||
| path.clone() | ||
| } else { | ||
| let storage_manager = match dash_spv::storage::MemoryStorageManager::new().await { | ||
| Ok(sm) => sm, | ||
| Err(e) => { | ||
| eprintln!("Failed to create memory storage manager: {}", e); | ||
| process::exit(1); | ||
| } | ||
| }; | ||
| run_client( | ||
| config, | ||
| network_manager, | ||
| storage_manager, | ||
| wallet, | ||
| enable_terminal_ui, | ||
| &matches, | ||
| wallet_id, | ||
| ) | ||
| .await?; | ||
| } | ||
| "./.tmp/main-exec-storage".into() | ||
| }; | ||
|
|
||
| let storage_manager = match dash_spv::storage::DiskStorageManager::new(path.clone()).await { | ||
xdustinface marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Ok(sm) => sm, | ||
| Err(e) => { | ||
| eprintln!("Failed to create disk storage manager: {}", e); | ||
| process::exit(1); | ||
| } | ||
| }; | ||
|
Comment on lines
+310
to
+322
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's understand the config structure and with_storage_path method
rg -n "with_storage_path" dash-spv/src --type rs -A 5 -B 2Repository: dashpay/rust-dashcore Length of output: 91 🏁 Script executed: # Check the config.storage_path field definition
rg -n "storage_path" dash-spv/src/client/config.rs -B 2 -A 2Repository: dashpay/rust-dashcore Length of output: 630 🏁 Script executed: # Examine the main.rs code flow around lines 183-320 to trace storage_path
sed -n '183,320p' dash-spv/src/main.rs | cat -nRepository: dashpay/rust-dashcore Length of output: 5974 🏁 Script executed: # Check DiskStorageManager::new signature
rg -n "impl DiskStorageManager" dash-spv/src/storage -A 10Repository: dashpay/rust-dashcore Length of output: 3356 🏁 Script executed: # Find the validate() method implementation in config.rs
rg -n "fn validate" dash-spv/src/client/config.rs -A 20Repository: dashpay/rust-dashcore Length of output: 886 🏁 Script executed: # Check if storage_path is modified after being set with_storage_path
rg -n "storage_path" dash-spv/src/main.rs -B 2 -A 2Repository: dashpay/rust-dashcore Length of output: 377 🏁 Script executed: # Check the actual line numbers in main.rs to confirm the code snippet
sed -n '230,260p' dash-spv/src/main.rs | cat -nRepository: dashpay/rust-dashcore Length of output: 1332 Remove the unreachable fallback path that violates coding guidelines. The - let path = if let Some(path) = &config.storage_path {
- path.clone()
- } else {
- "./.tmp/main-exec-storage".into()
- };
-
- let storage_manager = match dash_spv::storage::DiskStorageManager::new(path.clone()).await {
+ let storage_path = config.storage_path.clone()
+ .expect("storage_path must be set via with_storage_path");
+
+ let storage_manager = match dash_spv::storage::DiskStorageManager::new(storage_path).await {🤖 Prompt for AI Agents |
||
| run_client( | ||
| config, | ||
| network_manager, | ||
| storage_manager, | ||
| wallet, | ||
| enable_terminal_ui, | ||
| &matches, | ||
| wallet_id, | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably use the proper temp dir constructor here and probably good to log the temp dir then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor is only available in the unit test since it is marked with cfg(test), tempdir is a dev-dependency and i didnt want to modify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay then at least have a separate directory for each example. There is also the third "with wallet" example that could be aligned with those changes but thats a separate issue i guess.