From d964ad2d94b2ae7ce00d80ae4161a63ab83c47b5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 30 Aug 2016 20:11:57 +0930 Subject: [PATCH] daemon: don't restart newhtlc/failhtlc/fulfill htlc commands on reconnect, These low level commands we restarted on reconnect for ease of testing. Don't do that, and check that we're connected when those commands occur. This introduces subtle issues with --manual-commit --reconnect: restarting node1 also forgets uncommitted things from node2, requiring reordering for some tests. Signed-off-by: Rusty Russell --- daemon/peer.c | 79 +++++++++++++-------------------------------- daemon/test/test.sh | 74 +++++++++++++++++++++++++++++++----------- 2 files changed, 78 insertions(+), 75 deletions(-) diff --git a/daemon/peer.c b/daemon/peer.c index e949a0fcf..598e2e121 100644 --- a/daemon/peer.c +++ b/daemon/peer.c @@ -1837,42 +1837,6 @@ static void retransmit_updates(struct peer *peer) assert(!peer->feechanges[SENT_FEECHANGE]); } -/* FIXME: Maybe it would be neater to remember all pay commands, and simply - * re-run them after reconnect if they didn't get committed. */ -static void resend_local_requests(struct peer *peer) -{ - struct htlc_map_iter it; - struct htlc *h; - - for (h = htlc_map_first(&peer->htlcs, &it); - h; - h = htlc_map_next(&peer->htlcs, &it)) { - switch (h->state) { - case SENT_ADD_HTLC: - /* We removed everything which was routed. */ - assert(!h->src); - log_debug(peer->log, "Re-sending local add HTLC %"PRIu64, - h->id); - queue_pkt_htlc_add(peer, h); - remote_changes_pending(peer); - break; - case SENT_REMOVE_HTLC: - /* We removed everything which was routed. */ - assert(!h->src); - log_debug(peer->log, "Re-sending local %s HTLC %"PRIu64, - h->r ? "fulfill" : "fail", h->id); - if (h->r) - queue_pkt_htlc_fulfill(peer, h); - else - queue_pkt_htlc_fail(peer, h); - remote_changes_pending(peer); - break; - default: - break; - } - } -} - /* BOLT #2: * * On disconnection, a node MUST reverse any uncommitted changes sent by the @@ -1926,13 +1890,6 @@ again: h = htlc_map_next(&peer->htlcs, &it)) { switch (h->state) { case SENT_ADD_HTLC: - /* FIXME: re-submit these after connect, instead? */ - /* Keep local adds. */ - if (!h->src) { - if (!cstate_add_htlc(peer->remote.staging_cstate, h)) - fatal("Could not add HTLC?"); - break; - } /* Adjust counter to lowest HTLC removed */ if (peer->htlc_id_counter > h->id) { log_debug(peer->log, @@ -1955,17 +1912,6 @@ again: SENT_ADD_ACK_REVOCATION); break; case SENT_REMOVE_HTLC: - /* Keep local removes. */ - /* FIXME: re-submit these after connect, instead? */ - if (!h->src) { - if (h->r) { - cstate_fulfill_htlc(peer->remote.staging_cstate, - h); - } else { - cstate_fail_htlc(peer->remote.staging_cstate, h); - } - break; - } log_debug(peer->log, "Undoing %s %"PRIu64, htlc_state_name(h->state), h->id); htlc_undostate(h, SENT_REMOVE_HTLC, @@ -2061,8 +2007,6 @@ static void retransmit_pkts(struct peer *peer, s64 ack) ack++; } - resend_local_requests(peer); - /* We might need to re-propose HTLCs which were from other peers. */ retry_all_routing(peer); } @@ -4197,7 +4141,7 @@ static void json_getpeers(struct command *cmd, json_add_pubkey(response, cmd->dstate->secpctx, "peerid", p->id); - json_add_bool(response, "connected", p->conn && !p->fake_close); + json_add_bool(response, "connected", p->connected); /* FIXME: Report anchor. */ @@ -4352,6 +4296,11 @@ static void json_newhtlc(struct command *cmd, return; } + if (!peer->connected) { + command_fail(cmd, "peer not connected"); + return; + } + if (!json_tok_u64(buffer, msatoshistok, &msatoshis)) { command_fail(cmd, "'%.*s' is not a valid number", (int)(msatoshistok->end - msatoshistok->start), @@ -4427,6 +4376,11 @@ static void json_fulfillhtlc(struct command *cmd, return; } + if (!peer->connected) { + command_fail(cmd, "peer not connected"); + return; + } + if (!json_tok_u64(buffer, idtok, &id)) { command_fail(cmd, "'%.*s' is not a valid id", (int)(idtok->end - idtok->start), @@ -4508,6 +4462,11 @@ static void json_failhtlc(struct command *cmd, return; } + if (!peer->connected) { + command_fail(cmd, "peer not connected"); + return; + } + if (!json_tok_u64(buffer, idtok, &id)) { command_fail(cmd, "'%.*s' is not a valid id", (int)(idtok->end - idtok->start), @@ -4566,6 +4525,11 @@ static void json_commit(struct command *cmd, return; } + if (!peer->connected) { + command_fail(cmd, "peer not connected"); + return; + } + if (!state_can_commit(peer->state)) { command_fail(cmd, "peer in state %s", state_name(peer->state)); return; @@ -4677,6 +4641,7 @@ static void json_disconnect(struct command *cmd, * one side to freak out. We just ensure we ignore it. */ log_debug(peer->log, "Pretending connection is closed"); peer->fake_close = true; + peer->connected = false; set_peer_state(peer, STATE_ERR_BREAKDOWN, "json_disconnect", false); peer_breakdown(peer); diff --git a/daemon/test/test.sh b/daemon/test/test.sh index c81193fd9..9b412ba13 100755 --- a/daemon/test/test.sh +++ b/daemon/test/test.sh @@ -103,6 +103,19 @@ else SHOW="cat" fi +# Peer $1 -> $2's htlc $3 is in state $4 +htlc_is_state() +{ + if [ $# != 4 ]; then echo "htlc_is_state got $# ARGS: $@" >&2; exit 1; fi + $1 gethtlcs $2 true | tr -s '\012\011\" ' ' ' | $FGREP "id : $3, state : $4 ," >&2 +} + +# Peer $1 -> $2's htlc $3 exists +htlc_exists() +{ + $1 gethtlcs $2 true | tr -s '\012\011\" ' ' ' | $FGREP "id : $3," >&2 +} + lcli1() { if [ -n "$VERBOSE" ]; then @@ -130,31 +143,51 @@ lcli1() reconnect) [ -z "$VERBOSE" ] || echo RECONNECTING >&2 $LCLI1 dev-reconnect $ID2 >/dev/null - sleep 1 ;; restart) [ -z "$VERBOSE" ] || echo RESTARTING >&2 - # FIXME: Instead, check if command was committed, and - # if not, resubmit! - if [ "$1" = "newhtlc" ]; then - $LCLI1 commit $ID2 >/dev/null 2>&1 || true - fi $LCLI1 -- dev-restart $LIGHTNINGD1 >/dev/null 2>&1 || true if ! check "$LCLI1 getlog 2>/dev/null | fgrep -q Hello"; then echo "dev-restart failed!">&2 exit 1 fi - # These are safe to resubmit, will simply fail. - if [ "$1" = "fulfillhtlc" -o "$1" = "failhtlc" ]; then - if [ -z "$VERBOSE" ]; then - $LCLI1 "$@" >/dev/null 2>&1 || true - else - echo "Rerunning $LCLI1 $@" >&2 - $LCLI1 "$@" 2>&1 || true - fi - fi ;; esac + # Wait for reconnect; + if ! check "$LCLI1 getpeers | tr -s '\012\011\" ' ' ' | fgrep -q 'connected : true'"; then + echo "Failed to reconnect!">&2 + exit 1 + fi + + if [ "$1" = "newhtlc" ]; then + # It might have gotten committed, or might be forgotten. + ID=`echo "$OUT" | extract_id` + if ! htlc_exists "$LCLI1" $2 $ID; then + if [ -z "$VERBOSE" ]; then + $LCLI1 "$@" >/dev/null 2>&1 || true + else + echo "Rerunning $LCLI1 $@" >&2 + $LCLI1 "$@" >&2 || true + fi + fi + # Make sure it's confirmed before we run next command, + # in case *that* restarts (unless manual commit) + [ -n "$MANUALCOMMIT" ] || check ! htlc_is_state \'"$LCLI1"\' $2 $ID SENT_ADD_HTLC + # Removals may also be forgotten. + elif [ "$1" = "fulfillhtlc" -o "$1" = "failhtlc" ]; then + ID="$3" + if htlc_is_state "$LCLI1" $2 $ID RCVD_ADD_ACK_REVOCATION; then + if [ -z "$VERBOSE" ]; then + $LCLI1 "$@" >/dev/null 2>&1 || true + else + echo "Rerunning $LCLI1 $@" >&2 + $LCLI1 "$@" >&2 || true + fi + # Make sure it's confirmed before we run next command, + # in case *that* restarts. + [ -n "$MANUALCOMMIT" ] || check ! htlc_is_state \'"$LCLI1"\' $2 $ID SENT_REMOVE_HTLC + fi + fi ;; esac fi @@ -767,13 +800,13 @@ check_status $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" # Now, two HTLCs at once, one from each direction. # Both sides can afford this. HTLC_AMOUNT=1000000 -HTLCID=`lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH | extract_id` SECRET2=1de08917a61cb2b62ed5937d38577f6a7bfe59c176781c6d8128018e8b5ccdfe RHASH2=`lcli1 dev-rhash $SECRET2 | sed 's/.*"\([0-9a-f]*\)".*/\1/'` +HTLCID=`lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH | extract_id` HTLCID2=`lcli2 newhtlc $ID1 $HTLC_AMOUNT $EXPIRY $RHASH2 | extract_id` -[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 check_status $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , state : SENT_ADD_ACK_REVOCATION } " $(($B_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($B_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 , state : RCVD_ADD_ACK_REVOCATION } " @@ -812,8 +845,8 @@ if [ -n "$CLOSE_WITH_HTLCS" ]; then all_ok fi -lcli2 failhtlc $ID1 $HTLCID lcli1 fulfillhtlc $ID2 $HTLCID2 $SECRET2 +lcli2 failhtlc $ID1 $HTLCID [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 @@ -879,6 +912,11 @@ case "$RECONNECT" in ;; esac +if ! check "$LCLI2 getpeers | tr -s '\012\011\" ' ' ' | fgrep -q 'connected : true'"; then + echo "Failed to reconnect!">&2 + exit 1 +fi + # Node2 collects the HTLCs. lcli2 fulfillhtlc $ID1 $HTLCID $SECRET lcli2 fulfillhtlc $ID1 $HTLCID2 $SECRET2