Send channel_announcement for splice transactions on public channels

[t-bast]

We now support splicing on public channels: once the splice transaction is confirmed and locked on both sides, nodes will exchange announcement signatures that allows them to create a channel_announcement that they then broadcast to the network.

This requires reworking the data model to include the announcement and the real short_channel_id in each commitment, which lets us cleanly distinguish real short_channel_ids from aliases (which are set at the channel level regardless of the current commitments).

The flow now becomes:

• when the funding transaction of a commitment confirms, we set the corresponding real short_channel_id in that commitment
• if the channel is public and we've received channel_ready or splice_locked, we send our announcement_signatures
• if we receive announcement_signatures for a commitment for which the funding transaction is unconfirmed, we stash it and replay it when the transaction confirms
• if we receive announcement_signatures for a confirmed commitment, and we don't have a more recently announced commitment, we generate a channel_announcement, store it with the commitment and update our router data

When creating a channel_update for a public channel, we always use the short_channel_id that matches the latest announcement we created. This is very important to guarantee that nodes receiving our updates will not discard them because they cannot match it to a channel.

For private channels, we stop allowing usage of the short_channel_id for routing: scid_alias MUST be used, which ensures that the channel utxo isn't revealed.

Note that when migrating to taproot channels, splice_locked will be used to transmit nonces for the announcement signatures, which will be compatible with the existing flow (and similarly, channel_ready will be used for the initial funding transaction). They are retransmitted on reconnection to ensure that the announcements can be generated.

Don't be scared by the large number of added lines: most of them are JSONs for backwards-compatibility serialization tests.

[t-bast]

Important note: for channels encoded with v3 or lower, we don't add any migration code, so it looks like we're losing information about the real short_channel_id, but that isn't the case: since we set localFundingStatus to unconfirmed (in ChannelTypes0.scala and ChannelTypes3.scala), we will set a WatchFundingConfirmed when restarting which lets us obtain details about the funding transaction and correctly set the short_channel_id.

[remyers]

I've taken a first pass and it looks good so far. I've focused mostly on the channel and router behavior; less on the serialization so far.

During a channel reestablishment it seems like we only resend announcement sigs when no announcement was sent during initial funding, but not during splices.

I also have a few questions and suggestions to add tests for a few incidental bug fixes you made during the refactor that probably don't have associated tests.

[remyers]

The serialization code looks pretty straight forward for the most part. I do have one suggestion and found one potential issue related to moving the real scid to the funding status.

The merge-base changed after approval.

[pm47]

LGTM, only nits.

[pm47]

It could also be that remoteAnnSigs.shortChannelId is wrong or unexisting. I find it a little weird that the condition to add to announcementSigsStash and remove from it are not symmetrical:

• we stash when the related commitment is not found
• we unstash when we receive a splice_locked from remote.

Shouldn't we instead:

• stash when the corresponding commit exists but is not locally locked (and drop if unexisting)
• unstash when the corresponding commit gets locally locked?

[t-bast]

I think you meant to put that comment on the last case None => below, right? The case where you currently that comment corresponds to the case where we have resolved our commitment for this scid, which means that this commitment is confirmed and thus locally locked: it shouldn't happen that we fail to generate the announcement sigs in this case (hence a warning). We probably don't need to stash the sigs, but it's more consistent?

The last case None => is the case where we cannot find a local commitment for that scid, which can happen in two cases:

• we actually have this commitment but we haven't seen it confirm, so we can't find it by scid and must wait for confirmation (and thus need to stash the remote sigs)
• this commitment doesn't match anything that we have (it could be an older commitment that we already pruned), in which case we should completely ignore

But we cannot distinguish those two cases, so we have to assume it's the first one?

[pm47]

we cannot distinguish those two cases, so we have to assume it's the first one

Doh, that is the part I was missing. We don't know the local scid until it is confirmed.

[pm47]

I think this would be more consistent and reduce a bit the mental load of bringing the concept of announcement in this pruning logic:

