[NishantBansal2003]

Depends-on: #212 and #259

Add ChannelReadyOracle and consolidate all previously scattered checks relevant to the channel_ready negotiation into it, including unknown channel_id, cases where the target omits the short_channel_id alias required by option_scid_alias, and cases where the target reuses a per-commitment point from an earlier negotiation.

Deferred oracle checks

• short_channel_id alias collisions: BOLT 2 requires aliases not to collide with any of the target's real short_channel_ids. Checking this requires fetching the short_channel_id for all channels we have with the target over RPC. Bulk fetching adds RPC overhead that reduces fuzzing throughput, while lazy lookups can still miss collisions. Until this can be checked more efficiently, it is not worthwhile for this narrow surface.

[erickcestari]

The last 2 commits LGTM!

[morehouse]

Is it the alias feature from init that matters, or the one from channel_type? BOLT 2 seems to say both at different places:

• https://github.com/lightning/bolts/blob/152897261850d93c4f4597f39cf22d7d22d6ede6/02-peer-protocol.md?plain=1#L1077
• https://github.com/lightning/bolts/blob/152897261850d93c4f4597f39cf22d7d22d6ede6/02-peer-protocol.md?plain=1#L1089

[NishantBansal2003]

I think it is init rather than channel_type specifically here (see: lightning/bolts#910 (comment)). I think the reason for separating the two is that the one coupled with channel_type enables required privacy, while the one coupled with init says that if the alias is present (which it must be if option_scid_alias is negotiated in init), the peer can resend multiple channel_ready with a different alias as well (see: lightning/bolts#910 (comment), lightning/bolts#910 (comment)).

[NishantBansal2003]

Some additional checks that could be added:

• In the accept_channel oracle -- accept_channel.minimum_depth > 2 for non 0-conf channels
• In the channel_ready oracle -- real_scid != alias for private channels
• In the channel_ready oracle -- real_scid (of one channel) != alias for other channels

[morehouse]

* In the `accept_channel` oracle -- `accept_channel.minimum_depth > 2` for non 0-conf channels

Why not > 1 or > 0?

* In the `channel_ready` oracle -- `real_scid != alias` for private channels

* In the `channel_ready` oracle -- `real_scid (of one channel) != alias` for other channels

These seem good to me, though I'd rather add them in follow-ups so this PR doesn't continue to grow.

I also wonder if it's worth checking for premature/unexpected channel_readys when funding_signed hasn't been received or when our funding_created signature was invalid.

[NishantBansal2003]

Why not > 1 or > 0?

I was thinking that, on mainnet, 2-block reorgs have already been observed (1 are frequent, while 2 are rare), so it's better to have minimum_depth > 2

These seem good to me, though I'd rather add them in follow-ups so this PR doesn't continue to grow.

I also wonder if it's worth checking for premature/unexpected channel_readys when funding_signed hasn't been received or when our funding_created signature was invalid.

Yes, I was thinking of adding these and the other mentioned checks in a follow-up

[morehouse]

Today we could also add a test for one channel's real SCID being reused as another channel's alias.

[NishantBansal2003]

Yes, I am aware of it, but that is a gap we currently have, and it is not allowed behaviour. So I don't think we should add tests allowing it. I think we should add only a rejection test in a follow-up PR when we reject those cases

[morehouse]

Actually I think we could write the test today, since we record the real SCID in revealed_scids.

The test we couldn't write yet is for the real SCID being used as the alias for the same channel, though an oracle unit test is probably good enough for that one.

Either way works for me, whether we add the test in this PR or a follow up.

[NishantBansal2003]

I added the case where the second channel_ready uses an alias that matches the real_scid of the first channel. I think the inverse case, where the first channel uses an alias that matches the real_scid of the second channel, could be added in a follow-up once we add the corresponding check

[morehouse]

Ready for squash and rebase.

[morehouse]

Actually I think we could write the test today, since we record the real SCID in revealed_scids.

The test we couldn't write yet is for the real SCID being used as the alias for the same channel, though an oracle unit test is probably good enough for that one.

Either way works for me, whether we add the test in this PR or a follow up.