lightningd/channel: hand back changed htlcs, not callbacks.

Means caller has to do some more work, but this is closer to what we want:
we're going to want to send them to the master daemon for atomic commit.

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

lightningd/channel.c

@@ -627,31 +627,24 @@ static void htlc_incstate(struct channel *channel,
 	}
 }
 
-static void check_lockedin(const struct htlc *h,
+static void append_htlc(const struct htlc ***htlcs, const struct htlc *h)
-			   void (*oursfail)(const struct htlc *, void *),
-			   void (*theirslocked)(const struct htlc *, void *),
-			   void (*theirsfulfilled)(const struct htlc *, void *),
-			   void *cbarg)
 {
-	/* If it was fulfilled, we handled it immediately. */
+	size_t n;
-	if (h->state == RCVD_REMOVE_ACK_REVOCATION && !h->r)
+
-		oursfail(h, cbarg);
+	if (!htlcs)
-	else if (h->state == RCVD_ADD_ACK_REVOCATION)
+		return;
-		theirslocked(h, cbarg);
+
-	else if (h->state == RCVD_REMOVE_ACK_COMMIT && h->r)
+	n = tal_count(*htlcs);
-		theirsfulfilled(h, cbarg);
+	tal_resize(htlcs, n+1);
+	(*htlcs)[n] = h;
 }
 
-/* FIXME: Commit to storage when this happens. */
 /* Returns flags which were changed. */
 static int change_htlcs(struct channel *channel,
-			 enum side sidechanged,
+			enum side sidechanged,
-			 const enum htlc_state *htlc_states,
+			const enum htlc_state *htlc_states,
-			 size_t n_hstates,
+			size_t n_hstates,
-			 void (*oursfail)(const struct htlc *, void *),
+			const struct htlc ***htlcs)
-			 void (*theirslocked)(const struct htlc *, void *),
-			 void (*theirsfulfilled)(const struct htlc *, void *),
-			 void *cbarg)
 {
 	struct htlc_map_iter it;
 	struct htlc *h;
@@ -664,11 +657,7 @@ static int change_htlcs(struct channel *channel,
 		for (i = 0; i < n_hstates; i++) {
 			if (h->state == htlc_states[i]) {
 				htlc_incstate(channel, h, sidechanged);
-				check_lockedin(h,
+				append_htlc(htlcs, h);
-					       oursfail,
-					       theirslocked,
-					       theirsfulfilled,
-					       cbarg);
 				cflags |= (htlc_state_flags(htlc_states[i])
 					   ^ htlc_state_flags(h->state));
 			}
@@ -678,7 +667,8 @@ static int change_htlcs(struct channel *channel,
 }
 
 /* FIXME: Handle fee changes too. */
-bool channel_sending_commit(struct channel *channel)
+bool channel_sending_commit(struct channel *channel,
+			    const struct htlc ***htlcs)
 {
 	int change;
 	const enum htlc_state states[] = { SENT_ADD_HTLC,
@@ -687,16 +677,12 @@ bool channel_sending_commit(struct channel *channel)
 					   SENT_REMOVE_HTLC };
 	status_trace("Trying commit");
 	change = change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states),
-			      NULL, NULL, NULL, NULL);
+			      htlcs);
 	return change & HTLC_REMOTE_F_COMMITTED;
 }
 
-bool channel_rcvd_revoke_and_ack_(struct channel *channel,
+bool channel_rcvd_revoke_and_ack(struct channel *channel,
-				  void (*oursfail)(const struct htlc *htlc,
+				 const struct htlc ***htlcs)
-						   void *cbarg),
-				  void (*theirslocked)(const struct htlc *htlc,
-						       void *cbarg),
-				  void *cbarg)
 {
 	int change;
 	const enum htlc_state states[] = { SENT_ADD_COMMIT,
@@ -706,15 +692,12 @@ bool channel_rcvd_revoke_and_ack_(struct channel *channel,
 
 	status_trace("Received revoke_and_ack");
 	change = change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states),
-			      oursfail, theirslocked, NULL, cbarg);
+			      htlcs);
 	return change & HTLC_LOCAL_F_COMMITTED;
 }
 
 /* FIXME: We can actually merge these two... */
