smite: use BOLT 9 feature bitfield primitives

[NishantBansal2003]

Depends-on: #185

Adds BOLT 9 feature primitives, which are useful in places where we need to query different features/bits in negotiated features, channel_type, etc, to proceed further. This will also be useful in the future when we add dual-fund support, and is already useful when working on the accept_channel oracle, where we need to query supported features/channel_type

This PR also aggregates all the existing feature-related utilities into a single Features struct. I've only added the features currently used by smite, we can add more as needed in the future.

Currently, I've only updated the places that actually need to query Features. I haven't changed wire messages that still use Vec<u8> for features, to avoid changing a lot of code without any immediate benefit. I think we can use features.rs as a utility whenever we need to query or manipulate Features, while continuing to use Vec<u8> for wire messages where we don't need to inspect the feature bits

[ekzyis]

Concept ACK

reviewed only 695d504 so far

[ekzyis]

If we allow usize, I can imagine the fuzzer calling Features::set_bit with a bit close to usize::MAX. This will then try to allocate up to 2^64 bytes on x86-64.

Since the max message size is 65_535 bytes, we know that feature bits can't be set higher than 65_535*8 - 1=524_279, so u32 should be enough (2^32 > 524_279 > 2^16):

Suggested change

	pub type FeatureBit = usize;
	pub type FeatureBit = u32;

However, since trying to allocate up to 2^32 bytes is still a lot, we could make FeatureBit a distinct type instead of only an alias for u32. We could then enforce the theoretical known limit as the maximum (524_279) instead of 2^32.

Suggested change

	pub type FeatureBit = usize;
	pub struct FeatureBit(u32);

Or is this not a reason for concern because we know the fuzzer won't call set_bit in the way I imagined?

[NishantBansal2003]

I don't expect the fuzzer to utilize the Features type at all. As mentioned above, Features is just a utility type and should be used where we need to extract or generate valid information from it. Otherwise, we should use the low-level Vec<u8> for the underlying fuzzer.

FYI, LDK also uses usize: https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/src/commit/8700bf1d1e9d9df1e7426aeb77d6c148b12656f7/lightning-types/src/features.rs#L1286

[ekzyis]

Otherwise, we should use the low-level Vec<u8> for the underlying fuzzer.

Ok makes sense thank you!

FYI, LDK also uses usize

Even if the fuzzer isn't going to call this, should we still add a check that the bit is within the theoretical limit? LDK also does this:

https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/src/commit/8700bf1d1e9d9df1e7426aeb77d6c148b12656f7/lightning-types/src/features.rs#L1289

[NishantBansal2003]

Makes sense!

[ekzyis]

Mhh, afaict, if I want to use the constants defined above but want to set the optional bit of a feature, I would need to call this function like this: f.set_bit(Features::GOSSIP_QUERIES ^ 1). I think that's awkward.

Maybe an interface with functions set_required_bit and set_optional_bit would be more intuitive? I also think reading set_bit(Features::GOSSIP_QUERIES) does not make it obvious which bit this is going to set. It requires knowledge of the convention of identifying features by their even bits in the list above.

[NishantBansal2003]

AFAICT, fuzzer can use the low-level Vec to generate whatever features it wants. If we do want to set a specific feature, I think we should set it as required only, rather than optional, otherwise, setting that feature might not be useful.

Can you tell me in which cases you think we might need to set an optional bit instead of a required one?

[ekzyis]

Can you tell me in which cases you think we might need to set an optional bit instead of a required one?

No. We don't need set_optional_bit then, but we could still rename set_bit to set_required_bit to make the function name more self-describing (and rename the rest consistently).

When I read "set bit of this feature", I don't know which bit is meant.

[NishantBansal2003]

we could still rename set_bit to set_required_bit

No, but set_bit can set an optional bit as well

When I read "set bit of this feature", I don't know which bit is meant.

set_bit takes the bit as input, so I expect the caller to understand which bit they want to set