Suggested change

	def updateRemoteFundingStatus(fundingTxId: TxId, lastAnnounced_opt: Option[AnnouncedCommitment])(implicit log: LoggingAdapter): Either[Commitments, (Commitments, Commitment)] =
	def updateRemoteFundingStatus(fundingTxId: TxId, lastAnnouncedFundingTxId_opt: Option[TxId])(implicit log: LoggingAdapter): Either[Commitments, (Commitments, Commitment)] =

[t-bast]

Done in fde052d

[pm47]

What are the side effects of putting fake real scid here?

[t-bast]

Unless I've missed something, it shouldn't have any impact, thanks to the comment in ChannelTypes4. We only have the following cases:

• private channels with phoenix wallets: we may have done splices and thus have a fundingTxIndex > 0
• channels with non-phoenix wallets: we don't have any splice and are at fundingTxIndex = 0

In the first case, this is a private channel, so we will never use the real short_channel_id. It's weird that it's set to 0, but it's unused so it's fine. Also, the next splice we do will correctly set its short_channel_id, so this will eventually disappear.

In the second case, we must set this to the real short_channel_id if this is a public channel. That is done by the migration in ChannelTypes4, where we pull it from the announcement and set it in the commitment with fundingTxIndex = 0.

The only issue we may have is if we're in the second case, but never received our peer's announcement_signatures: in that case we don't have the announcement whereas we should, and we'll lose the short_channel_id. But on reconnection we're re-sending our announcement_signatures and our peer should send theirs, so this should never happen unless our peer is not spec-compliant! But I'll double-check our DB to see if this is happening or not.

[t-bast]

We only have 8 such channels on our node, with the following channel IDs:

• "07d482c2f061e9511a60f47b1b75400e2bcb862681849bc5bcef9fa874e7db6d"
• "bcb43b0a7a030adb1186d008fca7e5e2aeca73ecc04424e7ce89d87d3440d242"
• "4ab844b8240e05df6565034b004d1fde0a224caf2a8da5fb5ae7688e1ea5934a"
• "fa01334c2a91de0ebba7ce11aec031e5c0a006fa9e6601faa71a2fa178a92a92"
• "d7c11e2b4e9af86724e79ff3686eae57a5b71fe10ef8a11404d9e421890824c5"
• "d2a44ec7c83d4d6feab7a85584943c11f1a160fee3ff2a9def05edcf95bca3a9"
• "5e5278b79ac46981a377bf58260a7fb1b6d88ab750c41a7acb050d9de00f2618"
• "4cc8c3e8832a734d5c9dcf58cdc604fd7c04b73ab4290f0dfc7679c33767c77f"

The only two that are active are "07d482c2f061e9511a60f47b1b75400e2bcb862681849bc5bcef9fa874e7db6d" and "fa01334c2a91de0ebba7ce11aec031e5c0a006fa9e6601faa71a2fa178a92a92", the others have been offline for years.

Since we don't have the announcement, we've never been able to broadcast our channel update, which means those channels have never generated routing fees to us. After this update, this won't change, we still won't be able to broadcast a channel_update. My recommendation is to do a mutual close on the two active channels and a force-close on the 6 inactive ones.

[pm47]

I think we should only provide a decoder (which will make an encoding attempt explicitly fail), and remove the .from() companion methods:

Suggested change

	("shortIds" | shortids)).as[ChannelTypes4.DATA_WAIT_FOR_CHANNEL_READY_0b].xmap(d => d.migrate(), d => ChannelTypes4.DATA_WAIT_FOR_CHANNEL_READY_0b.from(d))
	("shortIds" | shortids)).as[ChannelTypes4.DATA_WAIT_FOR_CHANNEL_READY_0b].map(d => d.migrate()).decodeOnly

[t-bast]

Done in fde052d, I thought the decodeOnly would create build trouble but it's actually exactly what we need!

[pm47]

we cannot distinguish those two cases, so we have to assume it's the first one

Doh, that is the part I was missing. We don't know the local scid until it is confirmed.