diff --git a/state.c b/state.c index a88e55ca3..1ea874267 100644 --- 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); diff --git a/state.h b/state.h index bf57ce59f..2cb85c4ae 100644 --- a/state.h +++ b/state.h @@ -6,6 +6,17 @@ #include #include +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); diff --git a/test/test_state_coverage.c b/test/test_state_coverage.c index e1811fe9a..97f49b593 100644 --- 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(©, effect, &output); + problem = apply_all_effects(peer, ©, effect, &output); update_trail(&t, ©, 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(©) && !has_packets(&other) + if (copy.cond != PEER_CLOSED + && !has_packets(©) && !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(©, 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(©, 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; }