Drop ChannelManager's ChannelMonitor Arc for Deref #443

valentinewallace · 2020-01-09T17:38:30Z

Partly closes #237.

TODO:

switch the functional tests from Arcs to references, as in WIP: Drop ChannelManager's ChannelMonitor Arc for reference #424
update BlockNotifier to register Derefs instead of Weak pointers
other ancillary Arc --> reference changes

TheBlueMatt

Cool! Wow, that was much more doable. Your TODOs look good but glad we found a good way forward here.

TheBlueMatt · 2020-01-13T17:56:09Z

lightning/src/chain/chaininterface.rs

 /// data is registered.
-pub struct BlockNotifier {
-	listeners: Mutex<Vec<Weak<ChainListener>>>, //TODO(vmw): try removing Weak
+pub struct BlockNotifier<CL: Deref<Target = ChainListener> + Sized + Clone> {


Man, Clone kinda sucks. Fine for this PR but we should try to remove it in a followup (unless you see an easy way to remove it now, I didn't play with it).

TheBlueMatt

Wow, this turned out simpler than I'd thought, even if the diff is huge :).

lightning/src/chain/chaininterface.rs

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt

Haven't gone through it with a super fine-toothed comb yet, but looks good.

TheBlueMatt · 2020-01-22T03:21:54Z

lightning/src/ln/functional_tests.rs

 	let keys_manager = Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new())));
-	let (_, nodes_0_deserialized) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+	let (_, nodes_0_deserialized_tmp) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys, &test_utils::TestChannelMonitor>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+	// let (_, nodes_0_deserialized_tmp) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {


Still a stray commented-previous-copy of this line?

TheBlueMatt · 2020-01-22T03:25:28Z

lightning/src/chain/chaininterface.rs

 /// data is registered.
-pub struct BlockNotifier {
-	listeners: Mutex<Vec<Weak<ChainListener>>>, //TODO(vmw): try removing Weak
+pub struct BlockNotifier<'a, CL: Deref<Target = ChainListener + 'a> + 'a> {


We should add docs that describe the new parameters (here and everywhere) and discuss different ways to use them. Because its super easy to end up with massive type gook, we should probably define a type that defines types for Arc and maybe one for &'a as well if thats easy (plus point the docs to them).

cool, hopefully the docs I added + their location make sense

TheBlueMatt · 2020-01-22T03:25:43Z

lightning/src/ln/channelmanager.rs

 /// spam due to quick disconnection/reconnection, updates are not sent until the channel has been
 /// offline for a full minute. In order to track this, you must call
 /// timer_chan_freshness_every_min roughly once per minute, though it doesn't have to be perfec.
-pub struct ChannelManager<ChanSigner: ChannelKeys> {


Same here, probably want type aliases for common usages and docs.

TheBlueMatt · 2020-01-23T03:06:32Z

Oops, I realized my comment was somewhat unclear, what I was looking at when I made it was this: TheBlueMatt/rust-lightning-bitcoinrpc@97d9e15 with wonderful tidbits like <A : ::std::ops::Deref<Target = lightning::chain::chaininterface::ChainListener + 'static> + Send + 'static and Arc<peer_handler::PeerManager<SocketDescriptor<channelmanager::ChannelManager<InMemoryChannelKeys, Arc<ChannelMonitor>>>, Arc<channelmanager::ChannelManager<InMemoryChannelKeys, Arc<ChannelMonitor>>>>>,.

So instead of type ManyChannelMonitorArc = Arc<ManyChannelMonitor>; do something more like type SimpleArcChannelManager = Arc<ChannelManager<InMemoryChannelKeys, Arc<SimpleManyChannelMonitor<chain::transaction::OutPoint>>>> (in channelmanager.rs) and something similar in peer_handler and channelmonitor.rs for their types. They should then have a description that details a bit more why you might use them (your description above ChannelManager is good, but be willing to be a bit repetitive on the types, I think :) ).

valentinewallace · 2020-01-23T19:13:22Z

Oops, I realized my comment was somewhat unclear, what I was looking at when I made it was this: TheBlueMatt/rust-lightning-bitcoinrpc@97d9e15 with wonderful tidbits like <A : ::std::ops::Deref<Target = lightning::chain::chaininterface::ChainListener + 'static> + Send + 'static and Arc<peer_handler::PeerManager<SocketDescriptor<channelmanager::ChannelManager<InMemoryChannelKeys, Arc<ChannelMonitor>>>, Arc<channelmanager::ChannelManager<InMemoryChannelKeys, Arc<ChannelMonitor>>>>>,.

So instead of type ManyChannelMonitorArc = Arc<ManyChannelMonitor>; do something more like type SimpleArcChannelManager = Arc<ChannelManager<InMemoryChannelKeys, Arc<SimpleManyChannelMonitor<chain::transaction::OutPoint>>>> (in channelmanager.rs) and something similar in peer_handler and channelmonitor.rs for their types. They should then have a description that details a bit more why you might use them (your description above ChannelManager is good, but be willing to be a bit repetitive on the types, I think :) ).

Ahh, I see what you're getting at. Let me know if these new changes seem improved. The channelmonitor type aliases seem possibly unnecessary.

TheBlueMatt · 2020-01-23T21:00:36Z

lightning-net-tokio/src/lib.rs

 	conn: Arc<Mutex<Connection>>,
 	id: u64,
-	peer_manager: Arc<peer_handler::PeerManager<SocketDescriptor>>,
+	peer_manager: SimpleArcPeerManager,


