From 0e78ccca56bc40da5ad20bd682aa5d5de0f9f883 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 18 Aug 2016 14:23:45 +0930 Subject: [PATCH] daemon: don't allow manual fulfill command until both sides committed. We had an occasional race where we hadn't gotten the remote revocation before submitting fulfill (spotted by the HTLC state transition code). Disallow this, but also add to the json output so we can wait for an HTLC to be irrevocably committed. Signed-off-by: Rusty Russell --- daemon/peer.c | 60 ++++++++++++++++++++++++++++++++++++++------- daemon/test/test.sh | 20 +++++++-------- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/daemon/peer.c b/daemon/peer.c index 29d5bdcc8..a5d7c789b 100644 --- a/daemon/peer.c +++ b/daemon/peer.c @@ -2874,20 +2874,55 @@ static void json_add_pubkey(struct json_result *response, json_add_hex(response, id, der, sizeof(der)); } +/* Is this side fully committed to this HTLC? */ +static bool htlc_fully_committed(const struct commit_info *ci, + const struct htlc *htlc) +{ + /* Must have it in our commitment. */ + if (!cstate_htlc_by_id(ci->cstate, htlc->id, htlc_channel_side(htlc))) + return false; + + /* If it wasn't in previous commitment, that must be revoked. */ + if (!ci->prev || ci->prev->revocation_preimage) + return true; + + if (!cstate_htlc_by_id(ci->prev->cstate, htlc->id, + htlc_channel_side(htlc))) + return false; + + return true; +} + static void json_add_htlcs(struct json_result *response, const char *id, - const struct channel_oneside *side) + const struct peer *peer, + enum channel_side side) { size_t i; + struct htlc **htlcs = peer->local.commit->cstate->side[side].htlcs; json_array_start(response, id); - for (i = 0; i < tal_count(side->htlcs); i++) { + for (i = 0; i < tal_count(htlcs); i++) { + const char *committed; + json_object_start(response, NULL); - json_add_u64(response, "msatoshis", side->htlcs[i]->msatoshis); - json_add_abstime(response, "expiry", &side->htlcs[i]->expiry); + json_add_u64(response, "msatoshis", htlcs[i]->msatoshis); + json_add_abstime(response, "expiry", &htlcs[i]->expiry); json_add_hex(response, "rhash", - &side->htlcs[i]->rhash, - sizeof(side->htlcs[i]->rhash)); + &htlcs[i]->rhash, sizeof(htlcs[i]->rhash)); + if (htlc_fully_committed(peer->local.commit, htlcs[i])) { + if (htlc_fully_committed(peer->remote.commit, htlcs[i])) + committed = "both"; + else + committed = "us"; + } else { + if (htlc_fully_committed(peer->remote.commit, htlcs[i])) + committed = "them"; + else + /* Weird, shouldn't happen. */ + committed = "none"; + } + json_add_string(response, "committed", committed); json_object_end(response); } json_array_end(response); @@ -2929,8 +2964,8 @@ static void json_getpeers(struct command *cmd, json_add_num(response, "our_fee", last->side[OURS].fee_msat); json_add_num(response, "their_amount", last->side[THEIRS].pay_msat); json_add_num(response, "their_fee", last->side[THEIRS].fee_msat); - json_add_htlcs(response, "our_htlcs", &last->side[OURS]); - json_add_htlcs(response, "their_htlcs", &last->side[THEIRS]); + json_add_htlcs(response, "our_htlcs", p, OURS); + json_add_htlcs(response, "their_htlcs", p, THEIRS); /* Any changes since then? */ if (p->local.staging_cstate->changes != last->changes) @@ -3050,8 +3085,15 @@ const struct json_command newhtlc_command = { static struct htlc *find_their_committed_htlc(struct peer *peer, const struct sha256 *rhash) { + struct htlc *htlc; + /* Must be in last committed cstate. */ - if (!cstate_find_htlc(peer->remote.commit->cstate, rhash, THEIRS)) + htlc = cstate_find_htlc(peer->remote.commit->cstate, rhash, THEIRS); + if (!htlc) + return NULL; + + /* Dangerous to fulfill unless the remote side is obliged to honor it. */ + if (!htlc_fully_committed(peer->remote.commit, htlc)) return NULL; return cstate_find_htlc(peer->remote.staging_cstate, rhash, THEIRS); diff --git a/daemon/test/test.sh b/daemon/test/test.sh index 1caaabf4d..fdb3eea97 100755 --- a/daemon/test/test.sh +++ b/daemon/test/test.sh @@ -426,7 +426,7 @@ if [ -n "$DIFFERENT_FEES" ]; then lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 - check_status_single lcli2 0 0 "" $(($AMOUNT - $HTLC_AMOUNT - $ONE_HTLCS_FEE2)) $(($ONE_HTLCS_FEE2)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " + check_status_single lcli2 0 0 "" $(($AMOUNT - $HTLC_AMOUNT - $ONE_HTLCS_FEE2)) $(($ONE_HTLCS_FEE2)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " lcli2 fulfillhtlc $ID1 $SECRET [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 @@ -487,7 +487,7 @@ if [ -n "$MANUALCOMMIT" ]; then A_FEE=$(($A_FEE + $EXTRA_FEE)) # Node 2 has it committed. - check_status_single lcli2 $B_AMOUNT $B_FEE "" $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " + check_status_single lcli2 $B_AMOUNT $B_FEE "" $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : us } " # There should be no "both committed" here yet if lcli1 getlog debug | $FGREP "Both committed"; then @@ -511,7 +511,7 @@ else fi # Both should have committed tx. -check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " $B_AMOUNT $B_FEE "" +check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $B_AMOUNT $B_FEE "" if [ -n "$STEAL" ]; then STEAL_TX=`$LCLI1 dev-signcommit $ID2 | cut -d\" -f4` @@ -532,7 +532,7 @@ if [ -n "$DUMP_ONCHAIN" ]; then check_peerstate lcli2 STATE_CLOSE_ONCHAIN_THEIR_UNILATERAL # both still know about htlc - check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " $B_AMOUNT $B_FEE "" + check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $B_AMOUNT $B_FEE "" # Generate 6 blocks so CSV timeout has expired. $CLI generate 6 @@ -596,7 +596,7 @@ lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH # Check channel status A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) A_FEE=$(($A_FEE + $EXTRA_FEE)) -check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " $B_AMOUNT $B_FEE "" +check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $B_AMOUNT $B_FEE "" lcli2 failhtlc $ID1 $RHASH [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 @@ -616,7 +616,7 @@ lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH # Check channel status A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) A_FEE=$(($A_FEE + $EXTRA_FEE)) -check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " $B_AMOUNT $B_FEE "" +check_status $A_AMOUNT $A_FEE "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $B_AMOUNT $B_FEE "" # Make sure node1 accepts the expiry packet. while [ $(blockheight) != $EXPIRY ]; do @@ -667,7 +667,7 @@ lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH [ ! -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 } " $B_AMOUNT $B_FEE "" +check_status $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $B_AMOUNT $B_FEE "" lcli2 fulfillhtlc $ID1 $SECRET [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 @@ -693,7 +693,7 @@ lcli2 newhtlc $ID1 $HTLC_AMOUNT $EXPIRY $RHASH2 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 -check_status $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " $(($B_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($B_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 } " +check_status $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both } " $(($B_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE)) $(($B_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 , committed : both } " if [ -n "$CLOSE_WITH_HTLCS" ]; then # Now begin close @@ -748,7 +748,7 @@ lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH # Make sure node1 sends commit (in the background, since it will block!) [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 & # node2 will consider this committed. -check_status_single lcli2 $(($B_AMOUNT - $EXTRA_FEE/2)) $(($B_FEE + $EXTRA_FEE/2)) "" $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE/2)) $(($A_FEE + $EXTRA_FEE/2)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH } " +check_status_single lcli2 $(($B_AMOUNT - $EXTRA_FEE/2)) $(($B_FEE + $EXTRA_FEE/2)) "" $(($A_AMOUNT - $HTLC_AMOUNT - $EXTRA_FEE/2)) $(($A_FEE + $EXTRA_FEE/2)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : us } " # Now send another offer, and enable node2 output. lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH2 @@ -759,7 +759,7 @@ lcli2 dev-output $ID1 true [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 # Both sides should be committed to htlcs -check_status $(($A_AMOUNT - $HTLC_AMOUNT*2 - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH }, { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 } " $(($B_AMOUNT - $EXTRA_FEE)) $(($B_FEE + $EXTRA_FEE)) "" +check_status $(($A_AMOUNT - $HTLC_AMOUNT*2 - $EXTRA_FEE)) $(($A_FEE + $EXTRA_FEE)) "{ msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , committed : both }, { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 , committed : both } " $(($B_AMOUNT - $EXTRA_FEE)) $(($B_FEE + $EXTRA_FEE)) "" # Node2 collects the HTLCs. lcli2 fulfillhtlc $ID1 $SECRET