From e984df486da09b1248c7fdf7734932294f7a2163 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 22 Jan 2016 06:41:47 +1030 Subject: [PATCH] state: return Pkt to be queued directly. Instead of effect->send_pkt. Signed-off-by: Rusty Russell --- state.c | 68 +++++++++++++++++------------- state.h | 5 +-- test/test_state_coverage.c | 86 ++++++++++++++------------------------ 3 files changed, 72 insertions(+), 87 deletions(-) diff --git a/state.c b/state.c index 2a2ac5418..9d66cdcc1 100644 --- a/state.c +++ b/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); diff --git a/state.h b/state.h index cae0e2005..e4769395e 100644 --- a/state.h +++ b/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_* */ diff --git a/test/test_state_coverage.c b/test/test_state_coverage.c index 04b340aab..3ca12e617 100644 --- a/test/test_state_coverage.c +++ b/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, - Pkt **output) + uint64_t *effects) { 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, ©, i, idata, &effect); + cstatus = state(ctx, ©, 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, - get_send_pkt(effect)); + add_dot(&hist->edges, oldstr, newstr, i, output); } - problem = apply_all_effects(peer, cstatus, i, ©, effect, &output); + problem = apply_all_effects(peer, cstatus, i, ©, effect, output); update_trail(&t, ©, 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; }