Hmm, doesn't this restrict us to very specific use-cases eg InMemoryChannelKeys + SimpleManyChannelMonitor? I think we may need to template + default these too :(.

I reverted back to the previous version. This makes it "templated" iiuc, but not sure what

+ default

means 😅 Soz if missing the obvious here..

TheBlueMatt · 2020-01-23T21:11:31Z

lightning/src/ln/channelmonitor.rs

+/// with static lifetimes). Other times you can afford a reference, which is more efficient, in
+/// which case SimpleRefChannelMonitor is the more appropriate type. Defining these type aliases
+/// prevents issues such as overly long function definitions.
+pub type SimpleArcChannelMonitor = Arc<SimpleManyChannelMonitor<OutPoint>>;


I'm really torn on what to do here - SimpleManyChannelMonitor is great, but its also meant as a thing you need to put a wrapper around to store to disk. That probably needs to be clarified in the docs for ManyChannelMonitor, but maybe for now replace SimpleArcChannelMonitor with a template A: ManyChannelMonitor -> Arc...I know, I contradicted myself here, sorry.

Ah I'm not sure I understood this. What do you mean by

but its also meant as a thing you need to put a wrapper around to store to disk.

? Can't quite see the relevance. And, does "put a wrapper" mean put it into something that implements Serializable, or something similar?
I replaced the SimpleArcChannelMonitor with a template within the SimpleArcChannelManager.

As in users should almost certainly not be using SimpleManyChannelMonitor directly - they should be using MyCustomerManyChannelMonitor { in_memory_stuff: SimpleManyChannelMonitor } where the impl first writes the latest channel monitor to disk, then passes it to the in memory version. Thus, having a SimpleArcChannelMonitor is....not quite what we want (nor using SimpleManyChannelMonitor in any of the "suggested good default" types).

lightning/src/chain/chaininterface.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2020-01-24T01:33:44Z

lightning/src/ln/channelmanager.rs

+/// usage of lightning-net-tokio (since tokio::spawn requires parameters with static lifetimes).
+/// But if this is not necessary, using a reference is more efficient. Defining these type aliases
+/// helps with issues such as long function definitions.
+pub type SimpleRefChannelManager<'a, 'b, CS, M> = &'a ChannelManager<CS, &'b M>;


Same here, dont think it needs the outside reference. Also would be nice to use it in the tests.

Also would be nice to use it in the tests.

Hmm, the tests use EnforcingChannelKeys but I switched the SimpleRefChannelManager to use InMemoryChannelKeys per this comment: #443 (comment)

Could create another SimpleRefChannelManager type that uses EnforcingChannelKeys instead!

lightning/src/ln/chanmon_update_fail_tests.rs

Additional changes: * Update fuzz crate to match ChannelManager's new API * Update lightning-net-tokio library to match ChannelManager's new ChannelMonitor Deref API * Update tests to match ChannelManager's new ChannelMonitor Deref API

TheBlueMatt

Alright. Looks good with 3 tiny nits left. I'll just fix them and merge to get this done in a bit.

TheBlueMatt · 2020-01-25T19:24:37Z

lightning/src/ln/functional_tests.rs

+	let node_cfgs = create_node_cfgs(2);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+				let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::supported(), InitFeatures::supported());


nit: stray indentation.

TheBlueMatt · 2020-01-25T19:33:39Z

lightning/src/ln/functional_tests.rs

 	let keys_manager = Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new())));
-	let (_, nodes_0_deserialized) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+	let (_, nodes_0_deserialized_tmp) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys, &test_utils::TestChannelMonitor>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {
+	// let (_, nodes_0_deserialized_tmp) = <(Sha256dHash, ChannelManager<EnforcingChannelKeys>)>::read(&mut nodes_0_read, ChannelManagerReadArgs {


Still a stray commented-previous-copy of this line?

TheBlueMatt · 2020-01-25T19:35:34Z

lightning/src/ln/functional_tests.rs

 	// HTLC could have been removed from lastest local commitment tx but still valid until we get remote RAA

-	let nodes = create_network(2, &[None, None]);
+	let node_cfgs = create_node_cfgs(3);


2 -> 3 here.

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch from 06cbb0b to c08c163 Compare January 9, 2020 21:57

TheBlueMatt reviewed Jan 13, 2020

View reviewed changes

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch 3 times, most recently from 8ebf3e2 to c61f5b8 Compare January 16, 2020 19:08

valentinewallace marked this pull request as ready for review January 16, 2020 19:17

valentinewallace changed the title ~~[WIP] Drop ChannelManager's ChannelMonitor Arc for Deref~~ Drop ChannelManager's ChannelMonitor Arc for Deref Jan 16, 2020

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch from c61f5b8 to d9828de Compare January 16, 2020 20:03

valentinewallace requested a review from TheBlueMatt January 16, 2020 21:09

TheBlueMatt reviewed Jan 16, 2020

View reviewed changes

lightning/src/chain/chaininterface.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch 2 times, most recently from 775a964 to 5890c29 Compare January 21, 2020 22:01

TheBlueMatt reviewed Jan 22, 2020

View reviewed changes

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch 2 times, most recently from e4a1437 to a63599c Compare January 22, 2020 23:59

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch from a63599c to e3524d1 Compare January 23, 2020 19:17

TheBlueMatt reviewed Jan 23, 2020

View reviewed changes

valentinewallace force-pushed the channelmgr-arcs-to-derefs branch from e3524d1 to d054889 Compare January 24, 2020 00:27