Preparations for storing channels per peer #1542

ViktorTigerstrom · 2022-06-14T20:11:43Z

With #1507, we will store channels per peer in the per_peer_state map, instead of the channel_state. This PR splits up some of the functionality of #1507. To enable the transition to storing channels per peer, we need to update the short_to_id map to also include the counterparty_node_id.

The counterparty_node_id isn't passed with MonitorEvents currently, we also need to add an additional map id_to_peer, which gives us the counterparty_node_id when we only have access to the channel_id. However, we would preferably like to avoid the need for this map, so it is only used for MonitorEvent handling, so that it can be removed once we have the counterparty_node_id as an required field in ChannelMonitors.

ViktorTigerstrom · 2022-06-14T20:13:54Z

lightning/src/ln/channelmanager.rs

@@ -7579,6 +7619,121 @@ mod tests {
 		// Check that using the original payment hash succeeds.
 		assert!(inbound_payment::verify(payment_hash, &payment_data, nodes[0].node.highest_seen_timestamp.load(Ordering::Acquire) as u64, &nodes[0].node.inbound_payment_key, &nodes[0].logger).is_ok());
 	}
+
+	#[test]
+	fn test_id_to_peer_coverage() {


Let me know if you'd like me to add the by_id map (channel_by_id in #1507) and the short_to_chan_info map to this test. Also let me know if you'd like me to update the test with force closing the channel etc. :)

I'm not too picky here, given the code is in the same place as other similar changes I'm not too worried about it.

If you want to improve test coverage, I'd instead recommend debug_assert!()s during channelmanager operation, eg in the removal macro, etc.

Thanks! I'll look into that.

lightning/src/ln/channelmanager.rs

ariard · 2022-06-16T18:21:26Z

lightning/src/ln/channelmanager.rs

+	/// We should add `counterparty_node_id`s to `MonitorEvent`s, and eventually rely on it in the
+	/// future. That would make this map redundant, as only the `ChannelManager::per_peer_state` is
+	/// required to access the channel with the `counterparty_node_id`.
+	id_to_peer: Mutex<HashMap<[u8; 32], PublicKey>>,


What consistency do we expect between id_to_peer and ChannelHolder::by_id, like anything in the later should be in the former ? If yes, I think there are few call sites where we retain(), e.g in timer_tick_occurred

I think all cases where false is returned to retain for a channel and we have received an OpenChannel or AcceptChannel msg for the channel, update_maps_on_chan_removal! is triggered, which removes the channel entry from the id_to_peer map :).

Okay, I went to verify the retain callsites and I think this statement is correct. Even if id_to_peer might not be long-lived on the great scheme of things, it would be nice to document the consistency requirements among the two maps. Like say by_id removal should always be follow-up by a call to update_maps_on_chan_removal!.

Thanks! Oh yeah, that's a great idea! I'll update the docs with that in #1507 :)

codecov-commenter · 2022-06-16T23:43:51Z

Codecov Report

Merging #1542 (93497bf) into main (e254912) will decrease coverage by 0.22%.
The diff coverage is 92.50%.

❗ Current head 93497bf differs from pull request most recent head fa7f170. Consider uploading reports for the commit fa7f170 to get more accurate results

@@            Coverage Diff             @@
##             main    #1542      +/-   ##
==========================================
- Coverage   91.04%   90.81%   -0.23%     
==========================================
  Files          80       80              
  Lines       44194    44526     +332     
  Branches    44194    44526     +332     
