From eeb9b9de848dcece7d27d865be717b32cae23b01 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 29 Jun 2016 06:49:20 +0930 Subject: [PATCH] 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 --- daemon/packets.c | 73 ++++++++++++++++++++++++------------------------ funding.c | 37 +++++++++++++----------- funding.h | 18 ++++++------ 3 files changed, 67 insertions(+), 61 deletions(-) diff --git a/daemon/packets.c b/daemon/packets.c index 850dc95c9..a0ef7373c 100644 --- 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; diff --git a/funding.c b/funding.c index f1dc6141c..fb7610d0a 100644 --- 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, diff --git a/funding.h b/funding.h index 72799f2dc..f8a84e8d6 100644 --- 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.