lightningd/opening: receive details for remote config acceptance, not min/max.

The requirements for accepting the remote config are more complex than
a simple min/max value, as various parameters are related.  It turns
out that with a few assumptions, we can boil this down to:

1.  The valid feerate range.
2.  The minimum effective HTLC throughput we want
3.  The a maximum delay we'll accept for us to redeem.

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

lightningd/opening/opening.c

@@ -59,7 +59,11 @@ struct state {
 
 	/* Our shaseed for generating per-commitment-secrets. */
 	struct sha256 shaseed;
-	struct channel_config localconf, *remoteconf, minconf, maxconf;
+	struct channel_config localconf, *remoteconf;
+
+	/* Limits on what remote config we accept */
+	u32 max_to_self_delay;
+	u64 min_effective_htlc_capacity_msat;
 
 	struct channel *channel;
 };
@@ -120,55 +124,94 @@ static void derive_our_basepoints(const struct privkey *seed,
 					     &per_commit_secret));
 }
 
-/* Yes, this multi-evaluates, and isn't do-while wrapped. */
-#define test_config_inrange(conf, min, max, field, fmt)			\
-	if ((conf)->field < (min)->field || (conf)->field > (max)->field) \
-		peer_failed(PEER_FD, &state->cs, NULL, WIRE_OPENING_PEER_BAD_CONFIG, \
-			      #field " %"fmt" too large (%"fmt"-%"fmt")", \
-			      (conf)->field, (min)->field, (max)->field)
-
-#define test_config_inrange_u64(conf, min, max, field)	\
-	test_config_inrange(conf, min, max, field, PRIu64)
-#define test_config_inrange_u32(conf, min, max, field)	\
-	test_config_inrange(conf, min, max, field, "u")
-#define test_config_inrange_u16(conf, min, max, field)	\
-	test_config_inrange(conf, min, max, field, "u")
-
 static void check_config_bounds(struct state *state,
-				const struct channel_config *remoteconf,
+				const struct channel_config *remoteconf)
-				const struct channel_config *minc,
-				const struct channel_config *maxc)
 {
-	/* BOLT #2:
+	u64 capacity_msat;
-	 *
+	u64 reserve_msat;
-	 * It MUST fail the channel if `max-accepted-htlcs` is greater than
-	 * 511.
-	 */
-	if (maxc->max_accepted_htlcs > 511)
-		peer_failed(PEER_FD, &state->cs, NULL, WIRE_OPENING_BAD_PARAM,
-			      "max->max_accepted_htlcs %u too large",
-			      maxc->max_accepted_htlcs);
 
 	/* BOLT #2:
 	 *
 	 * The receiving node MUST fail the channel if `to-self-delay` is
-	 * unreasonably large.  The receiver MAY fail the channel if
+	 * unreasonably large.
-	 * `funding-satoshis` is too small....  The receiving node MAY fail
+	 */
-	 * the channel if it considers `htlc-minimum-msat` too large,
+	if (remoteconf->to_self_delay > state->max_to_self_delay)
+		peer_failed(PEER_FD, &state->cs, NULL,
+			    WIRE_OPENING_PEER_BAD_CONFIG,
+			    "to_self_delay %u larger than %u",
+			    remoteconf->to_self_delay, state->max_to_self_delay);
+
+	/* BOLT #2:
+	 *
+	 * The receiver MAY fail the channel if `funding-satoshis` is too
+	 * small, and MUST fail the channel if `push-msat` is greater than
+	 * `funding-amount` * 1000.  The receiving node MAY fail the channel
+	 * if it considers `htlc-minimum-msat` too large,
 	 * `max-htlc-value-in-flight` too small, `channel-reserve-satoshis`
 	 * too large, or `max-accepted-htlcs` too small.
-	 *
-	 * The receiver MUST fail the channel if it considers `feerate-per-kw`
-	 * too small for timely processing, or unreasonably large.
 	 */
-	/* We simply compare every field, and let the master daemon sort out
+	/* We accumulate this into an effective bandwidth minimum. */
-	   the bounds. */
+
-	test_config_inrange_u64(remoteconf, minc, maxc, dust_limit_satoshis);
+	/* Overflow check before capacity calc. */
-	test_config_inrange_u64(remoteconf, minc, maxc, channel_reserve_satoshis);
+	if (remoteconf->channel_reserve_satoshis > state->funding_satoshis)
-	test_config_inrange_u32(remoteconf, minc, maxc, minimum_depth);
+		peer_failed(PEER_FD, &state->cs, NULL,
-	test_config_inrange_u32(remoteconf, minc, maxc, htlc_minimum_msat);
+			    WIRE_OPENING_PEER_BAD_CONFIG,
-	test_config_inrange_u16(remoteconf, minc, maxc, to_self_delay);
+			    "Invalid channel_reserve_satoshis %"PRIu64
-	test_config_inrange_u16(remoteconf, minc, maxc, max_accepted_htlcs);
+			    " for funding_satoshis %"PRIu64,
+			    remoteconf->channel_reserve_satoshis,
+			    state->funding_satoshis);
+
+	/* Consider highest reserve. */
+	reserve_msat = remoteconf->channel_reserve_satoshis * 1000;
+	if (state->localconf.channel_reserve_satoshis * 1000 > reserve_msat)
+		reserve_msat = state->localconf.channel_reserve_satoshis * 1000;
+
+	capacity_msat = state->funding_satoshis * 1000 - reserve_msat;
+
+	if (remoteconf->max_htlc_value_in_flight_msat < capacity_msat)
+		capacity_msat = remoteconf->max_htlc_value_in_flight_msat;
+
+	if (remoteconf->htlc_minimum_msat * (u64)1000 > capacity_msat)
+		peer_failed(PEER_FD, &state->cs, NULL,
+			    WIRE_OPENING_PEER_BAD_CONFIG,
+			    "Invalid htlc_minimum_msat %u"
+			    " for funding_satoshis %"PRIu64
+			    " capacity_msat %"PRIu64,
+			    remoteconf->htlc_minimum_msat,
+			    state->funding_satoshis,
+			    capacity_msat);
+
+	if (capacity_msat < state->min_effective_htlc_capacity_msat)
+		peer_failed(PEER_FD, &state->cs, NULL,
+			    WIRE_OPENING_PEER_BAD_CONFIG,
+			    "Channel capacity with funding %"PRIu64" msat,"
+			    " reserves %"PRIu64"/%"PRIu64" msat,"
+			    " max_htlc_value_in_flight_msat %"PRIu64
+			    " is %"PRIu64" msat, which is below %"PRIu64" msat",
+			    state->funding_satoshis * 1000,
+			    remoteconf->channel_reserve_satoshis * 1000,
+			    state->localconf.channel_reserve_satoshis * 1000,
+			    remoteconf->max_htlc_value_in_flight_msat,
+			    capacity_msat,
+			    state->min_effective_htlc_capacity_msat);
+
+	/* We don't worry about how many HTLCs they accept, as long as > 0! */
+	if (remoteconf->max_accepted_htlcs == 0)
+		peer_failed(PEER_FD, &state->cs, NULL,
+			    WIRE_OPENING_PEER_BAD_CONFIG,
+			    "max_accepted_htlcs %u invalid",
+			    remoteconf->max_accepted_htlcs);
+
+	/* BOLT #2:
+	 *
+	 * It MUST fail the channel if `max-accepted-htlcs` is greater
+	 * than 511.
+	 */
+	if (remoteconf->max_accepted_htlcs > 511)
+		peer_failed(PEER_FD, &state->cs, NULL,
+			    WIRE_OPENING_PEER_BAD_CONFIG,
+			    "max_accepted_htlcs %u too large",
+			    remoteconf->max_accepted_htlcs);
 }
 
 static bool check_commit_sig(const struct state *state,
@@ -207,7 +250,14 @@ sign_remote_commit(const struct state *state,
 	return sig;
 }
 
-static void open_channel(struct state *state, const struct points *ours)
+/* We always set channel_reserve_satoshis to 1%, rounded up. */
+static void set_reserve(u64 *reserve, u64 funding)
+{
+	*reserve = (funding + 99) / 100;
+}
+
+static void open_channel(struct state *state, const struct points *ours,
+			 u32 max_minimum_depth)
 {
 	struct channel_id tmpid, tmpid2;
 	u8 *msg;
@@ -215,6 +265,9 @@ static void open_channel(struct state *state, const struct points *ours)
 	struct points theirs;
 	secp256k1_ecdsa_signature sig;
 
+	set_reserve(&state->localconf.channel_reserve_satoshis,
+		    state->funding_satoshis);
+
 	/* BOLT #2:
 	 *
 	 * A sending node MUST set the most significant bit in
@@ -279,8 +332,8 @@ static void open_channel(struct state *state, const struct points *ours)
 					->max_htlc_value_in_flight_msat,
 				     &state->remoteconf
 					->channel_reserve_satoshis,
+				     &state->remoteconf->minimum_depth,
 				     &state->remoteconf->htlc_minimum_msat,
-				     &state->feerate_per_kw,
 				     &state->remoteconf->to_self_delay,
 				     &state->remoteconf->max_accepted_htlcs,
 				     &theirs.funding_pubkey,
@@ -301,8 +354,19 @@ static void open_channel(struct state *state, const struct points *ours)
 			      type_to_string(msg, struct channel_id, &tmpid),
 			      type_to_string(msg, struct channel_id, &tmpid2));
 
-	check_config_bounds(state, state->remoteconf,
+	/* BOLT #2:
-			    &state->minconf, &state->maxconf);
+	 *
+	 * The receiver MAY reject the `minimum-depth` if it considers it
+	 * unreasonably large.
+	 *
+	 * Other fields have the same requirements as their counterparts in
+	 * `open_channel`.
+	 */
+	if (state->remoteconf->minimum_depth > max_minimum_depth)
+		peer_failed(PEER_FD, &state->cs, NULL, WIRE_OPENING_BAD_PARAM,
+			    "minimum_depth %u larger than %u",
+			    state->remoteconf->minimum_depth, max_minimum_depth);
+	check_config_bounds(state, state->remoteconf);
 
 	/* Now, ask master create a transaction to pay those two addresses. */
 	msg = towire_opening_open_resp(state, &ours->funding_pubkey,
@@ -419,7 +483,7 @@ static void open_channel(struct state *state, const struct points *ours)
 /* This is handed the message the peer sent which caused gossip to stop:
  * it should be an open_channel */
 static void recv_channel(struct state *state, const struct points *ours,
-			 const u8 *peer_msg)
+			 u32 min_feerate, u32 max_feerate, const u8 *peer_msg)
 {
 	struct channel_id tmpid, tmpid2;
 	struct points theirs;
@@ -474,16 +538,32 @@ static void recv_channel(struct state *state, const struct points *ours,
 			      " too large for funding_satoshis %"PRIu64,
 			      state->push_msat, state->funding_satoshis);
 
-	check_config_bounds(state, state->remoteconf,
+	/* BOLT #3:
-			    &state->minconf, &state->maxconf);
+	 *
+	 * The receiver MUST fail the channel if it considers `feerate-per-kw`
+	 * too small for timely processing, or unreasonably large.
+	 */
+	if (state->feerate_per_kw < min_feerate)
+		peer_failed(PEER_FD, &state->cs, NULL, WIRE_OPENING_PEER_BAD_FUNDING,
+			    "feerate_per_kw %u below minimum %u",
+			    state->feerate_per_kw, min_feerate);
+
+	if (state->feerate_per_kw > max_feerate)
+		peer_failed(PEER_FD, &state->cs, NULL, WIRE_OPENING_PEER_BAD_FUNDING,
+			    "feerate_per_kw %u above maximum %u",
+			    state->feerate_per_kw, max_feerate);
+
+	set_reserve(&state->localconf.channel_reserve_satoshis,
+		    state->funding_satoshis);
+	check_config_bounds(state, state->remoteconf);
 
 	msg = towire_accept_channel(state, &tmpid,
 				    state->localconf.dust_limit_satoshis,
 				    state->localconf
 				      .max_htlc_value_in_flight_msat,
 				    state->localconf.channel_reserve_satoshis,
+				    state->localconf.minimum_depth,
 				    state->localconf.htlc_minimum_msat,
-				    state->feerate_per_kw,
 				    state->localconf.to_self_delay,
 				    state->localconf.max_accepted_htlcs,
 				    &ours->funding_pubkey,
@@ -595,6 +675,8 @@ int main(int argc, char *argv[])
 	struct state *state = tal(NULL, struct state);
 	struct privkey seed;
 	struct points our_points;
+	u32 max_minimum_depth;
+	u32 min_feerate, max_feerate;
 
 	if (argc == 2 && streq(argv[1], "--version")) {
 		printf("%s\n", version());
@@ -615,8 +697,8 @@ int main(int argc, char *argv[])
 
 	if (!fromwire_opening_init(msg, NULL,
 				   &state->localconf,
-				   &state->minconf,
+				   &state->max_to_self_delay,
-				   &state->maxconf,
+				   &state->min_effective_htlc_capacity_msat,
 				   &state->cs,
 				   &seed))
 		status_failed(WIRE_OPENING_BAD_COMMAND, "%s", strerror(errno));
@@ -630,10 +712,12 @@ int main(int argc, char *argv[])
 	if (fromwire_opening_open(msg, NULL,
 				  &state->funding_satoshis,
 				  &state->push_msat,
-				  &state->feerate_per_kw))
+				  &state->feerate_per_kw, &max_minimum_depth))
-		open_channel(state, &our_points);
+		open_channel(state, &our_points, max_minimum_depth);
-	else if (fromwire_opening_accept(state, msg, NULL, &peer_msg))
+	else if (fromwire_opening_accept(state, msg, NULL, &min_feerate,
-		recv_channel(state, &our_points, peer_msg);
+					 &max_feerate, &peer_msg))
+		recv_channel(state, &our_points, min_feerate, max_feerate,
+			     peer_msg);
 
 	/* Hand back the fd. */
 	fdpass_send(REQ_FD, PEER_FD);

lightningd/opening/opening_control_wire.csv

@@ -1,20 +1,21 @@
 #include <lightningd/cryptomsg.h>
 #include <lightningd/channel_config.h>
 opening_init,0
-# What configuration we'll offer
+# Base configuration we'll offer (channel reserve will vary with amount)
 opening_init,0,our_config,36,struct channel_config
 # Minimum/maximum configuration values we'll accept
-opening_init,36,min_config,36,struct channel_config
+opening_init,36,max_to_self_delay,4
-opening_init,72,max_config,36,struct channel_config
+opening_init,40,min_effective_htlc_capacity_msat,8
-opening_init,108,crypto_state,144,struct crypto_state
+opening_init,48,crypto_state,144,struct crypto_state
 # Seed to generate all the keys from
-opening_init,252,seed,32,struct privkey
+opening_init,196,seed,32,struct privkey
 
 # This means we offer the open.
 opening_open,1
 opening_open,0,funding_satoshis,8
 opening_open,8,push_msat,8
 opening_open,16,feerate_per_kw,4
+opening_open,20,max_minimum_depth,4
 
 # Response asks for txid of funding transaction.
 opening_open_resp,101
@@ -38,8 +39,10 @@ opening_open_funding_resp,343,their_per_commit_point,33
 
 # This means they offer the open (contains their offer packet)
 opening_accept,3
-opening_accept,0,len,2
+opening_accept,0,min_feerate,4
-opening_accept,2,msg,len,u8
+opening_accept,4,max_feerate,4
+opening_accept,8,len,2
+opening_accept,10,msg,len,u8
 
 # This gives the txid of their funding tx: we're done.
 opening_accept_resp,103