From f1af56fceed6fbda22d4084f17916efa51643191 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 29 Jun 2016 06:49:20 +0930 Subject: [PATCH] daemon: save acked changes, so we can process them when confirmed on both sides. We need to know when changes are fully committed by both sides: 1) For their HTLC_ADDs, this is when we can fulfill/fail/route. 2) For their HTLC_FAILs, this is when we can fail incoming. For HTLC_FULFULL we don't need to wait: as soon as we know the preimage we can propogate it. For the moment, we simply log and assert; acting on it comes later. Signed-off-by: Rusty Russell --- daemon/packets.c | 15 ++++++++++ daemon/peer.c | 71 +++++++++++++++++++++++++++++++++++++++++++++ daemon/peer.h | 10 +++++++ daemon/test/test.sh | 29 ++++++++++++++++++ 4 files changed, 125 insertions(+) diff --git a/daemon/packets.c b/daemon/packets.c index a0ef7373c..78e534643 100644 --- a/daemon/packets.c +++ b/daemon/packets.c @@ -85,6 +85,7 @@ static struct commit_info *new_commit_info(const tal_t *ctx) { struct commit_info *ci = talz(ctx, struct commit_info); ci->unacked_changes = tal_arr(ci, union htlc_staging, 0); + ci->acked_changes = tal_arr(ci, union htlc_staging, 0); return ci; } @@ -402,6 +403,7 @@ void queue_pkt_revocation(struct peer *peer) */ /* Note: this means the unacked changes as of the commit we're * revoking */ + add_acked_changes(&peer->remote.commit->acked_changes, ci->unacked_changes); apply_changeset(peer, &peer->remote, THEIRS, ci->unacked_changes, tal_count(ci->unacked_changes)); @@ -410,6 +412,12 @@ void queue_pkt_revocation(struct peer *peer) /* We should never look at this again. */ ci->unacked_changes = tal_free(ci->unacked_changes); + + /* That revocation has committed us to changes in the current commitment. + * Any acked changes come from their commitment, so those are now committed + * by both of us. + */ + peer_both_committed_to(peer, ci->acked_changes, OURS); } Pkt *pkt_err(struct peer *peer, const char *msg, ...) @@ -848,6 +856,7 @@ Pkt *accept_pkt_revocation(struct peer *peer, const Pkt *pkt) * The receiver of `update_revocation`... MUST add the remote * unacked changes to the set of local acked changes. */ + add_acked_changes(&peer->local.commit->acked_changes, ci->unacked_changes); apply_changeset(peer, &peer->local, OURS, ci->unacked_changes, tal_count(ci->unacked_changes)); @@ -855,6 +864,12 @@ Pkt *accept_pkt_revocation(struct peer *peer, const Pkt *pkt) /* Should never examine these again. */ ci->unacked_changes = tal_free(ci->unacked_changes); + /* That revocation has committed them to changes in the current commitment. + * Any acked changes come from our commitment, so those are now committed + * by both of us. + */ + peer_both_committed_to(peer, ci->acked_changes, THEIRS); + return NULL; } diff --git a/daemon/peer.c b/daemon/peer.c index 179a192dc..063b22625 100644 --- a/daemon/peer.c +++ b/daemon/peer.c @@ -2144,6 +2144,77 @@ void add_unacked(struct peer_visible_state *which, which->commit->unacked_changes[n] = *stage; } +void add_acked_changes(union htlc_staging **acked, + const union htlc_staging *changes) +{ + size_t n_acked, n_changes; + + n_acked = tal_count(*acked); + n_changes = tal_count(changes); + tal_resize(acked, n_acked + n_changes); + memcpy(*acked + n_acked, changes, n_changes * sizeof(*changes)); +} + +static const char *owner_name(enum channel_side side) +{ + return side == OURS ? "our" : "their"; +} + +/* When changes are committed to. */ +void peer_both_committed_to(struct peer *peer, + const union htlc_staging *changes, + enum channel_side side) +{ + size_t i, n = tal_count(changes); + + for (i = 0; i < n; i++) { + u64 htlc_id; + const char *type, *owner; + + switch (changes[i].type) { + case HTLC_ADD: + type = "ADD"; + htlc_id = changes[i].add.htlc.id; + owner = owner_name(side); + assert(funding_htlc_by_id(peer->remote.commit->cstate, htlc_id, + side)); + assert(funding_htlc_by_id(peer->local.commit->cstate, htlc_id, + side)); + goto print; + case HTLC_FAIL: + type = "FAIL"; + htlc_id = changes[i].fail.id; + owner = owner_name(!side); + assert(!funding_htlc_by_id(peer->remote.commit->cstate, htlc_id, + !side)); + assert(!funding_htlc_by_id(peer->local.commit->cstate, htlc_id, + !side)); + assert(funding_htlc_by_id(peer->remote.commit->prev->cstate, + htlc_id, !side) + || funding_htlc_by_id(peer->local.commit->prev->cstate, + htlc_id, !side)); + goto print; + case HTLC_FULFILL: + type = "FULFILL"; + htlc_id = changes[i].fulfill.id; + owner = owner_name(!side); + assert(!funding_htlc_by_id(peer->remote.commit->cstate, htlc_id, + !side)); + assert(!funding_htlc_by_id(peer->local.commit->cstate, htlc_id, + !side)); + assert(funding_htlc_by_id(peer->remote.commit->prev->cstate, + htlc_id, !side) + || funding_htlc_by_id(peer->local.commit->prev->cstate, + htlc_id, !side)); + goto print; + } + abort(); + print: + log_debug(peer->log, "Both committed to %s of %s HTLC %"PRIu64, + type, owner, htlc_id); + } +} + /* Sets up the initial cstate and commit tx for both nodes: false if * insufficient funds. */ bool setup_first_commit(struct peer *peer) diff --git a/daemon/peer.h b/daemon/peer.h index d5083122a..77335a101 100644 --- a/daemon/peer.h +++ b/daemon/peer.h @@ -73,6 +73,8 @@ struct commit_info { struct sha256 *revocation_preimage; /* unacked changes (already applied to staging_cstate) */ union htlc_staging *unacked_changes; + /* acked changes (already applied to staging_cstate) */ + union htlc_staging *acked_changes; }; struct peer_visible_state { @@ -228,6 +230,14 @@ void remote_changes_pending(struct peer *peer); void add_unacked(struct peer_visible_state *which, const union htlc_staging *stage); +/* These unacked changes are now acked; add them to acked set. */ +void add_acked_changes(union htlc_staging **acked, + const union htlc_staging *changes); + +/* Both sides are committed to these changes they proposed. */ +void peer_both_committed_to(struct peer *peer, + const union htlc_staging *changes, enum channel_side side); + /* Peer has recieved revocation. */ void peer_update_complete(struct peer *peer); diff --git a/daemon/test/test.sh b/daemon/test/test.sh index 413a06ac6..85eddc912 100755 --- a/daemon/test/test.sh +++ b/daemon/test/test.sh @@ -439,8 +439,22 @@ if [ -n "$MANUALCOMMIT" ]; then # Node 2 has it committed. check_status_single lcli2 $B_AMOUNT $B_FEE "" $A_AMOUNT $A_FEE '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "second" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' + # There should be no "both committed" here yet + if lcli1 getlog debug | $FGREP "Both committed"; then + echo "Node1 thinks they are both committed"; + exit 1 + fi + if lcli2 getlog debug | $FGREP "Both committed"; then + echo "Node2 thinks they are both committed"; + exit 1 + fi + # Now node2 gives commitment to node1. lcli2 commit $ID1 + + # After revocation, they should know they're both committed. + check lcli1 "getlog debug | $FGREP 'Both committed to ADD of our HTLC'" + check lcli2 "getlog debug | $FGREP 'Both committed to ADD of their HTLC'" else A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) A_FEE=$(($A_FEE + $EXTRA_FEE)) @@ -498,8 +512,23 @@ fi lcli2 fulfillhtlc $ID1 $SECRET [ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 + +# Without manual commit, this check is racy. +if [ -n "$MANUALCOMMIT" ]; then + if lcli1 getlog debug | $FGREP 'Both committed to FULFILL'; then + echo "Node1 thinks they are both committed"; + exit 1 + fi + if lcli2 getlog debug | $FGREP 'Both committed to FULFILL'; then + echo "Node2 thinks they are both committed"; + exit 1 + fi +fi [ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 +check lcli1 "getlog debug | $FGREP 'Both committed to FULFILL of our HTLC'" +check lcli2 "getlog debug | $FGREP 'Both committed to FULFILL of their HTLC'" + # We've transferred the HTLC amount to 2, who now has to pay fees, # so no net change for A who saves on fees. B_FEE=$HTLC_AMOUNT