Skip to content

Commit

Permalink
Persist feature bits across restarts
Browse files Browse the repository at this point in the history
[ Whitespace fix and remove gratuitous tal_free(peer->their_features) -- RR ]
  • Loading branch information
instagibbs authored and rustyrussell committed Jun 20, 2023
1 parent 9f1e1ad commit e125640
Showing 7 changed files with 54 additions and 23 deletions.
1 change: 1 addition & 0 deletions lightningd/opening_control.c
Original file line number Diff line number Diff line change
@@ -1350,6 +1350,7 @@ static struct channel *stub_chan(struct command *cmd,
0,
&nodeid,
&wint,
NULL,
false);
}

29 changes: 15 additions & 14 deletions lightningd/peer_control.c
Original file line number Diff line number Diff line change
@@ -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);
1 change: 1 addition & 0 deletions lightningd/peer_control.h
Original file line number Diff line number Diff line change
@@ -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. */
24 changes: 24 additions & 0 deletions tests/test_connection.py
Original file line number Diff line number Diff line change
@@ -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},
1 change: 1 addition & 0 deletions wallet/db.c
Original file line number Diff line number Diff line change
@@ -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},
};

/**
6 changes: 3 additions & 3 deletions wallet/test/run-wallet.c
Original file line number Diff line number Diff line change
@@ -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);
15 changes: 9 additions & 6 deletions wallet/wallet.c
Original file line number Diff line number Diff line change
@@ -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)));
}

0 comments on commit e125640

Please sign in to comment.