Repository navigation
smite: add channel_ready oracle - #260
NishantBansal2003 wants to merge 8 commits into
Conversation
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
| let key = pubkey(1); | ||
| let party = || ChannelPartyConfig { | ||
| funding_pubkey: key, | ||
| payment_basepoint: key, | ||
| revocation_basepoint: key, | ||
| delayed_payment_basepoint: key, |
There was a problem hiding this comment.
Should these be distinct keys?
| channel_ready: &ChannelReady, | ||
| channel: Option<&ChannelState>, | ||
| negotiated_features: &Features, | ||
| per_commitment_points: &HashSet<PublicKey>, |
There was a problem hiding this comment.
Maybe we should consider adding this same parameter for the accept_channel oracle tests. It might make some of the tests a bit cleaner.
| /// # 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. |
There was a problem hiding this comment.
Even without the lookups we could at least check that aliases aren't reused by keeping an in-memory set like we do for PCPs.
| if context | ||
| .negotiated_features | ||
| .supports_feature(Features::OPTION_SCID_ALIAS) | ||
| && context.channel_ready.tlvs.short_channel_id.is_none() |
There was a problem hiding this comment.
Is it the alias feature from init that matters, or the one from channel_type? BOLT 2 seems to say both at different places:
| ), | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we also check that minimum_depth was satisfied?
I know it's unlikely to even run this oracle if the depth isn't reached, since we turn RecvChannelReady into a no-op. But I think it's still possible if there's multiple negotiations in flight.
Also it will be easier to actually receive early channel_readys if we later implement the "implicit receive" logic from #111 (comment). Then we could be attempting to receive a different message and get an unexpected channel_ready instead, allowing us to run this oracle.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Might be worth some executor tests as well like we have for the accept_channel oracle checks.
| // earlier negotiation. | ||
| if context | ||
| .per_commitment_points | ||
| .contains(&context.channel_ready.second_per_commitment_point) |
There was a problem hiding this comment.
The spec does explicitly allow sending multiple channel_ready messages with the same PCPs:
I'm not sure if anyone implements that, but I think we should probably be more careful with this check. Probably we should allow reuse if the first use was for the same channel (i.e. channel.next_counterparty_per_commitment_point exists and matches), otherwise report a violation.
3 participants
Depends-on: #212 and #259
Add
ChannelReadyOracleand consolidate all previously scattered checks relevant to thechannel_readynegotiation into it, including unknownchannel_id, cases where the target omits theshort_channel_id aliasrequired byoption_scid_alias, and cases where the target reuses a per-commitment point from an earlier negotiation.Deferred oracle checks
short_channel_idalias collisions: BOLT 2 requires aliases not to collide with any of the target's realshort_channel_ids. Checking this requires fetching theshort_channel_idfor 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.