state: set peercond in peer directly.

Instead of stop_packets or stop_commands effects. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
9 years ago · 14ede84d87
3 changed files with 96 additions and 73 deletions
--- a/state.c
+++ b/state.c
@ -33,9 +33,15 @@ static struct state_effect *next_state(const tal_t *ctx,
 	return effect;
 }

+static void set_peer_cond(struct peer *peer, enum state_peercond cond)
+{
+	assert(peer->cond != cond);
+	peer->cond = cond;
+}
+
 struct state_effect *state(const tal_t *ctx,
 			   const enum state state,
-			   const struct peer *peer,
+			   struct peer *peer,
 			   const enum state_input input,
 			   const union input *idata)
 {
@ -541,8 +547,7 @@ struct state_effect *state(const tal_t *ctx,
 				   pkt_close_ack(ctx, peer));
 			add_effect(&effect, broadcast_tx,
 				   bitcoin_close(ctx, peer));
-			add_effect(&effect, stop_commands, true);
-			add_effect(&effect, stop_packets, true);
+			set_peer_cond(peer, PEER_CLOSED);
 			return next_state(ctx, effect, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is(input, PKT_CLOSE)) {
 			/* We can use the sig just like CLOSE_COMPLETE */
@ -556,8 +561,7 @@ struct state_effect *state(const tal_t *ctx,
 				   pkt_close_ack(ctx, peer));
 			add_effect(&effect, broadcast_tx,
 				   bitcoin_close(ctx, peer));
-			add_effect(&effect, stop_commands, true);
-			add_effect(&effect, stop_packets, true);
+			set_peer_cond(peer, PEER_CLOSED);
 			return next_state(ctx, effect, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is(input, PKT_ERROR)) {
 			add_effect(&effect, in_error,
@ -572,7 +576,7 @@ struct state_effect *state(const tal_t *ctx,
 			goto err_start_unilateral_close_already_closing;
 		}
 		add_effect(&effect, cmd_close_done, false);
-		add_effect(&effect, stop_commands, true);
+		set_peer_cond(peer, PEER_CLOSING);
 		goto fail_during_close;

 	case STATE_WAIT_FOR_CLOSE_ACK:
@ -581,7 +585,7 @@ struct state_effect *state(const tal_t *ctx,
 						   &effect);
 			if (err)
 				add_effect(&effect, send_pkt, err);
-			add_effect(&effect, stop_packets, true);
+			set_peer_cond(peer, PEER_CLOSED);
 			/* Just wait for close to happen now. */
 			return next_state(ctx, effect, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is_pkt(input)) {
@ -592,12 +596,12 @@ struct state_effect *state(const tal_t *ctx,
 				add_effect(&effect, send_pkt,
 					   unexpected_pkt(ctx, input));
 			}
-			add_effect(&effect, stop_packets, true);
+			set_peer_cond(peer, PEER_CLOSED);
 			/* Just wait for close to happen now. */
 			return next_state(ctx, effect, STATE_CLOSE_WAIT_CLOSE);
 		} else if (input_is(input, BITCOIN_CLOSE_DONE)) {
 			/* They didn't ack, but we're closed, so stop. */
-			add_effect(&effect, stop_packets, true);
+			set_peer_cond(peer, PEER_CLOSED);
 			return next_state(ctx, effect, STATE_CLOSED);
 		}
 		goto fail_during_close;
@ -883,8 +887,7 @@ err_close_nocleanup:
 	 * so there's nothing to clean up.
 	 */
 	add_effect(&effect, send_pkt, err);
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);
 	return next_state(ctx, effect, STATE_CLOSED);

 err_start_unilateral_close:
@ -898,8 +901,7 @@ start_unilateral_close:
 	 * Close unilaterally.
 	 */
 	/* No more inputs, no more commands. */
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);
 	tx = bitcoin_commit(ctx, peer);
 	add_effect(&effect, broadcast_tx, tx);
 	add_effect(&effect, watch,
@ -931,8 +933,7 @@ start_unilateral_close_already_closing:
 	 * Close unilaterally.
 	 */
 	/* No more inputs, no more commands. */
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);
 	tx = bitcoin_commit(ctx, peer);
 	add_effect(&effect, broadcast_tx, tx);
 	add_effect(&effect, watch,
@ -954,8 +955,7 @@ them_unilateral:
 	add_effect(&effect, send_pkt, pkt_err(ctx, "Commit tx noticed"));

 	/* No more inputs, no more commands. */
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);
 	tx = bitcoin_spend_theirs(ctx, peer, idata->btc);
 	add_effect(&effect, broadcast_tx, tx);
 	add_effect(&effect, watch,
@ -1044,7 +1044,7 @@ accept_closing:
 		   bitcoin_watch_close(ctx, peer, BITCOIN_CLOSE_DONE));
 	add_effect(&effect, send_pkt, pkt_close_complete(ctx, peer));
 	/* No more commands, we're already closing. */
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSING);
 	return next_state(ctx, effect, STATE_WAIT_FOR_CLOSE_ACK);
 	
 instant_close:
@ -1054,8 +1054,7 @@ instant_close:
 	 */
 	add_effect(&effect, cmd_close_done, true);
 	/* FIXME: Should we tell other side we're going? */
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);

 	/* We can't have any HTLCs, since we haven't started. */
 	if (committed_to_htlcs(peer))
