Move `short_to_chan_info` into standalone lock #1639

ViktorTigerstrom · 2022-07-28T15:28:07Z

Based on #1638 and a prerequisite for #1507.

As the channel_state (ChannelHolder) struct will be removed, this commit moves the short_to_chan_info map from that lock into a separate lock, and cleans up a few macros that can be cleaned up due to moving it into a separate lock.

ViktorTigerstrom · 2022-07-28T15:30:13Z

lightning/src/ln/channelmanager.rs

 	};
-	($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr) => {


Would have preferred to place this cleanup in the final commit, but keeping them in the first commit messed up the macro. I hope that's ok.

ViktorTigerstrom · 2022-08-01T23:09:18Z

Realized I need to go through this closer another time, as the current consistency guarantees ensures that the channel is guaranteed to exist in the by_id map if also existing in the short_to_chan_info map while the channel_state lock is taken. Just need to ensure that we aren't unwrapping the result from getting the channel in the by_id map when using an id from the short_to_chan_info map, if the channel_state map isn't taken while the id is retrieved from short_to_chan_info map.

While open the pull request when I've checked this further.

ViktorTigerstrom · 2022-08-09T00:21:31Z

Added a fixup commit that ensures that the consistency guarantees that any channel_id in the short_to_chan_info map is guaranteed exist as a key in the channel_state::by_id map.

Also rebased this on main, now that the blocking PR has been merged.

TheBlueMatt · 2022-08-15T00:36:34Z

lightning/src/ln/channelmanager.rs

@@ -1256,9 +1272,12 @@ macro_rules! handle_error {
 }

 macro_rules! update_maps_on_chan_removal {
-	($self: expr, $short_to_chan_info: expr, $channel: expr) => {
+	($self: expr, $channel: expr) => {
+		// The lockorder for id_to_peer is prior to short_to_chan_info.


Sure, but there's also no reason to hold both locks at the same time here - just do self.id_to_peer.lock().unwrap().remove(&$channel.channel_id()); all in one statement at the top and then you don't have to worry about it :).

Fixed with 1891450

TheBlueMatt · 2022-08-15T00:46:16Z

lightning/src/ln/channelmanager.rs

+	/// SCIDs being added once the funding transaction is confirmed at the channel's required
+	/// confirmation depth.
+	///
+	/// Consistancy guarantees with the `channel_state::by_id` map, guarantees that any


Hmm, I mean while this patch may be fine, I'd kinda prefer if we just not have guarantees to begin with - is there a reason we need to have these guarantees, or can we just not guarantee anything and handle the None case in the by_id lookups when we fetch from this?

Ok sure I'll change to that instead :)! I did it this way mostly just to make this PR not change the current consistency guarantees, but if we're ok with changing that, I see no reason why we can't do what you suggest.

Yea, I mean the nice thing about this PR is its relatively small-ish, and aside from argument changes, is quite reviewable, so its totally find to slip in a change like that here, as it can also make future things easier to review. Plus it seems like a more robust design anyway.

Fixed with 919ec65

ViktorTigerstrom · 2022-08-18T23:40:37Z

Thanks for the review @TheBlueMatt! I've addressed your feedback with the latest pushed commits. Unfortunately I had to rebase to drop the patch I made before, but I rebased on the same HEAD as before.

ViktorTigerstrom · 2022-08-18T23:45:26Z

lightning/src/ln/channelmanager.rs

-					let forward_chan_id = match channel_state.short_to_chan_info.get(&short_chan_id) {
-						Some((_cp_id, chan_id)) => chan_id.clone(),
-						None => {
+					macro_rules! fail_pending_forwards {


Not super happy about this macro, and would prefer to break it out into a separate function due to the added complexity. But I figured process_pending_htlc_forwards was probably kept large with macros for efficacy reasons, so opted for the new macro instead. Let me know if my assumptions are wrong, and you'd prefer to break this out :)

I'm not sure here, I just know that nested macros are not fun to debug 🙈

Yeah definitely agree 😬...

TheBlueMatt · 2022-08-19T18:39:43Z

lightning/src/ln/channelmanager.rs

@@ -4098,7 +4129,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let mut expected_amt_msat = None;
 			let mut valid_mpp = true;
 			for htlc in sources.iter() {
-				if let None = channel_state.as_ref().unwrap().short_to_chan_info.get(&htlc.prev_hop.short_channel_id) {
+				if let None = self.short_to_chan_info.read().unwrap().get(&htlc.prev_hop.short_channel_id) {


You need to check that the channel is available now, too. And while we're at it, let's check that the channel isn't shutting down.

Oh yeah good catch! Fixed with 1f01103

TheBlueMatt · 2022-08-19T18:42:54Z

lightning/src/ln/channelmanager.rs

-				hash_map::Entry::Occupied(_) => continue,
-				hash_map::Entry::Vacant(_) => return scid_candidate
+			match short_to_chan_info.get(&scid_candidate) {
+				Some((_cp_id, _chan_id)) => continue,


Suggested change

Some((_cp_id, _chan_id)) => continue,

Some(_) => continue,

TheBlueMatt · 2022-08-19T18:43:42Z

lightning/src/ln/channelmanager.rs

@@ -5819,7 +5848,7 @@ where

 	fn get_relevant_txids(&self) -> Vec<Txid> {
 		let channel_state = self.channel_state.lock().unwrap();
-		let mut res = Vec::with_capacity(channel_state.short_to_chan_info.len());
+		let mut res = Vec::with_capacity(self.short_to_chan_info.read().unwrap().len());


lol let's not take a lock just for the capacity here, just use the by_id map instead, its close enough.

ViktorTigerstrom · 2022-08-22T21:18:51Z

Thanks! Addressed the latest feedback with 1f01103 :)

TheBlueMatt

Basically there, I think.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-08-22T23:50:34Z

lightning/src/ln/channelmanager.rs

@@ -5127,7 +5168,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	fn internal_channel_update(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelUpdate) -> Result<NotifyOption, MsgHandleErrInternal> {
 		let mut channel_state_lock = self.channel_state.lock().unwrap();
 		let channel_state = &mut *channel_state_lock;
-		let chan_id = match channel_state.short_to_chan_info.get(&msg.contents.short_channel_id) {
+		let chan_id = match self.short_to_chan_info.read().unwrap().get(&msg.contents.short_channel_id) {


Given there's no consistency requirements anymore, you can move the channel_state lock down below the short_to_chan_info lock.

Fixed with 41d275d

codecov-commenter · 2022-08-23T16:29:04Z

Codecov Report

Base: 90.79% // Head: 90.83% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (9e341c8) compared to base (8f525c4).
Patch coverage: 73.79% of modified lines in pull request are covered.

❗ Current head 9e341c8 differs from pull request most recent head 07664c2. Consider uploading reports for the commit 07664c2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1639      +/-   ##
==========================================
+ Coverage   90.79%   90.83%   +0.04%     
==========================================
  Files          87       86       -1     
  Lines       47604    46422    -1182     
  Branches    47604    46422    -1182     
==========================================
- Hits        43221    42168    -1053     
+ Misses       4383     4254     -129

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`84.93% <73.36%> (-0.48%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/onion_message/packet.rs	`72.52% <0.00%> (-3.51%)`	⬇️
lightning/src/util/errors.rs	`70.37% <0.00%> (-1.86%)`	⬇️
lightning/src/chain/onchaintx.rs	`93.79% <0.00%> (-1.22%)`	⬇️
lightning/src/routing/gossip.rs	`91.43% <0.00%> (-1.08%)`	⬇️
lightning/src/onion_message/messenger.rs	`89.50% <0.00%> (-1.03%)`	⬇️
lightning/src/routing/scoring.rs	`96.14% <0.00%> (-0.76%)`	⬇️
... and 49 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dunxen

Generally looks good to me. I'll try to give it another pass today, but not 100% confident with pending forwards yet :)

dunxen · 2022-08-24T11:21:12Z

lightning/src/ln/channelmanager.rs

-					let forward_chan_id = match channel_state.short_to_chan_info.get(&short_chan_id) {
-						Some((_cp_id, chan_id)) => chan_id.clone(),
-						None => {
+					macro_rules! fail_pending_forwards {


I'm not sure here, I just know that nested macros are not fun to debug 🙈

valentinewallace · 2022-08-24T19:40:13Z

lightning/src/ln/channelmanager.rs

@@ -2440,7 +2464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				}
 			}

-			let id = match channel_lock.short_to_chan_info.get(&path.first().unwrap().short_channel_id) {
+			let id = match self.short_to_chan_info.read().unwrap().get(&path.first().unwrap().short_channel_id) {


Looks like we should move this above the let payment_entry = ... line above, otherwise short_to_chan_info lock order also depends on pending_outbound_payments

Oh yes, good catch! Thanks :)

Fixed with 569f172

valentinewallace · 2022-08-24T19:43:59Z

lightning/src/ln/channelmanager.rs

@@ -5579,14 +5620,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	///
 	/// [phantom node payments]: crate::chain::keysinterface::PhantomKeysManager
 	pub fn get_phantom_scid(&self) -> u64 {
-		let mut channel_state = self.channel_state.lock().unwrap();
+		let short_to_chan_info = self.short_to_chan_info.read().unwrap();
 		let best_block = self.best_block.read().unwrap();


I think we should change this to just get the block height within a scope for use below. Otherwise a dependency between best_block and short_to_chan_info is introduced

Thanks :)

Fixed with 82b2138

valentinewallace · 2022-08-24T20:12:15Z

lightning/src/ln/channelmanager.rs

+					macro_rules! fail_pending_forwards {
+						() => {


Think you can get rid of the macro with something like:

Suggested change

macro_rules! fail_pending_forwards {

() => {

let forward_chan_id_opt = match self.short_to_chan_info.read().unwrap().get(&short_chan_id)

.map(|(_cp_id, chan_id)| chan_id.clone());

if forward_chan_id_opt.is_none() || !channel_state.by_id.contains_key(forward_chan_id_opt.unwrap()) {

but I haven't actually completed this patch myself

Oh, thanks for the suggestion :)!

That change would indeed work when only taking this PR into account. Sadly though, that change would not help when taking #1507 into consideration as well, which I'd like this PR to prepare for. 1507 is quite huge, so I'd like to do as many changes as possible within this PR, to make 1507 as reviewable as possible.

In 1507, we will lock the new channel lock in this scope by using the info retrieved from short_to_chan_info (The counterparty_node_id and the channel_id). Therefore we wouldn't be able to check that both short_to_chan_info contained a value for the scid and check that the new channel lock contains the channel for the unwrapped short_to_chan_info info in the same if clause, unless we'd take the new channel lock temporarily just for the contains check. Though by taking the channel lock temporarily, we would still need the Vaccant check below for the channel lock , hence requiring the second call site for the code in fail_pending_forwards :/

Ah okay, worth a shot!

valentinewallace · 2022-08-24T20:21:15Z

I think the fixups may be out of order? Correct me if I'm wrong, but it helps me when a fixup is next to the commit it's fixing up

ViktorTigerstrom · 2022-09-05T17:27:24Z

Thank you for the reviews @valentinewallace and @dunxen :)!

Sorry for taking so long to address the feedback, I've been traveling.

I think the fixups may be out of order? Correct me if I'm wrong, but it helps me when a fixup is next to the commit it's fixing up

Oh yes, you are correct. Thanks for informing me about that it helps in the review process to move up the fixup commits to the correct order. I was incorrectly under the impression that it was less confusing to not move them, but now I know better :). I'll move them up from now on.

TheBlueMatt · 2022-09-06T19:24:54Z

Looks like this needs rebase now :/

ViktorTigerstrom · 2022-09-07T18:12:23Z

Rebased on upstream without changes except fixing minor merge conflicts.

TheBlueMatt · 2022-09-08T16:47:55Z

lightning/src/ln/channelmanager.rs

-		$short_to_chan_info.remove(&$channel.outbound_scid_alias());
+	($self: expr, $channel: expr) => {
+		{
+			let mut id_to_peer = $self.id_to_peer.lock().unwrap();


nit: why not keep the line as-is previously vs assigning the mutexguard? Then you can drop the scope (as temporaries are drop'd automatically at the ;).

Good question 😅
Fixed with: e47fc06

Good question 😅 Fixed with: e47fc06

Hmm did this fix end up getting dropped?

Yeah I moved this change to the follow-up PR #1795. It's getting a bit unnecessary with a separate PR for the follow-ups now that I'm already doing some small fixes in this PR hah, so I will close that PR and bring the fixups into this PR.

Understood, since this is basically mergeable I support keeping this fix in followup! But up to you

Sorry missed this until I had already pushed the followups with this PR yesterday. Hope that's ok :)

TheBlueMatt · 2022-09-08T16:54:39Z

lightning/src/ln/channelmanager.rs

@@ -2246,7 +2263,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 				if let Some((err, code, chan_update)) = loop {
 					let mut channel_state = self.channel_state.lock().unwrap();
-					let id_option = channel_state.short_to_chan_info.get(&short_channel_id).cloned();
+					let id_option = self.short_to_chan_info.read().unwrap().get(&short_channel_id).cloned();


This should be above the channel_state lock, no?

Yeah sure :)! Sadly we're already holding the channel_state when taking the short_to_chan_info lock in multiple other places, but might as well fix that here.

e2a0fc8

TheBlueMatt · 2022-09-08T16:58:30Z

lightning/src/ln/channelmanager.rs

@@ -2428,6 +2452,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let err: Result<(), _> = loop {
 			let mut channel_lock = self.channel_state.lock().unwrap();

+			let id = match self.short_to_chan_info.read().unwrap().get(&path.first().unwrap().short_channel_id) {


Should short_to_chan_info comment be updated in consequence "Locked after channel_state" ?

Unfortunately we can't remove the lock order requirement for short_to_chan_info to be locked after the channel_state lock with this PR, as it's still required in multiple other places in the code, but with #1507 we'll be able to remove that lock order however :).
I think that this order change here and above mainly a preparation for that PR, and is ok here since we only take the short_to_chan_info lock temporarily and drop it before taking the channel_state lock.

I agree though that this is a bit confusing, and if you'd like, I could remove this temporary order change.

Even with the order, if this hunk were moved above the channel_lock it would be fine - because we take the read lock and then copy out of it the read lock is dropped at the semicolon after the match, which means no lock order would be implied. Not a big deal, though, its fine to leave it for now, just nice to avoid holding both locks at the same time if we can avoid it.

Yes, agree. Once #1772 is merged as well, I'll look into if it makes sense to already change the lock order so that short_to_chan_info and id_to_peer is before the channel_state, as it would be nice to make #1507 less complicated.

ViktorTigerstrom · 2022-09-08T19:40:06Z

Addressed the latest feedback, and restricted the access for the short_to_chain_info field!

ariard

Looks good, first parse

ariard · 2022-09-13T16:04:31Z

lightning/src/ln/channelmanager.rs

@@ -2428,6 +2452,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let err: Result<(), _> = loop {
 			let mut channel_lock = self.channel_state.lock().unwrap();

+			let id = match self.short_to_chan_info.read().unwrap().get(&path.first().unwrap().short_channel_id) {


Should short_to_chan_info comment be updated in consequence "Locked after channel_state" ?

ariard · 2022-09-13T16:07:55Z

lightning/src/ln/channelmanager.rs

@@ -2426,8 +2447,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);

 		let err: Result<(), _> = loop {
-			let mut channel_lock = self.channel_state.lock().unwrap();
+			let id = match self.short_to_chan_info.read().unwrap().get(&path.first().unwrap().short_channel_id) {
+				None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!".to_owned()}),


Note, error message could be "No channel found with first-hop", historically ChannelUnavailable has been used to flag a channel !is.live(), e.g when the monitor is not ready. I think it's more accurate to indicate non-availability in a way different than non-presence.

Ah ok, I didn't know this, so thanks for informing me! Is it mainly the error message that we'd like to change, or would we like to create a new APIError variant as well?

ariard · 2022-09-13T16:13:45Z

lightning/src/ln/channelmanager.rs

@@ -2506,7 +2526,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					},
 					None => { insert_outbound_payment!(); },
 				}
-			} else { unreachable!(); }
+			} else {
+				return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!".to_owned()});


Could recall the lack of consistency between short_to_chan_info and by_id, what we hit afaiu. Or move this indication as a general comment of short_to_chan_info.

Hmm I'm not sure if you mean that we'd like to change the error message here to indicate to the user the lack between the short_to_chan_info and the by_id maps, or if you mean that we should just add a code comment here to indicate it. I don't think we should change the message to surface the reason for the problem to the user, as neither the short_to_chan_info nor the by_id map are public to the user, and it would therefore make no sense to refer to them in the message.

I'll add a code comment here to note why this was triggered :). See: a8b9fd0

ariard · 2022-09-13T16:40:10Z

lightning/src/ln/channelmanager.rs

-					let forward_chan_id = match channel_state.short_to_chan_info.get(&short_chan_id) {
-						Some((_cp_id, chan_id)) => chan_id.clone(),
-						None => {
+					macro_rules! fail_pending_forwards {


Note, I think of the phantom invoices handling logic is actually in this branch (L3168-3193), so I'm not sure about qualifying it as "fail_pending_forwards" as in some case we might fetch the phantom payment.

Not necessarily in that PR, but I think it could be interesting to break up process_pending_htlc_forwards in few smaller helpers, especially as here we're starting to reuse some code paths.

Ah good point! Perhaps we'd like to name the macro to something else than fail_pending_forwards then? I'll rename it to forwarding_channel_not_found temporarily, but if you have a better suggestion let me know :).

I also agree that process_pending_htlc_forwards is starting to get very long and complicated, and would be nice to brake up if possible. Adding this macro also makes everything more complicated and harder to debug.

Name changed with: c95961e

ariard · 2022-09-13T17:06:07Z

lightning/src/ln/channelmanager.rs

@@ -5907,14 +5943,15 @@ where
 							// enforce option_scid_alias then), and if the funding tx is ever
 							// un-confirmed we force-close the channel, ensuring short_to_chan_info
 							// is always consistent.
+							let mut short_to_chan_info = self.short_to_chan_info.write().unwrap();


Still confused here by the what is the correct lock order between short_to_chan_info and channel_state one L5895

See #1639 (comment)

TheBlueMatt

This LGTM. Please squash and lets land it!

TheBlueMatt · 2022-11-02T00:43:56Z

lightning/src/ln/channelmanager.rs

 		$pending_msg_events.push(events::MessageSendEvent::SendChannelReady {
 			node_id: $channel.get_counterparty_node_id(),
 			msg: $channel_ready_msg,
 		});
 		// Note that we may send a `channel_ready` multiple times for a channel if we reconnect, so
 		// we allow collisions, but we shouldn't ever be updating the channel ID pointed to.
-		let outbound_alias_insert = $short_to_chan_info.insert($channel.outbound_scid_alias(), ($channel.get_counterparty_node_id(), $channel.channel_id()));
+		let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap();


nit: Can we scope this lock (or the macro, by adding extra {}s in the macro)? I'm not sure its required but I also don't really understand the rules for drop when leaving a macro.

Fixed with the latest push. I didn't push in a separate fixup commit, hope that's ok!

TheBlueMatt · 2022-11-02T00:57:46Z

lightning/src/ln/channelmanager.rs

@@ -2428,6 +2452,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let err: Result<(), _> = loop {
 			let mut channel_lock = self.channel_state.lock().unwrap();

+			let id = match self.short_to_chan_info.read().unwrap().get(&path.first().unwrap().short_channel_id) {


Even with the order, if this hunk were moved above the channel_lock it would be fine - because we take the read lock and then copy out of it the read lock is dropped at the semicolon after the match, which means no lock order would be implied. Not a big deal, though, its fine to leave it for now, just nice to avoid holding both locks at the same time if we can avoid it.

ViktorTigerstrom · 2022-11-02T20:26:08Z

Squashed and addressed latest feedback :)!

lightning/src/ln/channelmanager.rs

valentinewallace

LGTM, feel free to take or leave nits

valentinewallace · 2022-11-03T16:34:13Z

lightning/src/ln/channelmanager.rs

-		$short_to_chan_info.remove(&$channel.outbound_scid_alias());
+	($self: expr, $channel: expr) => {
+		{
+			let mut id_to_peer = $self.id_to_peer.lock().unwrap();


Good question 😅 Fixed with: e47fc06

Hmm did this fix end up getting dropped?

lightning/src/ln/channelmanager.rs

ViktorTigerstrom · 2022-11-03T21:01:07Z

Thanks @dunxen & @valentinewallace.
Will address the nits and move the follow-ups from #1795 into this PR

ViktorTigerstrom · 2022-11-03T21:30:56Z

Addressed the latest feedback, and also brought in the followups from #1795 into this PR (ie. scope fix for update_maps_on_chan_removal, as well as locking short_to_chan_info temporarily before locking channel_state in 2 occasions as a preparation for moving the lock_order for short_to_chain_info to before the channel_state lock).

I fixed up these changes into the corresponding commit already as they were minor. I hope that's ok :)

valentinewallace · 2022-11-04T15:22:43Z

LGTM after rebase

ViktorTigerstrom · 2022-11-04T17:34:23Z

Rebased on main. The only changes were merge conflict fixes 🚀!

TheBlueMatt · 2022-11-04T18:41:25Z

Grrrrr, I'm sorry, this was dumb #1639 (comment) . We should not be checking is_usable on a channel before claiming, especially not if its shutting down - a channel shutting down is still able to clear HTLCs, and in fact we want to clear the HTLCs on it by claiming, that way it can actually progress to mutual shutdown from simply being "in shutdown". Feel free to just remove that and squash and push. I'm ready to ack after that, I believe.

I think what i was thinking is we should check if the peer is online before claiming, which, like, maybe, but its not critical, and we'd need to check specifically only for if they're connected.

ViktorTigerstrom · 2022-11-04T19:19:15Z

Removed the is_usable check and squashed

As the `channel_state` (`ChannelHolder`) struct will be removed, this commit moves the `short_to_chan_info` map from that lock into a seperate lock.

As the `short_to_chan_info` has been moved out of the `channel_state` to a standalone lock, several macros no longer need the `channel_state` passed into the macro.

As the `short_to_chan_info` map has been removed from the `channel_state`, there is no longer any consistency guarantees between the `by_id` and `short_to_chan_info` maps. This commit ensures that we don't force unwrap channels where the channel_id has been queried from the `short_to_chan_info` map.

Refactor `process_pending_htlc_forwards` to ensure that both branches that fails `pending_forwards` are placed next to eachother for improved readability.

ViktorTigerstrom · 2022-11-04T19:28:44Z

Sorry, reverted once more to just remove the is_usable check and not the existence check 😅

ViktorTigerstrom commented Jul 28, 2022

View reviewed changes

ViktorTigerstrom mentioned this pull request Aug 1, 2022

Don't return channel_state from decode_update_add_htlc_onion #1638

Merged

ViktorTigerstrom closed this Aug 1, 2022

ViktorTigerstrom reopened this Aug 9, 2022

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from d9eb4c1 to 1da7a95 Compare August 9, 2022 00:17

TheBlueMatt reviewed Aug 15, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 1da7a95 to edbff4c Compare August 18, 2022 23:36

ViktorTigerstrom commented Aug 18, 2022

View reviewed changes

TheBlueMatt reviewed Aug 19, 2022

View reviewed changes

ViktorTigerstrom mentioned this pull request Aug 22, 2022

Move forward_htlcs into standalone lock #1656

Merged

TheBlueMatt reviewed Aug 22, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label Aug 22, 2022

dunxen reviewed Aug 24, 2022

View reviewed changes

valentinewallace reviewed Aug 24, 2022

View reviewed changes

valentinewallace removed the Seeking Code Review label Aug 24, 2022

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 41d275d to 913daf8 Compare September 5, 2022 17:18

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 913daf8 to 3e3d954 Compare September 7, 2022 18:11

TheBlueMatt reviewed Sep 8, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 3e3d954 to 9e341c8 Compare September 8, 2022 19:35

ariard reviewed Sep 13, 2022

View reviewed changes

TheBlueMatt reviewed Nov 2, 2022

View reviewed changes

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 1ea75e3 to 590eae3 Compare November 2, 2022 20:18

TheBlueMatt assigned dunxen, valentinewallace and TheBlueMatt Nov 2, 2022

dunxen reviewed Nov 3, 2022

View reviewed changes

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

dunxen previously approved these changes Nov 3, 2022

View reviewed changes

valentinewallace previously approved these changes Nov 3, 2022

View reviewed changes

ViktorTigerstrom dismissed stale reviews from valentinewallace and dunxen via a38c9c4 November 3, 2022 21:26

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 590eae3 to a38c9c4 Compare November 3, 2022 21:26

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from a38c9c4 to acf17e7 Compare November 4, 2022 17:32

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from acf17e7 to 07664c2 Compare November 4, 2022 19:16

ViktorTigerstrom added 4 commits November 4, 2022 20:26

Move short_to_chan_info into standalone lock

c82a65a

As the `channel_state` (`ChannelHolder`) struct will be removed, this commit moves the `short_to_chan_info` map from that lock into a seperate lock.

Remove excess channel_state passing to macros

3fa10c8

As the `short_to_chan_info` has been moved out of the `channel_state` to a standalone lock, several macros no longer need the `channel_state` passed into the macro.

Make process_pending_htlc_forwards more readable

b368941

Refactor `process_pending_htlc_forwards` to ensure that both branches that fails `pending_forwards` are placed next to eachother for improved readability.

ViktorTigerstrom force-pushed the 2022-07-move-short-to-chan-info branch from 07664c2 to b368941 Compare November 4, 2022 19:27

TheBlueMatt approved these changes Nov 4, 2022

View reviewed changes

valentinewallace approved these changes Nov 4, 2022

View reviewed changes

TheBlueMatt merged commit 505102d into lightningdevkit:main Nov 4, 2022

ViktorTigerstrom mentioned this pull request Nov 29, 2022

Store channels per-peer #1507

Merged

		};
		($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr) => {

-					macro_rules! fail_pending_forwards {
-						() => {
+					let forward_chan_id_opt = match self.short_to_chan_info.read().unwrap().get(&short_chan_id)
+						.map(|(_cp_id, chan_id)| chan_id.clone());
+					if forward_chan_id_opt.is_none() || !channel_state.by_id.contains_key(forward_chan_id_opt.unwrap()) {

Move short_to_chan_info into standalone lock #1639

Move short_to_chan_info into standalone lock #1639

Conversation

ViktorTigerstrom commented Jul 28, 2022

Choose a reason for hiding this comment

ViktorTigerstrom commented Aug 1, 2022

ViktorTigerstrom commented Aug 9, 2022

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

ViktorTigerstrom commented Aug 18, 2022

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

ViktorTigerstrom commented Aug 22, 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

codecov-commenter commented Aug 23, 2022 • edited Loading

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Aug 24, 2022

ViktorTigerstrom commented Sep 5, 2022

TheBlueMatt commented Sep 6, 2022

ViktorTigerstrom commented Sep 7, 2022

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 Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Sep 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

ViktorTigerstrom commented Sep 8, 2022

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

ViktorTigerstrom Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Move `short_to_chan_info` into standalone lock #1639

Move `short_to_chan_info` into standalone lock #1639

codecov-commenter commented Aug 23, 2022 •

edited

Loading

ViktorTigerstrom Sep 5, 2022 •

edited

Loading

ViktorTigerstrom Sep 5, 2022 •

edited

Loading

ViktorTigerstrom Sep 5, 2022 •

edited

Loading

ViktorTigerstrom Nov 4, 2022 •

edited

Loading

ViktorTigerstrom Sep 8, 2022 •

edited

Loading

ViktorTigerstrom Sep 18, 2022 •

edited

Loading

ViktorTigerstrom Sep 18, 2022 •

edited

Loading

TheBlueMatt commented Nov 4, 2022 •

edited

Loading