smite: extend accept_channel oracle with key reuse checks by NishantBansal2003 · Pull Request #259 · lnfuzz/smite

NishantBansal2003 · 2026-09-17T06:54:44Z

ref: #209 (comment)

Some more oracles added in this PR:

`open_channel`

chain_hash != regtest
channel_reserve_satoshis >= funding_satoshis
dust_limit_satoshis > 10_000 sat
to_self_delay > 2016
feerate_per_kw == 0 for non 0FC channels

`accept_channel`

dust_limit_satoshis > 10_000 sat
htlc_minimum_msat > max_htlc_value_in_flight_msat
htlc_minimum_msat > open_channel.funding_satoshis
to_self_delay == 0
max_accepted_htlcs == 0
pubkey match any pubkeys from open_channel and accept_channel
per_commitment_point should not be reused

Notes

CLN defaults to_self_delay to 6 in accept_channel on regtest and 144 on mainnet, so I think we should only check that to_self_delay != 0
CLN defaults the feerate_per_kw floor to 1 on regtest and 253 on mainnet when accepting the open_channel message, so I think we should only check that feerate_per_kw != 0

Record each pubkey sent in open_channel, accept_channel and channel_ready with its origin (side, channel, field). We may send the same pubkey more than once, so ours can have several origins. The target must never send a pubkey already sent by either side, so each of its pubkeys has exactly one origin. This prepares for an oracle that detects pubkey reuse by the target. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

Report accept_channel messages that use a pubkey already revealed by either side, or share one across their own fields. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

morehouse · 2026-09-30T19:26:34Z

+    // Check that each pubkey is unrevealed.
+    for (field, pubkey) in &pubkeys {
+        if let Some(origins) = revealed_pubkeys.get(pubkey) {
+            let origins: Vec<String> = origins.iter().map(ToString::to_string).collect();
+            return Err(format!(
+                "{field} {pubkey} was already sent as {}",
+                origins.join(", "),
+            ));
+        }
+    }
+
+    // Check that each pubkey appears in only one field.
+    let mut fields = HashMap::new();
+    for (field, pubkey) in &pubkeys {
+        if let Some(other_field) = fields.insert(pubkey, field) {
+            return Err(format!("{other_field} and {field} share pubkey {pubkey}"));
+        }
+    }


Nit: these loops can be combined.

Suggested change

// Check that each pubkey is unrevealed.

for (field, pubkey) in &pubkeys {

if let Some(origins) = revealed_pubkeys.get(pubkey) {

let origins: Vec<String> = origins.iter().map(ToString::to_string).collect();

return Err(format!(

"{field} {pubkey} was already sent as {}",

origins.join(", "),

));

}

}

// Check that each pubkey appears in only one field.

let mut fields = HashMap::new();

for (field, pubkey) in &pubkeys {

if let Some(other_field) = fields.insert(pubkey, field) {

return Err(format!("{other_field} and {field} share pubkey {pubkey}"));

}

}

// Check that each pubkey is unrevealed and appears in only one field.

let mut seen = HashMap::new();

for (field, pubkey) in accept_channel.pubkeys() {

if let Some(origins) = revealed_pubkeys.get(pubkey) {

let origins: Vec<String> = origins.iter().map(ToString::to_string).collect();

return Err(format!(

"{field} {pubkey} was already sent as {}",

origins.join(", "),

));

}

if let Some(other_field) = seen.insert(pubkey, field) {

return Err(format!("{other_field} and {field} share pubkey {pubkey}"));

}

}

Yes, but the reason to keep them separate is that if a pubkey is reused from an earlier negotiation, where its secret has already been revealed (eg in commitment dance), that is a much more serious bug than a key being reused within the same negotiation

morehouse · 2026-09-30T19:51:07Z

+    /// Every pubkey sent on the wire by either side, mapped to each of its
+    /// origins. We may send the same pubkey multiple times, so ours can have
+    /// multiple origins. The target must never send a pubkey already sent by
+    /// either side, so each of theirs has exactly one origin.
+    revealed_pubkeys: HashMap<PublicKey, Vec<KeyOrigin>>,


Do we really need to track multiple origins? I'd be happy with just the first one getting printed in the error message since it simplifies the code.

My thinking is that tracking only the first origin is not really useful, the reasons are:

This key-reuse check is likely to be non-deterministic (or maybe it would be deterministic in Bedrock? Not sure)

If the target reuses a pubkey from its own previously sent pubkeys, we can be fairly sure that there is an issue somewhere in its key generation in that path. But if the target generates a key that we previously sent to it, I don't think we would be able to pinpoint the issue if we only track the first origin

If it's nondeterministic, that probably means the target has weak entropy, in which case I doubt having multiple origins helps.

If the target reuses one of our keys, can't we easily check the program to any additional places we send them our key, if needed?

In every case I can think of, one origin is enough to diagnose the bug.

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

morehouse · 2026-10-01T13:13:38Z

+    /// Every pubkey sent on the wire by either side, mapped to each of its
+    /// origins. We may send the same pubkey multiple times, so ours can have
+    /// multiple origins. The target must never send a pubkey already sent by
+    /// either side, so each of theirs has exactly one origin.
+    revealed_pubkeys: HashMap<PublicKey, Vec<KeyOrigin>>,


