common/features: don't use internal global.

Turns out that unnecessary: all callers can access the feature_set,
so make it much more like a normal primitive.

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

channeld/channeld.c

@@ -74,6 +74,9 @@ struct peer {
 	/* Features peer supports. */
 	u8 *features;
 
+	/* Features we support. */
+	struct feature_set *fset;
+
 	/* Tolerable amounts for feerate (only relevant for fundee). */
 	u32 feerate_min, feerate_max;
 
@@ -413,7 +416,7 @@ static void send_announcement_signatures(struct peer *peer)
 static u8 *create_channel_announcement(const tal_t *ctx, struct peer *peer)
 {
 	int first, second;
-	u8 *cannounce, *features = get_agreed_channelfeatures(tmpctx, peer->features);
+	u8 *cannounce, *features = get_agreed_channelfeatures(tmpctx, peer->fset, peer->features);
 
 	if (peer->channel_direction == 0) {
 		first = LOCAL;
@@ -2043,7 +2046,7 @@ static void peer_reconnect(struct peer *peer,
 	bool dataloss_protect, check_extra_fields;
 	const u8 **premature_msgs = tal_arr(peer, const u8 *, 0);
 
-	dataloss_protect = feature_negotiated(peer->features,
+	dataloss_protect = feature_negotiated(peer->fset, peer->features,
 					      OPT_DATA_LOSS_PROTECT);
 
 	/* Both these options give us extra fields to check. */
@@ -2753,7 +2756,6 @@ static void init_channel(struct peer *peer)
 	secp256k1_ecdsa_signature *remote_ann_node_sig;
 	secp256k1_ecdsa_signature *remote_ann_bitcoin_sig;
 	bool option_static_remotekey;
-	struct feature_set *feature_set;
 #if !DEVELOPER
 	bool dev_fail_process_onionpacket; /* Ignored */
 #endif
@@ -2765,7 +2767,7 @@ static void init_channel(struct peer *peer)
 	msg = wire_sync_read(tmpctx, MASTER_FD);
 	if (!fromwire_channel_init(peer, msg,
 				   &chainparams,
-				   &feature_set,
+				   &peer->fset,
 				   &funding_txid, &funding_txout,
 				   &funding,
 				   &minimum_depth,
@@ -2821,9 +2823,6 @@ static void init_channel(struct peer *peer)
 		master_badmsg(WIRE_CHANNEL_INIT, msg);
 	}
 
-	/* Now we know what features to advertize. */
-	features_init(take(feature_set));
-
 	/* stdin == requests, 3 == peer, 4 = gossip, 5 = gossip_store, 6 = HSM */
 	per_peer_state_set_fds(peer->pps, 3, 4, 5);
 

common/bolt11.c

@@ -489,6 +489,7 @@ static void shift_bitmap_down(u8 *bitmap, size_t bits)
  *  See [Feature Bits](#feature-bits).
  */
 static char *decode_9(struct bolt11 *b11,
+		      const struct feature_set *fset,
 		      struct hash_u5 *hu5,
 		      u5 **data, size_t *data_len,
 		      size_t data_length)
@@ -511,13 +512,12 @@ static char *decode_9(struct bolt11 *b11,
 	 * - if the `9` field contains unknown _even_ bits that are non-zero:
 	 *   - MUST fail the payment.
 	 */
-	/* BOLT #11:
-	 * The field is big-endian.  The least-significant bit is numbered 0,
-	 * which is _even_, and the next most significant bit is numbered 1,
-	 * which is _odd_. */
-	badf = features_unsupported(b11->features);
-	if (badf != -1)
-		return tal_fmt(b11, "9: unknown feature bit %i", badf);
+	/* We skip this check for the cli tool, which sets fset to NULL */
+	if (fset) {
+		badf = features_unsupported(fset, b11->features, BOLT11_FEATURE);
+		if (badf != -1)
+			return tal_fmt(b11, "9: unknown feature bit %i", badf);
+	}
 
 	return NULL;
 }
@@ -545,6 +545,7 @@ struct bolt11 *new_bolt11(const tal_t *ctx,
 
 /* Decodes and checks signature; returns NULL on error. */
 struct bolt11 *bolt11_decode(const tal_t *ctx, const char *str,
+			     const struct feature_set *fset,
 			     const char *description, char **fail)
 {
 	char *hrp, *amountstr, *prefix;
@@ -739,7 +740,7 @@ struct bolt11 *bolt11_decode(const tal_t *ctx, const char *str,
 					   data_length);
 			break;
 		case '9':
-			problem = decode_9(b11, &hu5, &data, &data_len,
+			problem = decode_9(b11, fset, &hu5, &data, &data_len,
 					   data_length);
 			break;
 		case 's':

common/bolt11.h

@@ -14,6 +14,8 @@
 /* We only have 10 bits for the field length, meaning < 640 bytes */
 #define BOLT11_FIELD_BYTE_LIMIT ((1 << 10) * 5 / 8)
 
+struct feature_set;
+
 struct bolt11_field {
 	struct list_node list;
 
@@ -74,8 +76,11 @@ struct bolt11 {
 };
 
 /* Decodes and checks signature; returns NULL on error; description is
- * (optional) out-of-band description of payment, for `h` field. */
+ * (optional) out-of-band description of payment, for `h` field.
+ * fset is NULL to accept any features (usually not desirable!).
+ */
 struct bolt11 *bolt11_decode(const tal_t *ctx, const char *str,
+			     const struct feature_set *fset,
 			     const char *description, char **fail);
 
 /* Initialize an empty bolt11 struct with optional amount */

common/features.c

@@ -5,10 +5,6 @@
 #include <common/utils.h>
 #include <wire/peer_wire.h>
 
-/* We keep a map of our features for each context, with the assumption that
- * the init features is a superset of the others. */
-static struct feature_set *our_features;
-
 enum feature_copy_style {
 	/* Feature is not exposed (importantly, being 0, this is the default!). */
 	FEATURE_DONT_REPRESENT,
@@ -67,84 +63,53 @@ static enum feature_copy_style feature_copy_style(u32 f, enum feature_place p)
 	abort();
 }
 
-static u8 *mkfeatures(const tal_t *ctx, enum feature_place place)
+struct feature_set *feature_set_for_feature(const tal_t *ctx, int feature)
 {
-	u8 *f = tal_arr(ctx, u8, 0);
-	const u8 *base = our_features->bits[INIT_FEATURE];
-
-	assert(place != INIT_FEATURE);
-	for (size_t i = 0; i < tal_bytelen(base)*8; i++) {
-		if (!feature_is_set(base, i))
-			continue;
+	struct feature_set *fs = tal(ctx, struct feature_set);
 
-		switch (feature_copy_style(i, place)) {
+	for (size_t i = 0; i < ARRAY_SIZE(fs->bits); i++) {
+		fs->bits[i] = tal_arr(fs, u8, 0);
+		switch (feature_copy_style(feature, i)) {
 		case FEATURE_DONT_REPRESENT:
 			continue;
 		case FEATURE_REPRESENT:
-			set_feature_bit(&f, i);
+			set_feature_bit(&fs->bits[i], feature);
 			continue;
 		case FEATURE_REPRESENT_AS_OPTIONAL:
-			set_feature_bit(&f, OPTIONAL_FEATURE(i));
+			set_feature_bit(&fs->bits[i], OPTIONAL_FEATURE(feature));
 			continue;
 		}
 		abort();
 	}
-	return f;
-}
-
-struct feature_set *features_core_init(const u8 *feature_bits)
-{
-	assert(!our_features);
-	our_features = notleak(tal(NULL, struct feature_set));
-
-	our_features->bits[INIT_FEATURE]
-		= tal_dup_talarr(our_features, u8, feature_bits);
-
-	/* Make other masks too */
-	for (enum feature_place f = INIT_FEATURE+1; f < NUM_FEATURE_PLACE; f++)
-		our_features->bits[f] = mkfeatures(our_features, f);
-
-	return our_features;
-}
-
-void features_init(struct feature_set *fset TAKES)
-{
-	assert(!our_features);
-
-	if (taken(fset))
-		our_features = notleak(tal_steal(NULL, fset));
-	else {
-		our_features = notleak(tal(NULL, struct feature_set));
-		for (size_t i = 0; i < ARRAY_SIZE(fset->bits); i++)
-			our_features->bits[i] = tal_dup_talarr(our_features, u8,
-							       fset->bits[i]);
-	}
-}
-
-void features_cleanup(void)
-{
-	our_features = tal_free(our_features);
+	return fs;
 }
 
-bool features_additional(const struct feature_set *newfset)
+bool feature_set_or(struct feature_set *a,
+		    const struct feature_set *b TAKES)
 {
 	/* Check first, before we change anything! */
-	for (size_t i = 0; i < ARRAY_SIZE(newfset->bits); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(b->bits); i++) {
 		/* FIXME: We could allow a plugin to upgrade an optional feature
 		 * to a compulsory one? */
-		for (size_t b = 0; b < tal_bytelen(newfset->bits[i])*8; b++) {
-			if (feature_is_set(newfset->bits[i], b)
-			    && feature_is_set(our_features->bits[i], b))
+		for (size_t j = 0; j < tal_bytelen(b->bits[i])*8; j++) {
+			if (feature_is_set(b->bits[i], j)
+			    && feature_offered(a->bits[i], j)) {
+				if (taken(b))
+					tal_free(b);
 				return false;
+			}
 		}
 	}
 
-	for (size_t i = 0; i < ARRAY_SIZE(newfset->bits); i++) {
-		for (size_t b = 0; b < tal_bytelen(newfset->bits[i])*8; b++) {
-			if (feature_is_set(newfset->bits[i], b))
-				set_feature_bit(&our_features->bits[i], b);
+	for (size_t i = 0; i < ARRAY_SIZE(a->bits); i++) {
+		for (size_t j = 0; j < tal_bytelen(b->bits[i])*8; j++) {
+			if (feature_is_set(b->bits[i], j))
+				set_feature_bit(&a->bits[i], j);
 		}
 	}
+
+	if (taken(b))
+		tal_free(b);
 	return true;
 }
 
@@ -172,21 +137,6 @@ static bool test_bit(const u8 *features, size_t byte, unsigned int bit)
 	return features[tal_count(features) - 1 - byte] & (1 << (bit % 8));
 }
 
-u8 *get_offered_nodefeatures(const tal_t *ctx)
-{
-	return tal_dup_talarr(ctx, u8, our_features->bits[NODE_ANNOUNCE_FEATURE]);
-}
-
-u8 *get_offered_initfeatures(const tal_t *ctx)
-{
-	return tal_dup_talarr(ctx, u8, our_features->bits[INIT_FEATURE]);
-}
-
-u8 *get_offered_globalinitfeatures(const tal_t *ctx)
-{
-	return tal_dup_talarr(ctx, u8, our_features->bits[GLOBAL_INIT_FEATURE]);
-}
-
 static void clear_feature_bit(u8 *features, u32 bit)
 {
 	size_t bytenum = bit / 8, bitnum = bit % 8, len = tal_count(features);
@@ -203,9 +153,11 @@ static void clear_feature_bit(u8 *features, u32 bit)
  *  - MUST set `len` to the minimum length required to hold the `features` bits
  *  it sets.
  */
-u8 *get_agreed_channelfeatures(const tal_t *ctx, const u8 *theirfeatures)
+u8 *get_agreed_channelfeatures(const tal_t *ctx,
+			       const struct feature_set *ours,
+			       const u8 *theirfeatures)
 {
-	u8 *f = tal_dup_talarr(ctx, u8, our_features->bits[CHANNEL_FEATURE]);
+	u8 *f = tal_dup_talarr(ctx, u8, ours->bits[CHANNEL_FEATURE]);
 	size_t max_len = 0;
 
 	/* Clear any features which they didn't offer too */
@@ -225,11 +177,6 @@ u8 *get_agreed_channelfeatures(const tal_t *ctx, const u8 *theirfeatures)
 	return f;
 }
 
-u8 *get_offered_bolt11features(const tal_t *ctx)
-{
-	return tal_dup_talarr(ctx, u8, our_features->bits[BOLT11_FEATURE]);
-}
-
 bool feature_is_set(const u8 *features, size_t bit)
 {
 	size_t bytenum = bit / 8;
@@ -246,10 +193,11 @@ bool feature_offered(const u8 *features, size_t f)
 		|| feature_is_set(features, OPTIONAL_FEATURE(f));
 }
 
-bool feature_negotiated(const u8 *lfeatures, size_t f)
+bool feature_negotiated(const struct feature_set *ours,
+			const u8 *lfeatures, size_t f)
 {
 	return feature_offered(lfeatures, f)
-		&& feature_offered(our_features->bits[INIT_FEATURE], f);
+		&& feature_offered(ours->bits[INIT_FEATURE], f);
 }
 
 /**
@@ -263,7 +211,9 @@ bool feature_negotiated(const u8 *lfeatures, size_t f)
  *
  * Returns -1 on success, or first unsupported feature.
  */
-static int all_supported_features(const u8 *bitmap)
+static int all_supported_features(const struct feature_set *ours,
+				  const u8 *bitmap,
+				  enum feature_place p)
 {
 	size_t len = tal_count(bitmap) * 8;
 
@@ -272,23 +222,24 @@ static int all_supported_features(const u8 *bitmap)
 		if (!test_bit(bitmap, bitnum/8, bitnum%8))
 			continue;
 
-		if (feature_offered(our_features->bits[INIT_FEATURE], bitnum))
+		if (feature_offered(ours->bits[p], bitnum))
 			continue;
 
 		return bitnum;
 	}
 	return -1;
 }
 
-int features_unsupported(const u8 *features)
+int features_unsupported(const struct feature_set *ours, const u8 *theirs,
+			 enum feature_place p)
 {
 	/* BIT 2 would logically be "compulsory initial_routing_sync", but
 	 * that does not exist, so we special case it. */
-	if (feature_is_set(features,
+	if (feature_is_set(theirs,
 			   COMPULSORY_FEATURE(OPT_INITIAL_ROUTING_SYNC)))
 		return COMPULSORY_FEATURE(OPT_INITIAL_ROUTING_SYNC);
 
-	return all_supported_features(features);
+	return all_supported_features(ours, theirs, p);
 }
 
 static const char *feature_name(const tal_t *ctx, size_t f)
@@ -313,12 +264,13 @@ static const char *feature_name(const tal_t *ctx, size_t f)
 		       fnames[f / 2], (f & 1) ? "odd" : "even");
 }
 
-const char **list_supported_features(const tal_t *ctx)
+const char **list_supported_features(const tal_t *ctx,
+				     const struct feature_set *ours)
 {
 	const char **list = tal_arr(ctx, const char *, 0);
 
-	for (size_t i = 0; i < tal_bytelen(our_features->bits[INIT_FEATURE]) * 8; i++) {
-		if (test_bit(our_features->bits[INIT_FEATURE], i / 8, i % 8))
+	for (size_t i = 0; i < tal_bytelen(ours->bits[INIT_FEATURE]) * 8; i++) {
+		if (test_bit(ours->bits[INIT_FEATURE], i / 8, i % 8))
 			tal_arr_expand(&list, feature_name(list, i));
 	}