funding: make funding_htlc_by_id() return pointer, not offset.

While the pointer is only valid until the funding changes, that's also true of the offset; and a pointer has a convenient "not found" sentinal value. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
9 years ago · eeb9b9de84
3 changed files with 67 additions and 61 deletions
--- a/daemon/packets.c
+++ b/daemon/packets.c
@ -208,7 +208,7 @@ void queue_pkt_htlc_add(struct peer *peer, const struct channel_htlc *htlc)
 void queue_pkt_htlc_fulfill(struct peer *peer, u64 id, const struct rval *r)
 {
 	UpdateFulfillHtlc *f = tal(peer, UpdateFulfillHtlc);
-	size_t n;
+	struct channel_htlc *htlc;
 	union htlc_staging stage;

 	update_fulfill_htlc__init(f);
@ -220,9 +220,9 @@ void queue_pkt_htlc_fulfill(struct peer *peer, u64 id, const struct rval *r)
 	 * The sending node MUST add the HTLC fulfill/fail to the
 	 * unacked changeset for its remote commitment
 	 */
-	n = funding_htlc_by_id(peer->remote.staging_cstate, f->id, THEIRS);
-	assert(n != -1);
-	funding_fulfill_htlc(peer->remote.staging_cstate, n, THEIRS);
+	htlc = funding_htlc_by_id(peer->remote.staging_cstate, f->id, THEIRS);
+	assert(htlc);
+	funding_fulfill_htlc(peer->remote.staging_cstate, htlc, THEIRS);

 	stage.fulfill.fulfill = HTLC_FULFILL;
 	stage.fulfill.id = f->id;
@ -237,7 +237,7 @@ void queue_pkt_htlc_fulfill(struct peer *peer, u64 id, const struct rval *r)
 void queue_pkt_htlc_fail(struct peer *peer, u64 id)
 {
 	UpdateFailHtlc *f = tal(peer, UpdateFailHtlc);
-	size_t n;
+	struct channel_htlc *htlc;
 	union htlc_staging stage;

 	update_fail_htlc__init(f);
@ -252,9 +252,9 @@ void queue_pkt_htlc_fail(struct peer *peer, u64 id)
 	 * The sending node MUST add the HTLC fulfill/fail to the
 	 * unacked changeset for its remote commitment
 	 */
-	n = funding_htlc_by_id(peer->remote.staging_cstate, f->id, THEIRS);
-	assert(n != -1);
-	funding_fail_htlc(peer->remote.staging_cstate, n, THEIRS);
+	htlc = funding_htlc_by_id(peer->remote.staging_cstate, f->id, THEIRS);
+	assert(htlc);
+	funding_fail_htlc(peer->remote.staging_cstate, htlc, THEIRS);

 	stage.fail.fail = HTLC_FAIL;
 	stage.fail.id = f->id;
@ -328,14 +328,14 @@ static void apply_changeset(struct peer *peer,
 			    size_t num_changes)
 {
 	size_t i;
-	size_t n;
+	struct channel_htlc *htlc;

 	for (i = 0; i < num_changes; i++) {
 		switch (changes[i].type) {
 		case HTLC_ADD:
-			n = funding_htlc_by_id(which->staging_cstate,
-					       changes[i].add.htlc.id, side);
-			if (n != -1)
+			htlc = funding_htlc_by_id(which->staging_cstate,
+						  changes[i].add.htlc.id, side);
+			if (htlc)
 				fatal("Can't add duplicate HTLC id %"PRIu64,
 				      changes[i].add.htlc.id);
 			if (!funding_add_htlc(which->staging_cstate,
@ -347,20 +347,20 @@ static void apply_changeset(struct peer *peer,
 				      side == OURS ? "ours" : "theirs");
 			continue;
 		case HTLC_FAIL:
-			n = funding_htlc_by_id(which->staging_cstate,
+			htlc = funding_htlc_by_id(which->staging_cstate,
 					       changes[i].fail.id, !side);
-			if (n == -1)
+			if (!htlc)
 				fatal("Can't fail non-exisent HTLC id %"PRIu64,
 				      changes[i].fail.id);
-			funding_fail_htlc(which->staging_cstate, n, !side);
+			funding_fail_htlc(which->staging_cstate, htlc, !side);
 			continue;
 		case HTLC_FULFILL:
-			n = funding_htlc_by_id(which->staging_cstate,
-					       changes[i].fulfill.id, !side);
-			if (n == -1)
+			htlc = funding_htlc_by_id(which->staging_cstate,
+						  changes[i].fulfill.id, !side);
+			if (!htlc)
 				fatal("Can't fulfill non-exisent HTLC id %"PRIu64,
 				      changes[i].fulfill.id);
-			funding_fulfill_htlc(which->staging_cstate, n, !side);
+			funding_fulfill_htlc(which->staging_cstate, htlc, !side);
 			continue;
 		}
 		abort();
@ -628,13 +628,13 @@ Pkt *accept_pkt_htlc_add(struct peer *peer, const Pkt *pkt)
 	/* Note that it's not *our* problem if they do this, it's
 	 * theirs (future confusion).  Nonetheless, we detect and
 	 * error for them. */
-	if (funding_htlc_by_id(peer->remote.staging_cstate, u->id, THEIRS) != -1
-	    || funding_htlc_by_id(peer->remote.commit->cstate, u->id, THEIRS) != -1) {
+	if (funding_htlc_by_id(peer->remote.staging_cstate, u->id, THEIRS)
+	    || funding_htlc_by_id(peer->remote.commit->cstate, u->id, THEIRS)) {
 		return pkt_err(peer, "HTLC id %"PRIu64" clashes for you", u->id);
 	}

-	if (funding_htlc_by_id(peer->local.staging_cstate, u->id, THEIRS) != -1
-	    || funding_htlc_by_id(peer->local.commit->cstate, u->id, THEIRS) != -1) {
+	if (funding_htlc_by_id(peer->local.staging_cstate, u->id, THEIRS)
+	    || funding_htlc_by_id(peer->local.commit->cstate, u->id, THEIRS)) {
 		return pkt_err(peer, "HTLC id %"PRIu64" clashes for you", u->id);
 	}

@ -672,9 +672,10 @@ Pkt *accept_pkt_htlc_add(struct peer *peer, const Pkt *pkt)
 	return NULL;
 }

-static Pkt *find_commited_htlc(struct peer *peer, uint64_t id, size_t *n_local)
+static Pkt *find_commited_htlc(struct peer *peer, uint64_t id,
+			       struct channel_htlc **local_htlc)
 {
-	size_t n;
+	struct channel_htlc *htlc;

 	/* BOLT #2:
 	 *
@ -682,13 +683,13 @@ static Pkt *find_commited_htlc(struct peer *peer, uint64_t id, size_t *n_local)
 	 * current commitment transaction, and MUST fail the
 	 * connection if it does not.
 	 */
-	n = funding_htlc_by_id(peer->local.commit->cstate, id, OURS);
-	if (n == -1)
+	htlc = funding_htlc_by_id(peer->local.commit->cstate, id, OURS);
+	if (!htlc)
 		return pkt_err(peer, "Did not find HTLC %"PRIu64, id);

 	/* They must not fail/fulfill twice, so it should be in staging, too. */
-	*n_local = funding_htlc_by_id(peer->local.staging_cstate, id, OURS);
-	if (*n_local == -1)
+	*local_htlc = funding_htlc_by_id(peer->local.staging_cstate, id, OURS);
+	if (!*local_htlc)
 		return pkt_err(peer, "Already removed HTLC %"PRIu64, id);

 	return NULL;
@ -697,17 +698,17 @@ static Pkt *find_commited_htlc(struct peer *peer, uint64_t id, size_t *n_local)
 Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt)
 {
 	const UpdateFailHtlc *f = pkt->update_fail_htlc;
-	size_t n_local;
+	struct channel_htlc *htlc;
 	Pkt *err;
 	union htlc_staging stage;

-	err = find_commited_htlc(peer, f->id, &n_local);
+	err = find_commited_htlc(peer, f->id, &htlc);
 	if (err)
 		return err;

 	/* FIXME: Save reason. */

-	funding_fail_htlc(peer->local.staging_cstate, n_local, OURS);
+	funding_fail_htlc(peer->local.staging_cstate, htlc, OURS);

 	/* BOLT #2:
 	 *
@ -723,13 +724,13 @@ Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt)
 Pkt *accept_pkt_htlc_fulfill(struct peer *peer, const Pkt *pkt)
 {
 	const UpdateFulfillHtlc *f = pkt->update_fulfill_htlc;
-	size_t n_local;
+	struct channel_htlc *htlc;
 	struct sha256 rhash;
 	struct rval r;
 	Pkt *err;
 	union htlc_staging stage;

-	err = find_commited_htlc(peer, f->id, &n_local);
+	err = find_commited_htlc(peer, f->id, &htlc);
 	if (err)
 		return err;

@ -737,7 +738,7 @@ Pkt *accept_pkt_htlc_fulfill(struct peer *peer, const Pkt *pkt)
 	proto_to_rval(f->r, &r);
 	sha256(&rhash, &r, sizeof(r));

-	if (!structeq(&rhash, &peer->local.staging_cstate->side[OURS].htlcs[n_local].rhash))
+	if (!structeq(&rhash, &htlc->rhash))
 		return pkt_err(peer, "Invalid r for %"PRIu64, f->id);

 	/* BOLT #2:
@ -745,7 +746,7 @@ Pkt *accept_pkt_htlc_fulfill(struct peer *peer, const Pkt *pkt)
 	 * ... and the receiving node MUST add the HTLC fulfill/fail
 	 * to the unacked changeset for its local commitment.
 	 */
-	funding_fulfill_htlc(peer->local.staging_cstate, n_local, OURS);
+	funding_fulfill_htlc(peer->local.staging_cstate, htlc, OURS);

 	stage.fulfill.fulfill = HTLC_FULFILL;
 	stage.fulfill.id = f->id;
--- a/funding.c
+++ b/funding.c
@ -251,45 +251,48 @@ struct channel_htlc *funding_add_htlc(struct channel_state *cstate,
 static void remove_htlc(struct channel_state *cstate,
 			enum channel_side creator,
 			enum channel_side beneficiary,
-			size_t i)
+			struct channel_htlc *htlc)
 {
-	size_t n = tal_count(cstate->side[creator].htlcs);
 	size_t nondust;
+	size_t n = tal_count(cstate->side[creator].htlcs); 
+	const struct channel_htlc *end;

-	assert(i < n);
+	end = cstate->side[creator].htlcs + n;
+
+	assert(htlc >= cstate->side[creator].htlcs && htlc < end);

 	/* Remember to remove this one in total txsize if not dust! */
 	nondust = total_nondust_htlcs(cstate);
-	if (!is_dust_amount(cstate->side[creator].htlcs[i].msatoshis / 1000)) {
+	if (!is_dust_amount(htlc->msatoshis / 1000)) {
 		assert(nondust > 0);
 		nondust--;
 	}

 	/* Can't fail since msatoshis is positive. */
 	if (!change_funding(cstate->anchor, cstate->fee_rate,
-			    -(int64_t)cstate->side[creator].htlcs[i].msatoshis,
+			    -(int64_t)htlc->msatoshis,
 			    &cstate->side[beneficiary],
 			    &cstate->side[!beneficiary], nondust))
 		abort();

 	/* Actually remove the HTLC. */
-	memmove(cstate->side[creator].htlcs + i,
-		cstate->side[creator].htlcs + i + 1,
-		(n - i - 1) * sizeof(*cstate->side[creator].htlcs));
+	memmove(htlc, htlc + 1, (end - htlc - 1) * sizeof(*htlc));
 	tal_resize(&cstate->side[creator].htlcs, n-1);
 	cstate->changes++;
 }

-void funding_fail_htlc(struct channel_state *cstate, size_t index,
+void funding_fail_htlc(struct channel_state *cstate,
+		       struct channel_htlc *htlc,
 		       enum channel_side side)
 {
-	remove_htlc(cstate, side, side, index);
+	remove_htlc(cstate, side, side, htlc);
 }

-void funding_fulfill_htlc(struct channel_state *cstate, size_t index,
+void funding_fulfill_htlc(struct channel_state *cstate,
+			  struct channel_htlc *htlc,
 			  enum channel_side side)
 {
-	remove_htlc(cstate, side, !side, index);
+	remove_htlc(cstate, side, !side, htlc);
 }

 size_t funding_find_htlc(const struct channel_state *cstate,
@ -305,17 +308,17 @@ size_t funding_find_htlc(const struct channel_state *cstate,
 	return -1;
 }

-size_t funding_htlc_by_id(const struct channel_state *cstate,
-			  uint64_t id,
-			  enum channel_side side)
+struct channel_htlc *funding_htlc_by_id(const struct channel_state *cstate,
+					uint64_t id,
+					enum channel_side side)
 {
 	size_t i;

 	for (i = 0; i < tal_count(cstate->side[side].htlcs); i++) {
 		if (cstate->side[side].htlcs[i].id == id)
-			return i;
+			return &cstate->side[side].htlcs[i];
 	}
-	return -1;
+	return NULL;
 }

 struct channel_state *copy_funding(const tal_t *ctx,
--- a/funding.h
+++ b/funding.h
@ -82,25 +82,27 @@ struct channel_htlc *funding_add_htlc(struct channel_state *cstate,
 /**
 * funding_fail_htlc: remove an HTLC, funds to the side which offered it.
 * @cstate: The channel state
- * @index: the index into cstate->side[dir].htlcs[].
+ * @htlc: the htlc in cstate->side[dir].htlcs[].
 * @side: OURS or THEIRS
 *
 * This will remove the @index'th entry in cstate->side[dir].htlcs[], and credit
 * the value of the HTLC (back) to cstate->side[dir].
 */
-void funding_fail_htlc(struct channel_state *cstate, size_t index,
+void funding_fail_htlc(struct channel_state *cstate,
+		       struct channel_htlc *htlc,
 		       enum channel_side side);

 /**
 * funding_fulfill_htlc: remove an HTLC, funds to side which accepted it.
 * @cstate: The channel state
- * @index: the index into cstate->a.htlcs[].
+ * @htlc: the htlc in cstate->side[dir].htlcs[].
 * @side: OURS or THEIRS
 *
 * This will remove the @index'th entry in cstate->side[dir].htlcs[], and credit
 * the value of the HTLC to cstate->side[!dir].
 */
-void funding_fulfill_htlc(struct channel_state *cstate, size_t index,
+void funding_fulfill_htlc(struct channel_state *cstate,
+			  struct channel_htlc *htlc,
 			  enum channel_side side);

 /**
@ -139,11 +141,11 @@ size_t funding_find_htlc(const struct channel_state *cstate,
 * @id: id for HTLC.
 * @side: OURS or THEIRS
 *
- * Returns a number < tal_count(cstate->side[dir].htlcs), or -1 on fail.
+ * Returns a pointer into cstate->side[@side].htlcs, or NULL.
 */
-size_t funding_htlc_by_id(const struct channel_state *cstate,
-			  uint64_t id,
-			  enum channel_side side);
+struct channel_htlc *funding_htlc_by_id(const struct channel_state *cstate,
+					uint64_t id,
+					enum channel_side side);

 /**
 * fee_for_feerate: calculate the fee (in satoshi) for a given fee_rate.