state: return Pkt to be queued directly.

Instead of effect->send_pkt.

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

state.c

@@ -84,10 +84,18 @@ static void complete_cmd(struct peer *peer, enum command_status *statusp,
 	*statusp = status;
 }
 
+static void queue_pkt(Pkt **out, Pkt *pkt)
+{
+	assert(!*out);
+	assert(pkt);
+	*out = pkt;
+}
+
 enum command_status state(const tal_t *ctx,
 			  struct peer *peer,
 			  const enum state_input input,
 			  const union input *idata,
+			  Pkt **out,
 			  struct state_effect **effect)
 {
 	Pkt *decline;
@@ -96,6 +104,8 @@ enum command_status state(const tal_t *ctx,
 	struct htlc_watch *htlcs;
 	enum command_status cstatus = CMD_NONE;
 
+	*out = NULL;
+
 	/* NULL-terminated linked list. */
 	*effect = NULL;
 
@@ -105,7 +115,7 @@ enum command_status state(const tal_t *ctx,
 	 */
 	case STATE_INIT:
 		if (input_is(input, CMD_OPEN_WITH_ANCHOR)) {
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_open(ctx, peer,
 					    OPEN_CHANNEL__ANCHOR_OFFER__WILL_CREATE_ANCHOR));
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
@@ -113,7 +123,7 @@ enum command_status state(const tal_t *ctx,
 					  STATE_OPEN_WAIT_FOR_OPEN_WITHANCHOR);
 		} else if (input_is(input, CMD_OPEN_WITHOUT_ANCHOR)) {
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_open(ctx, peer,
 					    OPEN_CHANNEL__ANCHOR_OFFER__WONT_CREATE_ANCHOR));
 			return next_state(peer, cstatus,
@@ -143,7 +153,7 @@ enum command_status state(const tal_t *ctx,
 				complete_cmd(peer, &cstatus, CMD_FAIL);
 				goto err_close_nocleanup;
 			}
-			add_effect(effect, send_pkt, pkt_anchor(ctx, peer));
+			queue_pkt(out, pkt_anchor(ctx, peer));
 			return next_state(peer, cstatus,
 					  STATE_OPEN_WAIT_FOR_COMMIT_SIG);
 		} else if (input_is(input, CMD_CLOSE)) {
@@ -161,7 +171,7 @@ enum command_status state(const tal_t *ctx,
 				complete_cmd(peer, &cstatus, CMD_FAIL);
 				goto err_close_nocleanup;
 			}
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_open_commit_sig(ctx, peer));
 			add_effect(effect, watch,
 				   bitcoin_watch_anchor(ctx, peer,
@@ -216,7 +226,7 @@ enum command_status state(const tal_t *ctx,
 	/* Fall thru */
 	case STATE_OPEN_WAITING_OURANCHOR_THEYCOMPLETED:
 		if (input_is(input, BITCOIN_ANCHOR_DEPTHOK)) {
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_open_complete(ctx, peer));
 			if (peer->state == STATE_OPEN_WAITING_OURANCHOR_THEYCOMPLETED) {
 				complete_cmd(peer, &cstatus, CMD_SUCCESS);
@@ -274,11 +284,11 @@ enum command_status state(const tal_t *ctx,
 	case STATE_OPEN_WAITING_THEIRANCHOR_THEYCOMPLETED:
 		if (input_is(input, BITCOIN_ANCHOR_TIMEOUT)) {
 			/* Anchor didn't reach blockchain in reasonable time. */
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_err(ctx, "Anchor timed out"));
 			return next_state(peer, cstatus, STATE_ERR_ANCHOR_TIMEOUT);
 		} else if (input_is(input, BITCOIN_ANCHOR_DEPTHOK)) {
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_open_complete(ctx, peer));
 			if (peer->state == STATE_OPEN_WAITING_THEIRANCHOR_THEYCOMPLETED) {
 				complete_cmd(peer, &cstatus, CMD_SUCCESS);
@@ -372,7 +382,7 @@ enum command_status state(const tal_t *ctx,
 		assert(peer->cond == PEER_CMD_OK);
 		if (input_is(input, CMD_SEND_HTLC_UPDATE)) {
 			/* We are to send an HTLC update. */
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_htlc_update(ctx, peer,
 						   idata->htlc_prog));
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
@@ -380,7 +390,7 @@ enum command_status state(const tal_t *ctx,
 					  prio(peer->state, STATE_WAIT_FOR_HTLC_ACCEPT));
 		} else if (input_is(input, CMD_SEND_HTLC_FULFILL)) {
 			/* We are to send an HTLC fulfill. */
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_htlc_fulfill(ctx, peer,
 						    idata->htlc_prog));
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
@@ -388,7 +398,7 @@ enum command_status state(const tal_t *ctx,
 					  prio(peer->state, STATE_WAIT_FOR_UPDATE_ACCEPT));
 		} else if (input_is(input, CMD_SEND_HTLC_TIMEDOUT)) {
 			/* We are to send an HTLC timedout. */
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_htlc_timedout(ctx, peer,
 						     idata->htlc_prog));
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
@@ -396,7 +406,7 @@ enum command_status state(const tal_t *ctx,
 					  prio(peer->state, STATE_WAIT_FOR_UPDATE_ACCEPT));
 		} else if (input_is(input, CMD_SEND_HTLC_ROUTEFAIL)) {
 			/* We are to send an HTLC routefail. */
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_htlc_routefail(ctx, peer,
 						      idata->htlc_prog));
 			change_peer_cond(peer, PEER_CMD_OK, PEER_BUSY);
@@ -492,7 +502,7 @@ enum command_status state(const tal_t *ctx,
 				complete_cmd(peer, &cstatus, CMD_FAIL);
 				goto err_start_unilateral_close;
 			}
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_update_signature(ctx, peer));
 			/* HTLC is signed (though old tx not revoked yet!) */
 			return next_state(peer, cstatus,
@@ -570,7 +580,7 @@ enum command_status state(const tal_t *ctx,
 				peer_htlc_aborted(peer);
 				goto err_start_unilateral_close;
 			}
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_update_complete(ctx, peer));
 			
 			peer_htlc_done(peer);
@@ -605,7 +615,7 @@ enum command_status state(const tal_t *ctx,
 							effect);
 			if (err)
 				goto err_start_unilateral_close_already_closing;
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_close_ack(ctx, peer));
 			add_effect(effect, broadcast_tx,
 				   bitcoin_close(ctx, peer));
@@ -618,7 +628,7 @@ enum command_status state(const tal_t *ctx,
 							    effect);
 			if (err)
 				goto err_start_unilateral_close_already_closing;
-			add_effect(effect, send_pkt,
+			queue_pkt(out,
 				   pkt_close_ack(ctx, peer));
 			add_effect(effect, broadcast_tx,
 				   bitcoin_close(ctx, peer));
@@ -642,7 +652,7 @@ enum command_status state(const tal_t *ctx,
 			err = accept_pkt_close_ack(ctx, peer, idata->pkt,
 						   effect);
 			if (err)
-				add_effect(effect, send_pkt, err);
+				queue_pkt(out, err);
 			set_peer_cond(peer, PEER_CLOSED);
 			/* Just wait for close to happen now. */
 			return next_state(peer, cstatus, STATE_CLOSE_WAIT_CLOSE);
@@ -650,7 +660,7 @@ enum command_status state(const tal_t *ctx,
 			peer_unexpected_pkt(peer, idata->pkt);
 			/* Don't reply to an error with an error. */
 			if (!input_is(input, PKT_ERROR)) {
-				add_effect(effect, send_pkt,
+				queue_pkt(out,
 					   pkt_err_unexpected(ctx, idata->pkt));
 			}
 			set_peer_cond(peer, PEER_CLOSED);
@@ -944,7 +954,7 @@ err_close_nocleanup:
 	 * Something went wrong, but we haven't sent anything to the blockchain
 	 * so there's nothing to clean up.
 	 */
-	add_effect(effect, send_pkt, err);
+	queue_pkt(out, err);
 
 close_nocleanup:
 	change_peer_cond(peer, PEER_CMD_OK, PEER_CLOSED);
@@ -954,7 +964,7 @@ err_start_unilateral_close:
 	/*
 	 * They timed out, or were broken; we are going to close unilaterally.
 	 */
-	add_effect(effect, send_pkt, err);
+	queue_pkt(out, err);
 
 start_unilateral_close:
 	/*
@@ -984,7 +994,7 @@ err_start_unilateral_close_already_closing:
 	/*
 	 * They timed out, or were broken; we are going to close unilaterally.
 	 */
-	add_effect(effect, send_pkt, err);
+	queue_pkt(out, err);
 
 start_unilateral_close_already_closing:
 	/*
@@ -1010,7 +1020,7 @@ them_unilateral:
 	/*
 	 * Bitcoind tells us they did unilateral close.
 	 */
-	add_effect(effect, send_pkt, pkt_err(ctx, "Commit tx noticed"));
+	queue_pkt(out, pkt_err(ctx, "Commit tx noticed"));
 
 	/* No more inputs, no more commands. */
 	set_peer_cond(peer, PEER_CLOSED);
@@ -1038,7 +1048,7 @@ accept_htlc_update:
 	if (err)
 		goto err_start_unilateral_close;
 	if (decline) {
-		add_effect(effect, send_pkt, decline);
+		queue_pkt(out, decline);
 		peer_htlc_declined(peer, decline);
 		/* No update means no priority change. */
 		change_peer_cond(peer, PEER_BUSY, PEER_CMD_OK);
@@ -1046,7 +1056,7 @@ accept_htlc_update:
 		return next_state_nocheck(peer, cstatus,
 					  prio(peer->state, STATE_NORMAL));
 	}
-	add_effect(effect, send_pkt, pkt_update_accept(ctx, peer));
+	queue_pkt(out, pkt_update_accept(ctx, peer));
 	return next_state(peer, cstatus,
 			  prio(peer->state, STATE_WAIT_FOR_UPDATE_SIG));
 
@@ -1054,7 +1064,7 @@ accept_htlc_routefail:
 	err = accept_pkt_htlc_routefail(ctx, peer, idata->pkt, effect);
 	if (err)
 		goto err_start_unilateral_close;
-	add_effect(effect, send_pkt, pkt_update_accept(ctx, peer));
+	queue_pkt(out, pkt_update_accept(ctx, peer));
 	return next_state(peer, cstatus,
 			  prio(peer->state, STATE_WAIT_FOR_UPDATE_SIG));
 
@@ -1062,7 +1072,7 @@ accept_htlc_timedout:
 	err = accept_pkt_htlc_timedout(ctx, peer, idata->pkt, effect);
 	if (err)
 		goto err_start_unilateral_close;
-	add_effect(effect, send_pkt, pkt_update_accept(ctx, peer));
+	queue_pkt(out, pkt_update_accept(ctx, peer));
 	return next_state(peer, cstatus,
 			  prio(peer->state, STATE_WAIT_FOR_UPDATE_SIG));
 
@@ -1070,7 +1080,7 @@ accept_htlc_fulfill:
 	err = accept_pkt_htlc_fulfill(ctx, peer, idata->pkt);
 	if (err)
 		goto err_start_unilateral_close;
-	add_effect(effect, send_pkt, pkt_update_accept(ctx, peer));
+	queue_pkt(out, pkt_update_accept(ctx, peer));
 	return next_state(peer, cstatus,
 			  prio(peer->state, STATE_WAIT_FOR_UPDATE_SIG));
 
@@ -1092,7 +1102,7 @@ start_closing:
 	set_peer_cond(peer, PEER_CLOSING);
 
 	/* As soon as we send packet, they could close. */
-	add_effect(effect, send_pkt, pkt_close(ctx, peer));
+	queue_pkt(out, pkt_close(ctx, peer));
 	return next_state(peer, cstatus, STATE_WAIT_FOR_CLOSE_COMPLETE);
 
 accept_closing:
@@ -1102,7 +1112,7 @@ accept_closing:
 	/* As soon as we send packet, they could close. */
 	add_effect(effect, watch,
 		   bitcoin_watch_close(ctx, peer, BITCOIN_CLOSE_DONE));
-	add_effect(effect, send_pkt, pkt_close_complete(ctx, peer));
+	queue_pkt(out, pkt_close_complete(ctx, peer));
 	/* No more commands, we're already closing. */
 	set_peer_cond(peer, PEER_CLOSING);
 	return next_state(peer, cstatus, STATE_WAIT_FOR_CLOSE_ACK);
@@ -1171,7 +1181,7 @@ old_commit_spotted:
 	/*
 	 * bitcoind reported a broadcast of the not-latest commit tx.
 	 */
-	add_effect(effect, send_pkt, pkt_err(ctx, "Otherspend noticed"));
+	queue_pkt(out, pkt_err(ctx, "Otherspend noticed"));
 
 	/* No more packets, no more commands. */
 	set_peer_cond(peer, PEER_CLOSED);

state.h

@@ -8,7 +8,6 @@
 
 enum state_effect_type {
 	STATE_EFFECT_broadcast_tx,
-	STATE_EFFECT_send_pkt,
 	STATE_EFFECT_watch,
 	STATE_EFFECT_unwatch,
 	/* FIXME: Use a watch for this?. */
@@ -34,9 +33,6 @@ struct state_effect {
 		/* Transaction to broadcast. */
 		struct bitcoin_tx *broadcast_tx;
 
-		/* Packet to send. */
-		Pkt *send_pkt;
-
 		/* Event to watch for. */
 		struct watch *watch;
 
@@ -84,6 +80,7 @@ enum command_status state(const tal_t *ctx,
 			  struct peer *peer,
 			  const enum state_input input,
 			  const union input *idata,
+			  Pkt **out,
 			  struct state_effect **effect);
 
 /* Any CMD_SEND_HTLC_* */

test/test_state_coverage.c

@@ -1475,8 +1475,7 @@ void peer_tx_revealed_r_value(struct peer *peer,
  * required. */
 static const char *apply_effects(struct peer *peer,
 				 const struct state_effect *effect,
-				 uint64_t *effects,
+				 uint64_t *effects)
-				 Pkt **output)
 {
 	const struct htlc *h;
 
@@ -1485,7 +1484,7 @@ static const char *apply_effects(struct peer *peer,
 
 	if (effect->next) {
 		const char *problem = apply_effects(peer, effect->next,
-						    effects, output);
+						    effects);
 		if (problem)
 			return problem;
 	}
@@ -1497,30 +1496,6 @@ static const char *apply_effects(struct peer *peer,
 	switch (effect->etype) {
 	case STATE_EFFECT_broadcast_tx:
 		break;
-	case STATE_EFFECT_send_pkt: {
-		const char *pkt = (const char *)effect->u.send_pkt;
-		*output = effect->u.send_pkt;
-
-		/* Check for errors. */
-		if (strstarts(pkt, "PKT_ERROR: ")) {
-			/* Some are expected. */
-			if (!streq(pkt, "PKT_ERROR: Commit tx noticed")
-			    && !streq(pkt, "PKT_ERROR: Otherspend noticed")
-			    && !streq(pkt, "PKT_ERROR: Error inject")
-			    && !streq(pkt, "PKT_ERROR: Anchor timed out")
-			    && !streq(pkt, "PKT_ERROR: Close timed out")
-			    && !streq(pkt, "PKT_ERROR: Close forced due to HTLCs")) {
-				return pkt;
-			}
-		}
-		if (peer->core.num_outputs >= ARRAY_SIZE(peer->core.outputs))
-			return "Too many outputs";
-		peer->core.outputs[peer->core.num_outputs]
-			= input_by_name(pkt);
-		peer->pkt_data[peer->core.num_outputs++]
-			= htlc_id_from_pkt(effect->u.send_pkt);
-		break;
-	}
 	case STATE_EFFECT_watch:
 		/* We can have multiple steals or spendtheirs
 		   in flight, so make exceptions for
@@ -1693,11 +1668,10 @@ static const char *apply_all_effects(const struct peer *old,
 				     enum state_input input,
 				     struct peer *peer,
 				     const struct state_effect *effect,
-				     Pkt **output)
+				     Pkt *output)
 {
 	const char *problem;
 	uint64_t effects = 0;
-	*output = NULL;
 
 	if (cstatus != CMD_NONE) {
 		assert(peer->core.current_command != INPUT_NONE);
@@ -1724,7 +1698,29 @@ static const char *apply_all_effects(const struct peer *old,
 		peer->core.current_command = INPUT_NONE;
 	}
 
-	problem = apply_effects(peer, effect, &effects, output);
+	if (output) {
+		const char *pkt = (const char *)output;
+		/* Check for errors. */
+		if (strstarts(pkt, "PKT_ERROR: ")) {
+			/* Some are expected. */
+			if (!streq(pkt, "PKT_ERROR: Commit tx noticed")
+			    && !streq(pkt, "PKT_ERROR: Otherspend noticed")
+			    && !streq(pkt, "PKT_ERROR: Error inject")
+			    && !streq(pkt, "PKT_ERROR: Anchor timed out")
+			    && !streq(pkt, "PKT_ERROR: Close timed out")
+			    && !streq(pkt, "PKT_ERROR: Close forced due to HTLCs")) {
+				return pkt;
+			}
+		}
+		if (peer->core.num_outputs >= ARRAY_SIZE(peer->core.outputs))
+			return "Too many outputs";
+		peer->core.outputs[peer->core.num_outputs]
+			= input_by_name(pkt);
+		peer->pkt_data[peer->core.num_outputs++]
+			= htlc_id_from_pkt(output);
+	}
+	
+	problem = apply_effects(peer, effect, &effects);
 	if (!problem)
 		problem = check_changes(old, peer, input);
 	return problem;
@@ -1917,25 +1913,6 @@ static bool has_packets(const struct peer *peer)
 	return peer->core.num_outputs != 0;
 }
 
-static struct state_effect *get_effect(const struct state_effect *effect,
-				       enum state_effect_type type)
-{
-	while (effect) {
-		if (effect->etype == type)
-			break;
-		effect = effect->next;
-	}
-	return cast_const(struct state_effect *, effect);
-}
-
-static Pkt *get_send_pkt(const struct state_effect *effect)
-{
-	effect = get_effect(effect, STATE_EFFECT_send_pkt);
-	if (effect)
-		return effect->u.send_pkt;
-	return NULL;
-}			
-
 static void try_input(const struct peer *peer,
 		      enum state_input i,
 		      const union input *idata,
@@ -1959,7 +1936,7 @@ static void try_input(const struct peer *peer,
 	copy.trail = &t;
 
 	eliminate_input(&hist->inputs_per_state[copy.state], i);
-	cstatus = state(ctx, &copy, i, idata, &effect);
+	cstatus = state(ctx, &copy, i, idata, &output, &effect);
 
 	normalpath &= normal_path(i, peer->state, copy.state);
 	errorpath |= error_path(i, peer->state, copy.state);
@@ -1979,11 +1956,10 @@ static void try_input(const struct peer *peer,
 			newstr = state_name(copy.state) + 6;
 		}
 		if (newstr != oldstr || include_nops)
-			add_dot(&hist->edges, oldstr, newstr, i,
+			add_dot(&hist->edges, oldstr, newstr, i, output);
-				get_send_pkt(effect));
 	}
 
-	problem = apply_all_effects(peer, cstatus, i, &copy, effect, &output);
+	problem = apply_all_effects(peer, cstatus, i, &copy, effect, output);
 	update_trail(&t, &copy, output);
 	if (problem)
 		report_trail(&t, problem);
@@ -2374,9 +2350,11 @@ static enum state_input **map_inputs(void)
 		if (!state_is_error(i)) {
 			struct peer dummy;
 			struct state_effect *effect;
+			Pkt *dummy_pkt;
 			memset(&dummy, 0, sizeof(dummy));
 			dummy.state = i;
-			state(ctx, &dummy, INPUT_NONE, NULL, &effect);
+			state(ctx, &dummy, INPUT_NONE, NULL, &dummy_pkt,
+			      &effect);
 		}
 		inps[i] = mapping_inputs;
 	}