multi: record channel negotiation state and fold build/send operations#134
multi: record channel negotiation state and fold build/send operations#134NishantBansal2003 wants to merge 4 commits into
Conversation
Store negotiated open_channel/accept_channel messages during execution so later commitment steps use the parameters actually exchanged on the wire. This lets BuildFundingCreated derive the negotiated parameters from temporary_channel_id, reducing its inputs and ensuring commitment transaction signing and signature verification always use the negotiated state stored by the executor. This change also intentionally does not fold BuildOpenChannel into SendOpenChannel. Both require many inputs, and keeping them separate makes it easier to test duplicate open_channel send cases. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Build the funding_created message from the channel parameters negotiated during the open_channel/accept_channel exchange. This ensures commitment transaction signing and signature verification always use the negotiated state stored by the executor, rather than separate inputs. Reducing the inputs to just the required parameters also makes it possible to test invalid-signature cases while ensuring the stored negotiation state remains correct. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Fold BuildFundingCreated into SendFundingCreated. Since the intermediate funding_created message is not used elsewhere, merging the two operations simplifies the API. Unlike BuildOpenChannel and SendOpenChannel, which remain separate because of their large number of inputs and the need to test duplicate open_channel sends, funding_created requires far fewer inputs. This makes it straightforward to exercise duplicate send cases even after combining the build and send steps. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Fold BuildChannelReady into SendChannelReady. Since the intermediate channel_ready message is not used elsewhere, merging the two operations simplifies the API. As with funding_created, channel_ready requires relatively few inputs, making it straightforward to exercise duplicate send cases even after combining the build and send steps. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
NishantBansal2003
force-pushed
the
channel-negotiation
branch
from
3289dd1 to
74ec820
Compare
| let open_channel = match Message::decode(bytes) { | ||
| Ok(Message::OpenChannel(oc)) => oc, | ||
| other => panic!("SendOpenChannel did not produce an open_channel message: {other:?}"), | ||
| }; |
There was a problem hiding this comment.
Nit: we now do an extra open_channel decode. Perhaps we should make the OpenChannelMessage type contain an OpenChannel instead of a Vec<u8> to avoid the unnecessary work.
| let pending = negotiations | ||
| .get_mut(&accept_channel.temporary_channel_id) | ||
| .ok_or(ExecuteError::UnknownChannel( | ||
| accept_channel.temporary_channel_id, | ||
| ))?; | ||
| pending.accept_channel = Some(accept_channel.clone()); |
There was a problem hiding this comment.
Theoretically, if we sent two open_channel messages with the same temporary_channel_id, we would record the first one. But if the target responded to both, we would record the second accept_channel, which would not align with the recorded open_channel message.
I would be surprised if any implementation actually responds to the second open_channel, and I'm not sure what the best action is in that case, but just wanted to flag this for discussion.
There was a problem hiding this comment.
temporary_channel_id reuse should be rejected until the previous negotiation reaches funding_created. So if the target sends a second accept_channel reusing the same temporary_channel_id, I think we should return an error
| let accept_channel = pending.accept_channel.as_ref().unwrap(); | ||
| assert_eq!(accept_channel.temporary_channel_id, temporary_channel_id); |
There was a problem hiding this comment.
Nit: we could check that the entire accept_channel message is equal to sample_accept_channel().
| let pending = negotiations | ||
| .get(&temporary_channel_id) | ||
| .ok_or(ExecuteError::IncompleteNegotiation(temporary_channel_id))?; | ||
| let open_channel = &pending.open_channel; | ||
| let accept_channel = pending | ||
| .accept_channel | ||
| .as_ref() | ||
| .ok_or(ExecuteError::IncompleteNegotiation(temporary_channel_id))?; | ||
|
|
There was a problem hiding this comment.
By returning IncompleteNegotiation, we lose all ability to send funding_created out of order. I think it would be better to send some default-constructed funding_created when we're missing the data we need for a valid signature.
Something like this:
temporary_channel_id,funding_txid, andfunding_output_indexdetermined from inputssignatureis all zeroes- nothing saved to
channel_states
| // Building the same message again must not clobber a channel whose state | ||
| // has already been established (and possibly advanced). | ||
| channel_states | ||
| .entry(channel_id) | ||
| .or_insert_with(|| ChannelState::new(config, holder, state)); |
There was a problem hiding this comment.
I think we also want to either delete or mark the entry in negotiations so that we're able to open another channel using the same temporary_channel_id and record the parameters for that channel.
The simplest approach is to delete the entry from negotiations once the channel "graduates" to a real channel_id. But if we want to keep the ability to easily send duplicate funding_created messages, we can also keep the entry around and just set a flag on the negotiations entry to mark the channel as "graduated". Graduated entries can still be used for subsequent funding_created messages but also get overwritten by the next SendOpenChannel that uses the same temporary_channel_id.
|
I was also concerned about
Based on that, I expected implementations to reject any reused But, I checked the code in CLN and LDK (and also verified it by running https://github.com/NishantBansal2003/smite/tree/funding-created-generator), and it turns out that once CLN: https://github.com/ElementsProject/lightning/blob/58f0c16a57e324529d8ce9f40e6511d31b004c0e/connectd/multiplex.c#L73-L78
I think this is the better way to handle such cases |
2 participants
Fixes: #130
Store negotiated
open_channel/accept_channelmessages during execution so later commitment steps use the parameters actually exchanged on the wire.This change is accompanied by
BuildFundingCreatedusing the negotiated state to build the channel state used for signing and verification. Also, all the states are now recorded on send, so there is no state corruption between the target and smite if we store a different state from what was actually sent to the peer.Mostly, this change is done so that we can extend this approach to add
open_channel <-> accept_channeloracles, which require a mapping between these two messages. This change lays the groundwork so we can add those oracles in subsequent PRs.