smite: validate negotiated features and channel types in `accept_channel` oracle by NishantBansal2003 · Pull Request #209 · lnfuzz/smite

NishantBansal2003 · 2026-08-19T13:13:02Z

Depends-on: #192
Depends-on: #186 (added some changes to it, which will be removed once that PR is finalized)
Opening this as a draft since it depends on two PRs. Otherwise, the changes are ready for review.

I tried to add all the definitive checks for open_channel and accept_channel mentioned in the BOLT 2, including ones that aren't currently possible, such as option_dual_fund, since we haven't negotiated it. I think having parity with the BOLTs is good, especially for the future when we extend smite to support more protocols

Some more missing oracles that I will add in follow-up PR are:

`open_channel`

chain_hash != regtest
channel_reserve_satoshis >= funding_satoshis
dust_limit_satoshis > 10_000 sat
to_self_delay > 2016
feerate_per_kw < 253 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 < 144
max_accepted_htlcs == 0
pubkey match any pubkeys from open_channel and accept_channel
per_commitment_point should not be reused

Known violations found by this that need to be suppressed are:

ekzyis · 2026-08-30T15:47:14Z

Depends-on: #186 (added some changes to it, which will be removed once that PR is finalized)

Oh, I like how you added is_acceptable_shutdown_script. Would adding this to #186 resolve #186 (comment) for you?

NishantBansal2003 · 2026-08-31T06:17:49Z

Oh, I like how you added is_acceptable_shutdown_script. Would adding this to #186 resolve #186 (comment) for you?

Yeah, I just added that function to keep things simple, but this could be done via an enum Side as well. I thought this was simple enough to get the work done

ekzyis · 2026-09-07T13:51:27Z

+    /// Returns whether every bit set here is supported by `other`, where the
+    /// feature's required (even) or optional (odd) bit both count as support.
+    #[must_use]
+    pub fn is_supported(&self, other: &Features) -> bool {


nit: Maybe is_compatible would be a better name?

Renamed to is_supported_by, I think that would be clearer when checking whether something is a subset of a feature

ekzyis · 2026-09-07T14:05:57Z

 }

