Don't return channel_state from decode_update_add_htlc_onion
#1638
Conversation
lightning/src/ln/channelmanager.rs
Outdated
| @@ -2246,13 +2241,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
| } | |||
| }; | |||
|
|
|||
| channel_state = Some(self.channel_state.lock().unwrap()); | |||
| let mut channel_state = self.channel_state.lock().unwrap(); | |||
There was a problem hiding this comment.
While you're here, can you move this down inside if let Some((err, code, chan_update)) = loop {? That way we're not holding the lock at all if we call return_err later.
There was a problem hiding this comment.
Nvm, I'll take the channel_state lock temporarily for the short_to_chan_info and then drop it.
36ce228
There was a problem hiding this comment.
Please don't lock+unlock twice immediately back-to-back? That's unnecessary.
There was a problem hiding this comment.
Wow.. Honestly disappointed in myself that I pushed that yesterday night.
Fixed with: 4a07ac0
ViktorTigerstrom
force-pushed
the
2022-07-update-decode-update-add-htlc-onion-return-parameters
branch
2 times, most recently
from
36ce228 to
4a07ac0
Compare
|
LGTM, feel free to squash. |
Currently `decode_update_add_htlc_onion` returns the `channel_state` lock to ensure that `internal_update_add_htlc` holds a single `channel_state` lock in when the entire function execution. This is unnecessary, and since we are moving the channel storage to the `per_peer_state`, this no longer achieves the goal it was intended for. We therefore avoid returning the `channel_state` from `decode_update_add_htlc_onion`, and just retake the lock in `internal_update_add_htlc` instead.
ViktorTigerstrom
force-pushed
the
2022-07-update-decode-update-add-htlc-onion-return-parameters
branch
from
4a07ac0 to
65e6fb7
Compare
|
Squashed without changes. Thanks for the reviews @dunxen & @TheBlueMatt! |
There was a problem hiding this comment.
ACK 65e6fb7
Unrelated: I remember now @TheBlueMatt mentioned that GH actions weirdness with that specific Rust version :/
Prerequisite for #1507, and addresses comment #1507 (comment).
Currently
decode_update_add_htlc_onionreturns thechannel_statelock to ensure that
internal_update_add_htlcholds a singlechannel_statelock in when the entire function execution. This isunnecessary, and since we are moving the channel storage to the
per_peer_state, this no longer achieves the goal it was intended for.We therefore avoid returning the
channel_statefromdecode_update_add_htlc_onion, and just retake the lock ininternal_update_add_htlcinstead.