common/features: clean up feature handling for different cases.

The spec is (RSN!) going to explicitly denote where each feature should
be presented, so create that infrastructure.

Incorporate the new proposed bolt11 features, which need this.

Signed-off-by: Rusty Russell <[email protected]>

common/features.c

@@ -15,6 +15,72 @@ static const u32 our_features[] = {
 	OPTIONAL_FEATURE(OPT_STATIC_REMOTEKEY),
 };
 
+enum feature_copy_style {
+	/* Feature is not exposed (importantly, being 0, this is the default!). */
+	FEATURE_DONT_REPRESENT,
+	/* Feature is exposed. */
+	FEATURE_REPRESENT,
+	/* Feature is exposed, but always optional. */
+	FEATURE_REPRESENT_AS_OPTIONAL,
+};
+
+enum feature_place {
+	INIT_FEATURE,
+	GLOBAL_INIT_FEATURE,
+	NODE_ANNOUNCE_FEATURE,
+	BOLT11_FEATURE,
+};
+#define NUM_FEATURE_PLACE (BOLT11_FEATURE+1)
+
+struct feature_style {
+	u32 bit;
+	enum feature_copy_style copy_style[NUM_FEATURE_PLACE];
+};
+
+static const struct feature_style feature_styles[] = {
+	{ OPT_DATA_LOSS_PROTECT,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_INITIAL_ROUTING_SYNC,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT_AS_OPTIONAL,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_DONT_REPRESENT } },
+	{ OPT_UPFRONT_SHUTDOWN_SCRIPT,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_GOSSIP_QUERIES,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_GOSSIP_QUERIES_EX,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_VAR_ONION,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [GLOBAL_INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT,
+			  [BOLT11_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_STATIC_REMOTEKEY,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [GLOBAL_INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_PAYMENT_SECRET,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT,
+			  [BOLT11_FEATURE] = FEATURE_REPRESENT } },
+	{ OPT_BASIC_MPP,
+	  .copy_style = { [INIT_FEATURE] = FEATURE_REPRESENT,
+			  [NODE_ANNOUNCE_FEATURE] = FEATURE_REPRESENT,
+			  [BOLT11_FEATURE] = FEATURE_REPRESENT } },
+};
+
+static enum feature_copy_style feature_copy_style(u32 f, enum feature_place p)
+{
+	for (size_t i = 0; i < ARRAY_SIZE(feature_styles); i++) {
+		if (feature_styles[i].bit == COMPULSORY_FEATURE(f))
+			return feature_styles[i].copy_style[p];
+	}
+	abort();
+}
+
 /* BOLT #1:
  *
  * All data fields are unsigned big-endian unless otherwise specified.
@@ -39,31 +105,44 @@ static bool test_bit(const u8 *features, size_t byte, unsigned int bit)
 	return features[tal_count(features) - 1 - byte] & (1 << (bit % 8));
 }
 
-static u8 *mkfeatures(const tal_t *ctx, const u32 *arr, size_t n)
+static u8 *mkfeatures(const tal_t *ctx, enum feature_place place)
 {
 	u8 *f = tal_arr(ctx, u8, 0);
 
-	for (size_t i = 0; i < n; i++)
-		set_feature_bit(&f, arr[i]);
+	for (size_t i = 0; i < ARRAY_SIZE(our_features); i++) {
+		switch (feature_copy_style(our_features[i], place)) {
+		case FEATURE_DONT_REPRESENT:
+			continue;
+		case FEATURE_REPRESENT:
+			set_feature_bit(&f, our_features[i]);
+			continue;
+		case FEATURE_REPRESENT_AS_OPTIONAL:
+			set_feature_bit(&f, OPTIONAL_FEATURE(our_features[i]));
+			continue;
+		}
+		abort();
+	}
 	return f;
 }
 
-/* We currently advertize everything in node_announcement, except
- * initial_routing_sync which the spec says not to (and we don't set
- * any more anyway).
- *
- * FIXME: Add bolt ref when finalized!
- */
 u8 *get_offered_nodefeatures(const tal_t *ctx)
 {
-	return mkfeatures(ctx,
-			  our_features, ARRAY_SIZE(our_features));
+	return mkfeatures(ctx, NODE_ANNOUNCE_FEATURE);
+}
+
+u8 *get_offered_initfeatures(const tal_t *ctx)
+{
+	return mkfeatures(ctx, INIT_FEATURE);
+}
+
+u8 *get_offered_globalinitfeatures(const tal_t *ctx)
+{
+	return mkfeatures(ctx, GLOBAL_INIT_FEATURE);
 }
 
-u8 *get_offered_features(const tal_t *ctx)
+u8 *get_offered_bolt11features(const tal_t *ctx)
 {
-	return mkfeatures(ctx,
-			  our_features, ARRAY_SIZE(our_features));
+	return mkfeatures(ctx, BOLT11_FEATURE);
 }
 
 bool feature_is_set(const u8 *features, size_t bit)
@@ -155,7 +234,10 @@ static const char *feature_name(const tal_t *ctx, size_t f)
 		"option_gossip_queries",
 		"option_var_onion_optin",
 		"option_gossip_queries_ex",
-		"option_static_remotekey" };
+		"option_static_remotekey",
+		"option_payment_secret",
+		"option_basic_mpp",
+	};
 
 	assert(f / 2 < ARRAY_SIZE(fnames));
 	return tal_fmt(ctx, "%s/%s",

common/features.h

@@ -9,8 +9,10 @@
 int features_unsupported(const u8 *features);
 
 /* For sending our features: tal_count() returns length. */
-u8 *get_offered_features(const tal_t *ctx);
+u8 *get_offered_initfeatures(const tal_t *ctx);
+u8 *get_offered_globalinitfeatures(const tal_t *ctx);
 u8 *get_offered_nodefeatures(const tal_t *ctx);
+u8 *get_offered_bolt11features(const tal_t *ctx);
 
 /* Is this feature bit requested? (Either compulsory or optional) */
 bool feature_offered(const u8 *features, size_t f);
@@ -55,6 +57,14 @@ void set_feature_bit(u8 **ptr, u32 bit);
 #define OPT_GOSSIP_QUERIES_EX			10
 #define OPT_STATIC_REMOTEKEY			12
 
+/* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #9:
+ *
+ * | 14/15 | `payment_secret` |... IN9 ...
+ * | 16/17 | `basic_mpp`      |... IN9 ...
+ */
+#define OPT_PAYMENT_SECRET			14
+#define OPT_BASIC_MPP				16
+
 /* BOLT #9:
  *
  * ## Assigned `globalfeatures` flags

common/test/run-features.c

@@ -87,7 +87,7 @@ int main(void)
 	assert(features_unsupported(bits) == -1);
 
 	/* We must support our own features. */
-	lf = get_offered_features(tmpctx);
+	lf = get_offered_initfeatures(tmpctx);
 	assert(features_unsupported(lf) == -1);
 
 	/* We can add random odd features, no problem. */

connectd/peer_exchange_initmsg.c

@@ -35,6 +35,7 @@ static struct io_plan *peer_init_received(struct io_conn *conn,
 {
 	u8 *msg = cryptomsg_decrypt_body(tmpctx, &peer->cs, peer->msg);
 	u8 *globalfeatures, *features;
+	int unsup;
 
 	if (!msg)
 		return io_close(conn);
@@ -73,12 +74,10 @@ static struct io_plan *peer_init_received(struct io_conn *conn,
 	 *  - upon receiving unknown _even_ feature bits that are non-zero:
 	 *    - MUST fail the connection.
 	 */
-	if (features_unsupported(features) != -1) {
-		const u8 *our_features = get_offered_features(msg);
-		msg = towire_errorfmt(NULL, NULL, "Unsupported features %s:"
-				      " we only offer features %s",
-				      tal_hex(msg, features),
-				      tal_hex(msg, our_features));
+	unsup = features_unsupported(features);
+	if (unsup != -1) {
+		msg = towire_errorfmt(NULL, NULL, "Unsupported feature %u",
+				      unsup);
 		msg = cryptomsg_encrypt_msg(NULL, &peer->cs, take(msg));
 		return io_write(conn, msg, tal_count(msg), io_close_cb, NULL);
 	}
@@ -157,17 +156,18 @@ struct io_plan *peer_exchange_initmsg(struct io_conn *conn,
 	 * But we didn't have any globals for a long time, and it turned out
 	 * that people wanted us to broadcast local features so they could do
 	 * peer selection.  We agreed that the number spaces should be distinct,
-	 * but debate still rages on how to handle them.
+	 * but debate still raged on how to handle them.
 	 *
 	 * Meanwhile, we finally added a global bit to the spec, so now it
 	 * matters.  And LND v0.8 decided to make option_static_remotekey a
 	 * GLOBAL bit, not a local bit, so we need to send that as a global
-	 * bit here.  Thus, we send our full, combo, bitset as both global
-	 * and local bits. */
+	 * bit here.
+	 *
+	 * Finally, we agreed that bits below 13 could be put in both, but
+	 * from now on they'll all go in initfeatures. */
 	peer->msg = towire_init(NULL,
-				/* Features so nice, we send it twice! */
-				get_offered_features(tmpctx),
-				get_offered_features(tmpctx));
+				get_offered_globalinitfeatures(tmpctx),
+				get_offered_initfeatures(tmpctx));
 	status_peer_io(LOG_IO_OUT, &peer->id, peer->msg);
 	peer->msg = cryptomsg_encrypt_msg(peer, &peer->cs, take(peer->msg));
 

tests/test_connection.py

@@ -1835,10 +1835,7 @@ def test_dataloss_protection(node_factory, bitcoind):
 
     l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
     # l1 should send out WIRE_INIT (0010)
-    l1.daemon.wait_for_log(r"\[OUT\] 0010"
-                           # gflen
-                           + format(len(lf) // 2, '04x')
-                           + lf
+    l1.daemon.wait_for_log(r"\[OUT\] 0010.*"
                            # lflen
                            + format(len(lf) // 2, '04x')
                            + lf)