smite: add channel_ready oracle by NishantBansal2003 · Pull Request #260 · lnfuzz/smite

NishantBansal2003 · 2026-09-17T07:43:01Z

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 · 2026-09-21T20:06:11Z

+        if context
+            .negotiated_features
+            .supports_feature(Features::OPTION_SCID_ALIAS)
+            && context.channel_ready.tlvs.short_channel_id.is_none()


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

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 · 2026-10-13T08:45:16Z

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 · 2026-10-13T20:20:53Z

+        assert_pass(
+            &channel_ready(),
+            Some(&channel_state()),
+            &Features::new(),
+            &HashMap::new(),
+            CONFIRMED_DEPTH,
+            &HashMap::new(),
+        );


These parameter lists add a lot of bloat to the tests. It might be worth a test fixture to simplify things:

struct Inputs { channel_ready: &ChannelReady, channel: Option<&ChannelState>, funding_confirmations: u32, negotiated_features: &Features, revealed_pubkeys: &HashMap<PublicKey, KeyOrigin>, revealed_scids: &HashMap<ShortChannelId, ChannelId>, } // ... implement ::default() values // ... implement needed helper methods fn assert_pass(inputs: &Inputs); fn assert_fail(inputs: &Inputs, expected: &str);

Then a test only needs to specify the non-default fields:

let inputs = Inputs { funding_confirmations: MINIMUM_DEPTH - 1, ..Inputs::default() }; assert_fail(&inputs, ...);

To avoid churn on this PR, probably better as a follow-up PR.

morehouse · 2026-10-13T20:23:14Z

+/// neither an `M` nor implicitly handled, or any error from [`recv_non_ping`].
+fn recv_bolt<M: FromMessage>(
    conn: &mut impl Connection,
    channel_states: &mut HashMap<ChannelId, ChannelState>,
    revealed_pubkeys: &mut HashMap<PublicKey, KeyOrigin>,
-) -> Result<(), ExecuteError> {
-    let cr: ChannelReady = recv_bolt(conn, RECV_CHANNEL_READY_TIMEOUT)?;
-
-    let state = channel_states
-        .get_mut(&cr.channel_id)
-        .ok_or(Violation::UnknownChannel(cr.channel_id))?;
-    *state.next_counterparty_per_commitment_point_mut() = Some(cr.second_per_commitment_point);
-
-    // Record the `second_per_commitment_point` as a per-commitment point the
-    // target revealed, for commitment number 1. It is unique, so record its
-    // only origin.
-    //
-    // TODO: Once we have the channel_ready oracle, flag a reused
-    // `second_per_commitment_point`, i.e. one already present in
-    // `revealed_pubkeys`, unless the entry is a `TargetPcp` for this same
-    // channel and commitment number. That exception allows the two legitimate
-    // repeats: the target may resend `channel_ready` with a different alias but
-    // the same point, and we may receive the same point again after a
-    // reconnection.
-    revealed_pubkeys.insert(
-        cr.second_per_commitment_point,
-        KeyOrigin::TargetPcp {
-            channel: cr.channel_id,
-            commitment_number: 1,
-        },
-    );
-
-    Ok(())
+    revealed_scids: &mut HashMap<ShortChannelId, ChannelId>,
+    context: &ProgramContext,
+    bitcoin_cli: &mut impl BitcoinRpc,
+    timeout: Duration,
+) -> Result<M, ExecuteError> {


The parameter lists for recv_bolt and recv_non_ping are getting very large. We should probably convert them to methods instead (as a follow-up PR).

morehouse · 2026-10-13T20:48:22Z

* 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 · 2026-10-14T05:54:39Z

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 · 2026-10-14T15:28:28Z

 }

+#[test]
+fn execute_recv_channel_ready_rejects_alias_reused_by_another_channel() {


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

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

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.

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 · 2026-10-15T14:19:55Z

 }

+#[test]
+fn execute_recv_channel_ready_rejects_alias_reused_by_another_channel() {


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.

morehouse · 2026-10-15T15:04:28Z

+            Message::ChannelReady(ref cr) => {
+                log::debug!("received channel_ready on {}", cr.channel_id);
+                let channel = channel_states.get(&cr.channel_id);
+                // TODO: Both lookups below call `getrawtransaction`. Returning
+                // confirmations alongside the block position would save one RPC.
+                let funding_outpoint = channel
+                    .filter(|c| c.is_funding_outpoint_valid)
+                    .map(|c| c.config.funding_outpoint);
+                let funding_confirmations = funding_outpoint
+                    .map_or(0, |o| bitcoin_cli.get_transaction_confirmations(o.txid));
+                let funding_scid =
+                    funding_outpoint.and_then(|o| lookup_short_channel_id(bitcoin_cli, o));
+                ChannelReadyOracle.evaluate(&ChannelReadyContext {
+                    channel_ready: cr,
+                    channel,
+                    negotiated_features: &context.negotiated_features,
+                    revealed_pubkeys,
+                    funding_confirmations,
+                    revealed_scids,
+                })?;
+                record_recv_channel_ready(
+                    channel_states,
+                    revealed_pubkeys,
+                    revealed_scids,
+                    funding_scid,
+                    cr,
+                );
+                return Ok(msg);
+            }


The implicit handling is starting to bloat recv_non_ping into something its name doesn't accurately describe.

I think it would be good to move the implicit handling into a handle_implicit helper called from recv_bolt in a follow up.

morehouse · 2026-10-15T15:06:07Z

+    // Only the first `channel_ready` sets these. `ChannelReadyOracle` checks
+    // resends against its point, and a resend's point is stale once the
+    // commitment state advances, so it must not overwrite the next point.
+    if state.received_channel_ready_point.is_none() {


I think we could also simplify is_channel_ready_expected slightly by checking the new received_channel_ready_point. Fine as a follow up.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

erickcestari approved these changes Sep 17, 2026

View reviewed changes

morehouse requested changes Sep 21, 2026

View reviewed changes

NishantBansal2003 force-pushed the channel-ready-oracle branch 2 times, most recently from 12ede2f to e99f4c9 Compare October 13, 2026 08:25

morehouse reviewed Oct 13, 2026

View reviewed changes

morehouse reviewed Oct 14, 2026

View reviewed changes

morehouse approved these changes Oct 15, 2026

View reviewed changes

NishantBansal2003 added 6 commits October 15, 2026 21:57

smite: add channel_ready oracle

772c88a

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

smite: add alias and point reuse checks to channel_ready oracle

c9645c1

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

smite: reject channel_ready resends with mismatched point

f8670e6

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

smite: reject channel_ready before funding minimum_depth

77e4374

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

smite: reject channel_ready with invalid funding outpoint

186aac7

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

smite: reject channel_ready short_channel_id alias reuse

42e3871

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

NishantBansal2003 force-pushed the channel-ready-oracle branch from ebc13c9 to 42e3871 Compare October 15, 2026 17:03

morehouse approved these changes Oct 15, 2026

View reviewed changes

morehouse merged commit 089ca0c into lnfuzz:master Oct 15, 2026
5 checks passed

This was referenced Oct 15, 2026

smite-scenarios: use helpers for second funded channel #286

Merged

smite-scenarios: give each mock transaction its own block position #287

Merged

smite-scenarios: simplify channel_ready key-reuse tests #291

Merged

Conversation

NishantBansal2003 commented Sep 17, 2026

Deferred oracle checks

Uh oh!

erickcestari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NishantBansal2003 commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Oct 13, 2026

Uh oh!

NishantBansal2003 commented Oct 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erickcestari left a comment •

edited

Loading

NishantBansal2003 commented Oct 13, 2026 •

edited

Loading