From 46cc7c281e4f2895f35159511834c84262582b6f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 14 Mar 2018 02:11:55 +1030 Subject: [PATCH] features: more general accessor functions. As we add more features, the current code is insufficient. 1. Keep an array of single feature bits, for easy switching on and off. 2. Create feature_offered() which checks for both compulsory and optional variants. 3. Invert requires_unsupported_features() and unsupported_features() which tend to be double-negative, all_supported_features() and features_supported(). 4. Move single feature definition from wire/peer_wire.h to common/features.h. Signed-off-by: Rusty Russell --- common/features.c | 133 ++++++++++++++++++------- common/features.h | 27 ++++- gossipd/routing.c | 4 +- gossipd/test/run-bench-find_route.c | 2 +- gossipd/test/run-find_route-specific.c | 2 +- gossipd/test/run-find_route.c | 2 +- lightningd/gossip_control.c | 6 +- lightningd/peer_control.c | 20 ++-- wallet/test/run-wallet.c | 18 ++-- wire/peer_wire.h | 13 --- 10 files changed, 145 insertions(+), 82 deletions(-) diff --git a/common/features.c b/common/features.c index a710ee458..b986a156f 100644 --- a/common/features.c +++ b/common/features.c @@ -1,62 +1,119 @@ #include "features.h" +#include +#include #include -static const u8 supported_local_features[] -= {LOCALFEATURES_INITIAL_ROUTING_SYNC}; -static const u8 supported_global_features[] -= {}; +static const u32 local_features[] = { + LOCAL_INITIAL_ROUTING_SYNC +}; -u8 *get_supported_global_features(const tal_t *ctx) +static const u32 global_features[] = { +}; + +static void set_bit(u8 **ptr, u32 bit) { - return tal_dup_arr(ctx, u8, supported_global_features, - sizeof(supported_global_features), 0); + if (bit / 8 >= tal_len(*ptr)) + tal_resizez(ptr, (bit / 8) + 1); + (*ptr)[bit / 8] |= (1 << (bit % 8)); +} + +/* We don't insist on anything, it's all optional. */ +static u8 *mkfeatures(const tal_t *ctx, const u32 *arr, size_t n) +{ + u8 *f = tal_arr(ctx, u8, 0); + + for (size_t i = 0; i < n; i++) + set_bit(&f, OPTIONAL_FEATURE(arr[i])); + return f; } -u8 *get_supported_local_features(const tal_t *ctx) +u8 *get_offered_global_features(const tal_t *ctx) { - return tal_dup_arr(ctx, u8, supported_local_features, - sizeof(supported_local_features), 0); + return mkfeatures(ctx, global_features, ARRAY_SIZE(global_features)); +} + +u8 *get_offered_local_features(const tal_t *ctx) +{ + return mkfeatures(ctx, local_features, ARRAY_SIZE(local_features)); +} + +static bool test_bit(const u8 *features, size_t byte, unsigned int bit) +{ + return features[byte] & (1 << (bit % 8)); +} + +static bool feature_set(const u8 *features, size_t bit) +{ + size_t bytenum = bit / 8; + + if (bytenum >= tal_len(features)) + return false; + + return test_bit(features, bytenum, bit % 8); +} + +bool feature_offered(const u8 *features, size_t f) +{ + assert(f % 2 == 0); + + return feature_set(features, f) + || feature_set(features, OPTIONAL_FEATURE(f)); +} + +static bool feature_supported(int feature_bit, + const u32 *supported, + size_t num_supported) +{ + for (size_t i = 0; i < num_supported; i++) { + if (supported[i] == feature_bit) + return true; + } + return false; } /** - * requires_unsupported_features - Check if we support what's being asked + * all_supported_features - Check if we support what's being asked * * Given the features vector that the remote connection is expecting * from us, we check to see if we support all even bit features, i.e., - * the required features. We do so by subtracting our own features in - * the provided positions and see if even bits remain. + * the required features. * * @bitmap: the features bitmap the peer is asking for - * @supportmap: what do we support - * @smlen: how long is our supportmap + * @supported: array of features we support + * @num_supported: how many elements in supported */ -static bool requires_unsupported_features(const u8 *bitmap, - const u8 *supportmap, - size_t smlen) +static bool all_supported_features(const u8 *bitmap, + const u32 *supported, + size_t num_supported) { size_t len = tal_count(bitmap); - u8 support; - for (size_t i=0; i smlen) { - support = 0x00; - } else { - support = supportmap[smlen-1]; - } - - /* Cancel out supported bits, check for even bits */ - if ((~support & bitmap[i]) & 0x55) - return true; + + /* It's OK to be odd: only check even bits. */ + for (size_t bitnum = 0; bitnum < len; bitnum += 2) { + if (!test_bit(bitmap, bitnum/8, bitnum%8)) + continue; + + if (feature_supported(bitnum, supported, num_supported)) + continue; + + return false; } - return false; + return true; } -bool unsupported_features(const u8 *gfeatures, const u8 *lfeatures) +bool features_supported(const u8 *gfeatures, const u8 *lfeatures) { - return requires_unsupported_features(gfeatures, - supported_global_features, - sizeof(supported_global_features)) - || requires_unsupported_features(lfeatures, - supported_local_features, - sizeof(supported_local_features)); + /* BIT 2 would logically be "compulsory initial_routing_sync", but + * that does not exist, so we special case it. */ + if (feature_set(lfeatures, + COMPULSORY_FEATURE(LOCAL_INITIAL_ROUTING_SYNC))) + return false; + + return all_supported_features(gfeatures, + global_features, + ARRAY_SIZE(global_features)) + || all_supported_features(lfeatures, + local_features, + ARRAY_SIZE(local_features)); } + diff --git a/common/features.h b/common/features.h index 5a5c268e0..82a9fbdd3 100644 --- a/common/features.h +++ b/common/features.h @@ -4,11 +4,30 @@ #include #include -/* Returns true if these contain any unsupported features. */ -bool unsupported_features(const u8 *gfeatures, const u8 *lfeatures); +/* Returns true if we're OK with all these offered features. */ +bool features_supported(const u8 *gfeatures, const u8 *lfeatures); /* For sending our features: tal_len() returns length. */ -u8 *get_supported_global_features(const tal_t *ctx); -u8 *get_supported_local_features(const tal_t *ctx); +u8 *get_offered_global_features(const tal_t *ctx); +u8 *get_offered_local_features(const tal_t *ctx); + +/* Is this feature bit requested? (Either compulsory or optional) */ +bool feature_offered(const u8 *features, size_t f); + +#define COMPULSORY_FEATURE(x) (x) +#define OPTIONAL_FEATURE(x) ((x)+1) + +/* BOLT #9: + * + * ## Assigned `localfeatures` flags + *... + * | Bits | Name |... + * | 0/1 | `option-data-loss-protect` |... + * | 3 | `initial_routing_sync` |... + * | 4/5 | `option_upfront_shutdown_script` |... + */ +#define LOCAL_DATA_LOSS_PROTECT 0 +#define LOCAL_INITIAL_ROUTING_SYNC 2 +#define LOCAL_UPFRONT_SHUTDOWN_SCRIPT 4 #endif /* LIGHTNING_COMMON_FEATURES_H */ diff --git a/gossipd/routing.c b/gossipd/routing.c index b39f2494b..48c8b420a 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -657,7 +657,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate, * and MUST NOT add the channel to its local network view, and * SHOULD NOT forward the announcement. */ - if (unsupported_features(features, NULL)) { + if (!features_supported(features, NULL)) { status_trace("Ignoring channel announcement, unsupported features %s.", tal_hex(pending, features)); goto ignored; @@ -1070,7 +1070,7 @@ u8 *handle_node_announcement(struct routing_state *rstate, const u8 *node_ann) * receiving node MUST NOT parse the remainder of the message * and MAY discard the message altogether. */ - if (unsupported_features(features, NULL)) { + if (!features_supported(features, NULL)) { status_trace("Ignoring node announcement for node %s, unsupported features %s.", type_to_string(tmpctx, struct pubkey, &node_id), tal_hex(tmpctx, features)); diff --git a/gossipd/test/run-bench-find_route.c b/gossipd/test/run-bench-find_route.c index fa0deaaf6..15e45e8b4 100644 --- a/gossipd/test/run-bench-find_route.c +++ b/gossipd/test/run-bench-find_route.c @@ -77,7 +77,7 @@ const char *onion_type_name(int e UNNEEDED) bool replace_broadcast(const tal_t *ctx UNNEEDED, struct broadcast_state *bstate UNNEEDED, u64 *index UNNEEDED, - const u8 *payload UNNEEDED) + const u8 *payload TAKES UNNEEDED) { fprintf(stderr, "replace_broadcast called!\n"); abort(); } /* Generated stub for sanitize_error */ char *sanitize_error(const tal_t *ctx UNNEEDED, const u8 *errmsg UNNEEDED, diff --git a/gossipd/test/run-find_route-specific.c b/gossipd/test/run-find_route-specific.c index 8cf85acd0..6a1677bbb 100644 --- a/gossipd/test/run-find_route-specific.c +++ b/gossipd/test/run-find_route-specific.c @@ -41,7 +41,7 @@ const char *onion_type_name(int e UNNEEDED) bool replace_broadcast(const tal_t *ctx UNNEEDED, struct broadcast_state *bstate UNNEEDED, u64 *index UNNEEDED, - const u8 *payload UNNEEDED) + const u8 *payload TAKES UNNEEDED) { fprintf(stderr, "replace_broadcast called!\n"); abort(); } /* Generated stub for sanitize_error */ char *sanitize_error(const tal_t *ctx UNNEEDED, const u8 *errmsg UNNEEDED, diff --git a/gossipd/test/run-find_route.c b/gossipd/test/run-find_route.c index 522c4eeca..f896c78c2 100644 --- a/gossipd/test/run-find_route.c +++ b/gossipd/test/run-find_route.c @@ -39,7 +39,7 @@ const char *onion_type_name(int e UNNEEDED) bool replace_broadcast(const tal_t *ctx UNNEEDED, struct broadcast_state *bstate UNNEEDED, u64 *index UNNEEDED, - const u8 *payload UNNEEDED) + const u8 *payload TAKES UNNEEDED) { fprintf(stderr, "replace_broadcast called!\n"); abort(); } /* Generated stub for sanitize_error */ char *sanitize_error(const tal_t *ctx UNNEEDED, const u8 *errmsg UNNEEDED, diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index aeab89f66..fa692a24a 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -46,7 +46,7 @@ static void peer_nongossip(struct subd *gossip, const u8 *msg, tal_hex(msg, msg)); /* We already checked the features when it first connected. */ - if (unsupported_features(gfeatures, lfeatures)) { + if (!features_supported(gfeatures, lfeatures)) { log_unusual(gossip->log, "Gossip gave unsupported features %s/%s", tal_hex(msg, gfeatures), @@ -204,8 +204,8 @@ void gossip_init(struct lightningd *ld) msg = towire_gossipctl_init( tmpctx, ld->config.broadcast_interval, &get_chainparams(ld)->genesis_blockhash, &ld->id, ld->portnum, - get_supported_global_features(tmpctx), - get_supported_local_features(tmpctx), ld->wireaddrs, ld->rgb, + get_offered_global_features(tmpctx), + get_offered_local_features(tmpctx), ld->wireaddrs, ld->rgb, ld->alias, ld->config.channel_update_interval); subd_send_msg(ld->gossip, msg); tal_free(tmpctx); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f640576c1..777bac4a9 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -282,8 +282,8 @@ void peer_connected(struct lightningd *ld, const u8 *msg, struct crypto_state cs; u8 *gfeatures, *lfeatures; u8 *error; - u8 *supported_global_features; - u8 *supported_local_features; + u8 *global_features; + u8 *local_features; struct channel *channel; struct wireaddr addr; u64 gossip_index; @@ -295,22 +295,22 @@ void peer_connected(struct lightningd *ld, const u8 *msg, fatal("Gossip gave bad GOSSIP_PEER_CONNECTED message %s", tal_hex(msg, msg)); - if (unsupported_features(gfeatures, lfeatures)) { + if (!features_supported(gfeatures, lfeatures)) { log_unusual(ld->log, "peer %s offers unsupported features %s/%s", type_to_string(msg, struct pubkey, &id), tal_hex(msg, gfeatures), tal_hex(msg, lfeatures)); - supported_global_features = get_supported_global_features(msg); - supported_local_features = get_supported_local_features(msg); + global_features = get_offered_global_features(msg); + local_features = get_offered_local_features(msg); error = towire_errorfmt(msg, NULL, - "We only support globalfeatures %s" + "We only offer globalfeatures %s" " and localfeatures %s", tal_hexstr(msg, - supported_global_features, - tal_len(supported_global_features)), + global_features, + tal_len(global_features)), tal_hexstr(msg, - supported_local_features, - tal_len(supported_local_features))); + local_features, + tal_len(local_features))); goto send_error; } diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 035eec229..c02c30bfc 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -69,6 +69,9 @@ bool derive_basepoints(const struct privkey *seed UNNEEDED, /* Generated stub for extract_channel_id */ bool extract_channel_id(const u8 *in_pkt UNNEEDED, struct channel_id *channel_id UNNEEDED) { fprintf(stderr, "extract_channel_id called!\n"); abort(); } +/* Generated stub for features_supported */ +bool features_supported(const u8 *gfeatures UNNEEDED, const u8 *lfeatures UNNEEDED) +{ fprintf(stderr, "features_supported called!\n"); abort(); } /* Generated stub for fromwire_gossipctl_peer_disconnect_reply */ bool fromwire_gossipctl_peer_disconnect_reply(const void *p UNNEEDED) { fprintf(stderr, "fromwire_gossipctl_peer_disconnect_reply called!\n"); abort(); } @@ -90,12 +93,12 @@ enum watch_result funding_spent(struct channel *channel UNNEEDED, /* Generated stub for get_feerate */ u32 get_feerate(const struct chain_topology *topo UNNEEDED, enum feerate feerate UNNEEDED) { fprintf(stderr, "get_feerate called!\n"); abort(); } -/* Generated stub for get_supported_global_features */ -u8 *get_supported_global_features(const tal_t *ctx UNNEEDED) -{ fprintf(stderr, "get_supported_global_features called!\n"); abort(); } -/* Generated stub for get_supported_local_features */ -u8 *get_supported_local_features(const tal_t *ctx UNNEEDED) -{ fprintf(stderr, "get_supported_local_features called!\n"); abort(); } +/* Generated stub for get_offered_global_features */ +u8 *get_offered_global_features(const tal_t *ctx UNNEEDED) +{ fprintf(stderr, "get_offered_global_features called!\n"); abort(); } +/* Generated stub for get_offered_local_features */ +u8 *get_offered_local_features(const tal_t *ctx UNNEEDED) +{ fprintf(stderr, "get_offered_local_features called!\n"); abort(); } /* Generated stub for invoices_create */ bool invoices_create(struct invoices *invoices UNNEEDED, struct invoice *pinvoice UNNEEDED, @@ -356,9 +359,6 @@ u8 *towire_gossip_disable_channel(const tal_t *ctx UNNEEDED, const struct short_ /* Generated stub for towire_gossip_getpeers_request */ u8 *towire_gossip_getpeers_request(const tal_t *ctx UNNEEDED, const struct pubkey *id UNNEEDED) { fprintf(stderr, "towire_gossip_getpeers_request called!\n"); abort(); } -/* Generated stub for unsupported_features */ -bool unsupported_features(const u8 *gfeatures UNNEEDED, const u8 *lfeatures UNNEEDED) -{ fprintf(stderr, "unsupported_features called!\n"); abort(); } /* Generated stub for watch_txid */ struct txwatch *watch_txid(const tal_t *ctx UNNEEDED, struct chain_topology *topo UNNEEDED, diff --git a/wire/peer_wire.h b/wire/peer_wire.h index acd51791b..c2430ca2e 100644 --- a/wire/peer_wire.h +++ b/wire/peer_wire.h @@ -31,19 +31,6 @@ bool extract_channel_id(const u8 *in_pkt, struct channel_id *channel_id); */ #define CHANNEL_FLAGS_ANNOUNCE_CHANNEL 1 -/* BOLT #7: - * - * Upon establishing a connection, the two endpoints negotiate whether - * to perform an initial sync by setting the `initial_routing_sync` - * flags in the `init` message. The endpoint SHOULD set the - * `initial_routing_sync` flag if it requires a full copy of the other - * endpoint's routing state. Upon receiving an `init` message with the - * `initial_routing_sync` flag set the node sends `channel_announcement`s, - * `channel_update`s and `node_announcement`s for all known channels and - * nodes as if they were just received. - */ -#define LOCALFEATURES_INITIAL_ROUTING_SYNC 0x08 - /* BOLT #2: * * The sender MUST set `funding_satoshis` to less than 2^24 satoshi.