Currently, I've only added required feature bits as constants, so set_bit can be used for required bits. But if, in the future, a caller needs to set an optional feature bit, I think we can define the optional feature bit as a constant as well, and suffix the constants with _REQUIRED and _OPTIONAL, respectively.

[ekzyis]

But if, in the future, a caller needs to set an optional feature bit, I think we can define the optional feature bit as a constant as well and suffix the constants with _REQUIRED and _OPTIONAL, respectively.

Makes sense. I think my only issue is that I think the _REQUIRED suffix would already be useful. But consider this a nit.

Edit: To clarify further:

the _REQUIRED suffix would already be useful

I think this is downstream of the inconsistency in how FeatureBit is treated as a function argument. Sometimes it's treated as a feature (supports_feature); sometimes as a bit (set_bit).

Anyway, I'm not going to not ACK this PR because of this.

[NishantBansal2003]

I think this is downstream of the inconsistency in how FeatureBit is treated as a function argument. Sometimes it's treated as a feature (supports_feature); sometimes as a bit (set_bit).

Hmm, that's true, and we can definitely fix this, But, I feel like it's a lot of updates to maintain three structs -- FeatureBit, Feature, and Features -- for a relatively small benefit, considering how we're going to operate on Features. But I can definitely add them and fix this ambiguity if others also think it's worth updating

CLN also closely follows the current model (https://github.com/ElementsProject/lightning/blob/master/common/features.h), with OPTIONAL and REQUIRED macros for updating the bit. I think we can keep the const as they are for now, and in the future, add required and optional func to get the required or optional bit from these defined const. WDYT?

[erickcestari]

LGTM!

[ekzyis]

partial review, got distracted, will continue review today

[ekzyis]

But if, in the future, a caller needs to set an optional feature bit, I think we can define the optional feature bit as a constant as well and suffix the constants with _REQUIRED and _OPTIONAL, respectively.

Makes sense. I think my only issue is that I think the _REQUIRED suffix would already be useful. But consider this a nit.

Edit: To clarify further:

the _REQUIRED suffix would already be useful

I think this is downstream of the inconsistency in how FeatureBit is treated as a function argument. Sometimes it's treated as a feature (supports_feature); sometimes as a bit (set_bit).

Anyway, I'm not going to not ACK this PR because of this.

[ekzyis]

43b1bf3 is failing to build:

cargo test -p smite

$ cargo test -p smite
   Compiling smite v0.0.0 (/home/ekzyis/prog/smite/smite)
error[E0308]: mismatched types
   --> smite/src/oracles/accept_channel.rs:227:76
    |
227 |     let commitment_cost = CommitmentCost::new(open_channel.feerate_per_kw, channel_type);
    |                           -------------------                              ^^^^^^^^^^^^ expected `&Features`, found `&[u8]`
    |                           |
    |                           arguments to this function are incorrect
    |
    = note: expected reference `&Features`
               found reference `&[u8]`
note: associated function defined here
   --> smite/src/channel_tx/commitment.rs:472:12
    |
472 |     pub fn new(feerate_per_kw: u32, channel_type: &Features) -> CommitmentCost {
    |            ^^^                      -----------------------

For more information about this error, try `rustc --explain E0308`.
error: could not compile `smite` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error[E0308]: mismatched types
   --> smite/src/oracles/accept_channel.rs:227:76
    |
227 |     let commitment_cost = CommitmentCost::new(open_channel.feerate_per_kw, channel_type);
    |                           -------------------                              ^^^^^^^^^^^^ expected `&Features`, found `&[u8]`
    |                           |
    |                           arguments to this function are incorrect
    |
    = note: expected reference `&features::Features`
               found reference `&[u8]`
note: associated function defined here
   --> smite/src/channel_tx/commitment.rs:472:12
    |
472 |     pub fn new(feerate_per_kw: u32, channel_type: &Features) -> CommitmentCost {
    |            ^^^                      -----------------------

error: could not compile `smite` (lib test) due to 1 previous error

I think 74ecd09 should be squashed into 43b1bf3.

Other than that, LGTM! Will ACK after rebase.