If it's nondeterministic, that probably means the target has weak entropy, in which case I doubt having multiple origins helps.

If the target reuses one of our keys, can't we easily check the program to any additional places we send them our key, if needed?

In every case I can think of, one origin is enough to diagnose the bug.

morehouse · 2026-10-01T13:38:38Z

+    // The target must never send a pubkey already sent by either side, so the
+    // `second_per_commitment_point` is recorded with this as its only origin.
+    //
+    // TODO: Once we have the channel_ready oracle, update this to return a
+    // violation from the target instead of panicking.
+    assert!(
+        !revealed_pubkeys.contains_key(&cr.second_per_commitment_point),
+        "target sent an already sent pubkey",
+    );


The spec actually allows second_per_commitment_point repeats: https://github.com/lightning/bolts/blob/1aadb719b4007c4cea0ba6e36b08c4fb53788dee/02-peer-protocol.md?plain=1#L1091

I think we'll be able to handle that in the channel_ready oracle, but in the meantime we shouldn't panic here. For this PR we should also swap the should_panic executor test with one that ensures channel_ready can be resent with a new alias, and we can add current test back in with the channel_ready oracle.

Also once we implement reconnect I think there will be other spec-compliant ways to get repeated points.

I think for reuse, we could check the whole origin along with the key. If both the key and origin match the new one, it means the pubkey was simply sent again for the same message and field. Otherwise, it is a violation. Once we have the commitment scenario, we could also add the commitment number to the origin and track and compare the origin with respect to the commitment number.

morehouse · 2026-10-01T13:46:15Z

+    let (mut fx, _) = recv_channel_ready_fixture();
+    let mut sk_bytes = [0u8; 32];
+    sk_bytes[31] = 1;
+    let oc = SampleOpenChannel::new(PointSource::Secret(sk_bytes));


Suggested change

let (mut fx, _) = recv_channel_ready_fixture();

let mut sk_bytes = [0u8; 32];

sk_bytes[31] = 1;

let oc = SampleOpenChannel::new(PointSource::Secret(sk_bytes));

let (mut fx, target_pcp) = recv_channel_ready_fixture();

let mut sk_bytes = [0u8; 32];

sk_bytes[31] = 1;

let oc = SampleOpenChannel::new(PointSource::Secret(sk_bytes));

assert_eq!(target_pcp, oc.funding_pubkey);

morehouse · 2026-10-01T13:48:28Z

+
+    fx.run(&b.build());
+}
+


The fixup removed a bunch of test coverage that we should still be able to recover using public-API tests.

target's channel_ready reuses accept_channel PCP

target's accept_channel reuses accept_channel pubkey

target's channel_ready reuses our channel_ready PCP

target's accept_channel reuses channel_ready PCP

Added for accept_channel pubkey reuse checks, since we removed the assert for channel_ready pubkey reuse. I will add those checks to the channel_ready oracle

morehouse · 2026-10-01T13:54:24Z

 //! Remembers the `open_channel`/`accept_channel` parameters of each channel
 //! being established, so later steps can build commitments from them.


Doc comments needs update.

morehouse · 2026-10-01T13:55:27Z

        to_self_delay: 144,
        max_accepted_htlcs: 483,
-        funding_pubkey: sample_pubkey(1),
+        funding_pubkey: sample_pubkey(7),


Let's add a comment explaining the reason the keys go 7-2-3-4-... instead of 1-2-3-4-...

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

NishantBansal2003 mentioned this pull request Sep 17, 2026

smite: add channel_ready oracle #260

Open

NishantBansal2003 marked this pull request as draft September 17, 2026 12:18

NishantBansal2003 force-pushed the more-accept-chan-oracles branch from cc96bd6 to 2a6f201 Compare September 19, 2026 09:12

morehouse reviewed Sep 21, 2026

View reviewed changes

NishantBansal2003 mentioned this pull request Sep 22, 2026

smite: extend accept_channel oracle with sanity checks #268

Merged

NishantBansal2003 added 2 commits September 27, 2026 22:20

smite: add pubkey reuse check to accept_channel oracle

57b44fa

Report accept_channel messages that use a pubkey already revealed by either side, or share one across their own fields. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>

NishantBansal2003 force-pushed the more-accept-chan-oracles branch from 2a6f201 to 57b44fa Compare September 30, 2026 06:37

NishantBansal2003 changed the title ~~smite: extend accept_channel oracle with sanity and key reuse checks~~ smite: extend accept_channel oracle with key reuse checks Sep 30, 2026

NishantBansal2003 marked this pull request as ready for review September 30, 2026 06:48

NishantBansal2003 requested a review from morehouse September 30, 2026 06:48

morehouse requested changes Sep 30, 2026

View reviewed changes

fixup!

6ae5679

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

NishantBansal2003 force-pushed the more-accept-chan-oracles branch from 4b5f3b9 to 6ae5679 Compare October 1, 2026 12:29

morehouse requested changes Oct 1, 2026

View reviewed changes

fixup!

7c48851

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

		//! Remembers the `open_channel`/`accept_channel` parameters of each channel
		//! being established, so later steps can build commitments from them.

Conversation

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

Uh oh!

open_channel

accept_channel

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NishantBansal2003 commented Sep 17, 2026 •

edited

Loading

`open_channel`

`accept_channel`