features: fix features_supported() function.

1. We need to test all bits, not all bytes.
2. Both local and global features need to be supported.
3. Untested code is broken code.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/features.c

@@ -98,7 +98,7 @@ static bool all_supported_features(const u8 *bitmap,
 				   const u32 *supported,
 				   size_t num_supported)
 {
-	size_t len = tal_count(bitmap);
+	size_t len = tal_count(bitmap) * 8;
 
 	/* It's OK to be odd: only check even bits. */
 	for (size_t bitnum = 0; bitnum < len; bitnum += 2) {
@@ -124,7 +124,7 @@ bool features_supported(const u8 *gfeatures, const u8 *lfeatures)
 	return all_supported_features(gfeatures,
 				      global_features,
 				      ARRAY_SIZE(global_features))
-		|| all_supported_features(lfeatures,
+		&& all_supported_features(lfeatures,
 					  local_features,
 					  ARRAY_SIZE(local_features));
 }

common/test/run-features.c

@@ -0,0 +1,106 @@
+#include "../features.c"
+#include <ccan/err/err.h>
+#include <ccan/mem/mem.h>
+#include <ccan/str/hex/hex.h>
+#include <common/utils.h>
+#include <stdio.h>
+#include <wally_core.h>
+
+/* AUTOGENERATED MOCKS START */
+/* AUTOGENERATED MOCKS END */
+
+int main(void)
+{
+	u8 *bits, *lf, *gf;
+
+	setup_locale();
+	secp256k1_ctx = wally_get_secp_context();
+	setup_tmpctx();
+
+	bits = tal_arr(tmpctx, u8, 0);
+	for (size_t i = 0; i < 100; i += 3)
+		set_bit(&bits, i);
+	for (size_t i = 0; i < 100; i++)
+		assert(test_bit(bits, i / 8, i % 8) == ((i % 3) == 0));
+
+	for (size_t i = 0; i < 100; i++)
+		assert(feature_set(bits, i) == ((i % 3) == 0));
+
+	/* Simple test: single byte */
+	bits = tal_arr(tmpctx, u8, 1);
+
+	/* Compulsory feature */
+	bits[0] = (1 << 0);
+	assert(feature_offered(bits, 0));
+	assert(!feature_offered(bits, 2));
+	assert(!feature_offered(bits, 8));
+	assert(!feature_offered(bits, 16));
+
+	/* Optional feature */
+	bits[0] = (1 << 1);
+	assert(feature_offered(bits, 0));
+	assert(!feature_offered(bits, 2));
+	assert(!feature_offered(bits, 8));
+	assert(!feature_offered(bits, 16));
+
+	/* Endian-sensitive test: big-endian means we frob last byte here */
+	bits = tal_arrz(tmpctx, u8, 2);
+
+	bits[1] = (1 << 0);
+	assert(feature_offered(bits, 0));
+	assert(!feature_offered(bits, 2));
+	assert(!feature_offered(bits, 8));
+	assert(!feature_offered(bits, 16));
+
+	/* Optional feature */
+	bits[1] = (1 << 1);
+	assert(feature_offered(bits, 0));
+	assert(!feature_offered(bits, 2));
+	assert(!feature_offered(bits, 8));
+	assert(!feature_offered(bits, 16));
+
+	/* We always support no features. */
+	memset(bits, 0, tal_len(bits));
+	assert(features_supported(bits, bits));
+
+	/* We must support our own features. */
+	lf = get_offered_global_features(tmpctx);
+	gf = get_offered_global_features(tmpctx);
+	assert(features_supported(gf, lf));
+
+	/* We can add random odd features, no problem. */
+	for (size_t i = 1; i < 16; i += 2) {
+		bits = tal_dup_arr(tmpctx, u8, lf, tal_len(lf), 0);
+		set_bit(&bits, i);
+		assert(features_supported(gf, bits));
+
+		bits = tal_dup_arr(tmpctx, u8, gf, tal_len(gf), 0);
+		set_bit(&bits, i);
+		assert(features_supported(bits, lf));
+	}
+
+	/* We can't add random even features. */
+	for (size_t i = 0; i < 16; i += 2) {
+		bits = tal_dup_arr(tmpctx, u8, lf, tal_len(lf), 0);
+		set_bit(&bits, i);
+
+		/* Special case for missing compulsory feature */
+		if (i == 2) {
+			assert(!features_supported(gf, bits));
+		} else {
+			assert(features_supported(gf, bits)
+			       == feature_supported(i, local_features,
+						    ARRAY_SIZE(local_features)));
+		}
+
+		bits = tal_dup_arr(tmpctx, u8, gf, tal_len(gf), 0);
+		set_bit(&bits, i);
+		assert(features_supported(bits, lf)
+		       == feature_supported(i, global_features,
+					    ARRAY_SIZE(global_features)));
+	}
+
+	wally_cleanup(0);
+	tal_free(tmpctx);
+	return 0;
+}