smite-ir: add RecvShutdown operation - #163
Conversation
6bf9458 to
753acf6
Compare
| /// Receives and decodes a `shutdown` message. | ||
| fn recv_shutdown(conn: &mut impl Connection) -> Result<Shutdown, ExecuteError> { | ||
| match recv_non_ping(conn, RECV_IDLE_TIMEOUT)? { | ||
| Message::Shutdown(sd) => Ok(sd), | ||
| other => Err(ExecuteError::UnexpectedMessage { | ||
| expected: msg_type::SHUTDOWN, | ||
| got: other.msg_type(), | ||
| }), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Should RecvShutdown take an affine SentShutdown input?
Right now it has no inputs, so nothing stops a mutator from orphaning it (e.g. InstructionDeleteMutator dropping the paired send), leaving a blocking recv for a message the target never sends that results on RECV_IDLE_TIMEOUT on every run.
If we model self-initiated cooperative close (SendShutdown -> RecvShutdown), an affine SentShutdown couples the pair through the existing delete/reorder guards and makes orphan waits unrepresentable, same as SentOpenChannel.
There was a problem hiding this comment.
I had the same question. In theory, the peer could send shutdown at any time, so it's not necessarily a response (which is why I also use "message" instead of "response" at operation.rs:224).
However, I agree that, for a self-initiated cooperative close, it makes sense to use affine inputs for the reasons you mentioned. I just wasn't sure whether that would limit us too much in the future, so I thought it would be better to discuss the more general problem of messages that can be either responses or unsolicited messages now. But since I can't think of a scenario where we would expect the peer to send an unsolicited shutdown and would therefore want to use RecvShutdown without an input, I'm going to accept your suggestion (will push the code later).
There was a problem hiding this comment.
We could add an Operation that triggers the target to initiate a cooperative close with us. I don't think any Lightning implementation will initiate a cooperative close on its own without an external request (e.g., an RPC call), so this operation could be paired with an affine type to model that behavior.
The same idea applies to channel opening. Instead of always acting as the funder, we could add an operation that triggers the target to open a channel with us, allowing us to act as the fundee. This would exercise additional protocol paths and cover a broader set of channel states.
I think this is not at the current IR design #5, but it could be a great addition to it.
cc @morehouse
There was a problem hiding this comment.
I also think this could be a great addition!
There was a problem hiding this comment.
I agree we should consume an affine variable to ensure we never execute a RecvShutdown before a corresponding SendShutdown has been sent.
In the short-medium term I think we should prioritize flows that are initiated by us (e.g., we send the first shutdown), since bugs found that way are generally more severe (an attacker can more easily trigger them).
Target-initiated flows can still have bad bugs, but since they're less exploitable I think that work should be lower-priority.
NishantBansal2003
left a comment
NishantBansal2003
left a comment
There was a problem hiding this comment.
I think this needs to be rebased on top of #162 to consume the affine type SentShutdown
| } | ||
|
|
||
| #[test] | ||
| fn display_recv_shutdown_program() { |
There was a problem hiding this comment.
Once rebased, I think we can merge this with display_send_shutdown_program, similar to displays_send_funding_created_recv_funding_signed_program
| let mut scriptpubkey = vec![0x00, 0x14]; | ||
| scriptpubkey.extend_from_slice(&[0xab; 20]); |
There was a problem hiding this comment.
Could use: ShutdownScriptVariant here
| Self::SendMessage | ||
| | Self::SendChannelReady { .. } | ||
| | Self::RecvChannelReady | ||
| | Self::RecvShutdown |
There was a problem hiding this comment.
I think this should output the spk so it can be used in subsequent channel closing messages, otherwise, we will always be rejected in those cases later
| } | ||
|
|
||
| /// Receives and decodes a `shutdown` message. | ||
| fn recv_shutdown(conn: &mut impl Connection) -> Result<Shutdown, ExecuteError> { |
There was a problem hiding this comment.
Nit: Could move this below recv_channel_ready for consistency
morehouse
left a comment
morehouse
left a comment
There was a problem hiding this comment.
I think we want the affine type wired in for sure.
But that probably isn't enough -- we can't expect the target to send us a shutdown message until all HTLCs have been resolved as well. There's also probably a case where we could send two shutdown messages and the target may ignore any of them after the first one.
I think for the HTLCs we can't really implement that part until #111 is implemented, so we can just add a TODO for that.
For the duplicate shutdown case we could probably add a flag to the channel state that indicates whether the peer has already responded to the first shutdown, and if they have then any subsequent RecvShutdown message becomes a no-op (similar to RecvChannelReady).
| /// Receives and decodes a `shutdown` message. | ||
| fn recv_shutdown(conn: &mut impl Connection) -> Result<Shutdown, ExecuteError> { | ||
| match recv_non_ping(conn, RECV_IDLE_TIMEOUT)? { | ||
| Message::Shutdown(sd) => Ok(sd), | ||
| other => Err(ExecuteError::UnexpectedMessage { | ||
| expected: msg_type::SHUTDOWN, | ||
| got: other.msg_type(), | ||
| }), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I agree we should consume an affine variable to ensure we never execute a RecvShutdown before a corresponding SendShutdown has been sent.
In the short-medium term I think we should prioritize flows that are initiated by us (e.g., we send the first shutdown), since bugs found that way are generally more severe (an attacker can more easily trigger them).
Target-initiated flows can still have bad bugs, but since they're less exploitable I think that work should be lower-priority.
4 participants
This implements the
RecvShutdownoperation for #98.TODO:
shutdownhas standard shutdown scriptshutdownmatches channel id we'd expectupfront_shutdown_scriptbut mark it as TODO if too much for now