smite-ir: add RecvShutdown operation

[ekzyis]

This implements the RecvShutdown operation for #98.

RecvShutdown consumes the SentShutdown of a previous SendShutdown, which now carries the shutdown we sent, and waits for the target's shutdown in reply. It returns the target's scriptpubkey, or empty bytes if no shutdown was received.

The reply is checked by a new ShutdownOracle. It flags the target if:

• the reply is for a channel we never established
• we sent an invalid signature, so the target should have failed the channel instead of replying
• our scriptpubkey doesn't match the upfront_shutdown_script we committed to and option_upfront_shutdown_script was negotiated, so the target must fail the connection instead of replying
• its scriptpubkey doesn't match the upfront_shutdown_script it committed to, or isn't a standard form for the negotiated features

If our own scriptpubkey isn't standard, BOLT 2 says the target should send a warning instead of replying. If it breaks our upfront_shutdown_script, the target may send a warning before failing the connection. In both cases we accept a warning for the channel as a reply.

A shutdown for another tracked channel may answer one we sent there earlier, which we can't check against the shutdown we sent here, so it ends the program as an unexpected message.

RecvShutdown is a no-op if we don't track the channel or the target already replied. Before the target sent channel_ready, it may choose not to reply, but we still expect one: LDK always replies, and a target doesn't only costs us a receive timeout, which isn't reported as a violation.

[NishantBansal2003]

I think this needs to be rebased on top of #162 to consume the affine type SentShutdown

[morehouse]

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).

[ekzyis]

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.

Added TODOs in 77cf4f8. I thought it would be useful to group all HTLC TODOs together with TODO(htlc) as the prefix, so I updated the existing HTLC TODOs in c9a2c60:

$ git grep "TODO(htlc)"
smite-scenarios/src/executor.rs:                    // TODO(htlc): we only expect `shutdown` when all HTLCs are resolved, else this
smite/src/channel_tx/commitment.rs:    // TODO(htlc): When adding HTLC support, store pending HTLCs (offered/received) for both sides
smite/src/channel_tx/commitment.rs:    // TODO(htlc): When adding HTLC support, add `get_next_commitment_state` to build the next
smite/src/oracles/shutdown.rs:        // TODO(htlc): BOLT 2 forbids sending `shutdown` while HTLCs are still pending

---

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

We now check in 77cf4f8 if we expect a shutdown on any channel. Only then we wait for a shutdown response. We don't know on which channel we will receive a shutdown, but I could change SentShutdown to SentShutdown(ChannelId), then we would know (just realized while typing this).

TODO:

• use SentShutdown(ChannelId) and check peer_shutdown_received for that channel in is_shutdown_expected

---

Mhh, I wonder if there's a conflict between this (#163 (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

and this (#163 (review)):

if they have then any subsequent RecvShutdown message becomes a no-op (similar to RecvChannelReady).

In 77cf4f8, RecvShutdown returns empty bytes as a sentinel similar to LookupShortChannelId when no shutdown is expected on any channel. It's clearly an invalid shutdown script. Could this be a problem for downstream consumers of its return value, who might expect a valid shutdown script? Should they then also be no-ops, at least in the case of RecvShutdown?

[ekzyis]

c9a2c60...d083969:

• SentShutdown(ChannelId) instead of SentShutdown so we can check if we expect a shutdown for the specific channel.
• Consider upfront_shutdown_script in ShutdownOracle. It doesn't check if it's standard as it should be according to BOLT 2, since I think this check should be added to AcceptChannelOracle.
• Updated commit message.

I forgot tests for ShutdownOracle like for AcceptChannelOracle. Will add them.

[ekzyis]

This is ready for review now. I'm working on a new PR with a ShutdownGenerator so we can test RecvShutdown against the targets.

[morehouse]

This PR has grown a bit.

@ekzyis would it be possible to split this into smaller PRs? Perhaps something like this:

• PR 1: RecvShutdown operation
• PR 2: initial shutdown oracle
• PR 3: check upfront_shutdown_script on shutdown

[ekzyis]

fd0bbbb: rebased on master (dc2ef28) and only includes the new RecvShutdown operation now. Will push PRs for the two other commits.

FYI, this question in #163 (comment) is still open for me:

RecvShutdown returns empty bytes as a sentinel similar to LookupShortChannelId when no shutdown is expected on any channel. It's clearly an invalid shutdown script. Could this be a problem for downstream consumers of its return value, who might expect a valid shutdown script? Should they then also be no-ops, at least in the case of RecvShutdown?

But I think it's more relevant when we start consuming the output of RecvShutdown.

[ekzyis]

8d65b49: add execute_recv_shutdown_unknown_channel test in this commit and update UnexpectedMessage with InvalidShutdown in the next PR, instead of adding the test in the next PR