db: Always fail HTLC inside a transaction.

This is important when we put payments in the database: they need to be
updated atomically as the HTLC is.

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

daemon/db.c

@@ -1407,8 +1407,7 @@ bool db_htlc_failed(struct peer *peer, const struct htlc *htlc)
 
 	log_debug(peer->log, "%s(%s)", __func__, peerid);
 
-	/* When called from their_htlc_added() we're routing a failure,
+	assert(peer->dstate->db->in_transaction);
-	 * we are in a transaction.  Otherwise, not. */
 	errmsg = db_exec(ctx, peer->dstate, 
 			 "UPDATE htlcs SET fail=x'%s' WHERE peer=x'%s' AND id=%"PRIu64" AND state='%s';",
 			 tal_hexstr(ctx, htlc->fail, sizeof(*htlc->fail)),

daemon/packets.c

@@ -444,7 +444,8 @@ static Pkt *find_commited_htlc(struct peer *peer, uint64_t id,
 	return NULL;
 }
 
-Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt, struct htlc **h)
+Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt, struct htlc **h,
+			  u8 **fail)
 {
 	const UpdateFailHtlc *f = pkt->update_fail_htlc;
 	Pkt *err;
@@ -457,12 +458,7 @@ Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt, struct htlc **h)
 		return pkt_err(peer, "HTLC %"PRIu64" already fulfilled",
 			       (*h)->id);
 
-	/* This can happen with re-transmissions; simply note it. */
+	*fail = tal_dup_arr(*h, u8, f->reason->info.data, f->reason->info.len,0);
-	if ((*h)->fail) {
-		log_debug(peer->log, "HTLC %"PRIu64" failed twice", (*h)->id);
-		(*h)->fail = tal_free((*h)->fail);
-	}
-	set_htlc_fail(peer, *h, f->reason->info.data, f->reason->info.len);
 	return NULL;
 }
 

daemon/packets.h

@@ -44,7 +44,8 @@ Pkt *accept_pkt_open_complete(struct peer *peer, const Pkt *pkt);
 	
 Pkt *accept_pkt_htlc_add(struct peer *peer, const Pkt *pkt, struct htlc **h);
 
-Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt, struct htlc **h);
+Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt, struct htlc **h,
+			  u8 **fail);
 
 Pkt *accept_pkt_htlc_fulfill(struct peer *peer, const Pkt *pkt, struct htlc **h,
 			     struct rval *r);

daemon/peer.c

@@ -411,7 +411,7 @@ static void set_htlc_rval(struct peer *peer,
 	db_htlc_fulfilled(peer, htlc);
 }
 
-void set_htlc_fail(struct peer *peer,
+static void set_htlc_fail(struct peer *peer,
 			  struct htlc *htlc, const void *fail, size_t len)
 {
 	assert(!htlc->r);
@@ -1112,12 +1112,28 @@ static Pkt *handle_pkt_htlc_add(struct peer *peer, const Pkt *pkt)
 static Pkt *handle_pkt_htlc_fail(struct peer *peer, const Pkt *pkt)
 {
 	struct htlc *htlc;
+	u8 *fail;
 	Pkt *err;
 
-	err = accept_pkt_htlc_fail(peer, pkt, &htlc);
+	err = accept_pkt_htlc_fail(peer, pkt, &htlc, &fail);
 	if (err)
 		return err;
 
+	/* This can happen with re-transmissions; simply note it. */
+	if (htlc->fail) {
+		log_debug(peer->log, "HTLC %"PRIu64" failed twice", htlc->id);
+		htlc->fail = tal_free(htlc->fail);
+	}
+	
+	if (!db_start_transaction(peer))
+		return pkt_err(peer, "database error");
+
+	set_htlc_fail(peer, htlc, fail, tal_count(fail));
+	tal_free(fail);
+
+	if (!db_commit_transaction(peer))
+		return pkt_err(peer, "database error");
+
 	cstate_fail_htlc(peer->local.staging_cstate, htlc);
 
 	/* BOLT #2:
@@ -2951,11 +2967,18 @@ static void check_htlc_expiry(struct peer *peer)
 		if (height <= abs_locktime_to_blocks(&h->expiry))
 			continue;
 
-		/* This can fail only if we're in an error state. */
+		if (!db_start_transaction(peer)) {
-		if (!command_htlc_set_fail(peer, h,
+			peer_fail(peer, __func__);
-					   REQUEST_TIMEOUT_408, "timed out"))
 			return;
 		}
+		/* This can fail only if we're in an error state. */
+		command_htlc_set_fail(peer, h,
+				      REQUEST_TIMEOUT_408, "timed out");
+		if (!db_commit_transaction(peer)) {
+			peer_fail(peer, __func__);
+			return;
+		}
+	}
 
 	/* BOLT #2:
 	 *
@@ -3196,8 +3219,11 @@ static const struct bitcoin_tx *irrevocably_resolved(struct peer *peer)
  * before we've confirmed it, this is exactly what happens. */
 static void fail_own_htlc(struct peer *peer, struct htlc *htlc)
 {
+	/* We can't do anything about db failures; peer already closed. */
+	db_start_transaction(peer);
 	set_htlc_fail(peer, htlc, "peer closed", strlen("peer closed"));
 	our_htlc_failed(peer, htlc);
+	db_commit_transaction(peer);
 }
 
 /* We've spent an HTLC output to get our funds back.  There's still a 
@@ -4580,8 +4606,19 @@ static void json_failhtlc(struct command *cmd,
 		return;
 	}
 
+	if (!db_start_transaction(peer)) {
+		command_fail(cmd, "database error");
+		return;
+	}
+
 	set_htlc_fail(peer, htlc, buffer + reasontok->start,
 		      reasontok->end - reasontok->start);
+
+	if (!db_commit_transaction(peer)) {
+		command_fail(cmd, "database error");
+		return;
+	}
+
 	if (command_htlc_fail(peer, htlc))
 		command_success(cmd, null_response(cmd));
 	else

daemon/peer.h

@@ -243,9 +243,6 @@ struct peer *new_peer(struct lightningd_state *dstate,
 		      enum state state,
 		      enum state_input offer_anchor);
 
-void set_htlc_fail(struct peer *peer,
-		   struct htlc *htlc, const void *fail, size_t fail_len);
-
 /* Populates very first peer->{local,remote}.commit->{tx,cstate} */
 bool setup_first_commit(struct peer *peer);