[ekzyis]

This implements the RecvShutdown operation for #98.

RecvShutdown consumes the SentShutdown of a previous SendShutdown and
waits for the target's `shutdown` on the given channel. It returns that
channel's scriptpubkey, or empty bytes if no `shutdown` was received.

The target can send `shutdown` at any time and on any channel, so a
received one is recorded into its channel's state like `channel_ready`,
and explicit receives skip past it instead of failing. SendShutdown
records the script we sent, which is what makes a reply expected there,
and RecvShutdown reads the reply from channel state, so one recorded
before it ran is reported just the same.

BOLT 2 lets the target warn about a non-standard scriptpubkey instead of
replying, but every implementation except LDK disconnects afterwards and
LDK acts as if the `shutdown` never came, so a warning just ends
execution like any other unexpected message.

A target that has not sent `channel_ready` may ignore our `shutdown`, so
RecvShutdown is a no-op until it has. That gives up coverage of targets
which do reply before then, in exchange for not spending a receive
timeout on every target which doesn't.

[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

[morehouse]

This is the first affine variable to contain actual data, and I don't think we really need to do this.

We could make RecvShutdown like RecvChannelReady and record our shutdown script in the channel state when we send shutdown. Then no matter which shutdown we get a response for, we can look it's channel up in the channel state to get our script.

[ekzyis]

The affine variable SentShutdown no longer contains any data in 677a7a6. We record the shutdown scripts we send and receive in our channel state now. RecvShutdown reads replies from there.

However, I did take this part less literally, though:

no matter which shutdown we get a response for, we can look its channel up in the channel state

I decided to include ChannelId as an input to RecvShutdown. If RecvShutdown doesn't know for which channel we want to receive shutdown and we expect shutdown on multiple channels, it wasn't clear to me which shutdown script RecvShutdown should return. That's the difference to RecvChannelReady: it has no output that depends on the channel.

I considered recording the timestamps of received shutdown messages to then always return the latest received shutdown script, but it seemed flawed compared to adding ChannelId to RecvShutdown. It would be more code to come up with some reasonable order for RecvShutdown and shutdown messages to deal with an issue in a less obvious way. WDYT?

[ekzyis]

@morehouse I'll get back to you on this once I've prepared my workshop for TABConf (TABConf/8.tabconf.com#10). Maybe I can include eclair nodes with the recently disclosed vulnerabilities to warnet.

[ekzyis]

677a7a6: addressed feedback (see replies) and updated commit message + PR description

[ekzyis]

20ac848: revert changes to consume_affine because the return value is no longer needed

[ekzyis]

6bdc2ea:

• improve oracle TODOs
• add tests for "first one is kept" when recording sent or received shutdowns
• improve tests with stricter assertions

[ekzyis]

09799ed:

• use received_channel_ready_point.is_some() instead of next_counterparty_per_commitment_point.is_some()
  in is_shutdown_expected
• mention RecvShutdown is a no-op if no channel_ready was received yet
• remove stray "no-op" comment

[ekzyis]

ce9d13d: rebased on master