From 3746ea36e26f1e13ec287d7f1b8c6dcb9d1d01d5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 22 Oct 2018 14:51:05 +1030 Subject: [PATCH] channeld: tiebreak identical HTLC outputs by CLTV. This was suggested by Pierre-Marie as the solution to the 'same HTLC, different CLTV' signature mismatch. Signed-off-by: Rusty Russell --- channeld/commit_tx.c | 14 ++++++++-- common/close_tx.c | 2 +- common/funding_tx.c | 4 +-- common/initial_commit_tx.c | 2 +- common/permute_tx.c | 56 ++++++++++++++++++++++++++++---------- common/permute_tx.h | 18 ++++++++---- common/withdraw_tx.c | 4 +-- tests/test_pay.py | 1 - 8 files changed, 73 insertions(+), 28 deletions(-) diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c index 1eddfcbcf..ca41d7220 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); @@ -160,6 +161,10 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, /* We keep track of which outputs have which HTLCs */ *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. */ @@ -176,6 +181,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, continue; add_offered_htlc_out(tx, n, htlcs[i], keyset); (*htlcmap)[n] = htlcs[i]; + cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry); n++; } @@ -191,6 +197,7 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, continue; add_received_htlc_out(tx, n, htlcs[i], keyset); (*htlcmap)[n] = htlcs[i]; + cltvs[n] = abs_locktime_to_blocks(&htlcs[i]->expiry); n++; } @@ -205,6 +212,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, tx->output[n].amount = self_pay_msat / 1000; tx->output[n].script = scriptpubkey_p2wsh(tx, wscript); (*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)); @@ -229,6 +238,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx, tx->output[n].script = scriptpubkey_p2wpkh(tx, &keyset->other_payment_key); (*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, @@ -245,8 +256,7 @@ struct bitcoin_tx *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), - (const void **)*htlcmap); + permute_outputs(tx->output, cltvs, (const void **)*htlcmap); /* BOLT #3: * diff --git a/common/close_tx.c b/common/close_tx.c index 9f880da50..6525c9b88 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 35ce457ee..28a5376e5 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 bdbff1232..a64b044a4 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 327d337d5..e2e8138e9 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 size_t find_best_out(struct bitcoin_tx_output *outputs, size_t num) +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, + 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 f5bbfad40..6968c94fa 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 7cb0ac433..05432c426 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/tests/test_pay.py b/tests/test_pay.py index e690bad95..5dca19586 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1049,7 +1049,6 @@ def test_forward_stats(node_factory, bitcoind): assert l3.rpc.getinfo()['msatoshi_fees_collected'] == 0 -@pytest.mark.xfail(strict=True) @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for dev_ignore_htlcs") def test_htlcs_cltv_only_difference(node_factory, bitcoind): # l1 -> l2 -> l3 -> l4