daemon: use htlc id for fulfillhtlc and failhtlc commands.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

daemon/channel.c

@@ -299,19 +299,6 @@ void cstate_fulfill_htlc(struct channel_state *cstate,
 	remove_htlc(cstate, side, !side, htlc);
 }
 
-struct htlc *cstate_find_htlc(const struct channel_state *cstate,
-			      const struct sha256 *rhash,
-			      enum channel_side side)
-{
-	size_t i;
-
-	for (i = 0; i < tal_count(cstate->side[side].htlcs); i++) {
-		if (structeq(&cstate->side[side].htlcs[i]->rhash, rhash))
-			return cstate->side[side].htlcs[i];
-	}
-	return NULL;
-}
-
 struct htlc *cstate_htlc_by_id(const struct channel_state *cstate,
 			       uint64_t id,
 			       enum channel_side side)

daemon/channel.h

@@ -111,18 +111,6 @@ void adjust_fee(struct channel_state *cstate, uint32_t fee_rate);
  */
 bool force_fee(struct channel_state *cstate, uint64_t fee);
 
-/**
- * cstate_find_htlc: find an HTLC on this side of the channel.
- * @cstate: The channel state
- * @rhash: hash of redeem secret
- * @side: OURS or THEIRS
- *
- * Returns the HTLC, or NULL on fail.
- */
-struct htlc *cstate_find_htlc(const struct channel_state *cstate,
-			      const struct sha256 *rhash,
-			      enum channel_side side);
-
 /**
  * cstate_htlc_by_id: find an HTLC on this side of the channel by ID.
  * @cstate: The channel state

daemon/peer.c

@@ -3210,25 +3210,6 @@ 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,
 			   struct peer *peer,
@@ -3406,38 +3387,22 @@ const struct json_command newhtlc_command = {
 	"Returns { id: u64 } result on success"
 };
 
-/* Looks for their HTLC, but must be committed. */
-static struct htlc *find_their_committed_htlc(struct peer *peer,
-					      const struct sha256 *rhash)
-{
-	struct htlc *htlc;
-
-	/* Must be in last committed cstate. */
-	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);
-}
-
 static void json_fulfillhtlc(struct command *cmd,
 			     const char *buffer, const jsmntok_t *params)
 {
 	struct peer *peer;
-	jsmntok_t *peeridtok, *rtok;
+	jsmntok_t *peeridtok, *idtok, *rtok;
-	struct rval r;
+	u64 id;
-	struct sha256 rhash;
 	struct htlc *htlc;
+	struct sha256 rhash;
+	struct rval r;
 
 	if (!json_get_params(buffer, params,
 			     "peerid", &peeridtok,
+			     "id", &idtok,
 			     "r", &rtok,
 			     NULL)) {
-		command_fail(cmd, "Need peerid and r");
+		command_fail(cmd, "Need peerid, id and r");
 		return;
 	}
 
@@ -3452,6 +3417,13 @@ static void json_fulfillhtlc(struct command *cmd,
 		return;
 	}
 
+	if (!json_tok_u64(buffer, idtok, &id)) {
+		command_fail(cmd, "'%.*s' is not a valid id",
+			     (int)(idtok->end - idtok->start),
+			     buffer + idtok->start);
+		return;
+	}
+
 	if (!hex_decode(buffer + rtok->start,
 			rtok->end - rtok->start,
 			&r, sizeof(r))) {
@@ -3461,14 +3433,24 @@ static void json_fulfillhtlc(struct command *cmd,
 		return;
 	}
 
-	sha256(&rhash, &r, sizeof(r));
+	htlc = htlc_get(&peer->htlcs, id, REMOTE);
-
-	htlc = find_their_committed_htlc(peer, &rhash);
 	if (!htlc) {
 		command_fail(cmd, "preimage htlc not found");
 		return;
 	}
 
+	if (htlc->state != RCVD_ADD_ACK_REVOCATION) {
+		command_fail(cmd, "htlc in state %s",
+			     htlc_state_name(htlc->state));
+		return;
+	}
+
+	sha256(&rhash, &r, sizeof(r));
+	if (!structeq(&htlc->rhash, &rhash)) {
+		command_fail(cmd, "preimage incorrect");
+		return;
+	}
+
 	assert(!htlc->r);
 	htlc->r = tal_dup(htlc, struct rval, &r);
 
@@ -3483,7 +3465,7 @@ static void json_fulfillhtlc(struct command *cmd,
 const struct json_command fulfillhtlc_command = {
 	"fulfillhtlc",
 	json_fulfillhtlc,
-	"Redeem htlc proposed by {peerid} using {r}",
+	"Redeem htlc proposed by {peerid} of {id} using {r}",
 	"Returns an empty result on success"
 };
 
@@ -3491,15 +3473,15 @@ static void json_failhtlc(struct command *cmd,
 			  const char *buffer, const jsmntok_t *params)
 {
 	struct peer *peer;
-	jsmntok_t *peeridtok, *rhashtok;
+	jsmntok_t *peeridtok, *idtok;
-	struct sha256 rhash;
+	u64 id;
 	struct htlc *htlc;
 
 	if (!json_get_params(buffer, params,
 			     "peerid", &peeridtok,
-			     "rhash", &rhashtok,
+			     "id", &idtok,
 			     NULL)) {
-		command_fail(cmd, "Need peerid and rhash");
+		command_fail(cmd, "Need peerid and id");
 		return;
 	}
 
@@ -3514,22 +3496,25 @@ static void json_failhtlc(struct command *cmd,
 		return;
 	}
 
-	if (!hex_decode(buffer + rhashtok->start,
+	if (!json_tok_u64(buffer, idtok, &id)) {
-			rhashtok->end - rhashtok->start,
+		command_fail(cmd, "'%.*s' is not a valid id",
-			&rhash, sizeof(rhash))) {
+			     (int)(idtok->end - idtok->start),
-		command_fail(cmd, "'%.*s' is not a valid sha256 preimage",
+			     buffer + idtok->start);
-			     (int)(rhashtok->end - rhashtok->start),
-			     buffer + rhashtok->start);
 		return;
 	}
 
-	/* Look in peer->remote.staging_cstate->a, as that's where we'll 
+	htlc = htlc_get(&peer->htlcs, id, REMOTE);
-	 * immediately remove it from: avoids double-handling. */
-	htlc = find_their_committed_htlc(peer, &rhash);
 	if (!htlc) {
-		command_fail(cmd, "htlc not found");
+		command_fail(cmd, "preimage htlc not found");
 		return;
 	}
+
+	if (htlc->state != RCVD_ADD_ACK_REVOCATION) {
+		command_fail(cmd, "htlc in state %s",
+			     htlc_state_name(htlc->state));
+		return;
+	}
+
 	if (command_htlc_fail(peer, htlc))
 		command_success(cmd, null_response(cmd));
 	else
@@ -3541,7 +3526,7 @@ static void json_failhtlc(struct command *cmd,
 const struct json_command failhtlc_command = {
 	"failhtlc",
 	json_failhtlc,
-	"Fail htlc proposed by {peerid} which has redeem hash {rhash}",
+	"Fail htlc proposed by {peerid} which has {id}",
 	"Returns an empty result on success"
 };
 

daemon/test/test.sh

@@ -431,7 +431,7 @@ if [ -n "$DIFFERENT_FEES" ]; then
     [ ! -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 , state : RCVD_ADD_ACK_REVOCATION } "
-    lcli2 fulfillhtlc $ID1 $SECRET
+    lcli2 fulfillhtlc $ID1 $HTLCID $SECRET
     [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
     [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
 
@@ -557,7 +557,7 @@ if [ -n "$DUMP_ONCHAIN" ]; then
     all_ok
 fi
     
-lcli2 fulfillhtlc $ID1 $SECRET
+lcli2 fulfillhtlc $ID1 $HTLCID $SECRET
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
 
 # Without manual commit, this check is racy.
@@ -597,7 +597,7 @@ 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 , state : SENT_ADD_ACK_REVOCATION } " $B_AMOUNT $B_FEE ""
 
-lcli2 failhtlc $ID1 $RHASH
+lcli2 failhtlc $ID1 $HTLCID
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
 
@@ -667,7 +667,7 @@ HTLCID=`lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH | extract_id`
 
 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 $B_FEE ""
 
-lcli2 fulfillhtlc $ID1 $SECRET
+lcli2 fulfillhtlc $ID1 $HTLCID $SECRET
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
 
@@ -702,12 +702,12 @@ if [ -n "$CLOSE_WITH_HTLCS" ]; then
     check_peerstate lcli2 STATE_CLEARING
 
     # Fail one, still waiting.
-    lcli2 failhtlc $ID1 $RHASH
+    lcli2 failhtlc $ID1 $HTLCID
     check_peerstate lcli1 STATE_CLEARING
     check_peerstate lcli2 STATE_CLEARING
 
     # Fulfill the other causes them to actually complete the close.
-    lcli1 fulfillhtlc $ID2 $SECRET2
+    lcli1 fulfillhtlc $ID2 $HTLCID2 $SECRET2
     check_peerstate lcli1 STATE_MUTUAL_CLOSING
     check_peerstate lcli2 STATE_MUTUAL_CLOSING
 
@@ -728,8 +728,8 @@ if [ -n "$CLOSE_WITH_HTLCS" ]; then
     all_ok
 fi
 
-lcli2 failhtlc $ID1 $RHASH
+lcli2 failhtlc $ID1 $HTLCID
-lcli1 fulfillhtlc $ID2 $SECRET2
+lcli1 fulfillhtlc $ID2 $HTLCID2 $SECRET2
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
@@ -776,8 +776,8 @@ lcli1 getpeers | tr -s '\012\011" ' ' ' | $FGREP "our_htlcs : [ { msatoshis : $H
 lcli2 getpeers | tr -s '\012\011" ' ' ' | $FGREP "our_htlcs : [ ], their_htlcs : [ { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , state : RCVD_ADD_ACK_REVOCATION }, { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 , state : RCVD_ADD_ACK_REVOCATION } ]" || lcli2 getpeers | tr -s '\012\011" ' ' ' | $FGREP "our_htlcs : [ ], their_htlcs : [ { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH2 , state : RCVD_ADD_ACK_REVOCATION }, { msatoshis : $HTLC_AMOUNT, expiry : { block : $EXPIRY }, rhash : $RHASH , state : RCVD_ADD_ACK_REVOCATION } ]"
 
 # Node2 collects the HTLCs.
-lcli2 fulfillhtlc $ID1 $SECRET
+lcli2 fulfillhtlc $ID1 $HTLCID $SECRET
-lcli2 fulfillhtlc $ID1 $SECRET2
+lcli2 fulfillhtlc $ID1 $HTLCID2 $SECRET2
 
 [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1
 [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2