==========================================
+ Hits        40235    40437     +202     
- Misses       3959     4089     +130

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.75% <ø> (+0.02%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.08% <92.30%> (+0.35%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning-invoice/src/payment.rs	`87.76% <0.00%> (-5.15%)`	⬇️
lightning/src/util/events.rs	`39.25% <0.00%> (-2.42%)`	⬇️
lightning/src/routing/scoring.rs	`96.08% <0.00%> (-0.73%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.57% <0.00%> (-0.69%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.78% <0.00%> (-0.24%)`	⬇️
lightning/src/routing/router.rs	`92.40% <0.00%> (-0.22%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e254912...fa7f170. Read the comment docs.

ViktorTigerstrom · 2022-06-16T23:46:31Z

Thanks for the reviews @TheBlueMatt & @ariard! Addressed the nits in the new fixup commits :)

ariard

Code Review ACK ab43fcd

I think better comment of the maps consistency can be addressed in #1507 or split-off.

ViktorTigerstrom · 2022-06-20T23:48:25Z

Code Review ACK ab43fcd

I think better comment of the maps consistency can be addressed in #1507 or split-off.

Thanks! Squashed the fix up commits without changes :).

lightning/src/ln/channelmanager.rs

ViktorTigerstrom · 2022-06-22T20:05:31Z

Thanks for the review @wpaulino :)! Adressed the feedback and squashed the changes in the last commit.

ariard

Code Review ACK 8a8a4de

TheBlueMatt · 2022-07-02T01:55:57Z

Oops sorry, this needs a rebase now :(. Should be good to go once that happens, though!

As the map values are no longer only `channel_id`s, but also a `counterparty_node_id`s, the map is renamed to better correspond to whats actually stored in the map.

ViktorTigerstrom · 2022-07-07T11:41:57Z

Rebased on upstream! Apologies for the delay.

TheBlueMatt

Oops, sorry, one issue.

TheBlueMatt · 2022-07-07T15:38:30Z

lightning/src/ln/channelmanager.rs

@@ -4511,12 +4538,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), funding_msg.channel_id))
 			},
 			hash_map::Entry::Vacant(e) => {
+				let mut id_to_peer = self.id_to_peer.lock().unwrap();
+				if id_to_peer.insert(funding_msg.channel_id, *counterparty_node_id).is_some() {
+					panic!("id_to_peer map already contained funding txid, which shouldn't be possible");


THis is definitely reachable - a counterparty could send us a channel with a conflicting txid. Sure, it wont have the output script pubkey that we agreed upon with the counterparty (which used unique keys), but we dont know that until we see the txid on-chain. Instead, this should be handled as a channel-closure error.

Well, okay, its obviously not reachable today as of this PR, but post-per-peer-channels it will be, so we should handle it here, no? If you want to handle it in the per-peer-storage PR that's okay too.

IIUC your concern, you think about a counterparty collisioning the id_to_peer map by announcing Channel 2 with temporary_id=XYZ from announced-by-us Channel 1 temporary_id ? Even if our temporary ids are generated by get_secure_random_bytes().

No, this is post-temporary ids, this is generated from the funding txid - the issue is a peer can open two channels and give us the same txid to us on both channels (using different peer ids/node private keys).

Oh yeah, good catch! I'll update this PR to fix it.

@ariard, no at this stage the temporary_channel_id is unrelated to the problem that @TheBlueMatt is raising, as this is when we are inserting the actual channel_id. As of today with this PR, the panic isn't reachable as the channel_state.by_id entry check right above will catch the scenario Matt raised as the channel_state.by_id isn't peer specific. But once per-peer-storage PR is merged, that check will be peer specific while the id_to_peer insert remains peer unspecific, and therefore 2 different peers could send us the same txid and not be catched by the first entry check above, hence triggering the panic.

Edit: Ah Matt got to it first :)

Hmmm @TheBlueMatt, won't the scenario you are describing already be caught by:

rust-lightning/lightning/src/ln/channelmanager.rs

Line 2900 in 4e5f74a

return Err(APIError::APIMisuseError {

That doesn't apply for funding_transaction_generated_unchecked of course, but that's for testing only.

We can of course add an extra guard for it in funding_transaction_generated_intern if you think that it's suitable though :)!

For outbound channels, yes, for inbound channels we don't see the funding transaction until it hits the chain.

Oh I'm stupid...!

Fixed in c09903b.

TheBlueMatt · 2022-07-07T15:50:24Z

lightning/src/ln/channelmanager.rs

+	/// TODO:
+	/// The `counterparty_node_id` isn't passed with `MonitorEvent`s currently. To pass it, we need
+	/// to add the `counterparty_node_id` to `ChannelMonitor`s. However, adding it as a required
+	/// field in `ChannelMonitor`s would break backwards compatability.


nit: THis is a bit confusing - its not adding the field that breaks backwards compat (in fact we're gonna do it in #1596), its requiring it.

ariard · 2022-07-07T18:38:23Z

lightning/src/ln/channelmanager.rs

+		{
+			// Assert that the channel is removed from both parties `id_to_peer` map once they both
+			// have everything required to fully close the channel.
+			assert_eq!(nodes[0].node.id_to_peer.lock().unwrap().len(), 0);


I think this check is redundant with the one above. Though can be kept if you wanna assert node[0]'s id_to_peer has not been modified.

ariard · 2022-07-07T18:41:43Z

lightning/src/ln/channelmanager.rs

+
+		// Clear the `ChannelClosed` event for the nodes.
+		nodes[0].node.get_and_clear_pending_events();
+		nodes[1].node.get_and_clear_pending_events();


I think you could use the macro get_event() to test the expected ChannelClosed event.

ariard · 2022-07-07T18:50:52Z

lightning/src/ln/channelmanager.rs

@@ -4511,12 +4538,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				return Err(MsgHandleErrInternal::send_err_msg_no_close("Already had channel with the new channel_id".to_owned(), funding_msg.channel_id))
 			},
 			hash_map::Entry::Vacant(e) => {
+				let mut id_to_peer = self.id_to_peer.lock().unwrap();
+				if id_to_peer.insert(funding_msg.channel_id, *counterparty_node_id).is_some() {
+					panic!("id_to_peer map already contained funding txid, which shouldn't be possible");


IIUC your concern, you think about a counterparty collisioning the id_to_peer map by announcing Channel 2 with temporary_id=XYZ from announced-by-us Channel 1 temporary_id ? Even if our temporary ids are generated by get_secure_random_bytes().

ariard

SGTM

TheBlueMatt · 2022-07-08T15:01:59Z

lightning/src/ln/channelmanager.rs

+				match id_to_peer.entry(chan.channel_id()) {
+					hash_map::Entry::Occupied(_) => {
+						return Err(MsgHandleErrInternal::send_err_msg_no_close(
+							"Funding txid already exists with for another peer (or likely the


Can we make the error here a bit more compact? Note that we send our peer the error and log it, so it should generally be readable for both. Maybe "funding_created message had the same funding txid as an existing channel - funding is not possible" or something simple like that?

Oh thanks, that makes sense:
81fc01f

TheBlueMatt · 2022-07-08T15:03:28Z

lightning/src/ln/channelmanager.rs

+
+		// Clear the `ChannelClosed` event for the nodes.
+		get_event!(&nodes[0], Event::ChannelClosed);
+		get_event!(&nodes[1], Event::ChannelClosed);


nit: check_closed_event?

TheBlueMatt · 2022-07-09T15:34:56Z

LGTM. Needs squash, but will let other reviewers comment on that.

TheBlueMatt · 2022-07-12T15:28:04Z

Can you squash the fixup commits down to clean up the history and I think we can basically land this.

ViktorTigerstrom · 2022-07-12T15:47:39Z

Squashed without changes

ariard

Code Review ACK fa7f170

Changes since last review :

update the sentence about breaking backwards compatibility
makes the collusion-triggerable-by-counterparty panic actually an error
fix few comments in test

ViktorTigerstrom commented Jun 14, 2022

View reviewed changes

ViktorTigerstrom mentioned this pull request Jun 14, 2022

Store channels per-peer #1507

Merged

TheBlueMatt previously approved these changes Jun 16, 2022

View reviewed changes

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

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

TheBlueMatt added the Seeking Code Review label Jun 16, 2022

ariard reviewed Jun 16, 2022

View reviewed changes

ViktorTigerstrom dismissed TheBlueMatt’s stale review via ab43fcd June 16, 2022 23:43

ariard previously approved these changes Jun 20, 2022

View reviewed changes

ViktorTigerstrom dismissed ariard’s stale review via 0644621 June 20, 2022 23:48

ViktorTigerstrom force-pushed the 2022-06-prepare-maps-for-channels-per-peer branch from ab43fcd to 0644621 Compare June 20, 2022 23:48

wpaulino previously approved these changes Jun 22, 2022

View reviewed changes

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

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

wpaulino removed the Seeking Code Review label Jun 22, 2022

ViktorTigerstrom dismissed wpaulino’s stale review via 8a8a4de June 22, 2022 20:03

ViktorTigerstrom force-pushed the 2022-06-prepare-maps-for-channels-per-peer branch from 0644621 to 8a8a4de Compare June 22, 2022 20:03

wpaulino previously approved these changes Jun 22, 2022

View reviewed changes

TheBlueMatt added the blocked on next release Should Wait Until Next Release To Land label Jun 22, 2022

ariard previously approved these changes Jun 22, 2022

View reviewed changes

TheBlueMatt removed the blocked on next release Should Wait Until Next Release To Land label Jul 2, 2022

ViktorTigerstrom added 2 commits July 7, 2022 13:34

Add counterparty_node_id to short_to_id map

908e898

Rename short_to_id map to short_to_chan_info

872c037

As the map values are no longer only `channel_id`s, but also a `counterparty_node_id`s, the map is renamed to better correspond to whats actually stored in the map.

ViktorTigerstrom dismissed stale reviews from ariard and wpaulino via 549c42f July 7, 2022 11:39

ViktorTigerstrom force-pushed the 2022-06-prepare-maps-for-channels-per-peer branch from 8a8a4de to 549c42f Compare July 7, 2022 11:39

TheBlueMatt reviewed Jul 7, 2022

View reviewed changes

ariard reviewed Jul 7, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-06-prepare-maps-for-channels-per-peer branch from 77b7477 to c09903b Compare July 8, 2022 14:54

TheBlueMatt reviewed Jul 8, 2022

View reviewed changes

wpaulino previously approved these changes Jul 11, 2022

View reviewed changes

ViktorTigerstrom added 2 commits July 12, 2022 17:47

Add id_to_peer map

4058861

Add ChannelManager:id_to_peer map coverage test

fa7f170

ViktorTigerstrom dismissed wpaulino’s stale review via fa7f170 July 12, 2022 15:47

ViktorTigerstrom force-pushed the 2022-06-prepare-maps-for-channels-per-peer branch from 93497bf to fa7f170 Compare July 12, 2022 15:47

wpaulino approved these changes Jul 12, 2022

View reviewed changes

ariard approved these changes Jul 13, 2022

View reviewed changes

TheBlueMatt approved these changes Jul 13, 2022

View reviewed changes

TheBlueMatt merged commit fda3819 into lightningdevkit:main Jul 13, 2022

Preparations for storing channels per peer #1542

Preparations for storing channels per peer #1542

Conversation

ViktorTigerstrom commented Jun 14, 2022

ViktorTigerstrom Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jun 16, 2022 • edited Loading

Codecov Report

ViktorTigerstrom commented Jun 16, 2022

ariard left a comment

Choose a reason for hiding this comment

ViktorTigerstrom commented Jun 20, 2022

ViktorTigerstrom commented Jun 22, 2022

ariard left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 2, 2022

ViktorTigerstrom commented Jul 7, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 9, 2022

TheBlueMatt commented Jul 12, 2022

ViktorTigerstrom commented Jul 12, 2022

ariard left a comment

Choose a reason for hiding this comment

ViktorTigerstrom Jun 14, 2022 •

edited

Loading

ViktorTigerstrom Jun 16, 2022 •

edited

Loading

ViktorTigerstrom Jun 20, 2022 •

edited

Loading

codecov-commenter commented Jun 16, 2022 •

edited

Loading

ViktorTigerstrom Jul 7, 2022 •

edited

Loading

ViktorTigerstrom Jul 8, 2022 •

edited

Loading