daemon: don't close with uncommitted changes.

It's undefined what happens: in our case, we disagree about the signature.
At very least, we shouldn't do this ourselves!

(Increasing commit timeout to exercise this also revealed a race in
the test, which an extra wait fixes).

Closes: #29
Reported-by: pm47
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Makefile

@@ -193,6 +193,7 @@ daemon-test-%:
 	NO_VALGRIND=$(NO_VALGRIND) daemon/test/test.sh --$*
 
 # These don't work in parallel, so chain the deps
+daemon-test-close-with-uncommitted: daemon-test-steal
 daemon-test-steal: daemon-test-dump-onchain
 daemon-test-dump-onchain: daemon-test-timeout-anchor
 daemon-test-timeout-anchor: daemon-test-different-fee-rates
@@ -201,7 +202,7 @@ daemon-test-normal: daemon-test-manual-commit
 daemon-test-manual-commit: daemon-test-mutual-close-with-htlcs
 daemon-test-mutual-close-with-htlcs: daemon-all
 
-daemon-tests: daemon-test-steal
+daemon-tests: daemon-test-close-with-uncommitted
 
 test-onion: test/test_onion test/onion_key
 	set -e; TMPF=/tmp/onion.$$$$; test/test_onion --generate $$(test/onion_key --pub `seq 20`) > $$TMPF; for k in `seq 20`; do test/test_onion --decode $$(test/onion_key --priv $$k) < $$TMPF > $$TMPF.unwrap; mv $$TMPF.unwrap $$TMPF; done; rm -f $$TMPF

daemon/peer.c

@@ -410,8 +410,12 @@ static bool clearing_pkt_in(struct peer *peer, const Pkt *pkt)
 
 static void peer_start_clearing(struct peer *peer)
 {
-	assert(peer->state == STATE_CLEARING
-	       || peer->state == STATE_CLEARING_COMMITTING);
+	if (peer->state == STATE_NORMAL)
+		set_peer_state(peer, STATE_CLEARING, __func__);
+	else {
+		assert(peer->state == STATE_NORMAL_COMMITTING);
+		set_peer_state(peer, STATE_CLEARING_COMMITTING, __func__);
+	}
 
 	/* If they started close, we might not have sent ours. */
 	if (!peer->closing.our_script) {
@@ -469,14 +473,6 @@ static bool normal_pkt_in(struct peer *peer, const Pkt *pkt)
 		err = accept_pkt_close_clearing(peer, pkt);
 		if (err)
 			break;
-		if (peer->state == STATE_NORMAL)
-			set_peer_state(peer, STATE_CLEARING, __func__);
-		else {
-			assert(peer->state == STATE_NORMAL_COMMITTING);
-			set_peer_state(peer, STATE_CLEARING_COMMITTING,
-				       __func__);
-		}
-
 		peer_start_clearing(peer);
 		return true;
 
@@ -871,19 +867,27 @@ static void peer_disconnect(struct io_conn *conn, struct peer *peer)
 	}
 }
 
-static void do_commit(struct peer *peer, struct command *jsoncmd)
+/* Returns true if nothing to commit, or commit sent. */
+static bool do_commit(struct peer *peer, struct command *jsoncmd)
 {
+	if (!state_can_commit(peer->state)) {
+		log_debug(peer->log, "do_commit: can't commit");
+		if (jsoncmd)
+			command_fail(jsoncmd, "peer in state %s",
+				     state_name(peer->state));
+		return false;
+	}
+
 	/* We can have changes we suggested, or changes they suggested. */
 	if (!peer_uncommitted_changes(peer)) {
 		log_debug(peer->log, "do_commit: no changes to commit");
 		if (jsoncmd)
 			command_fail(jsoncmd, "no changes to commit");
-		return;
+		return true;
 	}
 
 	log_debug(peer->log, "do_commit: sending commit command");
 
-	assert(state_can_commit(peer->state));
 	assert(!peer->commit_jsoncmd);
 
 	peer->commit_jsoncmd = jsoncmd;
@@ -894,15 +898,14 @@ static void do_commit(struct peer *peer, struct command *jsoncmd)
 		assert(peer->state == STATE_NORMAL);
 		set_peer_state(peer, STATE_NORMAL_COMMITTING, __func__);
 	}
+	return true;
 }
 
 static void try_commit(struct peer *peer)
 {
 	peer->commit_timer = NULL;
 
-	if (state_can_commit(peer->state))
-		do_commit(peer, NULL);
-	else {
+	if (!do_commit(peer, NULL)) {
 		/* FIXME: try again when we receive revocation, rather
 		 * than using timer! */
 		log_debug(peer->log, "try_commit: state=%s, re-queueing timer",
@@ -3189,11 +3192,6 @@ static void json_commit(struct command *cmd,
 		return;
 	}
 
-	if (!state_can_commit(peer->state)) {
-		command_fail(cmd, "peer in state %s", state_name(peer->state));
-		return;
-	}
-
 	do_commit(peer, cmd);
 }
 
@@ -3229,14 +3227,13 @@ static void json_close(struct command *cmd,
 		return;
 	}
 
-	if (peer->state == STATE_NORMAL_COMMITTING)
-		set_peer_state(peer, STATE_CLEARING_COMMITTING, __func__);
-	else
-		set_peer_state(peer, STATE_CLEARING, __func__);
+	/* We might have changes outstanding; if so, commit them now. */
+	do_commit(peer, NULL);
+
 	peer_start_clearing(peer);
 	command_success(cmd, null_response(cmd));
 }
-	
+
 const struct json_command close_command = {
 	"close",
 	json_close,

daemon/test/test.sh

@@ -56,6 +56,9 @@ while [ $# != 0 ]; do
 	x"--steal")
 	    STEAL=1
 	    ;;
+	x"--close-with-uncommitted")
+	    CLOSE_WITH_UNCOMMITTED=1
+	    ;;
 	x"--manual-commit")
 	    MANUALCOMMIT=1
 	    ;;
@@ -245,7 +248,7 @@ if [ -n "$MANUALCOMMIT" ]; then
     # Aka. never. 
     COMMIT_TIME=1h
 else
-    COMMIT_TIME=10ms
+    COMMIT_TIME=1s
 fi
 
 cat > $DIR1/config <<EOF
@@ -410,6 +413,7 @@ if [ -n "$DIFFERENT_FEES" ]; then
     [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
     [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
     check_status_single lcli2 0 0 "" $(($AMOUNT - $HTLC_AMOUNT - $ONE_HTLCS_FEE2)) $(($ONE_HTLCS_FEE2)) '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "block" : '$EXPIRY' }, "rhash" : "'$RHASH'" } '
+    check_status_single lcli1 $(($AMOUNT - $HTLC_AMOUNT - $ONE_HTLCS_FEE)) $(($ONE_HTLCS_FEE)) '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "block" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' 0 0 ""
     lcli2 fulfillhtlc $ID1 $SECRET
     [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
     [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
@@ -833,6 +837,10 @@ if [ ! -n "$MANUALCOMMIT" ]; then
     lcli3 close $ID2
 fi
 
+if [ -n "$CLOSE_WITH_UNCOMMITTED" ]; then
+    lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH3
+fi
+
 lcli1 close $ID2
 
 # They should be negotiating the close.