Persist feature bits across restarts

[ Whitespace fix and remove gratuitous tal_free(peer->their_features) -- RR ]

Author: instagibbs

lightningd/opening_control.c

@@ -1351,6 +1351,7 @@ static struct channel *stub_chan(struct command *cmd,
 				0,
 				&nodeid,
 				&wint,
+				NULL,
 				false);
 	}
 

lightningd/peer_control.c

@@ -76,13 +76,6 @@ static void destroy_peer(struct peer *peer)
 		peer_dbid_map_del(peer->ld->peers_by_dbid, peer);
 }
 
-static void peer_update_features(struct peer *peer,
-				 const u8 *their_features TAKES)
-{
-	tal_free(peer->their_features);
-	peer->their_features = tal_dup_talarr(peer, u8, their_features);
-}
-
 void peer_set_dbid(struct peer *peer, u64 dbid)
 {
 	assert(!peer->dbid);
@@ -94,6 +87,7 @@ void peer_set_dbid(struct peer *peer, u64 dbid)
 struct peer *new_peer(struct lightningd *ld, u64 dbid,
 		      const struct node_id *id,
 		      const struct wireaddr_internal *addr,
+		      const u8 *their_features,
 		      bool connected_incoming)
 {
 	/* We are owned by our channels, and freed manually by destroy_channel */
@@ -106,12 +100,16 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid,
 	peer->addr = *addr;
 	peer->connected_incoming = connected_incoming;
 	peer->remote_addr = NULL;
-	peer->their_features = NULL;
 	list_head_init(&peer->channels);
 	peer->direction = node_id_idx(&peer->ld->id, &peer->id);
 	peer->connected = PEER_DISCONNECTED;
 	peer->last_connect_attempt.ts.tv_sec
 		= peer->last_connect_attempt.ts.tv_nsec = 0;
+	if (their_features)
+		peer->their_features = tal_dup_talarr(peer, u8, their_features);
+	else
+		peer->their_features = NULL;
+
 #if DEVELOPER
 	peer->ignore_htlcs = false;
 #endif
@@ -1403,7 +1401,11 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
 	peer = peer_by_id(ld, &id);
 	if (!peer)
 		peer = new_peer(ld, 0, &id, &hook_payload->addr,
-				hook_payload->incoming);
+				their_features, hook_payload->incoming);
+	else {
+		tal_free(peer->their_features);
+		peer->their_features = tal_dup_talarr(peer, u8, their_features);
+	}
 
 	/* We track this, because messages can race between connectd and us.
 	 * For example, we could tell it to attach a subd, but it's actually
@@ -1423,7 +1425,6 @@ void peer_connected(struct lightningd *ld, const u8 *msg)
 	if (peer->remote_addr)
 		tal_free(peer->remote_addr);
 	peer->remote_addr = NULL;
-	peer_update_features(peer, their_features);
 	hook_payload->peer_id = id;
 
 	/* If there's a connect command, use its id as basis for hook id */
@@ -1942,9 +1943,6 @@ static void json_add_peer(struct lightningd *ld,
 		num_channels++;
 	json_add_num(response, "num_channels", num_channels);
 
-	/* If it's not connected, features are unreliable: we don't
-	 * store them in the database, and they would only reflect
-	 * their features *last* time they connected. */
 	if (p->connected == PEER_CONNECTED) {
 		json_array_start(response, "netaddr");
 		json_add_string(response, NULL,
@@ -1956,8 +1954,11 @@ static void json_add_peer(struct lightningd *ld,
 		if (p->remote_addr)
 			json_add_string(response, "remote_addr",
 					fmt_wireaddr(response, p->remote_addr));
-		json_add_hex_talarr(response, "features", p->their_features);
 	}
+
+	/* Note: If !PEER_CONNECTED, peer may use different features on reconnect */
+	json_add_hex_talarr(response, "features", p->their_features);
+
 	if (deprecated_apis) {
 		json_array_start(response, "channels");
 		json_add_uncommitted_channel(response, p->uncommitted_channel, NULL);

lightningd/peer_control.h

@@ -73,6 +73,7 @@ struct peer *find_peer_by_dbid(struct lightningd *ld, u64 dbid);
 struct peer *new_peer(struct lightningd *ld, u64 dbid,
 		      const struct node_id *id,
 		      const struct wireaddr_internal *addr,
+			  const u8 *their_features,
 		      bool connected_incoming);
 
 /* Last one out deletes peer.  Also removes from db. */

tests/test_connection.py

@@ -4393,6 +4393,30 @@ def test_peer_disconnected_reflected_in_channel_state(node_factory):
     wait_for(lambda: only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['peer_connected'] is False)
 
 
+def test_peer_disconnected_has_featurebits(node_factory):
+    """
+    Make sure that if a node is restarted, it still remembers feature
+    bits from a peer it has a channel with but isn't connected to
+    """
+    l1, l2 = node_factory.line_graph(2)
+
+    expected_features = expected_peer_features()
+
+    l1_features = only_one(l2.rpc.listpeers()['peers'])['features']
+    l2_features = only_one(l1.rpc.listpeers()['peers'])['features']
+    assert l1_features == expected_features
+    assert l2_features == expected_features
+
+    l1.stop()
+    l2.stop()
+
+    # Ensure we persisted feature bits and return them even when disconnected
+    l1.start()
+
+    wait_for(lambda: only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected'] is False)
+    wait_for(lambda: only_one(l1.rpc.listpeers()['peers'])['features'] == expected_features)
+
+
 @pytest.mark.developer("needs dev-no-reconnect")
 def test_reconnect_no_additional_transient_failure(node_factory, bitcoind):
     l1, l2 = node_factory.line_graph(2, opts=[{'may_reconnect': True},

wallet/db.c

@@ -949,6 +949,7 @@ static struct migration dbmigrations[] = {
     {NULL, migrate_invalid_last_tx_psbts},
     {SQL("ALTER TABLE channels ADD channel_type BLOB DEFAULT NULL;"), NULL},
     {NULL, migrate_fill_in_channel_type},
+    {SQL("ALTER TABLE peers ADD feature_bits BLOB DEFAULT NULL;"), NULL},
 };
 
 /**

wallet/test/run-wallet.c

@@ -1172,7 +1172,7 @@ static bool test_wallet_outputs(struct lightningd *ld, const tal_t *ctx)
 
 	/* Add another utxo that's CSV-locked for 5 blocks */
 	assert(parse_wireaddr_internal(tmpctx, "localhost:1234", 0, false, &addr) == NULL);
-	channel.peer = new_peer(ld, 0, &id, &addr, false);
+	channel.peer = new_peer(ld, 0, &id, &addr, NULL, false);
 	channel.dbid = 1;
 	channel.type = channel_type_anchor_outputs(tmpctx);
 	memset(&u.outpoint, 3, sizeof(u.outpoint));
@@ -1502,7 +1502,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	c1.first_blocknum = 1;
 	assert(parse_wireaddr_internal(tmpctx, "localhost:1234", 0, false, &addr) == NULL);
 	c1.final_key_idx = 1337;
-	p = new_peer(ld, 0, &id, &addr, false);
+	p = new_peer(ld, 0, &id, &addr, NULL, false);
 	c1.peer = p;
 	c1.dbid = wallet_get_channel_dbid(w);
 	c1.state = CHANNELD_NORMAL;
@@ -1665,7 +1665,7 @@ static bool test_channel_inflight_crud(struct lightningd *ld, const tal_t *ctx)
 	assert(parse_wireaddr_internal(tmpctx, "localhost:1234", 0, false, &addr) == NULL);
 
 	/* new channel! */
-	p = new_peer(ld, 0, &id, &addr, false);
+	p = new_peer(ld, 0, &id, &addr, NULL, false);
 
 	funding_sats = AMOUNT_SAT(4444444);
 	our_sats = AMOUNT_SAT(3333333);

wallet/wallet.c

@@ -859,7 +859,7 @@ static struct peer *wallet_peer_load(struct wallet *w, const u64 dbid)
 	struct db_stmt *stmt;
 
 	stmt = db_prepare_v2(
-	    w->db, SQL("SELECT id, node_id, address FROM peers WHERE id=?;"));
+	    w->db, SQL("SELECT id, node_id, address, feature_bits FROM peers WHERE id=?;"));
 	db_bind_u64(stmt, 0, dbid);
 	db_query_prepared(stmt);
 
@@ -869,6 +869,7 @@ static struct peer *wallet_peer_load(struct wallet *w, const u64 dbid)
 	if (db_col_is_null(stmt, "node_id")) {
 		db_col_ignore(stmt, "address");
 		db_col_ignore(stmt, "id");
+		db_col_ignore(stmt, "feature_bits");
 		goto done;
 	}
 
@@ -886,7 +887,7 @@ static struct peer *wallet_peer_load(struct wallet *w, const u64 dbid)
 	}
 
 	/* FIXME: save incoming in db! */
-	peer = new_peer(w->ld, db_col_u64(stmt, "id"), &id, &addr, false);
+	peer = new_peer(w->ld, db_col_u64(stmt, "id"), &id, &addr, db_col_arr(stmt, stmt, "feature_bits", u8), false);
 
 done:
 	tal_free(stmt);
@@ -2271,21 +2272,23 @@ static void wallet_peer_save(struct wallet *w, struct peer *peer)
 		peer_set_dbid(peer, db_col_u64(stmt, "id"));
 		tal_free(stmt);
 
-		/* Since we're at it update the wireaddr */
+		/* Since we're at it update the wireaddr, feature bits */
 		stmt = db_prepare_v2(
-		    w->db, SQL("UPDATE peers SET address = ? WHERE id = ?"));
+		    w->db, SQL("UPDATE peers SET address = ?, feature_bits = ? WHERE id = ?"));
 		db_bind_text(stmt, 0, addr);
-		db_bind_u64(stmt, 1, peer->dbid);
+		db_bind_talarr(stmt, 1, peer->their_features);
+		db_bind_u64(stmt, 2, peer->dbid);
 		db_exec_prepared_v2(take(stmt));
 
 	} else {
 		/* Unknown peer, create it from scratch */
 		tal_free(stmt);
 		stmt = db_prepare_v2(w->db,
-				     SQL("INSERT INTO peers (node_id, address) VALUES (?, ?);")
+				     SQL("INSERT INTO peers (node_id, address, feature_bits) VALUES (?, ?, ?);")
 			);
 		db_bind_node_id(stmt, 0, &peer->id);
 		db_bind_text(stmt, 1,addr);
+		db_bind_talarr(stmt, 2, peer->their_features);
 		db_exec_prepared_v2(stmt);
 		peer_set_dbid(peer, db_last_insert_id_v2(take(stmt)));
 	}