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.