Fix "Bad commit_sig signature" error.

CHANGELOG.md

@@ -59,6 +59,7 @@ Note: You should always set `allow-deprecated-apis=false` to test for changes.
 
 ### Fixed
 
+- Fixed bogus "Bad commit_sig signature" which caused channel closures when reconnecting after updating fees under simultaneous bidirectional traffic.
 - Relative `--lightning_dir` is now working again.
 - Build: MacOS now builds again (missing pwritev).
 

lightningd/channel_control.c

@@ -742,3 +742,51 @@ struct command_result *cancel_channel_before_broadcast(struct command *cmd,
 			  notleak(cc));
 	return command_still_pending(cmd);
 }
+
+#if DEVELOPER
+static struct command_result *json_dev_feerate(struct command *cmd,
+					       const char *buffer,
+					       const jsmntok_t *obj UNNEEDED,
+					       const jsmntok_t *params)
+{
+	u32 *feerate;
+	struct node_id *id;
+	struct peer *peer;
+	struct json_stream *response;
+	struct channel *channel;
+	const u8 *msg;
+
+	if (!param(cmd, buffer, params,
+		   p_req("id", param_node_id, &id),
+		   p_req("feerate", param_number, &feerate),
+		   NULL))
+		return command_param_failed();
+
+	peer = peer_by_id(cmd->ld, id);
+	if (!peer)
+		return command_fail(cmd, LIGHTNINGD, "Peer not connected");
+
+	channel = peer_active_channel(peer);
+	if (!channel || !channel->owner || channel->state != CHANNELD_NORMAL)
+		return command_fail(cmd, LIGHTNINGD, "Peer bad state");
+
+	msg = towire_channel_feerates(NULL, *feerate,
+				      feerate_min(cmd->ld, NULL),
+				      feerate_max(cmd->ld, NULL));
+	subd_send_msg(channel->owner, take(msg));
+
+	response = json_stream_success(cmd);
+	json_add_node_id(response, "id", id);
+	json_add_u32(response, "feerate", *feerate);
+
+	return command_success(cmd, response);
+}
+
+static const struct json_command dev_feerate_command = {
+	"dev-feerate",
+	"developer",
+	json_dev_feerate,
+	"Set feerate for {id} to {feerate}"
+};
+AUTODATA(json_command, &dev_feerate_command);
+#endif /* DEVELOPER */

lightningd/peer_htlcs.c

@@ -1343,8 +1343,10 @@ void peer_sending_commitsig(struct channel *channel, const u8 *msg)
 		channel->next_htlc_id += num_local_added;
 	}
 
-	/* Update their feerate. */
-	channel->channel_info.feerate_per_kw[REMOTE] = feerate;
+	/* Update remote feerate if we are funder. */
+	if (channel->funder == LOCAL)
+		channel->channel_info.feerate_per_kw[REMOTE] = feerate;
+
 	if (feerate > channel->max_possible_feerate)
 		channel->max_possible_feerate = feerate;
 	if (feerate < channel->min_possible_feerate)
@@ -1546,12 +1548,13 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg)
 		}
 	}
 
-	/* Update both feerates: if we're funder, REMOTE should already be
-	 * that feerate, if we're not, we're about to ACK anyway. */
-	channel->channel_info.feerate_per_kw[LOCAL]
-		= channel->channel_info.feerate_per_kw[REMOTE]
-		= feerate;
-
+	/* Update both feerates if we're not funder (for funder, receiving
+	 * commitment_signed doesn't alter fees). */
+	if (channel->funder == REMOTE) {
+		channel->channel_info.feerate_per_kw[LOCAL]
+			= channel->channel_info.feerate_per_kw[REMOTE]
+			= feerate;
+	}
 	if (feerate > channel->max_possible_feerate)
 		channel->max_possible_feerate = feerate;
 	if (feerate < channel->min_possible_feerate)
@@ -1658,9 +1661,10 @@ void peer_got_revoke(struct channel *channel, const u8 *msg)
 		return;
 	}
 
-	/* Update feerate: if we are funder, their revoke_and_ack has set
+	/* Update feerate if we are funder, their revoke_and_ack has set
 	 * this for local feerate. */
-	channel->channel_info.feerate_per_kw[LOCAL] = feerate;
+	if (channel->funder == LOCAL)
+		channel->channel_info.feerate_per_kw[LOCAL] = feerate;
 
 	/* FIXME: Check per_commitment_secret -> per_commit_point */
 	update_per_commit_point(channel, &next_per_commitment_point);

tests/test_connection.py

@@ -2146,3 +2146,53 @@ def test_feerate_spam(node_factory, chainparams):
     # But it won't do it again once it's at max.
     with pytest.raises(TimeoutError):
         l1.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE', timeout=5)
+
+
+@unittest.skipIf(not DEVELOPER, "need dev-feerate")
+def test_feerate_stress(node_factory, executor):
+    # Third node makes HTLC traffic less predictable.
+    l1, l2, l3 = node_factory.line_graph(3, opts={'commit-time': 100,
+                                                  'may_reconnect': True})
+
+    l1.pay(l2, 10**9 // 2)
+    scid12 = l1.get_channel_scid(l2)
+    scid23 = l2.get_channel_scid(l3)
+
+    routel1l3 = [{'msatoshi': '10002msat', 'id': l2.info['id'], 'delay': 11, 'channel': scid12},
+                 {'msatoshi': '10000msat', 'id': l3.info['id'], 'delay': 5, 'channel': scid23}]
+    routel2l1 = [{'msatoshi': '10000msat', 'id': l1.info['id'], 'delay': 5, 'channel': scid12}]
+
+    rate = 1875
+    NUM_ATTEMPTS = 25
+    l1done = 0
+    l2done = 0
+    prev_log = 0
+    while l1done < NUM_ATTEMPTS and l2done < NUM_ATTEMPTS:
+        try:
+            r = random.randrange(6)
+            if r == 5:
+                l1.rpc.sendpay(routel1l3, "{:064x}".format(l1done))
+                l1done += 1
+            elif r == 4:
+                l2.rpc.sendpay(routel2l1, "{:064x}".format(l2done))
+                l2done += 1
+            elif r > 0:
+                l1.rpc.call('dev-feerate', [l2.info['id'], rate])
+                rate += 5
+            else:
+                l2.rpc.disconnect(l1.info['id'], True)
+                time.sleep(1)
+        except RpcError:
+            time.sleep(0.01)
+            assert not l1.daemon.is_in_log('Bad.*signature', start=prev_log)
+            prev_log = len(l1.daemon.logs)
+
+    # Make sure it's reconnected, and wait for last payment.
+    wait_for(lambda: l1.rpc.getpeer(l2.info['id'])['connected'])
+    with pytest.raises(RpcError, match='WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS'):
+        l1.rpc.waitsendpay("{:064x}".format(l1done - 1))
+    with pytest.raises(RpcError, match='WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS'):
+        l2.rpc.waitsendpay("{:064x}".format(l2done - 1))
+    l1.rpc.call('dev-feerate', [l2.info['id'], rate - 5])
+    assert not l1.daemon.is_in_log('Bad.*signature')
+    assert not l2.daemon.is_in_log('Bad.*signature')