diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a871b20e31b..0dbc30980187 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ changes. - Protocol: fix occasional deadlock when both peers flood with gossip. - Protocol: fix occasional long delay on sending `reply_short_channel_ids_end`. - Protocol: re-send `node_announcement` when address/alias/color etc change. +- Protocol: multiple HTLCs with the same payment_hash are handled correctly. - Options: 'autotor' defaults to port 9051 if not specified. ### Security diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c index ad44e9982217..ca41d722053d 100644 --- a/channeld/commit_tx.c +++ b/channeld/commit_tx.c @@ -107,6 +107,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, u64 base_fee_msat; struct bitcoin_tx *tx; size_t i, n, untrimmed; + u32 *cltvs; assert(self_pay_msat + other_pay_msat <= funding_satoshis * 1000); @@ -158,9 +159,11 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, tx = bitcoin_tx(ctx, 1, untrimmed + 2); /* We keep track of which outputs have which HTLCs */ - if (htlcmap) - *htlcmap = tal_arr(tx, const struct htlc *, - tal_count(tx->output)); + *htlcmap = tal_arr(tx, const struct htlc *, tal_count(tx->output)); + + /* We keep cltvs for tie-breaking HTLC outputs; we use the same order + * for sending the htlc txs, so it may matter. */ + cltvs = tal_arr(tmpctx, u32, tal_count(tx->output)); /* This could be done in a single loop, but we follow the BOLT * literally to make comments in test vectors clearer. */ @@ -177,8 +180,9 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, if (trim(htlcs[i], feerate_per_kw, dust_limit_satoshis, side)) continue; add_offered_htlc_out(tx, n, htlcs[i], keyset); - if (htlcmap) - (*htlcmap)[n++] = htlcs[i]; + (*htlcmap)[n] = htlcs[i]; + cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry); + n++; } /* BOLT #3: @@ -192,8 +196,9 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, if (trim(htlcs[i], feerate_per_kw, dust_limit_satoshis, side)) continue; add_received_htlc_out(tx, n, htlcs[i], keyset); - if (htlcmap) - (*htlcmap)[n++] = htlcs[i]; + (*htlcmap)[n] = htlcs[i]; + cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry); + n++; } /* BOLT #3: @@ -206,8 +211,9 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, u8 *wscript = to_self_wscript(tmpctx, to_self_delay,keyset); tx->output[n].amount = self_pay_msat / 1000; tx->output[n].script = scriptpubkey_p2wsh(tx, wscript); - if (htlcmap) - (*htlcmap)[n] = NULL; + (*htlcmap)[n] = NULL; + /* We don't assign cltvs[n]: if we use it, order doesn't matter. + * However, valgrind will warn us something wierd is happening */ SUPERVERBOSE("# to-local amount %"PRIu64" wscript %s\n", tx->output[n].amount, tal_hex(tmpctx, wscript)); @@ -231,8 +237,9 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, tx->output[n].amount = other_pay_msat / 1000; tx->output[n].script = scriptpubkey_p2wpkh(tx, &keyset->other_payment_key); - if (htlcmap) - (*htlcmap)[n] = NULL; + (*htlcmap)[n] = NULL; + /* We don't assign cltvs[n]: if we use it, order doesn't matter. + * However, valgrind will warn us something wierd is happening */ SUPERVERBOSE("# to-remote amount %"PRIu64" P2WPKH(%s)\n", tx->output[n].amount, type_to_string(tmpctx, struct pubkey, @@ -242,16 +249,14 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, assert(n <= tal_count(tx->output)); tal_resize(&tx->output, n); - if (htlcmap) - tal_resize(htlcmap, n); + tal_resize(htlcmap, n); /* BOLT #3: * * 7. Sort the outputs into [BIP 69 * order](#transaction-input-and-output-ordering) */ - permute_outputs(tx->output, tal_count(tx->output), - htlcmap ? (const void **)*htlcmap : NULL); + permute_outputs(tx->output, cltvs, (const void **)*htlcmap); /* BOLT #3: * diff --git a/channeld/commit_tx.h b/channeld/commit_tx.h index 2cf0e7ec8c1c..6f8decd6ddd2 100644 --- a/channeld/commit_tx.h +++ b/channeld/commit_tx.h @@ -33,7 +33,7 @@ size_t commit_tx_num_untrimmed(const struct htlc **htlcs, * @self_pay_msat: amount to pay directly to self * @other_pay_msat: amount to pay directly to the other side * @htlcs: tal_arr of htlcs committed by transaction (some may be trimmed) - * @htlc_map: outputed map of outnum->HTLC (NULL for direct outputs), or NULL. + * @htlc_map: outputed map of outnum->HTLC (NULL for direct outputs). * @obscured_commitment_number: number to encode in commitment transaction * @side: side to generate commitment transaction for. * diff --git a/common/close_tx.c b/common/close_tx.c index 9f880da508d9..6525c9b88ef4 100644 --- a/common/close_tx.c +++ b/common/close_tx.c @@ -58,6 +58,6 @@ struct bitcoin_tx *create_close_tx(const tal_t *ctx, return tal_free(tx); tal_resize(&tx->output, num_outputs); - permute_outputs(tx->output, num_outputs, NULL); + permute_outputs(tx->output, NULL, NULL); return tx; } diff --git a/common/funding_tx.c b/common/funding_tx.c index 35ce457ee5ed..28a5376e5fed 100644 --- a/common/funding_tx.c +++ b/common/funding_tx.c @@ -52,12 +52,12 @@ struct bitcoin_tx *funding_tx(const tal_t *ctx, map[1] = int2ptr(1); tx->output[1].script = scriptpubkey_p2wpkh(tx, changekey); tx->output[1].amount = change_satoshis; - permute_outputs(tx->output, tal_count(tx->output), map); + permute_outputs(tx->output, NULL, map); *outnum = (map[0] == int2ptr(0) ? 0 : 1); } else { *outnum = 0; } - permute_inputs(tx->input, tal_count(tx->input), (const void **)utxomap); + permute_inputs(tx->input, (const void **)utxomap); return tx; } diff --git a/common/initial_commit_tx.c b/common/initial_commit_tx.c index bdbff1232060..a64b044a42c9 100644 --- a/common/initial_commit_tx.c +++ b/common/initial_commit_tx.c @@ -194,7 +194,7 @@ struct bitcoin_tx *initial_commit_tx(const tal_t *ctx, * 7. Sort the outputs into [BIP 69 * order](#transaction-input-and-output-ordering) */ - permute_outputs(tx->output, tal_count(tx->output), NULL); + permute_outputs(tx->output, NULL, NULL); /* BOLT #3: * diff --git a/common/permute_tx.c b/common/permute_tx.c index 327d337d5740..e2e8138e96a2 100644 --- a/common/permute_tx.c +++ b/common/permute_tx.c @@ -54,10 +54,11 @@ static void swap_inputs(struct bitcoin_tx_input *inputs, } } -void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs, +void permute_inputs(struct bitcoin_tx_input *inputs, const void **map) { size_t i; + size_t num_inputs = tal_count(inputs); /* We can't permute nothing! */ if (num_inputs == 0) @@ -73,10 +74,10 @@ void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs, static void swap_outputs(struct bitcoin_tx_output *outputs, const void **map, + u32 *cltvs, size_t i1, size_t i2) { struct bitcoin_tx_output tmpoutput; - const void *tmp; if (i1 == i2) return; @@ -86,49 +87,74 @@ static void swap_outputs(struct bitcoin_tx_output *outputs, outputs[i2] = tmpoutput; if (map) { - tmp = map[i1]; + const void *tmp = map[i1]; map[i1] = map[i2]; map[i2] = tmp; } + + if (cltvs) { + u32 tmp = cltvs[i1]; + cltvs[i1] = cltvs[i2]; + cltvs[i2] = tmp; + } } static bool output_better(const struct bitcoin_tx_output *a, - const struct bitcoin_tx_output *b) + u32 cltv_a, + const struct bitcoin_tx_output *b, + u32 cltv_b) { - size_t len; + size_t len, lena, lenb; int ret; if (a->amount != b->amount) return a->amount < b->amount; /* Lexicographical sort. */ - if (tal_count(a->script) < tal_count(b->script)) - len = tal_count(a->script); + lena = tal_count(a->script); + lenb = tal_count(b->script); + if (lena < lenb) + len = lena; else - len = tal_count(b->script); + len = lenb; ret = memcmp(a->script, b->script, len); if (ret != 0) return ret < 0; - return tal_count(a->script) < tal_count(b->script); + if (lena != lenb) + return lena < lenb; + + return cltv_a < cltv_b; +} + +static u32 cltv_of(const u32 *cltvs, size_t idx) +{ + if (!cltvs) + return 0; + return cltvs[idx]; } -static size_t find_best_out(struct bitcoin_tx_output *outputs, size_t num) +static size_t find_best_out(struct bitcoin_tx_output *outputs, + const u32 *cltvs, + size_t num) { size_t i, best = 0; for (i = 1; i < num; i++) { - if (output_better(&outputs[i], &outputs[best])) + if (output_better(&outputs[i], cltv_of(cltvs, i), + &outputs[best], cltv_of(cltvs, best))) best = i; } return best; } -void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs, +void permute_outputs(struct bitcoin_tx_output *outputs, + u32 *cltvs, const void **map) { size_t i; + size_t num_outputs = tal_count(outputs); /* We can't permute nothing! */ if (num_outputs == 0) @@ -137,7 +163,9 @@ void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs, /* Now do a dumb sort (num_outputs is small). */ for (i = 0; i < num_outputs-1; i++) { /* Swap best into first place. */ - swap_outputs(outputs, map, - i, i + find_best_out(outputs + i, num_outputs - i)); + swap_outputs(outputs, map, cltvs, + i, i + find_best_out(outputs + i, + cltvs ? cltvs + i : NULL, + num_outputs - i)); } } diff --git a/common/permute_tx.h b/common/permute_tx.h index f5bbfad405de..6968c94fa185 100644 --- a/common/permute_tx.h +++ b/common/permute_tx.h @@ -5,15 +5,23 @@ struct htlc; -/* Permute the transaction into BIP69 order. */ -void permute_inputs(struct bitcoin_tx_input *inputs, size_t num_inputs, - const void **map); +/** + * permute_inputs: permute the transaction inputs into BIP69 order. + * @inputs: usually bitcoin_tx->inputs, must be tal_arr. + * @map: if non-NULL, pointers to be permuted the same as the inputs. + */ +void permute_inputs(struct bitcoin_tx_input *inputs, const void **map); -/* If @map is non-NULL, it will be permuted the same as the outputs. +/** + * permute_outputs: permute the transaction outputs into BIP69 + cltv order. + * @outputs: usually bitcoin_tx->outputs, must be tal_arr. + * @cltvs: CLTV delays to use as a tie-breaker, or NULL. + * @map: if non-NULL, pointers to be permuted the same as the outputs. * * So the caller initiates the map with which htlcs are used, it * can easily see which htlc (if any) is in output #0 with map[0]. */ -void permute_outputs(struct bitcoin_tx_output *outputs, size_t num_outputs, +void permute_outputs(struct bitcoin_tx_output *outputs, + u32 *cltvs, const void **map); #endif /* LIGHTNING_COMMON_PERMUTE_TX_H */ diff --git a/common/withdraw_tx.c b/common/withdraw_tx.c index 7cb0ac433f1a..05432c426e85 100644 --- a/common/withdraw_tx.c +++ b/common/withdraw_tx.c @@ -38,9 +38,9 @@ struct bitcoin_tx *withdraw_tx(const tal_t *ctx, map[1] = int2ptr(1); tx->output[1].script = scriptpubkey_p2wpkh(tx, changekey); tx->output[1].amount = changesat; - permute_outputs(tx->output, tal_count(tx->output), map); + permute_outputs(tx->output, NULL, map); } - permute_inputs(tx->input, tal_count(tx->input), (const void **)utxos); + permute_inputs(tx->input, (const void **)utxos); return tx; } diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index de2c138de15f..38a40e7f217f 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -152,7 +152,7 @@ struct htlc_out *htlc_out_check(const struct htlc_out *hout, " less than %"PRIu64, hout->in->msatoshi, hout->msatoshi); if (hout->in->cltv_expiry <= hout->cltv_expiry) - return corrupt(abortstr, "Input ctlv_expiry %u" + return corrupt(abortstr, "Input cltv_expiry %u" " less than %u", hout->in->cltv_expiry, hout->cltv_expiry); if (!sha256_eq(&hout->in->payment_hash, &hout->payment_hash)) diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index d8d8ab553c76..13759b4cbfab 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -343,8 +343,12 @@ static bool tell_if_missing(const struct channel *channel, /* Keep valgrind happy. */ *tell_immediate = false; - /* Is it a current HTLC? */ - hout = find_htlc_out_by_ripemd(channel, &stub->ripemd); + /* Don't care about incoming HTLCs, just ones we offered. */ + if (stub->owner == REMOTE) + return false; + + /* Might not be a current HTLC. */ + hout = find_htlc_out(&channel->peer->ld->htlcs_out, channel, stub->id); if (!hout) return false; diff --git a/lightningd/options.c b/lightningd/options.c index 6edc57845aa2..e66600a9e943 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -316,10 +316,10 @@ static void config_register_opts(struct lightningd *ld) "Percentage of fee to request for their commitment"); opt_register_arg("--cltv-delta", opt_set_u32, opt_show_u32, &ld->config.cltv_expiry_delta, - "Number of blocks for ctlv_expiry_delta"); + "Number of blocks for cltv_expiry_delta"); opt_register_arg("--cltv-final", opt_set_u32, opt_show_u32, &ld->config.cltv_final, - "Number of blocks for final ctlv_expiry"); + "Number of blocks for final cltv_expiry"); opt_register_arg("--commit-time=", opt_set_u32, opt_show_u32, &ld->config.commit_time_ms, diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 55d8d9e58f22..8cdcea2414be 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -823,36 +823,16 @@ static bool peer_failed_our_htlc(struct channel *channel, return true; } -/* FIXME: Crazy slow! */ -struct htlc_out *find_htlc_out_by_ripemd(const struct channel *channel, - const struct ripemd160 *ripemd) -{ - struct htlc_out_map_iter outi; - struct htlc_out *hout; - struct lightningd *ld = channel->peer->ld; - - for (hout = htlc_out_map_first(&ld->htlcs_out, &outi); - hout; - hout = htlc_out_map_next(&ld->htlcs_out, &outi)) { - struct ripemd160 hash; - - if (hout->key.channel != channel) - continue; - - ripemd160(&hash, - &hout->payment_hash, sizeof(hout->payment_hash)); - if (ripemd160_eq(&hash, ripemd)) - return hout; - } - return NULL; -} - void onchain_failed_our_htlc(const struct channel *channel, const struct htlc_stub *htlc, const char *why) { struct lightningd *ld = channel->peer->ld; - struct htlc_out *hout = find_htlc_out_by_ripemd(channel, &htlc->ripemd); + struct htlc_out *hout; + + hout = find_htlc_out(&ld->htlcs_out, channel, htlc->id); + if (!hout) + return; /* Don't fail twice (or if already succeeded)! */ if (hout->failuremsg || hout->failcode || hout->preimage) @@ -863,7 +843,7 @@ void onchain_failed_our_htlc(const struct channel *channel, /* Force state to something which expects a failure, and save to db */ hout->hstate = RCVD_REMOVE_HTLC; htlc_out_check(hout, __func__); - wallet_htlc_update(channel->peer->ld->wallet, hout->dbid, hout->hstate, + wallet_htlc_update(ld->wallet, hout->dbid, hout->hstate, hout->preimage, hout->failcode, hout->failuremsg); if (hout->am_origin) { diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index a36c01b965b1..6f3ad3646e2b 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -50,8 +50,6 @@ enum onion_type send_htlc_out(struct channel *out, u64 amount, u32 cltv, struct htlc_in *in, struct htlc_out **houtp); -struct htlc_out *find_htlc_out_by_ripemd(const struct channel *channel, - const struct ripemd160 *ripemd160); void onchain_failed_our_htlc(const struct channel *channel, const struct htlc_stub *htlc, const char *why); diff --git a/onchaind/onchain_wire.c b/onchaind/onchain_wire.c index 18b11c2ce3e4..f6de30d7d62f 100644 --- a/onchaind/onchain_wire.c +++ b/onchaind/onchain_wire.c @@ -6,6 +6,7 @@ void towire_htlc_stub(u8 **pptr, const struct htlc_stub *htlc_stub) { towire_side(pptr, htlc_stub->owner); towire_u32(pptr, htlc_stub->cltv_expiry); + towire_u64(pptr, htlc_stub->id); towire_ripemd160(pptr, &htlc_stub->ripemd); } @@ -14,5 +15,6 @@ void fromwire_htlc_stub(const u8 **cursor, size_t *max, { htlc_stub->owner = fromwire_side(cursor, max); htlc_stub->cltv_expiry = fromwire_u32(cursor, max); + htlc_stub->id = fromwire_u64(cursor, max); fromwire_ripemd160(cursor, max, &htlc_stub->ripemd); } diff --git a/onchaind/onchain_wire.h b/onchaind/onchain_wire.h index b23a03e4bd1d..aeb0f50d7120 100644 --- a/onchaind/onchain_wire.h +++ b/onchaind/onchain_wire.h @@ -9,6 +9,7 @@ struct htlc_stub { enum side owner; u32 cltv_expiry; + u64 id; struct ripemd160 ripemd; }; diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index 835481f14589..26dc39a41438 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -144,18 +144,10 @@ static u64 grind_htlc_tx_fee(struct bitcoin_tx *tx, return fee; } - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "grind_fee failed from %u - %u" - " for tx %s, inputamount %"PRIu64", signature %s, wscript %s, multiplier %"PRIu64, - min_possible_feerate, max_possible_feerate, - type_to_string(tmpctx, struct bitcoin_tx, tx), - input_amount, - type_to_string(tmpctx, secp256k1_ecdsa_signature, remotesig), - tal_hex(tmpctx, wscript), - multiplier); + return UINT64_MAX; } -static void set_htlc_timeout_fee(struct bitcoin_tx *tx, +static bool set_htlc_timeout_fee(struct bitcoin_tx *tx, const secp256k1_ecdsa_signature *remotesig, const u8 *wscript) { @@ -170,21 +162,12 @@ static void set_htlc_timeout_fee(struct bitcoin_tx *tx, */ if (fee == UINT64_MAX) { fee = grind_htlc_tx_fee(tx, remotesig, wscript, 663); - return; + return fee != UINT64_MAX; } tx->output[0].amount = *tx->input[0].amount - fee; - if (check_tx_sig(tx, 0, NULL, wscript, - &keyset->other_htlc_key, remotesig)) - return; - - status_failed(STATUS_FAIL_INTERNAL_ERROR, - "htlc_timeout_fee %"PRIu64" failed sigcheck " - " for tx %s, signature %s, wscript %s", - fee, - type_to_string(tmpctx, struct bitcoin_tx, tx), - type_to_string(tmpctx, secp256k1_ecdsa_signature, remotesig), - tal_hex(tmpctx, wscript)); + return check_tx_sig(tx, 0, NULL, wscript, + &keyset->other_htlc_key, remotesig); } static void set_htlc_success_fee(struct bitcoin_tx *tx, @@ -1305,42 +1288,105 @@ static u8 **derive_htlc_scripts(const struct htlc_stub *htlcs, enum side side) return htlc_scripts; } -static void resolve_our_htlc_ourcommit(struct tracked_output *out) +static size_t resolve_our_htlc_ourcommit(struct tracked_output *out, + const size_t *matches, + const struct htlc_stub *htlcs, + u8 **htlc_scripts) { struct bitcoin_tx *tx; secp256k1_ecdsa_signature localsig; + size_t i; - /* BOLT #5: - * - * ## HTLC Output Handling: Local Commitment, Local Offers - * ... - * - if the commitment transaction HTLC output has *timed out* and - * hasn't been *resolved*: - * - MUST *resolve* the output by spending it using the HTLC-timeout - * transaction. - */ - tx = htlc_timeout_tx(out, &out->txid, out->outnum, out->satoshi * 1000, - out->htlc->cltv_expiry, - to_self_delay[LOCAL], 0, keyset); + assert(tal_count(matches)); - set_htlc_timeout_fee(tx, out->remote_htlc_sig, out->wscript); + /* These htlcs are all possibilities, but signature will only match + * one with the correct cltv: check which that is. */ + for (i = 0; i < tal_count(matches); i++) { + /* BOLT #5: + * + * ## HTLC Output Handling: Local Commitment, Local Offers + * ... + * - if the commitment transaction HTLC output has *timed out* + * and hasn't been *resolved*: + * - MUST *resolve* the output by spending it using the + * HTLC-timeout transaction. + */ + tx = htlc_timeout_tx(tmpctx, &out->txid, out->outnum, + out->satoshi * 1000, + htlcs[matches[i]].cltv_expiry, + to_self_delay[LOCAL], 0, keyset); - hsm_sign_local_htlc_tx(tx, out->wscript, &localsig); + if (set_htlc_timeout_fee(tx, out->remote_htlc_sig, + htlc_scripts[matches[i]])) + break; + } + + /* Since there's been trouble with this before, we go to some length + * to give details here! */ + if (i == tal_count(matches)) { + char *cltvs, *wscripts; + + cltvs = tal_fmt(tmpctx, "%u", htlcs[matches[0]].cltv_expiry); + wscripts = tal_hex(tmpctx, htlc_scripts[matches[0]]); + + for (i = 1; i < tal_count(matches); i++) { + tal_append_fmt(&cltvs, "/%u", + htlcs[matches[i]].cltv_expiry); + tal_append_fmt(&wscripts, "/%s", + tal_hex(tmpctx, htlc_scripts[matches[i]])); + } + + status_failed(STATUS_FAIL_INTERNAL_ERROR, + "No valid signature found for %zu htlc_timeout_txs" + " feerate %u-%u," + " last tx %s, inputamount %"PRIu64", signature %s," + " cltvs %s wscripts %s", + tal_count(matches), + min_possible_feerate, max_possible_feerate, + type_to_string(tmpctx, struct bitcoin_tx, tx), + out->satoshi, + type_to_string(tmpctx, secp256k1_ecdsa_signature, + out->remote_htlc_sig), + cltvs, wscripts); + } + + hsm_sign_local_htlc_tx(tx, htlc_scripts[matches[i]], &localsig); tx->input[0].witness = bitcoin_witness_htlc_timeout_tx(tx->input, &localsig, out->remote_htlc_sig, - out->wscript); + htlc_scripts[matches[i]]); - propose_resolution_at_block(out, tx, out->htlc->cltv_expiry, + /* Steals tx onto out */ + propose_resolution_at_block(out, tx, htlcs[matches[i]].cltv_expiry, OUR_HTLC_TIMEOUT_TX); + + return matches[i]; +} + +/* wscript for *received* htlcs (ie. our htlcs in their commit tx, or their + * htlcs in our commit tx) includes cltv, so they must be the same for all + * matching htlcs. Unless, of course, they've found a sha256 clash. */ +static u32 matches_cltv(const size_t *matches, + const struct htlc_stub *htlcs) +{ + for (size_t i = 1; i < tal_count(matches); i++) { + assert(matches[i] < tal_count(htlcs)); + assert(htlcs[matches[i]].cltv_expiry + == htlcs[matches[i-1]].cltv_expiry); + } + return htlcs[matches[0]].cltv_expiry; } -static void resolve_our_htlc_theircommit(struct tracked_output *out) +static size_t resolve_our_htlc_theircommit(struct tracked_output *out, + const size_t *matches, + const struct htlc_stub *htlcs, + u8 **htlc_scripts) { struct bitcoin_tx *tx; enum tx_type tx_type = OUR_HTLC_TIMEOUT_TO_US; + u32 cltv_expiry = matches_cltv(matches, htlcs); /* BOLT #5: * @@ -1353,15 +1399,24 @@ static void resolve_our_htlc_theircommit(struct tracked_output *out) * address. */ tx = tx_to_us(out, remote_htlc_to_us, - out, 0, out->htlc->cltv_expiry, NULL, 0, - out->wscript, + out, 0, cltv_expiry, NULL, 0, + htlc_scripts[matches[0]], &tx_type); - propose_resolution_at_block(out, tx, out->htlc->cltv_expiry, tx_type); + propose_resolution_at_block(out, tx, cltv_expiry, tx_type); + + /* They're all equivalent: might as well use first one. */ + return matches[0]; } -static void resolve_their_htlc(struct tracked_output *out) +/* Returns which htlcs it chose to use of matches[] */ +static size_t resolve_their_htlc(struct tracked_output *out, + const size_t *matches, + const struct htlc_stub *htlcs, + u8 **htlc_scripts) { + size_t which_htlc; + /* BOLT #5: * * ## HTLC Output Handling: Remote Commitment, Remote Offers @@ -1371,18 +1426,45 @@ static void resolve_their_htlc(struct tracked_output *out) * If not otherwise resolved, once the HTLC output has expired, it is * considered *irrevocably resolved*. */ + + /* BOLT #5: + * + * ## HTLC Output Handling: Local Commitment, Remote Offers + *... + * ### Requirements + *... + * If not otherwise resolved, once the HTLC output has expired, it is + * considered *irrevocably resolved*. + */ + + /* The two cases are identical as far as default handling goes. + * But in the remote commitment / remote offer (ie. caller is + * handle_their_unilateral), htlcs which match may have different cltvs. + * So wait until the worst case (largest HTLC). */ + assert(tal_count(matches)); + which_htlc = matches[0]; + for (size_t i = 1; i < tal_count(matches); i++) { + if (htlcs[matches[i]].cltv_expiry > htlcs[which_htlc].cltv_expiry) + which_htlc = matches[i]; + } + /* If we hit timeout depth, resolve by ignoring. */ - propose_resolution_at_block(out, NULL, out->htlc->cltv_expiry, + propose_resolution_at_block(out, NULL, htlcs[which_htlc].cltv_expiry, THEIR_HTLC_TIMEOUT_TO_THEM); + return which_htlc; } -static int match_htlc_output(const struct bitcoin_tx *tx, - unsigned int outnum, - u8 **htlc_scripts) +/* Return tal_arr of htlc indexes. */ +static const size_t *match_htlc_output(const tal_t *ctx, + const struct bitcoin_tx *tx, + unsigned int outnum, + u8 **htlc_scripts) { + size_t *matches = tal_arr(ctx, size_t, 0); + /* Must be a p2wsh output */ if (!is_p2wsh(tx->output[outnum].script, NULL)) - return -1; + return matches; for (size_t i = 0; i < tal_count(htlc_scripts); i++) { struct sha256 sha; @@ -1393,9 +1475,21 @@ static int match_htlc_output(const struct bitcoin_tx *tx, if (memeq(tx->output[outnum].script + 2, tal_count(tx->output[outnum].script) - 2, &sha, sizeof(sha))) - return i; + *tal_arr_expand(&matches) = i; } - return -1; + return matches; +} + +/* They must all be in the same direction, since the scripts are different for + * each dir. Unless, of course, they've found a sha256 clash. */ +static enum side matches_direction(const size_t *matches, + const struct htlc_stub *htlcs) +{ + for (size_t i = 1; i < tal_count(matches); i++) { + assert(matches[i] < tal_count(htlcs)); + assert(htlcs[matches[i]].owner == htlcs[matches[i-1]].owner); + } + return htlcs[matches[0]].owner; } /* Tell master about any we didn't use, if it wants to know. */ @@ -1508,7 +1602,8 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx, for (i = 0; i < tal_count(tx->output); i++) { struct tracked_output *out; - int j; + const size_t *matches; + size_t which_htlc; if (script[LOCAL] && scripteq(tx->output[i].script, script[LOCAL])) { @@ -1577,14 +1672,14 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx, continue; } + matches = match_htlc_output(tmpctx, tx, i, htlc_scripts); /* FIXME: limp along when this happens! */ - j = match_htlc_output(tx, i, htlc_scripts); - if (j == -1) + if (tal_count(matches) == 0) status_failed(STATUS_FAIL_INTERNAL_ERROR, "Could not find resolution for output %zu", i); - if (htlcs[j].owner == LOCAL) { + if (matches_direction(matches, htlcs) == LOCAL) { /* BOLT #5: * * - MUST handle HTLCs offered by itself as specified @@ -1596,17 +1691,19 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx, OUR_UNILATERAL, i, tx->output[i].amount, OUR_HTLC, - &htlcs[j], htlc_scripts[j], + NULL, NULL, remote_htlc_sigs); - resolve_our_htlc_ourcommit(out); + /* Tells us which htlc to use */ + which_htlc = resolve_our_htlc_ourcommit(out, matches, + htlcs, + htlc_scripts); } else { out = new_tracked_output(&outs, txid, tx_blockheight, OUR_UNILATERAL, i, tx->output[i].amount, THEIR_HTLC, - &htlcs[j], - htlc_scripts[j], + NULL, NULL, remote_htlc_sigs); /* BOLT #5: * @@ -1614,13 +1711,17 @@ static void handle_our_unilateral(const struct bitcoin_tx *tx, * as specified in [HTLC Output Handling: Local * Commitment, Remote Offers] */ - resolve_their_htlc(out); + /* Tells us which htlc to use */ + which_htlc = resolve_their_htlc(out, matches, htlcs, + htlc_scripts); } + out->htlc = &htlcs[which_htlc]; + out->wscript = htlc_scripts[which_htlc]; /* Each of these consumes one HTLC signature */ remote_htlc_sigs++; /* We've matched this HTLC, can't do again. */ - htlc_scripts[j] = NULL; + htlc_scripts[which_htlc] = NULL; } @@ -1809,7 +1910,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx, for (i = 0; i < tal_count(tx->output); i++) { struct tracked_output *out; - int j; + const size_t *matches; + size_t which_htlc; if (script[LOCAL] && scripteq(tx->output[i].script, script[LOCAL])) { @@ -1854,13 +1956,16 @@ static void handle_their_cheat(const struct bitcoin_tx *tx, continue; } - j = match_htlc_output(tx, i, htlc_scripts); - if (j == -1) + matches = match_htlc_output(tmpctx, tx, i, htlc_scripts); + if (tal_count(matches) == 0) status_failed(STATUS_FAIL_INTERNAL_ERROR, "Could not find resolution for output %zu", i); - if (htlcs[j].owner == LOCAL) { + /* In this case, we don't care which HTLC we choose; so pick + first one */ + which_htlc = matches[0]; + if (matches_direction(matches, htlcs) == LOCAL) { /* BOLT #5: * * - MUST *resolve* the _local node's offered HTLCs_ @@ -1877,7 +1982,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx, THEIR_REVOKED_UNILATERAL, i, tx->output[i].amount, OUR_HTLC, - &htlcs[j], htlc_scripts[j], + &htlcs[which_htlc], + htlc_scripts[which_htlc], NULL); steal_htlc(out); } else { @@ -1886,7 +1992,8 @@ static void handle_their_cheat(const struct bitcoin_tx *tx, THEIR_REVOKED_UNILATERAL, i, tx->output[i].amount, THEIR_HTLC, - &htlcs[j], htlc_scripts[j], + &htlcs[which_htlc], + htlc_scripts[which_htlc], NULL); /* BOLT #5: * @@ -1899,7 +2006,7 @@ static void handle_their_cheat(const struct bitcoin_tx *tx, */ steal_htlc(out); } - htlc_scripts[j] = NULL; + htlc_scripts[which_htlc] = NULL; } note_missing_htlcs(htlc_scripts, htlcs, @@ -2021,7 +2128,8 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx, for (i = 0; i < tal_count(tx->output); i++) { struct tracked_output *out; - int j; + const size_t *matches; + size_t which_htlc; if (script[LOCAL] && scripteq(tx->output[i].script, script[LOCAL])) { @@ -2068,12 +2176,13 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx, continue; } - j = match_htlc_output(tx, i, htlc_scripts); - if (j == -1) + matches = match_htlc_output(tmpctx, tx, i, htlc_scripts); + if (tal_count(matches) == 0) status_failed(STATUS_FAIL_INTERNAL_ERROR, "Could not find resolution for output %zu", i); - if (htlcs[j].owner == LOCAL) { + + if (matches_direction(matches, htlcs) == LOCAL) { /* BOLT #5: * * - MUST handle HTLCs offered by itself as specified in @@ -2085,16 +2194,19 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx, THEIR_UNILATERAL, i, tx->output[i].amount, OUR_HTLC, - &htlcs[j], htlc_scripts[j], + NULL, NULL, NULL); - resolve_our_htlc_theircommit(out); + which_htlc = resolve_our_htlc_theircommit(out, + matches, + htlcs, + htlc_scripts); } else { out = new_tracked_output(&outs, txid, tx_blockheight, THEIR_UNILATERAL, i, tx->output[i].amount, THEIR_HTLC, - &htlcs[j], htlc_scripts[j], + NULL, NULL, NULL); /* BOLT #5: * @@ -2102,9 +2214,12 @@ static void handle_their_unilateral(const struct bitcoin_tx *tx, * specified in [HTLC Output Handling: Remote * Commitment, Remote Offers] */ - resolve_their_htlc(out); + which_htlc = resolve_their_htlc(out, matches, htlcs, + htlc_scripts); } - htlc_scripts[j] = NULL; + out->htlc = &htlcs[which_htlc]; + out->wscript = htlc_scripts[which_htlc]; + htlc_scripts[which_htlc] = NULL; } note_missing_htlcs(htlc_scripts, htlcs, diff --git a/tests/test_closing.py b/tests/test_closing.py index 1744839cfe6c..37432aa861d4 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -634,13 +634,10 @@ def test_onchain_dust_out(node_factory, bitcoind, executor): def test_onchain_timeout(node_factory, bitcoind, executor): """Onchain handling of outgoing failed htlcs""" # HTLC 1->2, 1 fails just after it's irrevocably committed - disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail'] + disconnects = ['+WIRE_REVOKE_AND_ACK*3', 'permfail'] # Feerates identical so we don't get gratuitous commit to update them l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500)) l2 = node_factory.get_node() - l2 = node_factory.get_node() - - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.fund_channel(l2, 10**6) @@ -654,6 +651,15 @@ def test_onchain_timeout(node_factory, bitcoind, executor): 'channel': '1:1:1' } + l1.rpc.sendpay([routestep], rhash) + with pytest.raises(RpcError): + l1.rpc.waitsendpay(rhash) + + # Make sure CLTVs are different, in case it confuses onchaind. + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1]) + + # Second one will cause drop to chain. l1.rpc.sendpay([routestep], rhash) payfuture = executor.submit(l1.rpc.waitsendpay, rhash) @@ -1018,6 +1024,257 @@ def test_permfail_new_commit(node_factory, bitcoind, executor): wait_for(lambda: l2.rpc.listpeers()['peers'] == []) +def setup_multihtlc_test(node_factory, bitcoind): + # l1 -> l2 -> l3 -> l4 -> l5 -> l6 -> l7 + # l1 and l7 ignore and HTLCs they're sent. + # For each direction, we create these HTLCs with same payment_hash: + # 1 failed (CLTV1) + # 1 failed (CLTV2) + # 2 live (CLTV2) + # 1 live (CLTV3) + nodes = node_factory.line_graph(7, announce=True, + opts={'dev-no-reconnect': None, + 'may_reconnect': True}) + + # Balance by pushing half the funds. + b11 = nodes[-1].rpc.invoice(10**9 // 2, '1', 'balancer')['bolt11'] + nodes[0].rpc.pay(b11) + + nodes[0].rpc.dev_ignore_htlcs(id=nodes[1].info['id'], ignore=True) + nodes[-1].rpc.dev_ignore_htlcs(id=nodes[-2].info['id'], ignore=True) + + preimage = "0" * 64 + h = nodes[0].rpc.invoice(msatoshi=10**8, label='x', description='desc', + preimage=preimage)['payment_hash'] + nodes[-1].rpc.invoice(msatoshi=10**8, label='x', description='desc', + preimage=preimage)['payment_hash'] + + # First, the failed attempts (paying wrong node). CLTV1 + r = nodes[0].rpc.getroute(nodes[-2].info['id'], 10**8, 1)["route"] + nodes[0].rpc.sendpay(r, h) + with pytest.raises(RpcError, match=r'UNKNOWN_PAYMENT_HASH'): + nodes[0].rpc.waitsendpay(h) + + r = nodes[-1].rpc.getroute(nodes[1].info['id'], 10**8, 1)["route"] + nodes[-1].rpc.sendpay(r, h) + with pytest.raises(RpcError, match=r'UNKNOWN_PAYMENT_HASH'): + nodes[-1].rpc.waitsendpay(h) + + # Now increment CLTV -> CLTV2 + bitcoind.generate_block(1) + sync_blockheight(bitcoind, nodes) + + # Now, the live attempts with CLTV2 (blackholed by end nodes) + r = nodes[0].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"] + nodes[0].rpc.sendpay(r, h) + r = nodes[-1].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"] + nodes[-1].rpc.sendpay(r, h) + + # We send second HTLC from different node, since they refuse to send + # multiple with same hash. + r = nodes[1].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"] + nodes[1].rpc.sendpay(r, h) + r = nodes[-2].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"] + nodes[-2].rpc.sendpay(r, h) + + # Now increment CLTV -> CLTV3. + bitcoind.generate_block(1) + sync_blockheight(bitcoind, nodes) + + r = nodes[2].rpc.getroute(nodes[-1].info['id'], 10**8, 1)["route"] + nodes[2].rpc.sendpay(r, h) + r = nodes[-3].rpc.getroute(nodes[0].info['id'], 10**8, 1)["route"] + nodes[-3].rpc.sendpay(r, h) + + # Make sure HTLCs have reached the end. + nodes[0].daemon.wait_for_logs(['peer_in WIRE_UPDATE_ADD_HTLC'] * 3) + nodes[-1].daemon.wait_for_logs(['peer_in WIRE_UPDATE_ADD_HTLC'] * 3) + + return h, nodes + + +@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs") +def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind): + """Node pushes a channel onchain with multiple HTLCs with same payment_hash """ + h, nodes = setup_multihtlc_test(node_factory, bitcoind) + + mid = len(nodes) // 2 + + for i in range(len(nodes) - 1): + assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected'] + + # Now midnode goes onchain with n+1 channel. + nodes[mid].rpc.dev_fail(nodes[mid + 1].info['id']) + nodes[mid].wait_for_channel_onchain(nodes[mid + 1].info['id']) + + bitcoind.generate_block(1) + nodes[mid].daemon.wait_for_log(' to ONCHAIN') + nodes[mid + 1].daemon.wait_for_log(' to ONCHAIN') + + # Now, restart and manually reconnect end nodes (so they don't ignore HTLCs) + # In fact, they'll fail them with WIRE_TEMPORARY_NODE_FAILURE. + nodes[0].restart() + nodes[-1].restart() + + # We disabled auto-reconnect so we'd detect breakage, so manually reconnect. + nodes[0].rpc.connect(nodes[1].info['id'], 'localhost', nodes[1].port) + nodes[-1].rpc.connect(nodes[-2].info['id'], 'localhost', nodes[-2].port) + + # Wait for HTLCs to stabilize. + nodes[0].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3) + nodes[0].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED') + nodes[0].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') + nodes[-1].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3) + nodes[-1].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED') + nodes[-1].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') + + # After at depth 5, midnode will spend its own to-self output. + bitcoind.generate_block(4) + nodes[mid].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET', + 'OUR_UNILATERAL/DELAYED_OUTPUT_TO_US') + + # The three outgoing HTLCs time out at 21, 21 and 22 blocks. + bitcoind.generate_block(16) + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX', + 'OUR_UNILATERAL/OUR_HTLC') + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX', + 'OUR_UNILATERAL/OUR_HTLC') + bitcoind.generate_block(1) + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX', + 'OUR_UNILATERAL/OUR_HTLC') + + # And three more for us to consider them all settled. + bitcoind.generate_block(3) + + # Now, those nodes should have correctly failed the HTLCs + for n in nodes[:mid - 1]: + with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'): + n.rpc.waitsendpay(h, TIMEOUT) + + # Other timeouts are 27,27,28 blocks. + bitcoind.generate_block(2) + nodes[mid].daemon.wait_for_logs(['Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC'] * 2) + for _ in range(2): + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US', + 'THEIR_UNILATERAL/OUR_HTLC') + bitcoind.generate_block(1) + nodes[mid].daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC') + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US', + 'THEIR_UNILATERAL/OUR_HTLC') + + # Depth 3 to consider it settled. + bitcoind.generate_block(3) + + for n in nodes[mid + 1:]: + with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'): + n.rpc.waitsendpay(h, TIMEOUT) + + # At depth 100 it's all done (we didn't bother waiting for mid+1's + # spends, so that might still be going) + bitcoind.generate_block(96) + nodes[mid].daemon.wait_for_logs(['onchaind complete, forgetting peer']) + + # No other channels should have failed. + for i in range(len(nodes) - 1): + if i != mid: + assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected'] + + +@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs") +def test_onchain_multihtlc_their_unilateral(node_factory, bitcoind): + """Node pushes a channel onchain with multiple HTLCs with same payment_hash """ + h, nodes = setup_multihtlc_test(node_factory, bitcoind) + + mid = len(nodes) // 2 + + for i in range(len(nodes) - 1): + assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected'] + + # Now midnode+1 goes onchain with midnode channel. + nodes[mid + 1].rpc.dev_fail(nodes[mid].info['id']) + nodes[mid + 1].wait_for_channel_onchain(nodes[mid].info['id']) + + bitcoind.generate_block(1) + nodes[mid].daemon.wait_for_log(' to ONCHAIN') + nodes[mid + 1].daemon.wait_for_log(' to ONCHAIN') + + # Now, restart and manually reconnect end nodes (so they don't ignore HTLCs) + # In fact, they'll fail them with WIRE_TEMPORARY_NODE_FAILURE. + nodes[0].restart() + nodes[-1].restart() + + # We disabled auto-reconnect so we'd detect breakage, so manually reconnect. + nodes[0].rpc.connect(nodes[1].info['id'], 'localhost', nodes[1].port) + nodes[-1].rpc.connect(nodes[-2].info['id'], 'localhost', nodes[-2].port) + + # Wait for HTLCs to stabilize. + nodes[0].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3) + nodes[0].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED') + nodes[0].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') + nodes[-1].daemon.wait_for_logs(['peer_out WIRE_UPDATE_FAIL_HTLC'] * 3) + nodes[-1].daemon.wait_for_log('peer_out WIRE_COMMITMENT_SIGNED') + nodes[-1].daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') + + # At depth 5, midnode+1 will spend its own to-self output. + bitcoind.generate_block(4) + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET') + + # The three outgoing HTLCs time out at depth 21, 21 and 22 blocks. + bitcoind.generate_block(16) + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US', + 'THEIR_UNILATERAL/OUR_HTLC') + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US', + 'THEIR_UNILATERAL/OUR_HTLC') + bitcoind.generate_block(1) + nodes[mid].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TO_US', + 'THEIR_UNILATERAL/OUR_HTLC') + + # At depth 3 we consider them all settled. + bitcoind.generate_block(3) + + # Now, those nodes should have correctly failed the HTLCs + for n in nodes[:mid - 1]: + with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'): + n.rpc.waitsendpay(h, TIMEOUT) + + # Other timeouts are at depths 27,27,28 blocks. + bitcoind.generate_block(2) + nodes[mid].daemon.wait_for_logs(['Ignoring output.*: THEIR_UNILATERAL/THEIR_HTLC'] * 2) + for _ in range(2): + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX', + 'OUR_UNILATERAL/OUR_HTLC') + bitcoind.generate_block(1) + nodes[mid].daemon.wait_for_log('Ignoring output.*: THEIR_UNILATERAL/THEIR_HTLC') + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_HTLC_TIMEOUT_TX', + 'OUR_UNILATERAL/OUR_HTLC') + + # At depth 3 we consider them all settled. + bitcoind.generate_block(3) + + for n in nodes[mid + 1:]: + with pytest.raises(RpcError, match=r'WIRE_PERMANENT_CHANNEL_FAILURE'): + n.rpc.waitsendpay(h, TIMEOUT) + + # At depth 5, mid+1 can spend HTLC_TIMEOUT_TX output. + bitcoind.generate_block(1) + for _ in range(2): + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET', + 'OUR_HTLC_TIMEOUT_TX/DELAYED_OUTPUT_TO_US') + bitcoind.generate_block(1) + nodes[mid + 1].wait_for_onchaind_broadcast('OUR_DELAYED_RETURN_TO_WALLET', + 'OUR_HTLC_TIMEOUT_TX/DELAYED_OUTPUT_TO_US') + + # At depth 100 they're all done. + bitcoind.generate_block(100) + nodes[mid].daemon.wait_for_logs(['onchaind complete, forgetting peer']) + nodes[mid + 1].daemon.wait_for_logs(['onchaind complete, forgetting peer']) + + # No other channels should have failed. + for i in range(len(nodes) - 1): + if i != mid: + assert only_one(nodes[i].rpc.listpeers(nodes[i + 1].info['id'])['peers'])['connected'] + + @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_permfail_htlc_in(node_factory, bitcoind, executor): # Test case where we fail with unsettled incoming HTLC. diff --git a/tests/test_misc.py b/tests/test_misc.py index 627b63daf58c..9c1371b3efb5 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -291,7 +291,7 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor): # l1 will disconnect and not reconnect. l1.daemon.wait_for_log('dev_disconnect: -WIRE_REVOKE_AND_ACK') - # Deadline HTLC expiry minus 1/2 cltv-expiry delta (rounded up) (== cltv - 3). ctlv is 5+1. + # Deadline HTLC expiry minus 1/2 cltv-expiry delta (rounded up) (== cltv - 3). cltv is 5+1. bitcoind.generate_block(2) assert not l2.daemon.is_in_log('hit deadline') bitcoind.generate_block(1) diff --git a/tests/test_pay.py b/tests/test_pay.py index 4eea3398ea48..5dca195864ec 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1047,3 +1047,56 @@ def test_forward_stats(node_factory, bitcoind): assert l2.rpc.getinfo()['msatoshi_fees_collected'] == 1 + amount // 100000 assert l1.rpc.getinfo()['msatoshi_fees_collected'] == 0 assert l3.rpc.getinfo()['msatoshi_fees_collected'] == 0 + + +@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs") +def test_htlcs_cltv_only_difference(node_factory, bitcoind): + # l1 -> l2 -> l3 -> l4 + # l4 ignores htlcs, so they stay. + # l3 will see a reconnect from l4 when l4 restarts. + l1, l2, l3, l4 = node_factory.line_graph(4, announce=True, opts=[{}] * 2 + [{'dev-no-reconnect': None, 'may_reconnect': True}] * 2) + + h = l4.rpc.invoice(msatoshi=10**8, label='x', description='desc')['payment_hash'] + l4.rpc.dev_ignore_htlcs(id=l3.info['id'], ignore=True) + + # L2 tries to pay + r = l2.rpc.getroute(l4.info['id'], 10**8, 1)["route"] + l2.rpc.sendpay(r, h) + + # Now increment CLTV + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1, l2, l3, l4]) + + # L1 tries to pay + r = l1.rpc.getroute(l4.info['id'], 10**8, 1)["route"] + l1.rpc.sendpay(r, h) + + # Now increment CLTV + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1, l2, l3, l4]) + + # L3 tries to pay + r = l3.rpc.getroute(l4.info['id'], 10**8, 1)["route"] + l3.rpc.sendpay(r, h) + + # Give them time to go through. + time.sleep(5) + + # Will all be connected OK. + assert only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected'] + assert only_one(l2.rpc.listpeers(l3.info['id'])['peers'])['connected'] + assert only_one(l3.rpc.listpeers(l4.info['id'])['peers'])['connected'] + + # Restarting tail node will stop it ignoring HTLCs (it will actually + # fail them immediately). + l4.restart() + l3.rpc.connect(l4.info['id'], 'localhost', l4.port) + + wait_for(lambda: only_one(l1.rpc.listpayments()['payments'])['status'] == 'failed') + wait_for(lambda: only_one(l2.rpc.listpayments()['payments'])['status'] == 'failed') + wait_for(lambda: only_one(l3.rpc.listpayments()['payments'])['status'] == 'failed') + + # Should all still be connected. + assert only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['connected'] + assert only_one(l2.rpc.listpeers(l3.info['id'])['peers'])['connected'] + assert only_one(l3.rpc.listpeers(l4.info['id'])['peers'])['connected'] diff --git a/tools/check-spelling.sh b/tools/check-spelling.sh index e32ce573488f..a9f88697eeb0 100755 --- a/tools/check-spelling.sh +++ b/tools/check-spelling.sh @@ -5,3 +5,8 @@ if git --no-pager grep -nHiE 'l[ightn]{6}g|l[ightn]{8}g|ilghtning|lgihtning|lihg echo "Is this warning incorrect? Please teach tools/check-spelling.sh about the exciting new word." exit 1 fi + +if git --no-pager grep -nHiE 'ctlv' -- . ':!tools/check-spelling.sh'; then + echo "It's check lock time verify, not check time lock verify!" >&2 + exit 1 +fi diff --git a/wallet/wallet.c b/wallet/wallet.c index 5245da69a2e8..0048b47660c8 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1551,7 +1551,7 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, struct htlc_stub *stubs; struct sha256 payment_hash; sqlite3_stmt *stmt = db_prepare(wallet->db, - "SELECT channel_id, direction, cltv_expiry, payment_hash " + "SELECT channel_id, direction, cltv_expiry, channel_htlc_id, payment_hash " "FROM channel_htlcs WHERE channel_id = ?;"); sqlite3_bind_int64(stmt, 1, chan->dbid); @@ -1566,8 +1566,9 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, /* FIXME: merge these two enums */ stub->owner = sqlite3_column_int(stmt, 1)==DIRECTION_INCOMING?REMOTE:LOCAL; stub->cltv_expiry = sqlite3_column_int(stmt, 2); + stub->id = sqlite3_column_int(stmt, 3); - sqlite3_column_sha256(stmt, 3, &payment_hash); + sqlite3_column_sha256(stmt, 4, &payment_hash); ripemd160(&stub->ripemd, payment_hash.u.u8, sizeof(payment_hash.u)); } db_stmt_done(stmt);