-bool channel_rcvd_commit_(struct channel *channel,
+bool channel_rcvd_commit(struct channel *channel, const struct htlc ***htlcs)
-			  void (*theirsfulfilled)(const struct htlc *htlc,
-						  void *cbarg),
-			  void *cbarg)
 {
 	int change;
 	const enum htlc_state states[] = { RCVD_ADD_REVOCATION,
@@ -723,8 +706,7 @@ bool channel_rcvd_commit_(struct channel *channel,
 					   RCVD_REMOVE_REVOCATION };
 
 	status_trace("Received commit");
-	change = change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states),
+	change = change_htlcs(channel, LOCAL, states, ARRAY_SIZE(states), htlcs);
-			      NULL, NULL, theirsfulfilled, cbarg);
 	return change & HTLC_LOCAL_F_COMMITTED;
 }
 
@@ -736,8 +718,7 @@ bool channel_sending_revoke_and_ack(struct channel *channel)
 					   RCVD_ADD_COMMIT,
 					   RCVD_REMOVE_ACK_COMMIT };
 	status_trace("Sending revoke_and_ack");
-	change = change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states),
+	change = change_htlcs(channel, REMOTE, states, ARRAY_SIZE(states), NULL);
-			      NULL, NULL, NULL, NULL);
 	return change & HTLC_REMOTE_F_PENDING;
 }
 

lightningd/channel.h

@@ -5,7 +5,6 @@
 #include <bitcoin/shadouble.h>
 #include <ccan/short_types/short_types.h>
 #include <ccan/tal/tal.h>
-#include <ccan/typesafe_cb/typesafe_cb.h>
 #include <daemon/htlc.h>
 #include <lightningd/channel_config.h>
 #include <lightningd/derive_basepoints.h>
@@ -324,63 +323,34 @@ bool force_fee(struct channel *channel, u64 fee);
 /**
  * channel_sending_commit: commit all remote outstanding changes.
  * @channel: the channel
+ * @htlcs: initially-empty tal_arr() for htlcs which changed state.
  *
  * This is where we commit to pending changes we've added; returns true if
  * anything changed for the remote side (if not, don't send!) */
-bool channel_sending_commit(struct channel *channel);
+bool channel_sending_commit(struct channel *channel,
+			    const struct htlc ***htlcs);
 
 /**
  * channel_rcvd_revoke_and_ack: accept ack on remote committed changes.
  * @channel: the channel
- * @oursfail: callback for any unfilfilled htlcs which are now fully removed.
+ * @htlcs: initially-empty tal_arr() for htlcs which changed state.
- * @theirslocked: callback for any new htlcs which are now fully committed.
- * @cbarg: argument to pass through to @ourhtlcfail  & @theirhtlclocked
  *
  * This is where we commit to pending changes we've added; returns true if
  * anything changed for our local commitment (ie. we have pending changes).
  */
-#define channel_rcvd_revoke_and_ack(channel, oursfail, theirslocked, cbarg) \
+bool channel_rcvd_revoke_and_ack(struct channel *channel,
-	channel_rcvd_revoke_and_ack_((channel),				\
+				 const struct htlc ***htlcs);
-				     typesafe_cb_preargs(void, void *,	\
-							 (oursfail),	\
-							 (cbarg),	\
-							 const struct htlc *), \
-				     typesafe_cb_preargs(void, void *,	\
-							 (theirslocked), \
-							 (cbarg),	\
-							 const struct htlc *), \
-				     (cbarg))
-
-bool channel_rcvd_revoke_and_ack_(struct channel *channel,
-				  void (*oursfail)(const struct htlc *htlc,
-						   void *cbarg),
-				  void (*theirslocked)(const struct htlc *htlc,
-						       void *cbarg),
-				  void *cbarg);
 
 /**
  * channel_rcvd_commit: commit all local outstanding changes.
  * @channel: the channel
- * @theirsfulfilled: they are irrevocably committed to removal of htlc.
+ * @htlcs: initially-empty tal_arr() for htlcs which changed state.
- * @cbarg: argument to pass through to @theirsfulfilled
  *
  * This is where we commit to pending changes we've added; returns true if
  * anything changed for our local commitment (ie. we had pending changes).
- * @theirsfulfilled is called for any HTLC we fulfilled which they are
- * irrevocably committed to, and is in our current commitment.
  */
-#define channel_rcvd_commit(channel, theirsfulfilled, cbarg)		\
+bool channel_rcvd_commit(struct channel *channel,
-	channel_rcvd_commit_((channel),					\
+			 const struct htlc ***htlcs);
-			     typesafe_cb_preargs(void, void *,		\
-						 (theirsfulfilled),	\
-						 (cbarg),		\
-						 const struct htlc *),	\
-			     (cbarg))
-
-bool channel_rcvd_commit_(struct channel *channel,
-			  void (*theirsfulfilled)(const struct htlc *htlc,
-						  void *cbarg),
-			  void *cbarg);
 
 /**
  * channel_sending_revoke_and_ack: sending ack on local committed changes.

lightningd/channel/channel.c

@@ -370,7 +370,7 @@ static void send_commit(struct peer *peer)
 	 * A node MUST NOT send a `commitment_signed` message which does not
 	 * include any updates.
 	 */
