Uh oh!
There was an error while loading. Please reload this page.
Use MonitorUpdatingPersister#661
+128
−72
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
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 |
|---|---|---|
| @@ -11,14 +11,28 @@ use std::path::PathBuf; | ||
| use lightning::events::ClosureReason; | ||
| use lightning::ln::functional_test_utils::{ | ||
| connect_block, create_announced_chan_between_nodes, create_chanmon_cfgs, create_dummy_block, | ||
| create_network, create_node_cfgs, create_node_chanmgrs, send_payment, TestChanMonCfg, | ||
| }; | ||
| use lightning::util::persist::{ | ||
| KVStoreSync, MonitorUpdatingPersister, KVSTORE_NAMESPACE_KEY_MAX_LEN, | ||
| }; | ||
| use lightning::util::test_utils; | ||
| use lightning::{check_added_monitors, check_closed_broadcast, check_closed_event}; | ||
| use rand::distributions::Alphanumeric; | ||
| use rand::{thread_rng, Rng}; | ||
| type TestMonitorUpdatePersister<'a, K> = MonitorUpdatingPersister< | ||
| &'a K, | ||
| &'a test_utils::TestLogger, | ||
| &'a test_utils::TestKeysInterface, | ||
| &'a test_utils::TestKeysInterface, | ||
| &'a test_utils::TestBroadcaster, | ||
| &'a test_utils::TestFeeEstimator, | ||
| >; | ||
| const EXPECTED_UPDATES_PER_PAYMENT: u64 = 5; | ||
| pub(crate) fn random_storage_path() -> PathBuf { | ||
| let mut temp_path = std::env::temp_dir(); | ||
| let mut rng = thread_rng(); | ||
| @@ -77,54 +91,71 @@ pub(crate) fn do_read_write_remove_list_persist<K: KVStoreSync + RefUnwindSafe>( | ||
| assert_eq!(listed_keys.len(), 0); | ||
| } | ||
| pub(crate) fn create_persister<'a, K: KVStoreSync + Sync>( | ||
| store: &'a K, chanmon_cfg: &'a TestChanMonCfg, max_pending_updates: u64, | ||
| ) -> TestMonitorUpdatePersister<'a, K> { | ||
| MonitorUpdatingPersister::new( | ||
| store, | ||
| &chanmon_cfg.logger, | ||
| max_pending_updates, | ||
| &chanmon_cfg.keys_manager, | ||
| &chanmon_cfg.keys_manager, | ||
| &chanmon_cfg.tx_broadcaster, | ||
| &chanmon_cfg.fee_estimator, | ||
| ) | ||
| } | ||
| pub(crate) fn create_chain_monitor<'a, K: KVStoreSync + Sync>( | ||
| chanmon_cfg: &'a TestChanMonCfg, persister: &'a TestMonitorUpdatePersister<'a, K>, | ||
| ) -> test_utils::TestChainMonitor<'a> { | ||
| test_utils::TestChainMonitor::new( | ||
| Some(&chanmon_cfg.chain_source), | ||
| &chanmon_cfg.tx_broadcaster, | ||
| &chanmon_cfg.logger, | ||
| &chanmon_cfg.fee_estimator, | ||
| persister, | ||
| &chanmon_cfg.keys_manager, | ||
| ) | ||
| } | ||
| // Integration-test the given KVStore implementation. Test relaying a few payments and check that | ||
| // the persisted data is updated the appropriate number of times. | ||
| pub(crate) fn do_test_store<K: KVStoreSync + Sync>(store_0: &K, store_1: &K) { | ||
| // This value is used later to limit how many iterations we perform. | ||
| let persister_0_max_pending_updates = 7; | ||
| // Intentionally set this to a smaller value to test a different alignment. | ||
Contributor There was a problem hiding this comment. I've seen this also in rust-lightning, and also there I didn't understand what alignment there is to test because the nodes are independent? CollaboratorAuthor There was a problem hiding this comment. Yeah, you'd have to ask the OP regarding the comment, but it's also of course not wrong to set a different value here. Contributor There was a problem hiding this comment. It isn't wrong, but thought it might be worth knowing a bit more about it now that you are taking over. There was a problem hiding this comment. @joostjager I think I wrote this sentence first in I think what we were concerned with was weird edge cases / off-by-one errors in the compaction algorithm in particular. I think there was a state of this PR where odd numbered | ||
| let persister_1_max_pending_updates = 3; | ||
| let chanmon_cfgs = create_chanmon_cfgs(2); | ||
| let persister_0 = create_persister(store_0, &chanmon_cfgs[0], persister_0_max_pending_updates); | ||
| let persister_1 = create_persister(store_1, &chanmon_cfgs[1], persister_1_max_pending_updates); | ||
| let chain_mon_0 = create_chain_monitor(&chanmon_cfgs[0], &persister_0); | ||
| let chain_mon_1 = create_chain_monitor(&chanmon_cfgs[1], &persister_1); | ||
| let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs); | ||
| 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); | ||
| // Check that the persisted channel data is empty before any channels are | ||
| // open. | ||
| let mut persisted_chan_data_0 = persister_0.read_all_channel_monitors_with_updates().unwrap(); | ||
| assert_eq!(persisted_chan_data_0.len(), 0); | ||
| let mut persisted_chan_data_1 = persister_1.read_all_channel_monitors_with_updates().unwrap(); | ||
| assert_eq!(persisted_chan_data_1.len(), 0); | ||
| // Helper to make sure the channel is on the expected update ID. | ||
| macro_rules! check_persisted_data { | ||
| ($expected_update_id: expr) => { | ||
| persisted_chan_data_0 = persister_0.read_all_channel_monitors_with_updates().unwrap(); | ||
| assert_eq!(persisted_chan_data_0.len(), 1); | ||
| for (_, mon) in persisted_chan_data_0.iter() { | ||
| assert_eq!(mon.get_latest_update_id(), $expected_update_id); | ||
| } | ||
| persisted_chan_data_1 = persister_1.read_all_channel_monitors_with_updates().unwrap(); | ||
| assert_eq!(persisted_chan_data_1.len(), 1); | ||
| for (_, mon) in persisted_chan_data_1.iter() { | ||
| assert_eq!(mon.get_latest_update_id(), $expected_update_id); | ||
| @@ -137,10 +168,29 @@ pub(crate) fn do_test_store<K: KVStoreSync + Sync>(store_0: &K, store_1: &K) { | ||
| check_persisted_data!(0); | ||
| // Send a few payments and make sure the monitors are updated to the latest. | ||
| let expected_route = &[&nodes[1]][..]; | ||
| send_payment(&nodes[0], expected_route, 8_000_000); | ||
| check_persisted_data!(EXPECTED_UPDATES_PER_PAYMENT); | ||
| let expected_route = &[&nodes[0]][..]; | ||
| send_payment(&nodes[1], expected_route, 4_000_000); | ||
| check_persisted_data!(2 * EXPECTED_UPDATES_PER_PAYMENT); | ||
| // Send a few more payments to try all the alignments of max pending updates with | ||
| // updates for a payment sent and received. | ||
| let mut sender = 0; | ||
| for i in 3..=persister_0_max_pending_updates * 2 { | ||
Contributor There was a problem hiding this comment. This also seems a bit overkill at the ldk node level? CollaboratorAuthor There was a problem hiding this comment. Mhh, a bit more test coverage doesn't hurt? FWIW, this could be made a proptest to have it be less deterministic, but apart from that there isn't much harm? Contributor There was a problem hiding this comment. It doesn't hurt in terms over coverage, but it is probably duplicative, and code carries cost. | ||
| let receiver; | ||
| if sender == 0 { | ||
| sender = 1; | ||
| receiver = 0; | ||
| } else { | ||
| sender = 0; | ||
| receiver = 1; | ||
| } | ||
| let expected_route = &[&nodes[receiver]][..]; | ||
| send_payment(&nodes[sender], expected_route, 21_000); | ||
| check_persisted_data!(i * EXPECTED_UPDATES_PER_PAYMENT); | ||
| } | ||
| // Force close because cooperative close doesn't result in any persisted | ||
| // updates. | ||
| @@ -163,27 +213,18 @@ pub(crate) fn do_test_store<K: KVStoreSync + Sync>(store_0: &K, store_1: &K) { | ||
| check_closed_broadcast!(nodes[0], true); | ||
| check_added_monitors!(nodes[0], 1); | ||
| let node_txn = nodes[0].tx_broadcaster.txn_broadcast(); | ||
| assert_eq!(node_txn.len(), 1); | ||
| let txn = vec![node_txn[0].clone(), node_txn[0].clone()]; | ||
| let dummy_block = create_dummy_block(nodes[0].best_block_hash(), 42, txn); | ||
| connect_block(&nodes[1], &dummy_block); | ||
Contributor There was a problem hiding this comment. Some independent refactoring here? CollaboratorAuthor There was a problem hiding this comment. Yeah, doesn't hurt either though. Contributor There was a problem hiding this comment. I meant to say that it is better to isolate in a separate commit. | ||
| check_closed_broadcast!(nodes[1], true); | ||
| let reason = ClosureReason::CommitmentTxConfirmed; | ||
| let node_id_0 = nodes[0].node.get_our_node_id(); | ||
| check_closed_event!(nodes[1], 1, reason, false, [node_id_0], 100000); | ||
| check_added_monitors!(nodes[1], 1); | ||
| // Make sure everything is persisted as expected after close. | ||
| check_persisted_data!(persister_0_max_pending_updates * 2 * EXPECTED_UPDATES_PER_PAYMENT + 1); | ||
| } | ||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Is there science to the number 100?
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.
No, just handwaving currently. There has been some benchmarking over at lightningdevkit/rust-lightning#3834 but it never got anywhere super conclusive. Let me know if you have a better guess.
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.
Does not seem unreasonable. Going higher is probably not gaining much. Maybe it could be a bit lower. But also fine as is.