+/// Returns the maximum funding amount allowed by the negotiated features.
+pub fn max_funding_satoshis(negotiated_features: &Features) -> u64 {


Suggested change

pub fn max_funding_satoshis(negotiated_features: &Features) -> u64 {

fn max_funding_satoshis(negotiated_features: &Features) -> u64 {

ekzyis · 2026-09-07T14:06:11Z

+}
+
+/// Returns the maximum number of inbound HTLCs allowed by the channel type.
+pub fn max_accepted_htlcs_limit(channel_type: &Features) -> u16 {


Suggested change

pub fn max_accepted_htlcs_limit(channel_type: &Features) -> u16 {

fn max_accepted_htlcs_limit(channel_type: &Features) -> u16 {

ekzyis · 2026-09-07T14:36:35Z

+            // Since we echo the same features the target sent, but strip both
+            // required and optional bits to exercise only the single funded
+            // flow and avoid unrelated noise, negotiated features are just the
+            // features we sent in our init.
+            negotiated_features: Features::from(our_init.features),


Shouldn't this be a bitwise-or of our_init.globalfeatures and our_init.features?

MUST combine (bitwise OR) the two feature bitmaps into one logical features map.

https://github.com/lightning/bolts/blob/152897261850d93c4f4597f39cf22d7d22d6ede6/01-messaging.md?plain=1#L318

Spec-wise, yes, but I think globalfeatures is a legacy feature. From what I can see, the spec doesn't require the two vectors to be disjoint, and in practice, every target duplicates rather than partitions them: LDK sends a masked copy of bits 0-13 from its own features, CLN and LND send only var_onion/static_remotekey, which they also set in features, and Eclair sends it empty. globalfeatures is a subset of features for all four, so there shouldn't be any issues if we just use features instead of OR-ing them

see:

https://github.com/ACINQ/eclair/blob/232f721b39398b4a20ca896b6e24b64c53df069e/eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/LightningMessageCodecs.scala#L33

https://github.com/lightningnetwork/lnd/blob/1049d3010a4eb07bbf000f2af15f2bb3862365fb/lnwire/init_message.go#L13-L16

Signed-off-by: Nishant Bansal <[email protected]>

We strip certain feature bits during setup to exercise only the single funded flow, so the features stored here are what both sides have agreed to continue with, the negotiated feature set, not just the target's advertised features. This prepares for oracle validation that will use negotiated features to validate field constraints. If the target didn't disconnect after our init, that confirms it also conforms to our negotiated features, not its original advertised feature set. Signed-off-by: Nishant Bansal <[email protected]>

This is useful when comparing features in message fields against negotiated features. For eg., comparing channel_type in open_channel and accept_channel to ensure they match the features negotiated during setup. Signed-off-by: Nishant Bansal <[email protected]>

Signed-off-by: Nishant Bansal <[email protected]>

ekzyis · 2026-09-12T18:59:01Z

+    /// Features negotiated between the target node and Smite.
+    pub negotiated_features: Features,


What confused me a little here is that since these are already negotiated features, I'm not sure what would be the difference between an odd vs even bit for a feature.

is_supported_by checks both bits, so for the rest of the code, it doesn't matter which bit of a feature you set afaict. If I could tell correctly, I think this would be nice to clarify:

Suggested change

/// Features negotiated between the target node and Smite.

pub negotiated_features: Features,

/// Features negotiated between the target node and Smite. A feature may

/// appear as an even or odd bit; the distinction carries no meaning here.

pub negotiated_features: Features,

ekzyis · 2026-09-12T19:36:15Z

+    // be supported.
+    if !negotiated_features.supports_feature(Features::OPTION_CHANNEL_TYPE) {
+        return Err("option_channel_type is not supported".to_string());
+    }


I thought ASSUMED and "can safely be ignored" in BOLT-9 means that the peer doesn't need to set any bit for this feature, but we simply assume it's enabled. So I'm not sure if this check here makes sense.

Agree. I don't think it's a spec violation to omit the feature bit -- the idea behind making it ASSUMED was to allow reusing the feature bit eventually with a different meaning.

ekzyis · 2026-09-12T19:59:16Z

+        negotiated_features: Features::from_bits(&[
+            Features::OPTION_STATIC_REMOTEKEY,
+            Features::OPTION_ANCHORS,
+            Features::OPTION_CHANNEL_TYPE,
+        ]),


Related to my other comment about removing the check for OPTION_CHANNEL_TYPE: I interpret "can be safely ignored" as "don't check if the feature is enabled; assume it is", so a peer who didn't set this bit in init isn't wrong.

I think it could make sense to distinguish which bits we set here because we assume them to be negotiated (OPTION_STATIC_REMOTEKEY and OPTION_CHANNEL_TYPE) vs. for other reasons (OPTION_ANCHORS). So instead of adding assumed feature bits manually here, we could have a list of assumed features, and then include them here for our sample ProgramContext in the tests and in SnapshotSetup during fuzzing.

However, this is related to the question of whether we even need to include assumed bits in our context in the first place if we can assume them to be set, so all checks based on these features are unconditional.

We could also postpone this discussion for an InitOracle, because it would be more concerned with the meaning of bits, like that only one bit should be set for a feature.

WDYT?

Will remove the check for OPTION_CHANNEL_TYPE being present in the negotiated features, so I think we can safely remove OPTION_CHANNEL_TYPE from the negotiated features as well

ekzyis · 2026-09-12T20:17:20Z

    }

    // Check the channel reserve covers the dust limit.
    if accept_channel.dust_limit_satoshis > accept_channel.channel_reserve_satoshis {


Unrelated to this PR: Shouldn't this be this?

-if accept_channel.dust_limit_satoshis > accept_channel.channel_reserve_satoshis +if accept_channel.dust_limit_satoshis > open_channel.channel_reserve_satoshis

See #185 (comment).

ekzyis

Left some more nits, but LGTM! Will probably ACK tomorrow

ekzyis · 2026-09-13T01:00:23Z

+    }
+
+    // Check that option_scid_alias is only negotiated for private channels.
+    let announce_channel = (open_channel.channel_flags & 1) == 1;


nit: could define a constant ANNOUNCE_CHANNEL_FLAG so it's clear why we use 1 here without looking up the spec. It's kind of easy to miss in the spec:

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

ekzyis · 2026-09-13T01:05:42Z

+) -> Result<(), String> {
+    // Check that option_dual_fund has not been negotiated.
+    if negotiated_features.supports_feature(Features::OPTION_DUAL_FUND) {
+        return Err("option_dual_fund has been negotiated".to_string());


nit: We use supports_feature here and at line 138, but the error message uses "negotiated" vs "supported". I think we should use one verb consistently in the error messages. I have a slight preference towards "negotiated".

ekzyis · 2026-09-13T01:09:17Z

+    // Check option_channel_type in negotiated features since it is assumed to
+    // be supported.
+    if !negotiated_features.supports_feature(Features::OPTION_CHANNEL_TYPE) {
+        return Err("option_channel_type is not supported".to_string());


nit: see other comment

Suggested change

return Err("option_channel_type is not supported".to_string());

return Err("option_channel_type has not been negotiated".to_string());

but would also need replacement in tests

Removed this check. See: #209 (comment)

ekzyis · 2026-09-13T01:18:32Z

+        // The pairing is symmetric: an optional bit on the left is satisfied
+        // by the required bit on the right.


nit:

Suggested change

// The pairing is symmetric: an optional bit on the left is satisfied

// by the required bit on the right.

// The pairing is symmetric: an optional bit on the left is satisfied

// by the required bit on the right and vice versa.

ekzyis · 2026-09-13T01:23:10Z

+                return Err("upfront_shutdown_script is not valid".to_string());
+            }
+        } else {
+            return Err("open_channel does not include upfront_shutdown_script".to_string());


nit: I like how we use only upfront_shutdown_script here, but we use a channel_type in other places. Probably not worth fixing, but I thought it was worth mentioning.

ekzyis · 2026-09-13T01:31:07Z

+        if let Some(script) = &open_channel.tlvs.upfront_shutdown_script {
+            if !script.is_empty() && !is_acceptable_shutdown_script(script, negotiated_features) {
+                return Err("upfront_shutdown_script is not valid".to_string());
+            }
+        } else {
+            return Err("open_channel does not include upfront_shutdown_script".to_string());
+        }


refactor suggestion to avoid triple nesting:

diff --git a/smite/src/oracles/accept_channel.rs b/smite/src/oracles/accept_channel.rs index c07c378..a80d53b 100644 --- a/smite/src/oracles/accept_channel.rs +++ b/smite/src/oracles/accept_channel.rs @@ -123,12 +123,11 @@ fn verify_accepted_open_channel( // Check that the upfront shutdown script is present and valid when negotiated. if negotiated_features.supports_feature(Features::OPTION_UPFRONT_SHUTDOWN_SCRIPT) { - if let Some(script) = &open_channel.tlvs.upfront_shutdown_script { - if !script.is_empty() && !is_acceptable_shutdown_script(script, negotiated_features) { - return Err("upfront_shutdown_script is not valid".to_string()); - } - } else { + let Some(script) = &open_channel.tlvs.upfront_shutdown_script else { return Err("open_channel does not include upfront_shutdown_script".to_string()); + }; + if !script.is_empty() && !is_acceptable_shutdown_script(script, negotiated_features) { + return Err("upfront_shutdown_script is not valid".to_string()); } } @@ -218,12 +217,11 @@ fn verify_accept_channel( ) -> Result<(), String> { // Check that the upfront shutdown script is present and valid when negotiated. if negotiated_features.supports_feature(Features::OPTION_UPFRONT_SHUTDOWN_SCRIPT) { - if let Some(script) = &accept_channel.tlvs.upfront_shutdown_script { - if !script.is_empty() && !is_standard_shutdown_script(script, negotiated_features) { - return Err("upfront_shutdown_script is not valid".to_string()); - } - } else { + let Some(script) = &accept_channel.tlvs.upfront_shutdown_script else { return Err("accept_channel does not include upfront_shutdown_script".to_string()); + }; + if !script.is_empty() && !is_standard_shutdown_script(script, negotiated_features) { + return Err("upfront_shutdown_script is not valid".to_string()); } }

ekzyis · 2026-09-13T01:43:48Z

 /// - dust limit greater than channel reserve: BOLT 2 requires the dust limit to
 ///   be less than or equal to the channel reserve. However, implementations
 ///   such as LDK accept zero channel reserves on the receiving side, so we do
 ///   not enforce this check on the target's receiving side.


The following checks appear to be unintentionally missing:

unexpected chain_hash

unreasonably large to_self_delay

feerate_per_kw that is too small or unreasonably large

I think we could add 1. by adding expected_chain_hash to AcceptChannelContext. For 2. and 3., we would need to decide which values are too small or unreasonably large. I think we've done this before for other values, i.e. bitcoin amounts > 21m don't make sense.

Planned as a follow-up, see the PR description

Default Max for all is 2016, so maybe we should also check to_self_delay <= 2016 in open_channel only

The cap on small is already planned, see the PR description. For the large feerate, I don't think it affects any target, since it would only be set by the opener to pay

ekzyis · 2026-09-13T01:55:55Z

+const MAX_FUNDING_SATOSHIS_NO_WUMBO: u64 = (1 << 24) - 1;
+const MAX_ACCEPTED_HTLCS_ZERO_FEE_COMMITMENTS: u16 = 114;
+const MAX_ACCEPTED_HTLCS_DEFAULT: u16 = 483;


nit: could rename MAX_FUNDING_SATOSHIS_NO_WUMBO to MAX_FUNDING_SATOSHIS_DEFAULT to be consistent with MAX_ACCEPTED_HTLCS_DEFAULT

Would prefer MAX_FUNDING_SATOSHIS_NO_WUMBO only, since MAX_FUNDING_SATOSHIS_DEFAULT could be confused with the Max Bitcoin supply

morehouse · 2026-09-14T17:43:37Z

+    if announce_channel && channel_type.supports_feature(Features::OPTION_SCID_ALIAS) {
+        return Err("option_scid_alias requires the channel to be private".to_string());
+    }


I see the spec sender requirement for this but not the receiver one. Should we update the spec with it?

This could be clarified in the spec, and it would be great if the receiver rejected these. Since HTLCs are rejected immediately rather than held until expiry, the reputation loss would not be significant, though it might eventually affect the target. I think this would be beneficial for the network and nodes, but should not be required

I will update the spec and see what others think about it.

morehouse · 2026-09-14T17:51:14Z

    }

    // Check the channel reserve covers the dust limit.
    if accept_channel.dust_limit_satoshis > accept_channel.channel_reserve_satoshis {


See #185 (comment).

morehouse · 2026-09-14T18:02:51Z

+    #[test]
+    fn commitment_validation_skipped_for_zero_fee_commitments() {
+        let mut oc = open_channel();
+        oc.tlvs.channel_type = Some(ChannelTypeVariant::ZeroFeeCommitments.encode());
+        oc.feerate_per_kw = 0;
+        oc.push_msat = oc.funding_satoshis * 1000;
+        oc.max_accepted_htlcs = MAX_ACCEPTED_HTLCS_ZERO_FEE_COMMITMENTS;
+        let mut ac = accept_channel();
+        ac.tlvs.channel_type = Some(ChannelTypeVariant::ZeroFeeCommitments.encode());
+        ac.max_accepted_htlcs = MAX_ACCEPTED_HTLCS_ZERO_FEE_COMMITMENTS;
+
+        assert_pass(&ac, Some(&pending_negotiation(oc)));
+    }
+
+    #[test]
+    fn commitment_validation_skipped_for_option_simple_taproot() {
+        let mut oc = open_channel();
+        oc.tlvs.channel_type = Some(ChannelTypeVariant::SimpleTaproot.encode());
+        oc.push_msat = oc.funding_satoshis * 1000;
+        let mut ac = accept_channel();
+        ac.tlvs.channel_type = Some(ChannelTypeVariant::SimpleTaproot.encode());
+
+        assert_pass(&ac, Some(&pending_negotiation(oc)));
+    }


IIUC, the first test is for an acceptable behavior (i.e. the opener should be able to push the full channel balance out for 0FC since they don't have to pay for the anchor or commitment fee).

But I don't think that's true for taproot channels -- the opener still needs to afford the anchor output and commitment fees. So the second test may actually be something we should eventually reject...

The first test is currently passing by chance and will also pass once we add support for 0FC, so it is fine to keep it as is

The second test fails without a barrier and will also fail once we update the commitment fee for taproot channels, It should be updated once support for it is added

Will add comments to both of these tests to make it clear

morehouse · 2026-09-14T18:51:56Z

+    /// Returns whether every bit set here is supported by `other`, where the
+    /// feature's required (even) or optional (odd) bit both count as support.
+    #[must_use]
+    pub fn is_supported_by(&self, other: &Features) -> bool {


Nit: I think it's more consistent with the existing methods to call this supports_features and swap the usage of self and other

morehouse · 2026-09-14T20:18:06Z

-    fn open_channel_max_accepted_htlcs_above_the_limit() {
+    fn open_channel_channel_type_contains_non_negotiated_features() {
+        let mut oc = open_channel();
+        oc.tlvs.channel_type = Some(vec![0x10, 0x00]);


Can we use the existing constants here for clarity?

morehouse · 2026-09-14T20:19:40Z

+        oc.tlvs.channel_type = Some(vec![0x40, 0x40, 0x10, 0x00]);
+
+        let mut negotiated_features = Features::from(vec![0x40, 0x40, 0x10, 0x00]);


Can we use constants here for clarity?

morehouse · 2026-09-14T20:25:56Z

+    // be supported.
+    if !negotiated_features.supports_feature(Features::OPTION_CHANNEL_TYPE) {
+        return Err("option_channel_type is not supported".to_string());
+    }


Agree. I don't think it's a spec violation to omit the feature bit -- the idea behind making it ASSUMED was to allow reusing the feature bit eventually with a different meaning.

morehouse · 2026-09-14T20:41:28Z

+    // Check that the upfront shutdown script is present and valid when negotiated.
+    if negotiated_features.supports_feature(Features::OPTION_UPFRONT_SHUTDOWN_SCRIPT) {


I couldn't find anywhere in the spec that requires the receiver to validate the sender's shutdown script, and I'm not sure it's needed. IIUC the worst thing that happens in this case is that the sender uses an invalid/unspendable script, which harms themselves, not the receiver.

I think if the sender script is invalid, a cooperative close is impossible for the channel's lifetime, and if the target accepted it without validating it, the only option to close the channel is a force-close, leaving its to_local output delayed

NishantBansal2003 mentioned this pull request Aug 27, 2026

smite: compare channel types in minimal form #218

Closed

morehouse mentioned this pull request Sep 1, 2026

smite: add is_standard_shutdown_script helper #186

Merged

NishantBansal2003 force-pushed the accept-chan-feature-oracle branch from 33dd398 to 05bce1c Compare September 3, 2026 17:05

NishantBansal2003 marked this pull request as ready for review September 3, 2026 17:12

This was referenced Sep 4, 2026

smite: move ChannelTypeVariant to bolt::types #234

Merged

smite: check if upfront shutdown script is standard if negotiated #233

Open

ekzyis reviewed Sep 7, 2026

View reviewed changes

NishantBansal2003 added 4 commits September 10, 2026 01:12

smite: validate channel type variants in accept_channel oracle

852ac04

Signed-off-by: Nishant Bansal <[email protected]>

smite: add negotiated feature validation to accept_channel oracle

adece46

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 force-pushed the accept-chan-feature-oracle branch from 05bce1c to adece46 Compare September 9, 2026 19:48

ekzyis reviewed Sep 12, 2026

View reviewed changes

ekzyis reviewed Sep 13, 2026

View reviewed changes

morehouse requested changes Sep 14, 2026

View reviewed changes

	pub fn max_funding_satoshis(negotiated_features: &Features) -> u64 {
	fn max_funding_satoshis(negotiated_features: &Features) -> u64 {

	pub fn max_accepted_htlcs_limit(channel_type: &Features) -> u16 {
	fn max_accepted_htlcs_limit(channel_type: &Features) -> u16 {

		/// Features negotiated between the target node and Smite.
		pub negotiated_features: Features,

	return Err("option_channel_type is not supported".to_string());
	return Err("option_channel_type has not been negotiated".to_string());

		// The pairing is symmetric: an optional bit on the left is satisfied
		// by the required bit on the right.

		oc.tlvs.channel_type = Some(vec![0x40, 0x40, 0x10, 0x00]);

		let mut negotiated_features = Features::from(vec![0x40, 0x40, 0x10, 0x00]);

		// Check that the upfront shutdown script is present and valid when negotiated.
		if negotiated_features.supports_feature(Features::OPTION_UPFRONT_SHUTDOWN_SCRIPT) {

Uh oh!

Conversation

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

Uh oh!

open_channel

accept_channel

Uh oh!

ekzyis commented Aug 30, 2026

Uh oh!

NishantBansal2003 commented Aug 31, 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!

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!

Choose a reason for hiding this comment

Uh oh!

ekzyis left a comment

Choose a reason for hiding this comment

Uh oh!

ekzyis Sep 13, 2026 • 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!

ekzyis Sep 13, 2026 • 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!

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!

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!

NishantBansal2003 commented Aug 19, 2026 •

edited

Loading

`open_channel`

`accept_channel`

ekzyis Sep 13, 2026 •

edited

Loading

ekzyis Sep 13, 2026 •

edited

Loading