-	if (!channel_sending_commit(peer->channel)) {
+	if (!channel_sending_commit(peer->channel, NULL)) {
 		status_trace("Can't send commit: nothing to send");
 		tal_free(tmpctx);
 		return;
@@ -449,11 +449,6 @@ static void start_commit_timer(struct peer *peer)
 					  send_commit, peer);
 }
 
-static void theirs_fulfilled(const struct htlc *htlc, struct peer *peer)
-{
-	/* FIXME: Tell master, so it can disarm timer. */
-}
-
 static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 {
 	tal_t *tmpctx = tal_tmpctx(peer);
@@ -462,11 +457,12 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 	secp256k1_ecdsa_signature commit_sig, *htlc_sigs;
 	struct pubkey remotekey;
 	struct bitcoin_tx **txs;
-	const struct htlc **htlc_map;
+	const struct htlc **htlc_map, **changed_htlcs;
 	const u8 **wscripts;
 	size_t i;
 
-	if (!channel_rcvd_commit(peer->channel, theirs_fulfilled, peer)) {
+	changed_htlcs = tal_arr(msg, const struct htlc *, 0);
+	if (!channel_rcvd_commit(peer->channel, &changed_htlcs)) {
 		/* BOLT #2:
 		 *
 		 * A node MUST NOT send a `commitment_signed` message which
@@ -479,6 +475,8 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 			    "commit_sig with no changes");
 	}
 
+	/* FIXME: Tell master about HTLC changes. */
+
 	if (!fromwire_commitment_signed(tmpctx, msg, NULL,
 					&channel_id, &commit_sig, &htlc_sigs))
 		peer_failed(io_conn_fd(peer->peer_conn),
@@ -588,11 +586,6 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 	tal_free(tmpctx);
 }
 
-static void our_htlc_failed(const struct htlc *htlc, struct peer *peer)
-{
-	status_trace("FIXME: our htlc %"PRIu64" failed", htlc->id);
-}
-
 static void their_htlc_locked(const struct htlc *htlc, struct peer *peer)
 {
 	tal_t *tmpctx = tal_tmpctx(peer);
@@ -689,6 +682,7 @@ static void handle_peer_revoke_and_ack(struct peer *peer, const u8 *msg)
 	struct privkey privkey;
 	struct channel_id channel_id;
 	struct pubkey per_commit_point, next_per_commit;
+	const struct htlc **changed_htlcs = tal_arr(msg, const struct htlc *, 0);
 
 	if (!fromwire_revoke_and_ack(msg, NULL, &channel_id, &old_commit_secret,
 				     &next_per_commit)) {
@@ -747,13 +741,18 @@ static void handle_peer_revoke_and_ack(struct peer *peer, const u8 *msg)
 
 	/* We start timer even if this returns false: we might have delayed
 	 * commit because we were waiting for this! */
-	if (channel_rcvd_revoke_and_ack(peer->channel,
+	if (channel_rcvd_revoke_and_ack(peer->channel, &changed_htlcs))
-					our_htlc_failed, their_htlc_locked,
-					peer))
 		status_trace("Commits outstanding after recv revoke_and_ack");
 	else
 		status_trace("No commits outstanding after recv revoke_and_ack");
 
+	/* Tell master about locked-in htlcs. */
+	for (size_t i = 0; i < tal_count(changed_htlcs); i++) {
+		if (changed_htlcs[i]->state == RCVD_ADD_ACK_REVOCATION) {
+			their_htlc_locked(changed_htlcs[i], peer);
+		}
+	}
+
 	start_commit_timer(peer);
 }
 

lightningd/test/run-channel.c

@@ -82,10 +82,6 @@ static u64 feerates[] = {
 	9651936
 };
 
-static void do_nothing(const struct htlc *htlc, void *unused)
-{
-}
-
 /* BOLT #3:
  *
  *    htlc 0 direction: remote->local
@@ -113,6 +109,7 @@ static const struct htlc **include_htlcs(struct channel *channel, enum side side
 {
 	int i;
 	const struct htlc **htlcs = tal_arr(channel, const struct htlc *, 5);
+	const struct htlc **changed_htlcs;
 	u8 *dummy_routing = tal_arr(htlcs, u8, TOTAL_PACKET_SIZE);
 	bool ret;
 
@@ -155,17 +152,18 @@ static const struct htlc **include_htlcs(struct channel *channel, enum side side
 	tal_free(dummy_routing);
 
 	/* Now make HTLCs fully committed. */
-	ret = channel_sending_commit(channel);
+	changed_htlcs = tal_arr(htlcs, const struct htlc *, 0);
+	ret = channel_sending_commit(channel, &changed_htlcs);
 	assert(ret);
-	ret = channel_rcvd_revoke_and_ack(channel, NULL, NULL, NULL);
+	ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 	assert(!ret);
-	ret = channel_rcvd_commit(channel, NULL, NULL);
+	ret = channel_rcvd_commit(channel, &changed_htlcs);
 	assert(ret);
 	ret = channel_sending_revoke_and_ack(channel);
 	assert(ret);
-	ret = channel_sending_commit(channel);
+	ret = channel_sending_commit(channel, &changed_htlcs);
 	assert(ret);
-	ret = channel_rcvd_revoke_and_ack(channel, NULL, do_nothing, NULL);
+	ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 	assert(!ret);
 	return htlcs;
 }
@@ -239,6 +237,7 @@ static void send_and_fulfill_htlc(struct channel *channel,
 	struct sha256 rhash;
 	u8 *dummy_routing = tal_arr(channel, u8, TOTAL_PACKET_SIZE);
 	bool ret;
+	const struct htlc **changed_htlcs;
 
 	memset(&r, 0, sizeof(r));
 	sha256(&rhash, &r, sizeof(r));
@@ -246,45 +245,46 @@ static void send_and_fulfill_htlc(struct channel *channel,
 	assert(channel_add_htlc(channel, sender, 1337, msatoshi, 900, &rhash,
 				dummy_routing) == CHANNEL_ERR_ADD_OK);
 
+	changed_htlcs = tal_arr(channel, const struct htlc *, 0);
+
 	if (sender == LOCAL) {
 		/* Step through a complete cycle. */
-		ret = channel_sending_commit(channel);
+		ret = channel_sending_commit(channel, &changed_htlcs);
 		assert(ret);
-		ret = channel_rcvd_revoke_and_ack(channel, NULL, NULL, NULL);
+		ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 		assert(!ret);
-		ret = channel_rcvd_commit(channel, NULL, NULL);
+		ret = channel_rcvd_commit(channel, &changed_htlcs);
 		assert(ret);
 		ret = channel_sending_revoke_and_ack(channel);
 		assert(!ret);
 		assert(channel_fulfill_htlc(channel, LOCAL, 1337, &r)
 		       == CHANNEL_ERR_REMOVE_OK);
-		ret = channel_rcvd_commit(channel, NULL, NULL);
+		ret = channel_rcvd_commit(channel, &changed_htlcs);
 		assert(ret);
 		ret = channel_sending_revoke_and_ack(channel);
 		assert(ret);
-		ret = channel_sending_commit(channel);
+		ret = channel_sending_commit(channel, &changed_htlcs);
 		assert(ret);
-		ret = channel_rcvd_revoke_and_ack(channel, NULL, NULL, NULL);
+		ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 		assert(!ret);
 		assert(channel_get_htlc(channel, sender, 1337)->state
 		       == RCVD_REMOVE_ACK_REVOCATION);
 	} else {
-		ret = channel_rcvd_commit(channel, NULL, NULL);
+		ret = channel_rcvd_commit(channel, &changed_htlcs);
 		assert(ret);
 		ret = channel_sending_revoke_and_ack(channel);
 		assert(ret);
-		ret = channel_sending_commit(channel);
+		ret = channel_sending_commit(channel, &changed_htlcs);
 		assert(ret);
-		ret = channel_rcvd_revoke_and_ack(channel, NULL, do_nothing,
+		ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
-						  NULL);
 		assert(!ret);
 		assert(channel_fulfill_htlc(channel, REMOTE, 1337, &r)
 		       == CHANNEL_ERR_REMOVE_OK);
-		ret = channel_sending_commit(channel);
+		ret = channel_sending_commit(channel, &changed_htlcs);
 		assert(ret);
-		ret = channel_rcvd_revoke_and_ack(channel, NULL, NULL, NULL);
+		ret = channel_rcvd_revoke_and_ack(channel, &changed_htlcs);
 		assert(!ret);
-		ret = channel_rcvd_commit(channel, do_nothing, NULL);
+		ret = channel_rcvd_commit(channel, &changed_htlcs);
 		assert(ret);
 		ret = channel_sending_revoke_and_ack(channel);
 		assert(!ret);