lightningd/opening_control.c: fundchannel_cancel no longer requires a channel_id argument.

[ZmnSCPxj]

Fixes: #3785

Changelog-Changed: fundchannel_cancel no longer requires its undocumented channel_id argument after fundchannel_complete.

Note: this implies changes in #3763.

[cdecker]

Note: this implies changes in #3763.

How is the dependency on #3763? Is this assuming #3763 to be merged or would merging this cause incompatibilities in #3767 (and thus requiring that to be amended)?

Just wondering about the order in which to review / merge.

[ZmnSCPxj]

The sequence fundchannel_start fundchannel_complete fundchannel_cancel requires that the cancel command have a channel_id argument, because if there are recently-closed channels that have not yet passed the 100-block limit at which we forget them, fundchannel_cancel will fail with a message "Multiple channels". We only need to provide channel_id if there are recently-closed channels, but detecting those is strictly more complex. So #3763 just always provide channel_id to fundchannel_cancel when performed after fundchannel_complete.

Since this PR completely removes channel_id, the fundchannel_cancel command execution in #3763 will fail with an unknown keyword (or at least I think it should, do we not fail on seeing an unknown keyword on params?).

I am wary of retaining the channel_id parameter in deprecated_apis since it is not a documented parameter anyway, and it only applies for multifundchannel, because the argument only is needed after a successful fundchannel_complete, and a single-peer fundchannel will never fail after a successful fundchannel_complete (only multifundchannel could, if one of the other peers failed fundchannel_complete but this one did, we need to still cancel it even after success).

We can merge in any order, though if we merge one, the other has to be modified before merging.

[cdecker]

Hm, I'm not too familiar with the fundchannel_* semantics, I think @niftynei can say more about the internals and whether it makes sense to remove the channel_id, or whether we should just document it properly.

[ZmnSCPxj]

Well, fundchannel_start implicitly starts an uncommitted channel with the peer. This uncommitted channel has no channel ID (it has a temporary one that fundchannel_start automatically creates, the temporary_channel_id described in BOLT2 open_channel/accept_channel/funding_created, but fundchannel_start does not return it). So when cancelling between fundchannel_start and fundchannel_complete, the channel_id argument to fundchannel_cancel makes no sense (you can literally put a garbage string into it). But after fundchannel_complete, you have to provide the channel_id otherwise previous closed-but-not-forgotten channels will trigger the "Multiple channels" error, yet currently we only support one open channel per peer anyway, so "Multiple channels" makes no sense.

[ZmnSCPxj]

Rebased, because github says so.

[ZmnSCPxj]

@niftynei bump!!

[niftynei]

Error message needs a wording change, otherwise lgtm. This is the right direction to go for this imo.

[niftynei]

Suggested change

	"No channels matching that peer_id");
	"No channels awaiting lock-in for peer_id %s", ...);

There might be a channel, but in the wrong state.

[ZmnSCPxj]

Since fundchannel_cancel can cancel not just channels awaiting lockin, but also uncommitted channels (i.e. OPENGIND state), I reword instead: "No channels being opened or awaiting lock-in for peer_id".

[ZmnSCPxj]

I also took the liberty of giving a specific error code for fundchannel_cancel refusing to cancel if the channel is already too advanced to safely cancel (i.e. funding tx known to be broadcast, peer initiated channel, channel has HTLCs), as well as a separate error code for when we cannot find a channel to cancel for that peer.

[niftynei]

ACK bcd9e4c

I also took the liberty of giving a specific error code for fundchannel_cancel refusing to cancel if

Nice!