@ -1067,7 +1066,7 @@ fail_during_close:
 	 * We've broadcast close tx; if anything goes wrong, we just close
 	 * connection and wait.
 	 */
-	add_effect(&effect, stop_packets, true);
+	set_peer_cond(peer, PEER_CLOSED);

 	/* Once close tx is deep enough, we consider it done. */
 	if (input_is(input, BITCOIN_CLOSE_DONE)) {
@ -1113,8 +1112,7 @@ old_commit_spotted:
 	add_effect(&effect, send_pkt, pkt_err(ctx, "Otherspend noticed"));

 	/* No more packets, no more commands. */
-	add_effect(&effect, stop_packets, true);
-	add_effect(&effect, stop_commands, true);
+	set_peer_cond(peer, PEER_CLOSED);

 	/* If we can't find it, we're lost. */
 	tx = bitcoin_steal(ctx, peer, idata->btc);
--- a/state.h
+++ b/state.h
@ -6,6 +6,17 @@
 #include <state_types.h>
 #include <stdbool.h>

+enum state_peercond {
+	/* Ready to accept a new command. */
+	PEER_CMD_OK,
+	/* Don't send me commands, I'm busy. */
+	PEER_BUSY,
+	/* No more commands, I'm closing. */
+	PEER_CLOSING,
+	/* No more packets, I'm closed. */
+	PEER_CLOSED
+};
+
 enum state_effect_type {
 	STATE_EFFECT_new_state,
 	STATE_EFFECT_in_error,
@ -19,8 +30,6 @@ enum state_effect_type {
 	/* (never applies to CMD_CLOSE) */
 	STATE_EFFECT_cmd_fail,
 	STATE_EFFECT_cmd_close_done,
-	STATE_EFFECT_stop_packets,
-	STATE_EFFECT_stop_commands,
 	/* FIXME: Use a watch for this?. */
 	STATE_EFFECT_close_timeout,
 	STATE_EFFECT_htlc_in_progress,
@ -132,7 +141,7 @@ union input {

 struct state_effect *state(const tal_t *ctx,
 			   const enum state state,
-			   const struct peer *peer,
+			   struct peer *peer,
 			   const enum state_input input,
 			   const union input *idata);

--- a/test/test_state_coverage.c
+++ b/test/test_state_coverage.c
@ -93,9 +93,8 @@ struct core_state {
 	enum state_input outputs[MAX_OUTQ];

 	uint8_t num_outputs;
-	bool pkt_inputs;
-	bool cmd_inputs;
 	/* Here down need to be generated from other fields */
+	uint8_t peercond;
 	bool has_current_htlc;

 	uint8_t capped_htlcs_to_them;
@ -107,11 +106,14 @@ struct core_state {
 	uint8_t capped_live_htlcs_to_us;
 	bool closing_cmd;
 	bool valid;
-	bool pad[0];
+	bool pad[1];
 };

 struct peer {
 	struct core_state core;
+
+	enum state_peercond cond;
+
 	/* To store HTLC numbers. */
 	unsigned int pkt_data[MAX_OUTQ];

@ -191,8 +193,7 @@ static bool situation_eq(const struct situation *a, const struct situation *b)
 			 + sizeof(a->a.s.current_command)
 			 + sizeof(a->a.s.outputs)
 			 + sizeof(a->a.s.num_outputs)
-			 + sizeof(a->a.s.pkt_inputs)
-			 + sizeof(a->a.s.cmd_inputs)
+			 + sizeof(a->a.s.peercond)
 			 + sizeof(a->a.s.has_current_htlc)
 			 + sizeof(a->a.s.capped_htlcs_to_us)
 			 + sizeof(a->a.s.capped_htlcs_to_them)
@ -317,6 +318,7 @@ static void update_core(struct core_state *core, const struct peer *peer)
 		assert(core->outputs[i] == 0);
 		
 	core->has_current_htlc = peer->current_htlc.htlc.id != -1;
+	core->peercond = peer->cond;
 	core->capped_htlcs_to_us = cap(peer->num_htlcs_to_us);
 	core->capped_htlcs_to_them = cap(peer->num_htlcs_to_them);
 	core->capped_live_htlcs_to_us = cap(peer->num_live_htlcs_to_us);
@ -333,7 +335,8 @@ static void situation_init(struct situation *sit,
 		sit->a.s = peer->core;
 		update_core(&sit->a.s, peer);
 		/* If we're still talking to peer, their state matters. */
-		if (peer->core.pkt_inputs || peer->other->core.pkt_inputs) {
+		if (peer->cond != PEER_CLOSED
+		    || peer->other->cond != PEER_CLOSED) {
 			sit->b.s = peer->other->core;
 			update_core(&sit->b.s, peer->other);
 		} else
@ -342,7 +345,8 @@ static void situation_init(struct situation *sit,
 		sit->b.s = peer->core;
 		update_core(&sit->b.s, peer);
 		/* If we're still talking to peer, their state matters. */
-		if (peer->core.pkt_inputs || peer->other->core.pkt_inputs) {
+		if (peer->cond != PEER_CLOSED
+		    || peer->other->cond != PEER_CLOSED) {
 			sit->a.s = peer->other->core;
 			update_core(&sit->a.s, peer->other);
 		} else
@ -436,7 +440,8 @@ static bool fail(const struct peer *peer, enum failure which_fail)

 	/* First time here, save details, don't fail yet. */
 	f->details = tal(f, struct fail_details);
-	copy_peers(&f->details->us, &f->details->other, peer);
+	/* Copy old peer, in case it has been changed since. */
+	copy_peers(&f->details->us, &f->details->other, peer->trail->before);
 	f->details->us.trail = clone_trail(f->details, peer->trail);
 	f->details->input = peer->current_input;
 	f->details->idata = dup_idata(f->details,
@ -1167,6 +1172,7 @@ static void peer_init(struct peer *peer,
 		      struct peer *other,
 		      const char *name)
 {
+	peer->cond = PEER_CMD_OK;
 	peer->core.state = STATE_INIT;
 	peer->core.num_outputs = 0;
 	peer->current_htlc.htlc.id = -1;
@ -1182,8 +1188,6 @@ static void peer_init(struct peer *peer,
 	peer->pkt_data[0] = -1;
 	peer->core.current_command = INPUT_NONE;
 	peer->core.event_notifies = 0;
-	peer->core.pkt_inputs = true;
-	peer->core.cmd_inputs = true;
 	peer->core.closing_cmd = false;
 	peer->name = name;
 	peer->other = other;
@ -1384,15 +1388,9 @@ static char *check_effects(struct peer *peer,
 	while (effect) {
 		if (effect->etype == STATE_EFFECT_in_error) {
 			/* We should stop talking to them after error recvd. */
-			if (peer->core.pkt_inputs)
+			if (peer->cond != PEER_CLOSING
+			    && peer->cond != PEER_CLOSED)
 				return "packets still open after error pkt";
-		} else if (effect->etype == STATE_EFFECT_stop_commands) {
-			if (peer->core.current_command != INPUT_NONE)
-				return tal_fmt(NULL,
-					       "stop_commands with pending command %s",
-					       input_name(peer->core.current_command));
-			if (peer->core.closing_cmd)
-				return "stop_commands with pending CMD_CLOSE";
 		}
 		effect = effect->next;
 	}
@ -1503,20 +1501,6 @@ static const char *apply_effects(struct peer *peer,
 				       ? "Success" : "Failure");
 		peer->core.closing_cmd = false;
 		break;
-	case STATE_EFFECT_stop_packets:
-		if (!peer->core.pkt_inputs)
-			return "stop_packets twice";
-		peer->core.pkt_inputs = false;
-
-		/* Can no longer receive packet timeouts, either. */
-		remove_event_(&peer->core.event_notifies,
-			      INPUT_CLOSE_COMPLETE_TIMEOUT);
-		break;
-	case STATE_EFFECT_stop_commands:
-		if (!peer->core.cmd_inputs)
-			return "stop_commands twice";
-		peer->core.cmd_inputs = false;
-		break;
 	case STATE_EFFECT_close_timeout:
 		add_event(&peer->core.event_notifies,
 			  effect->u.close_timeout);
@ -1677,7 +1661,34 @@ static const char *apply_effects(struct peer *peer,
 	return NULL;
 }
 	
-static const char *apply_all_effects(struct peer *peer,
+static const char *check_changes(const struct peer *old, struct peer *new)
+{
+	if (new->cond != old->cond) {
+		/* Only BUSY -> CMD_OK can go backwards. */
+		if (!(old->cond == PEER_BUSY && new->cond == PEER_CMD_OK))
+			if (new->cond < old->cond)
+				return tal_fmt(NULL, "cond from %u to %u",
+					       old->cond, new->cond);
+	}
+	if (new->cond == PEER_CLOSING
+	    || new->cond == PEER_CLOSED) {
+		if (new->core.current_command != INPUT_NONE)
+			return tal_fmt(NULL,
+				       "cond CLOSE with pending command %s",
+				       input_name(new->core.current_command));
+		if (new->core.closing_cmd)
+			return "cond CLOSE with pending CMD_CLOSE";
+		/* FIXME: Move to state core */
+		/* Can no longer receive packet timeouts, either. */
+		remove_event_(&new->core.event_notifies,
+			      INPUT_CLOSE_COMPLETE_TIMEOUT);
+	}
+
+	return NULL;
+}
+
+static const char *apply_all_effects(const struct peer *old,
+				     struct peer *peer,
 				     const struct state_effect *effect,
 				     Pkt **output)
 {
@ -1687,9 +1698,11 @@ static const char *apply_all_effects(struct peer *peer,
 	problem = apply_effects(peer, effect, &effects, output);
 	if (!problem)
 		problem = check_effects(peer, effect);
+	if (!problem)
+		problem = check_changes(old, peer);
 	return problem;
 }
-	      
+
 static void eliminate_input(enum state_input **inputs, enum state_input in)
 {
 	size_t i, n = tal_count(*inputs);
@ -1954,7 +1967,7 @@ static void try_input(const struct peer *peer,
 				get_send_pkt(effect));
 	}

-	problem = apply_all_effects(&copy, effect, &output);
+	problem = apply_all_effects(peer, &copy, effect, &output);
 	update_trail(&t, &copy, output);
 	if (problem)
 		report_trail(&t, problem);
@ -2009,18 +2022,16 @@ static void try_input(const struct peer *peer,
 	/*
 	 * If we're listening, someone should be talking (usually).
 	 */
-	if (copy.core.pkt_inputs && !has_packets(&copy) && !has_packets(&other)
+	if (copy.cond != PEER_CLOSED
+	    && !has_packets(&copy) && !has_packets(&other)
 	    && !waiting_statepair(copy.core.state, other.core.state)) {
 		report_trail(&t, "Deadlock");
 	}

 	/* Finished? */
 	if (newstate == STATE_CLOSED) {
-		if (copy.core.pkt_inputs)
-			report_trail(&t, "CLOSED but taking packets?");
-
-		if (copy.core.cmd_inputs)
-			report_trail(&t, "CLOSED but taking commands?");
+		if (copy.cond != PEER_CLOSED)
+			report_trail(&t, "CLOSED but cond not CLOSED");

 		if (copy.core.current_command != INPUT_NONE)
 			report_trail(&t, input_name(copy.core.current_command));
@ -2038,7 +2049,8 @@ static void try_input(const struct peer *peer,
 	run_peer(&copy, normalpath, errorpath, &t, hist);

 	/* Don't bother running other peer we can't communicate. */
-	if (copy.core.pkt_inputs || other.core.pkt_inputs)
+	if (copy.cond != PEER_CLOSED
+	    || other.cond != PEER_CLOSED)
 		run_peer(&other, normalpath, errorpath, &t, hist);
 	tal_free(ctx);
 }
@ -2165,7 +2177,8 @@ static void run_peer(const struct peer *peer,
 	/* We can send a close command even if already sending a
 	 * (different) command. */
 	if (peer->core.state != STATE_INIT
-	    && peer->core.cmd_inputs
+	    && (peer->cond == PEER_CMD_OK
+		|| peer->cond == PEER_BUSY)
 	    && !peer->core.closing_cmd) {
 		copy.core.closing_cmd = true;
 		try_input(&copy, CMD_CLOSE, idata,
@ -2174,7 +2187,7 @@ static void run_peer(const struct peer *peer,
 	}

 	/* Try sending commands (unless closed or already doing one). */
-	if (peer->core.cmd_inputs
+	if (peer->cond == PEER_CMD_OK
 	    && peer->core.current_command == INPUT_NONE
 	    && !peer->core.closing_cmd) {
 		unsigned int i;
@ -2268,7 +2281,7 @@ static void run_peer(const struct peer *peer,
 	}

 	/* Allowed to send inputs? */
-	if (copy.core.pkt_inputs) {
+	if (copy.cond != PEER_CLOSED) {
 		enum state_input i;
 		
 		if (other.core.num_outputs) {
@ -2320,8 +2333,11 @@ static enum state_input **map_inputs(void)
 		mapping_inputs = tal_arr(inps, enum state_input, 0);

 		/* This adds to mapping_inputs every input_is() call */
-		if (!state_is_error(i))
-			state(ctx, i, NULL, INPUT_NONE, NULL);
+		if (!state_is_error(i)) {
+			struct peer dummy;
+			memset(&dummy, 0, sizeof(dummy));
+			state(ctx, i, &dummy, INPUT_NONE, NULL);
+		}
 		inps[i] = mapping_inputs;
 	}