state: use peer_unexpected_pkt() for an unexpected packet.

Instead of effect->in_error.

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

state.c

@@ -633,8 +633,7 @@ enum command_status state(const tal_t *ctx,
 			set_peer_cond(peer, PEER_CLOSED);
 			return next_state(peer, cstatus, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is(input, PKT_ERROR)) {
-			add_effect(effect, in_error,
-				   tal_steal(ctx, idata->pkt));
+			peer_unexpected_pkt(peer, idata->pkt);
 			goto start_unilateral_close_already_closing;
 		} else if (input_is_pkt(input)) {
 			/* We ignore all other packets while closing. */
@@ -656,12 +655,11 @@ enum command_status state(const tal_t *ctx,
 			/* Just wait for close to happen now. */
 			return next_state(peer, cstatus, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is_pkt(input)) {
-			if (input_is(input, PKT_ERROR)) {
-				add_effect(effect, in_error,
-					   tal_steal(ctx, idata->pkt));
-			} else {
+			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,
-					   unexpected_pkt(ctx, input));
+					   pkt_err_unexpected(ctx, idata->pkt));
 			}
 			set_peer_cond(peer, PEER_CLOSED);
 			/* Just wait for close to happen now. */
@@ -926,12 +924,13 @@ unexpected_pkt:
 	/*
 	 * We got a weird packet, so we need to close unilaterally.
 	 */
+	peer_unexpected_pkt(peer, idata->pkt);
+
 	/* Don't reply to an error with an error. */
 	if (input_is(input, PKT_ERROR)) {
-		add_effect(effect, in_error, tal_steal(ctx, idata->pkt));
 		goto start_unilateral_close;
 	}
-	err = unexpected_pkt(ctx, input);
+	err = pkt_err_unexpected(ctx, idata->pkt);
 	goto err_start_unilateral_close;
 
 unexpected_pkt_nocleanup:
@@ -942,7 +941,7 @@ unexpected_pkt_nocleanup:
 	if (input_is(input, PKT_ERROR)) {
 		goto close_nocleanup;
 	}
-	err = unexpected_pkt(ctx, input);
+	err = pkt_err_unexpected(ctx, idata->pkt);
 	goto err_close_nocleanup;
 
 anchor_unspent:

state.h

@@ -18,7 +18,6 @@ enum state_peercond {
 };
 
 enum state_effect_type {
-	STATE_EFFECT_in_error,
 	STATE_EFFECT_broadcast_tx,
 	STATE_EFFECT_send_pkt,
 	STATE_EFFECT_watch,
@@ -63,9 +62,6 @@ struct state_effect {
 		/* Set a timeout for close tx. */
 		enum state_input close_timeout;
 
-		/* Error received from other side. */
-		Pkt *in_error;
-
 		/* HTLC we're working on. */
 		struct htlc_progress *htlc_in_progress;
 
@@ -155,6 +151,9 @@ static inline bool input_is(enum state_input a, enum state_input b)
 
 struct signature;
 
+/* Inform peer have an unexpected packet. */
+void peer_unexpected_pkt(struct peer *peer, const Pkt *pkt);
+
 /* Create various kinds of packets, allocated off @ctx */
 Pkt *pkt_open(const tal_t *ctx, const struct peer *peer,
 	      OpenChannel__AnchorOffer anchor);
@@ -176,7 +175,7 @@ Pkt *pkt_err(const tal_t *ctx, const char *msg);
 Pkt *pkt_close(const tal_t *ctx, const struct peer *peer);
 Pkt *pkt_close_complete(const tal_t *ctx, const struct peer *peer);
 Pkt *pkt_close_ack(const tal_t *ctx, const struct peer *peer);
-Pkt *unexpected_pkt(const tal_t *ctx, enum state_input input);
+Pkt *pkt_err_unexpected(const tal_t *ctx, const Pkt *pkt);
 
 /* Process various packets: return an error packet on failure. */
 Pkt *accept_pkt_open(const tal_t *ctx,

test/test_state_coverage.c

@@ -623,9 +623,10 @@ Pkt *pkt_close_ack(const tal_t *ctx, const struct peer *peer)
 	return new_pkt(ctx, PKT_CLOSE_ACK);
 }
 
-Pkt *unexpected_pkt(const tal_t *ctx, enum state_input input)
+Pkt *pkt_err_unexpected(const tal_t *ctx, const Pkt *pkt)
 {
-	return pkt_err(ctx, "Unexpected pkt");
+	return (Pkt *)tal_fmt(ctx, "PKT_ERROR: Unexpected pkt %s",
+			      (const char *)pkt);
 }
 
 Pkt *accept_pkt_open(const tal_t *ctx,
@@ -1369,20 +1370,16 @@ static bool rval_known(const struct peer *peer, unsigned int id)
 	return false;
 }
 
-/* Some assertions once they've already been applied. */
-static char *check_effects(struct peer *peer,
-			   const struct state_effect *effect)
+void peer_unexpected_pkt(struct peer *peer, const Pkt *pkt)
 {
-	while (effect) {
-		if (effect->etype == STATE_EFFECT_in_error) {
-			/* We should stop talking to them after error recvd. */
-			if (peer->cond != PEER_CLOSING
-			    && peer->cond != PEER_CLOSED)
-				return "packets still open after error pkt";
-		}
-		effect = effect->next;
-	}
-	return NULL;
+	const char *str = (const char *)pkt;
+
+	/* We can get errors. */
+	if (strstarts(str, "PKT_ERROR:"))
+		return;
+
+	/* Shouldn't get any other unexpected packets. */
+	report_trail(peer->trail, "Unexpected packet");
 }
 
 /* We apply them backwards, which helps our assertions.  It's not actually
@@ -1409,8 +1406,6 @@ static const char *apply_effects(struct peer *peer,
 	*effects |= (1ULL << effect->etype);
 
 	switch (effect->etype) {
-	case STATE_EFFECT_in_error:
-		break;
 	case STATE_EFFECT_broadcast_tx:
 		break;
 	case STATE_EFFECT_send_pkt: {
@@ -1621,7 +1616,8 @@ static const char *apply_effects(struct peer *peer,
 	return NULL;
 }
 	
-static const char *check_changes(const struct peer *old, struct peer *new)
+static const char *check_changes(const struct peer *old, struct peer *new,
+				 enum state_input input)
 {
 	if (new->cond != old->cond) {
 		/* Only BUSY -> CMD_OK can go backwards. */
@@ -1644,11 +1640,18 @@ static const char *check_changes(const struct peer *old, struct peer *new)
 			      INPUT_CLOSE_COMPLETE_TIMEOUT);
 	}
 
+	if (input == PKT_ERROR) {
+		/* We should stop talking to them after error recvd. */
+		if (new->cond != PEER_CLOSING
+		    && new->cond != PEER_CLOSED)
+			return "packets still open after error pkt";
+	}
 	return NULL;
 }
 
 static const char *apply_all_effects(const struct peer *old,
 				     enum command_status cstatus,
+				     enum state_input input,
 				     struct peer *peer,
 				     const struct state_effect *effect,
 				     Pkt **output)
@@ -1669,9 +1672,7 @@ static const char *apply_all_effects(const struct peer *old,
 
 	problem = apply_effects(peer, effect, &effects, output);
 	if (!problem)
-		problem = check_effects(peer, effect);
-	if (!problem)
-		problem = check_changes(old, peer);
+		problem = check_changes(old, peer, input);
 	return problem;
 }
 
@@ -1928,7 +1929,7 @@ static void try_input(const struct peer *peer,
 				get_send_pkt(effect));
 	}
 
-	problem = apply_all_effects(peer, cstatus, &copy, effect, &output);
+	problem = apply_all_effects(peer, cstatus, i, &copy, effect, &output);
 	update_trail(&t, &copy, output);
 	if (problem)
 		report_trail(&t, problem);
@@ -2242,7 +2243,9 @@ static void run_peer(const struct peer *peer,
 		
 		if (other.core.num_outputs) {
 			i = other.core.outputs[0];
-			if (other.pkt_data[0] == -1U)
+			if (i == PKT_ERROR)
+				idata->pkt = pkt_err(idata, "");
+			else if (other.pkt_data[0] == -1U)
 				idata->pkt = (Pkt *)talz(idata, char);
 			else
 				idata->pkt = htlc_pkt(idata, input_name(i),