Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 480
feat: Make MonitorUpdatingPersister change persist type based on size#3834
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
Closed
Prabhat1308
wants to merge
4
commits into
lightningdevkit:main
from
Prabhat1308:probot/change_persist_type
Uh oh!
There was an error while loading. Please reload this page.
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
34fda75
Add size-based optimization to MonitorUpdatingPersister
Prabhat1308 b313e39
Add tests for persister size-based optimization
Prabhat1308 9b4508b
change threshold value
Prabhat1308 c7397bd
set a default const and maintain single constructor
Prabhat1308 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -98,6 +98,9 @@ pub const OUTPUT_SWEEPER_PERSISTENCE_KEY: &str = "output_sweeper"; | ||
| /// updates applied to be current) with another implementation. | ||
| pub const MONITOR_UPDATING_PERSISTER_PREPEND_SENTINEL: &[u8] = &[0xFF; 2]; | ||
| /// The default size of monitor where persistence is switched to update based. | ||
| pub const DEFAULT_MONITOR_SIZE_FOR_UPDATES_BYTES: usize = 8192; | ||
| /// Provides an interface that allows storage and retrieval of persisted values that are associated | ||
| /// with given keys. | ||
| /// | ||
| @@ -446,6 +449,13 @@ where | ||
| /// If you have many stale updates stored (such as after a crash with pending lazy deletes), and | ||
| /// would like to get rid of them, consider using the | ||
| /// [`MonitorUpdatingPersister::cleanup_stale_updates`] function. | ||
| /// | ||
| /// # Size-based persistence optimization | ||
| /// | ||
| /// For small channel monitors (below `min_monitor_size_for_updates_bytes` bytes when serialized), | ||
| /// this persister will always write the full monitor instead of individual updates. This avoids | ||
| /// the overhead of managing update files and later compaction for tiny monitors that don't benefit | ||
| /// from differential updates. | ||
| pub struct MonitorUpdatingPersister<K: Deref, L: Deref, ES: Deref, SP: Deref, BI: Deref, FE: Deref> | ||
| where | ||
| K::Target: KVStore, | ||
| @@ -458,6 +468,7 @@ where | ||
| kv_store: K, | ||
| logger: L, | ||
| maximum_pending_updates: u64, | ||
| min_monitor_size_for_updates_bytes: usize, | ||
| entropy_source: ES, | ||
| signer_provider: SP, | ||
| broadcaster: BI, | ||
| @@ -475,7 +486,7 @@ where | ||
| BI::Target: BroadcasterInterface, | ||
| FE::Target: FeeEstimator, | ||
| { | ||
| /// Constructs a new [`MonitorUpdatingPersister`]. | ||
| /// Constructs a new [`MonitorUpdatingPersister`] with a default minimum monitor size threshold. | ||
| /// | ||
| /// The `maximum_pending_updates` parameter controls how many updates may be stored before a | ||
| /// [`MonitorUpdatingPersister`] consolidates updates by writing a full monitor. Note that | ||
| @@ -491,14 +502,22 @@ where | ||
| /// less frequent "waves." | ||
| /// - [`MonitorUpdatingPersister`] will potentially have more listing to do if you need to run | ||
| /// [`MonitorUpdatingPersister::cleanup_stale_updates`]. | ||
| /// | ||
| /// This sets `min_monitor_size_for_updates_bytes` to 8192 bytes (8 KiB), which is a reasonable | ||
Prabhat1308 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /// default for most use cases. Monitors smaller than this will be persisted in full rather than | ||
| /// using update-based persistence. Pass a custom value for `min_monitor_size_for_updates_bytes` | ||
| /// if you need a custom threshold. | ||
| pub fn new( | ||
| kv_store: K, logger: L, maximum_pending_updates: u64, entropy_source: ES, | ||
| signer_provider: SP, broadcaster: BI, fee_estimator: FE, | ||
| min_monitor_size_for_updates_bytes: Option<usize>, | ||
| ) -> Self { | ||
| MonitorUpdatingPersister { | ||
| kv_store, | ||
| logger, | ||
| maximum_pending_updates, | ||
| min_monitor_size_for_updates_bytes: min_monitor_size_for_updates_bytes | ||
| .unwrap_or(DEFAULT_MONITOR_SIZE_FOR_UPDATES_BYTES), | ||
| entropy_source, | ||
| signer_provider, | ||
| broadcaster, | ||
| @@ -752,7 +771,12 @@ where | ||
| ) -> chain::ChannelMonitorUpdateStatus { | ||
| const LEGACY_CLOSED_CHANNEL_UPDATE_ID: u64 = u64::MAX; | ||
| if let Some(update) = update { | ||
| let persist_update = update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID | ||
| // Check if monitor is too small for update-based persistence | ||
| let monitor_size = monitor.serialized_length(); | ||
| let use_full_persistence = monitor_size < self.min_monitor_size_for_updates_bytes; | ||
| let persist_update = !use_full_persistence | ||
| && update.update_id != LEGACY_CLOSED_CHANNEL_UPDATE_ID | ||
| && update.update_id % self.maximum_pending_updates != 0; | ||
| if persist_update { | ||
| let monitor_key = monitor_name.to_string(); | ||
| @@ -1156,6 +1180,7 @@ mod tests { | ||
| kv_store: &TestStore::new(false), | ||
| logger: &TestLogger::new(), | ||
| maximum_pending_updates: persister_0_max_pending_updates, | ||
| min_monitor_size_for_updates_bytes: 0, | ||
| entropy_source: &chanmon_cfgs[0].keys_manager, | ||
| signer_provider: &chanmon_cfgs[0].keys_manager, | ||
| broadcaster: &chanmon_cfgs[0].tx_broadcaster, | ||
| @@ -1165,6 +1190,7 @@ mod tests { | ||
| kv_store: &TestStore::new(false), | ||
| logger: &TestLogger::new(), | ||
| maximum_pending_updates: persister_1_max_pending_updates, | ||
| min_monitor_size_for_updates_bytes: 0, | ||
| entropy_source: &chanmon_cfgs[1].keys_manager, | ||
| signer_provider: &chanmon_cfgs[1].keys_manager, | ||
| broadcaster: &chanmon_cfgs[1].tx_broadcaster, | ||
| @@ -1330,6 +1356,7 @@ mod tests { | ||
| kv_store: &TestStore::new(true), | ||
| logger: &TestLogger::new(), | ||
| maximum_pending_updates: 11, | ||
| min_monitor_size_for_updates_bytes: 0, | ||
| entropy_source: node_cfgs[0].keys_manager, | ||
| signer_provider: node_cfgs[0].keys_manager, | ||
| broadcaster: node_cfgs[0].tx_broadcaster, | ||
| @@ -1372,24 +1399,40 @@ mod tests { | ||
| fn clean_stale_updates_works() { | ||
| let test_max_pending_updates = 7; | ||
| let chanmon_cfgs = create_chanmon_cfgs(3); | ||
| let persister_0 = MonitorUpdatingPersister { | ||
| kv_store: &TestStore::new(false), | ||
| logger: &TestLogger::new(), | ||
| maximum_pending_updates: test_max_pending_updates, | ||
| entropy_source: &chanmon_cfgs[0].keys_manager, | ||
| signer_provider: &chanmon_cfgs[0].keys_manager, | ||
| broadcaster: &chanmon_cfgs[0].tx_broadcaster, | ||
| fee_estimator: &chanmon_cfgs[0].fee_estimator, | ||
| }; | ||
| let persister_1 = MonitorUpdatingPersister { | ||
| kv_store: &TestStore::new(false), | ||
| logger: &TestLogger::new(), | ||
| maximum_pending_updates: test_max_pending_updates, | ||
| entropy_source: &chanmon_cfgs[1].keys_manager, | ||
| signer_provider: &chanmon_cfgs[1].keys_manager, | ||
| broadcaster: &chanmon_cfgs[1].tx_broadcaster, | ||
| fee_estimator: &chanmon_cfgs[1].fee_estimator, | ||
| }; | ||
| let store_0 = TestStore::new(false); | ||
| let logger_0 = TestLogger::new(); | ||
| let store_1 = TestStore::new(false); | ||
| let logger_1 = TestLogger::new(); | ||
| // Test the default new() constructor (uses 8192 byte threshold) | ||
| let persister_0 = MonitorUpdatingPersister::new( | ||
| &store_0, | ||
| &logger_0, | ||
| test_max_pending_updates, | ||
| &chanmon_cfgs[0].keys_manager, | ||
| &chanmon_cfgs[0].keys_manager, | ||
| &chanmon_cfgs[0].tx_broadcaster, | ||
| &chanmon_cfgs[0].fee_estimator, | ||
| None, | ||
| ); | ||
| // Test the custom threshold constructor with zero threshold | ||
| let persister_1 = MonitorUpdatingPersister::new( | ||
| &store_1, | ||
| &logger_1, | ||
| test_max_pending_updates, | ||
| &chanmon_cfgs[1].keys_manager, | ||
| &chanmon_cfgs[1].keys_manager, | ||
| &chanmon_cfgs[1].tx_broadcaster, | ||
| &chanmon_cfgs[1].fee_estimator, | ||
| Some(0), // 0 byte threshold for maximum update usage | ||
| ); | ||
| // Verify the constructors set the thresholds correctly | ||
| assert_eq!( | ||
| persister_0.min_monitor_size_for_updates_bytes, | ||
| DEFAULT_MONITOR_SIZE_FOR_UPDATES_BYTES | ||
| ); | ||
| assert_eq!(persister_1.min_monitor_size_for_updates_bytes, 0); | ||
| let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let chain_mon_0 = test_utils::TestChainMonitor::new( | ||
| Some(&chanmon_cfgs[0].chain_source), | ||
| @@ -1452,6 +1495,118 @@ mod tests { | ||
| .is_err()); | ||
| } | ||
| #[test] | ||
| fn test_size_based_optimization() { | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let store_0 = TestStore::new(false); | ||
| let logger_0 = TestLogger::new(); | ||
| let store_1 = TestStore::new(false); | ||
| let logger_1 = TestLogger::new(); | ||
| // Create a persister with a huge minimum size threshold (100KB) | ||
| // This should force all monitors to use full persistence instead of updates | ||
| // Test the new constructor with large threshold | ||
| let large_threshold_persister = MonitorUpdatingPersister::new( | ||
| &store_0, | ||
| &logger_0, | ||
| 5, | ||
| &chanmon_cfgs[0].keys_manager, | ||
| &chanmon_cfgs[0].keys_manager, | ||
| &chanmon_cfgs[0].tx_broadcaster, | ||
| &chanmon_cfgs[0].fee_estimator, | ||
| Some(100_000), | ||
| ); | ||
| // Create a persister with zero minimum size threshold | ||
| // This should allow all monitors to use update-based persistence | ||
| // Test the new constructor with zero threshold | ||
| let small_threshold_persister = MonitorUpdatingPersister::new( | ||
| &store_1, | ||
| &logger_1, | ||
| 5, | ||
| &chanmon_cfgs[1].keys_manager, | ||
| &chanmon_cfgs[1].keys_manager, | ||
| &chanmon_cfgs[1].tx_broadcaster, | ||
| &chanmon_cfgs[1].fee_estimator, | ||
| Some(0), // allows all monitors to use updates | ||
| ); | ||
| // Verify the constructors set the thresholds correctly | ||
| assert_eq!(large_threshold_persister.min_monitor_size_for_updates_bytes, 100_000); | ||
| assert_eq!(small_threshold_persister.min_monitor_size_for_updates_bytes, 0); | ||
| let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| let chain_mon_0 = test_utils::TestChainMonitor::new( | ||
| Some(&chanmon_cfgs[0].chain_source), | ||
| &chanmon_cfgs[0].tx_broadcaster, | ||
| &chanmon_cfgs[0].logger, | ||
| &chanmon_cfgs[0].fee_estimator, | ||
| &large_threshold_persister, | ||
| &chanmon_cfgs[0].keys_manager, | ||
| ); | ||
| let chain_mon_1 = test_utils::TestChainMonitor::new( | ||
| Some(&chanmon_cfgs[1].chain_source), | ||
| &chanmon_cfgs[1].tx_broadcaster, | ||
| &chanmon_cfgs[1].logger, | ||
| &chanmon_cfgs[1].fee_estimator, | ||
| &small_threshold_persister, | ||
| &chanmon_cfgs[1].keys_manager, | ||
| ); | ||
| node_cfgs[0].chain_monitor = chain_mon_0; | ||
| node_cfgs[1].chain_monitor = chain_mon_1; | ||
| let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); | ||
| let nodes = create_network(2, &node_cfgs, &node_chanmgrs); | ||
| // Create a channel and make a payment to trigger monitor updates | ||
| let _ = create_announced_chan_between_nodes(&nodes, 0, 1); | ||
| send_payment(&nodes[0], &vec![&nodes[1]][..], 1_000_000); | ||
| // Test passes if we can create the channels and send payments without issues. | ||
| // The actual verification is that the different persisters behave differently | ||
| // based on their thresholds, which we can verify by ensuring no panics occur. | ||
| // Verify that monitors were created | ||
| let persisted_data_0 = | ||
| large_threshold_persister.read_all_channel_monitors_with_updates().unwrap(); | ||
| let persisted_data_1 = | ||
| small_threshold_persister.read_all_channel_monitors_with_updates().unwrap(); | ||
| assert_eq!(persisted_data_0.len(), 1); | ||
| assert_eq!(persisted_data_1.len(), 1); | ||
| // Verify the monitors exist and are of reasonable size | ||
| for (_, monitor) in persisted_data_0.iter() { | ||
| let monitor_size = monitor.serialized_length(); | ||
| // Verify the monitor is not empty and reasonably sized | ||
| assert!( | ||
| monitor_size > 1000, | ||
| "Monitor should be at least 1KB in size, got {} bytes", | ||
| monitor_size | ||
| ); | ||
| assert!( | ||
| monitor_size < 100_000, | ||
| "Monitor should be smaller than 100KB threshold, got {} bytes", | ||
| monitor_size | ||
| ); | ||
| } | ||
| for (_, monitor) in persisted_data_1.iter() { | ||
| let monitor_size = monitor.serialized_length(); | ||
| // Verify the monitor is not empty and reasonably sized | ||
| assert!( | ||
| monitor_size > 1000, | ||
| "Monitor should be at least 1KB in size, got {} bytes", | ||
| monitor_size | ||
| ); | ||
| // Since threshold is 0, this monitor should be large enough to use updates | ||
| assert!( | ||
| monitor_size > 0, | ||
| "Monitor should be larger than 0 byte threshold, got {} bytes", | ||
| monitor_size | ||
| ); | ||
| } | ||
| } | ||
| fn persist_fn<P: Deref, ChannelSigner: EcdsaChannelSigner>(_persist: P) -> bool | ||
| where | ||
| P::Target: Persist<ChannelSigner>, | ||
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.
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.
It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.