From 956ff8f325268ed8247c904cacdda24818b08b33 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:16:18 +1030 Subject: [PATCH 1/9] channeld: don't send update_fee after shutdown. See: https://github.com/lightningnetwork/lightning-rfc/pull/367 Signed-off-by: Rusty Russell --- channeld/channel.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/channeld/channel.c b/channeld/channel.c index 2948c611d785..7890468c284a 100644 --- a/channeld/channel.c +++ b/channeld/channel.c @@ -904,6 +904,19 @@ static void send_commit(struct peer *peer) return; } + /* BOLT #2: + * + * - if no HTLCs remain in either commitment transaction: + * - MUST NOT send any `update` message after a `shutdown`. + */ + if (peer->shutdown_sent[LOCAL] && !channel_has_htlcs(peer->channel)) { + status_trace("Can't send commit: final shutdown phase"); + + peer->commit_timer = NULL; + tal_free(tmpctx); + return; + } + /* If we wanted to update fees, do it now. */ if (peer->channel->funder == LOCAL && peer->desired_feerate != channel_feerate(peer->channel, REMOTE)) { From 901cd93bc7025c4de852b19a9ea98cad7be61193 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:16:18 +1030 Subject: [PATCH 2/9] lightningd: add more debugging info for closing. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f373460c7e18..468d2fc3f123 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1771,7 +1771,7 @@ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) s64 old_diff, new_diff; size_t i; - /* Calculate actual fee. */ + /* Calculate actual fee (adds in eliminated outputs) */ fee = peer->funding_satoshi; for (i = 0; i < tal_count(tx->output); i++) fee -= tx->output[i].amount; @@ -1780,12 +1780,20 @@ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) for (i = 0; i < tal_count(peer->last_tx); i++) last_fee -= peer->last_tx->output[i].amount; + log_debug(peer->log, "Their actual closing tx fee is %"PRIu64 + " vs previous %"PRIu64, fee, last_fee); + /* Weight once we add in sigs. */ weight = measure_tx_cost(tx) + 74 * 2; min_fee = get_feerate(peer->ld->topology, FEERATE_SLOW) * weight / 1000; - if (fee < min_fee) + if (fee < min_fee) { + log_debug(peer->log, "... That's below our min %"PRIu64 + " for weight %"PRIu64" at feerate %u", + min_fee, weight, + get_feerate(peer->ld->topology, FEERATE_SLOW)); return false; + } ideal_fee = get_feerate(peer->ld->topology, FEERATE_NORMAL) * weight / 1000; @@ -1793,6 +1801,14 @@ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) old_diff = imaxabs((s64)ideal_fee - (s64)last_fee); new_diff = imaxabs((s64)ideal_fee - (s64)fee); + log_debug(peer->log, "... That's %s our ideal %"PRIu64, + new_diff < old_diff + ? "closer to" + : new_diff > old_diff + ? "further from" + : "same distance to", + ideal_fee); + return (new_diff < old_diff); } From 6dd10fc2344ae81bfe7e5f3e1055a2e5229b6a8d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:16:18 +1030 Subject: [PATCH 3/9] test_lightning: add tests for closing with differetn feerates. Signed-off-by: Rusty Russell --- tests/test_lightningd.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 52c59993337d..386c2d13ba4a 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -980,6 +980,41 @@ def test_closing(self): wait_forget_channels(l1) wait_forget_channels(l2) + def test_closing_different_fees(self): + l1 = self.node_factory.get_node() + self.give_funds(l1, (10**6) * 5 + 1000000) + + # Default feerate = 15000/7500/1000 + # It will accept between upper and lower feerate, starting at normal. + for feerates in [ [ 20000, 15000, 7400 ], + [ 15000, 8000, 1000 ], + [ 15000, 6000, 1000 ], + [ 8000, 7500, 1000 ], + [ 8000, 1200, 100 ] ]: + # With and without dust + for pamount in [ 0, 545999, 546000, 546001, 10**6 // 2 ]: + l2 = self.node_factory.get_node(options=['--override-fee-rates={}/{}/{}' + .format(feerates[0], + feerates[1], + feerates[2])]) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + + self.fund_channel(l1, l2, 10**6) + if pamount > 0: + self.pay(l1, l2, pamount) + + l1.rpc.close(l2.info['id']) + l1.daemon.wait_for_log(' to CHANNELD_SHUTTING_DOWN') + l2.daemon.wait_for_log(' to CHANNELD_SHUTTING_DOWN') + + l1.daemon.wait_for_log(' to CLOSINGD_COMPLETE') + l2.daemon.wait_for_log(' to CLOSINGD_COMPLETE') + + bitcoind.generate_block(1) + l1.daemon.wait_for_log(' to ONCHAIND_MUTUAL') + l2.daemon.wait_for_log(' to ONCHAIND_MUTUAL') + @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_permfail(self): l1,l2 = self.connect() From 0739278cb98dada43b67190ee28d67f86792494d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:16:18 +1030 Subject: [PATCH 4/9] lightningd: prefer mutual close over unilateral, even if fee identical. Signed-off-by: Rusty Russell --- lightningd/peer_control.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 468d2fc3f123..ed9b136599f8 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1764,7 +1764,9 @@ void peer_last_tx(struct peer *peer, struct bitcoin_tx *tx, peer->last_tx = tal_steal(peer, tx); } -/* Is this better than the last tx we were holding? */ +/* Is this better than the last tx we were holding? This can happen + * even without closingd misbehaving, if we have multiple, + * interrupted, rounds of negotiation. */ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) { u64 weight, fee, last_fee, ideal_fee, min_fee; @@ -1801,6 +1803,8 @@ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) old_diff = imaxabs((s64)ideal_fee - (s64)last_fee); new_diff = imaxabs((s64)ideal_fee - (s64)fee); + /* In case of a tie, prefer new over old: this covers the preference + * for a mutual close over a unilateral one. */ log_debug(peer->log, "... That's %s our ideal %"PRIu64, new_diff < old_diff ? "closer to" @@ -1809,7 +1813,7 @@ static bool better_closing_fee(struct peer *peer, const struct bitcoin_tx *tx) : "same distance to", ideal_fee); - return (new_diff < old_diff); + return new_diff <= old_diff; } static void peer_received_closing_signature(struct peer *peer, const u8 *msg) From a0b7b4a2cd8b86b9bc78322935d64712762957a5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:16:18 +1030 Subject: [PATCH 5/9] closingd: use the final commitment tx fee as the maximum. We shouldn't fail negotiation just because they exceeded what we thought fair: we're better off as long as it's actually <= final commitment fee. Signed-off-by: Rusty Russell --- closingd/closing.c | 2 +- closingd/closing_wire.csv | 2 +- lightningd/peer_control.c | 8 ++------ 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/closingd/closing.c b/closingd/closing.c index 94f5561cc6e2..e0f84acd73a8 100644 --- a/closingd/closing.c +++ b/closingd/closing.c @@ -168,7 +168,7 @@ int main(int argc, char *argv[]) u16 funding_txout; u64 funding_satoshi, satoshi_out[NUM_SIDES]; u64 our_dust_limit; - u64 minfee, maxfee, sent_fee; + u64 minfee, feelimit, maxfee, sent_fee; s64 last_received_fee = -1; enum side funder; u8 *scriptpubkey[NUM_SIDES], *funding_wscript; diff --git a/closingd/closing_wire.csv b/closingd/closing_wire.csv index 4e4ad01d4498..8c68c1b70de4 100644 --- a/closingd/closing_wire.csv +++ b/closingd/closing_wire.csv @@ -14,7 +14,7 @@ closing_init,,local_msatoshi,u64 closing_init,,remote_msatoshi,u64 closing_init,,our_dust_limit,u64 closing_init,,min_fee_satoshi,u64 -closing_init,,max_fee_satoshi,u64 +closing_init,,fee_limit_satoshi,u64 closing_init,,initial_fee_satoshi,u64 closing_init,,local_scriptpubkey_len,u16 closing_init,,local_scriptpubkey,local_scriptpubkey_len*u8 diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index ed9b136599f8..1835b384a354 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1888,7 +1888,7 @@ static void peer_start_closingd(struct peer *peer, { const tal_t *tmpctx = tal_tmpctx(peer); u8 *initmsg, *local_scriptpubkey; - u64 minfee, maxfee, startfee, feelimit; + u64 minfee, startfee, feelimit; u64 num_revocations; u64 funding_msatoshi, our_msatoshi, their_msatoshi; @@ -1936,15 +1936,11 @@ static void peer_start_closingd(struct peer *peer, feelimit = commit_tx_base_fee(peer->channel_info->feerate_per_kw[LOCAL], 0); - maxfee = commit_tx_base_fee(get_feerate(peer->ld->topology, - FEERATE_IMMEDIATE), 0); minfee = commit_tx_base_fee(get_feerate(peer->ld->topology, FEERATE_SLOW), 0); startfee = commit_tx_base_fee(get_feerate(peer->ld->topology, FEERATE_NORMAL), 0); - if (maxfee > feelimit) - maxfee = feelimit; if (startfee > feelimit) startfee = feelimit; if (minfee > feelimit) @@ -1974,7 +1970,7 @@ static void peer_start_closingd(struct peer *peer, our_msatoshi / 1000, /* Rounds down */ their_msatoshi / 1000, /* Rounds down */ peer->our_config.dust_limit_satoshis, - minfee, maxfee, startfee, + minfee, feelimit, startfee, local_scriptpubkey, peer->remote_shutdown_scriptpubkey, reconnected, From 16aac04e560dba5285f125ecd30992ac86d7cfcb Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:25:03 +1030 Subject: [PATCH 6/9] closingd: rewrite negotiation. This follows the proposal to make the funder send the first offer. The dual loops are because there's initially very little restriction on the amounts, then once they're first established they tighten as necessary. Signed-off-by: Rusty Russell --- closingd/closing.c | 588 ++++++++++++++++++++++++++------------------- 1 file changed, 336 insertions(+), 252 deletions(-) diff --git a/closingd/closing.c b/closingd/closing.c index e0f84acd73a8..323c69d98c0f 100644 --- a/closingd/closing.c +++ b/closingd/closing.c @@ -74,15 +74,6 @@ static struct bitcoin_tx *close_tx(const tal_t *ctx, return tx; } -static u64 one_towards(u64 target, u64 value) -{ - if (value > target) - return value-1; - else if (value < target) - return value+1; - return value; -} - /* Handle random messages we might get, returning the first non-handled one. */ static u8 *closing_read_peer_msg(const tal_t *ctx, struct crypto_state *cs, @@ -157,6 +148,281 @@ static void do_reconnect(struct crypto_state *cs, tal_free(tmpctx); } +static void send_offer(struct crypto_state *cs, + const struct channel_id *channel_id, + const struct pubkey funding_pubkey[NUM_SIDES], + const u8 *funding_wscript, + u8 *scriptpubkey[NUM_SIDES], + const struct bitcoin_txid *funding_txid, + unsigned int funding_txout, + u64 funding_satoshi, + const u64 satoshi_out[NUM_SIDES], + enum side funder, + uint64_t our_dust_limit, + const struct secrets *secrets, + uint64_t fee_to_offer) +{ + const tal_t *tmpctx = tal_tmpctx(NULL); + struct bitcoin_tx *tx; + secp256k1_ecdsa_signature our_sig; + u8 *msg; + + /* BOLT #2: + * + * The sender MUST set `signature` to the Bitcoin signature of + * the close transaction as specified in [BOLT + * #3](03-transactions.md#closing-transaction). + */ + tx = close_tx(tmpctx, cs, channel_id, + scriptpubkey, + funding_txid, + funding_txout, + funding_satoshi, + satoshi_out, + funder, fee_to_offer, our_dust_limit); + + /* BOLT #3: + * + * ## Closing Transaction + *... + * Each node offering a signature... MAY also eliminate its + * own output. + */ + /* (We don't do this). */ + sign_tx_input(tx, 0, NULL, funding_wscript, + &secrets->funding_privkey, + &funding_pubkey[LOCAL], + &our_sig); + + status_trace("sending fee offer %"PRIu64, fee_to_offer); + + msg = towire_closing_signed(tmpctx, channel_id, fee_to_offer, &our_sig); + if (!sync_crypto_write(cs, PEER_FD, take(msg))) + status_failed(STATUS_FAIL_PEER_IO, + "Writing closing_signed"); + + tal_free(tmpctx); +} + +static void tell_master_their_offer(u64 their_offer, + const secp256k1_ecdsa_signature *their_sig, + const struct bitcoin_tx *tx) +{ + u8 *msg = towire_closing_received_signature(NULL, their_sig, tx); + if (!wire_sync_write(REQ_FD, take(msg))) + status_failed(STATUS_FAIL_MASTER_IO, + "Writing received to master: %s", + strerror(errno)); + + /* Wait for master to ack, to make sure it's in db. */ + msg = wire_sync_read(NULL, REQ_FD); + if (!fromwire_closing_received_signature_reply(msg,NULL)) + master_badmsg(WIRE_CLOSING_RECEIVED_SIGNATURE_REPLY, msg); + tal_free(msg); +} + +/* Returns fee they offered. */ +static uint64_t receive_offer(struct crypto_state *cs, + const struct channel_id *channel_id, + const struct pubkey funding_pubkey[NUM_SIDES], + const u8 *funding_wscript, + u8 *scriptpubkey[NUM_SIDES], + const struct bitcoin_txid *funding_txid, + unsigned int funding_txout, + u64 funding_satoshi, + const u64 satoshi_out[NUM_SIDES], + enum side funder, + uint64_t our_dust_limit, + u64 min_fee_to_accept) +{ + const tal_t *tmpctx = tal_tmpctx(NULL); + u8 *msg; + struct channel_id their_channel_id; + u64 received_fee; + secp256k1_ecdsa_signature their_sig; + struct bitcoin_tx *tx; + + /* Wait for them to say something interesting */ + do { + msg = closing_read_peer_msg(tmpctx, cs, channel_id); + + /* BOLT #2: + * + * On reconnection, a node MUST ignore a redundant + * `funding_locked` if it receives one. + */ + /* This should only happen if we've made no commitments, but + * we don't have to check that: it's their problem. */ + if (fromwire_peektype(msg) == WIRE_FUNDING_LOCKED) + msg = tal_free(msg); + /* BOLT #2: + * + * ...if the node has sent a previous `shutdown` it MUST + * retransmit it. + */ + else if (fromwire_peektype(msg) == WIRE_SHUTDOWN) + msg = tal_free(msg); + } while (!msg); + + if (!fromwire_closing_signed(msg, NULL, &their_channel_id, + &received_fee, &their_sig)) + peer_failed(PEER_FD, cs, channel_id, + "Expected closing_signed: %s", + tal_hex(trc, msg)); + + /* BOLT #2: + * + * The receiver MUST check `signature` is valid for either + * variant of close transaction specified in [BOLT + * #3](03-transactions.md#closing-transaction), and MUST fail + * the connection if it is not. + */ + tx = close_tx(tmpctx, cs, channel_id, + scriptpubkey, + funding_txid, + funding_txout, + funding_satoshi, + satoshi_out, funder, received_fee, our_dust_limit); + + if (!check_tx_sig(tx, 0, NULL, funding_wscript, + &funding_pubkey[REMOTE], &their_sig)) { + /* Trim it by reducing their output to minimum */ + struct bitcoin_tx *trimmed; + u64 trimming_satoshi_out[NUM_SIDES]; + + if (funder == REMOTE) + trimming_satoshi_out[REMOTE] = received_fee; + else + trimming_satoshi_out[REMOTE] = 0; + trimming_satoshi_out[LOCAL] = satoshi_out[LOCAL]; + + /* BOLT #3: + * + * Each node offering a signature MUST subtract the fee given + * by `fee_satoshis` from the output to the funder; it MUST + * then remove any output below its own `dust_limit_satoshis`, + * and MAY also eliminate its own output. + */ + trimmed = close_tx(tmpctx, cs, channel_id, + scriptpubkey, + funding_txid, + funding_txout, + funding_satoshi, + trimming_satoshi_out, + funder, received_fee, our_dust_limit); + if (!trimmed + || !check_tx_sig(trimmed, 0, NULL, funding_wscript, + &funding_pubkey[REMOTE], &their_sig)) { + peer_failed(PEER_FD, cs, channel_id, + "Bad closing_signed signature for" + " %s (and trimmed version %s)", + type_to_string(tmpctx, + struct bitcoin_tx, + tx), + trimmed ? + type_to_string(tmpctx, + struct bitcoin_tx, + trimmed) + : "NONE"); + } + tx = trimmed; + } + + status_trace("Received fee offer %"PRIu64, received_fee); + + /* Master sorts out what is best offer, we just tell it any above min */ + if (received_fee >= min_fee_to_accept) { + status_trace("...offer is reasonable"); + tell_master_their_offer(received_fee, &their_sig, tx); + } + + tal_free(tmpctx); + return received_fee; +} + +struct feerange { + enum side higher_side; + u64 min, max; +}; + +static void init_feerange(struct feerange *feerange, + u64 commitment_fee, + const u64 offer[NUM_SIDES]) +{ + feerange->min = 0; + + /* BOLT #2: + * + * - MUST set `fee_satoshis` less than or equal to the base + * fee of the final commitment transaction, as calculated + * in [BOLT #3](03-transactions.md#fee-calculation). + */ + feerange->max = commitment_fee; + + if (offer[LOCAL] > offer[REMOTE]) + feerange->higher_side = LOCAL; + else + feerange->higher_side = REMOTE; + + status_trace("Feerange init %"PRIu64"-%"PRIu64", %s higher", + feerange->min, feerange->max, + feerange->higher_side == LOCAL ? "local" : "remote"); +} + +static void adjust_feerange(struct crypto_state *cs, + const struct channel_id *channel_id, + struct feerange *feerange, + u64 offer, enum side side) +{ + if (offer < feerange->min || offer > feerange->max) + peer_failed(PEER_FD, cs, channel_id, + "%s offer %"PRIu64 + " not between %"PRIu64" and %"PRIu64, + side == LOCAL ? "local" : "remote", + offer, feerange->min, feerange->max); + + + /* BOLT #2: + * + * ...otherwise it MUST propose a value strictly between the received + * `fee_satoshis` and its previously-sent `fee_satoshis`. + */ + if (side == feerange->higher_side) + feerange->max = offer - 1; + else + feerange->min = offer + 1; + + status_trace("Feerange %s update %"PRIu64": now %"PRIu64"-%"PRIu64, + side == LOCAL ? "local" : "remote", + offer, feerange->min, feerange->max); +} + +/* Figure out what we should offer now. */ +static u64 adjust_offer(struct crypto_state *cs, + const struct channel_id *channel_id, + const struct feerange *feerange, + u64 remote_offer, + u64 min_fee_to_accept) +{ + /* Within 1 satoshi? Agree. */ + if (feerange->min + 1 >= feerange->max) + return remote_offer; + + /* Max is below our minimum acceptable? */ + if (feerange->max < min_fee_to_accept) + peer_failed(PEER_FD, cs, channel_id, + "Feerange %"PRIu64"-%"PRIu64 + " below minimum acceptable %"PRIu64, + feerange->min, feerange->max, + min_fee_to_accept); + + /* Bisect between our minimum and max. */ + if (feerange->min > min_fee_to_accept) + min_fee_to_accept = feerange->min; + + return (feerange->max + min_fee_to_accept)/2; +} + int main(int argc, char *argv[]) { struct crypto_state cs; @@ -168,16 +434,16 @@ int main(int argc, char *argv[]) u16 funding_txout; u64 funding_satoshi, satoshi_out[NUM_SIDES]; u64 our_dust_limit; - u64 minfee, feelimit, maxfee, sent_fee; - s64 last_received_fee = -1; + u64 min_fee_to_accept, commitment_fee, offer[NUM_SIDES]; + struct feerange feerange; enum side funder; u8 *scriptpubkey[NUM_SIDES], *funding_wscript; struct channel_id channel_id; struct secrets secrets; - secp256k1_ecdsa_signature sig; bool reconnected; u64 next_index[NUM_SIDES], revocations_received; u64 gossip_index; + enum side whose_turn; subdaemon_setup(argc, argv); @@ -193,7 +459,8 @@ int main(int argc, char *argv[]) &satoshi_out[LOCAL], &satoshi_out[REMOTE], &our_dust_limit, - &minfee, &maxfee, &sent_fee, + &min_fee_to_accept, &commitment_fee, + &offer[LOCAL], &scriptpubkey[LOCAL], &scriptpubkey[REMOTE], &reconnected, @@ -205,7 +472,7 @@ int main(int argc, char *argv[]) status_trace("satoshi_out = %"PRIu64"/%"PRIu64, satoshi_out[LOCAL], satoshi_out[REMOTE]); status_trace("dustlimit = %"PRIu64, our_dust_limit); - status_trace("fee = %"PRIu64, sent_fee); + status_trace("fee = %"PRIu64, offer[LOCAL]); derive_channel_id(&channel_id, &funding_txid, funding_txout); derive_basepoints(&seed, &funding_pubkey[LOCAL], NULL, &secrets, NULL); @@ -219,249 +486,66 @@ int main(int argc, char *argv[]) /* BOLT #2: * - * Nodes SHOULD send a `closing_signed` message after `shutdown` has - * been received and no HTLCs remain in either commitment transaction. - */ - - /* BOLT #2: - * - * On reconnection, ... if the node has sent a previous - * `closing_signed` it MUST send another `closing_signed`, otherwise - * if the node has sent a previous `shutdown` it MUST retransmit it. + * The funding node: + * - after `shutdown` has been received, AND no HTLCs remain in either + * commitment transaction: + * - SHOULD send a `closing_signed` message. */ - for (;;) { - const tal_t *tmpctx = tal_tmpctx(ctx); - struct bitcoin_tx *tx; - u64 received_fee, limit_fee, new_fee; - - /* BOLT #2: - * - * The sender MUST set `signature` to the Bitcoin signature of - * the close transaction as specified in [BOLT - * #3](03-transactions.md#closing-transaction). - */ - tx = close_tx(tmpctx, &cs, &channel_id, - scriptpubkey, - &funding_txid, - funding_txout, - funding_satoshi, - satoshi_out, funder, sent_fee, our_dust_limit); - - /* BOLT #3: - * - * ## Closing Transaction - *... - * Each node offering a signature... MAY also eliminate its - * own output. - */ - /* (We don't do this). */ - sign_tx_input(tx, 0, NULL, funding_wscript, - &secrets.funding_privkey, - &funding_pubkey[LOCAL], - &sig); - - status_trace("sending fee offer %"PRIu64, sent_fee); - - /* Now send closing offer */ - msg = towire_closing_signed(tmpctx, &channel_id, sent_fee, &sig); - if (!sync_crypto_write(&cs, PEER_FD, take(msg))) - status_failed(STATUS_FAIL_PEER_IO, - "Writing closing_signed"); - - /* Did we just agree with them? If so, we're done. */ - if (sent_fee == last_received_fee) - break; - - again: - /* Wait for them to say something interesting */ - msg = closing_read_peer_msg(tmpctx, &cs, &channel_id); - - /* BOLT #2: - * - * On reconnection, a node MUST ignore a redundant - * `funding_locked` if it receives one. - */ - /* This should only happen if we've made no commitments, but - * we don't have to check that: it's their problem. */ - if (fromwire_peektype(msg) == WIRE_FUNDING_LOCKED) { - tal_free(msg); - goto again; - } - - /* BOLT #2: - * - * ...if the node has sent a previous `shutdown` it MUST - * retransmit it. - */ - if (fromwire_peektype(msg) == WIRE_SHUTDOWN) { - tal_free(msg); - goto again; - } - - if (!fromwire_closing_signed(msg, NULL, &channel_id, - &received_fee, &sig)) - peer_failed(PEER_FD, &cs, &channel_id, - "Expected closing_signed: %s", - tal_hex(trc, msg)); - - /* BOLT #2: - * - * The receiver MUST check `signature` is valid for either - * variant of close transaction specified in [BOLT - * #3](03-transactions.md#closing-transaction), and MUST fail - * the connection if it is not. - */ - tx = close_tx(tmpctx, &cs, &channel_id, - scriptpubkey, - &funding_txid, - funding_txout, - funding_satoshi, - satoshi_out, funder, received_fee, our_dust_limit); - - if (!check_tx_sig(tx, 0, NULL, funding_wscript, - &funding_pubkey[REMOTE], &sig)) { - /* Trim it by reducing their output to minimum */ - struct bitcoin_tx *trimmed; - u64 trimming_satoshi_out[NUM_SIDES]; - - if (funder == REMOTE) - trimming_satoshi_out[REMOTE] = received_fee; - else - trimming_satoshi_out[REMOTE] = 0; - trimming_satoshi_out[LOCAL] = satoshi_out[LOCAL]; - - /* BOLT #3: - * - * Each node offering a signature MUST subtract the - * fee given by `fee_satoshis` from the output to the - * funder; it MUST then remove any output below its - * own `dust_limit_satoshis`, and MAY also eliminate - * its own output. - */ - trimmed = close_tx(tmpctx, &cs, &channel_id, - scriptpubkey, - &funding_txid, - funding_txout, - funding_satoshi, - trimming_satoshi_out, - funder, received_fee, our_dust_limit); - if (!trimmed - || !check_tx_sig(trimmed, 0, NULL, funding_wscript, - &funding_pubkey[REMOTE], &sig)) { - peer_failed(PEER_FD, &cs, &channel_id, - "Bad closing_signed signature for" - " %s (and trimmed version %s)", - type_to_string(tmpctx, - struct bitcoin_tx, - tx), - trimmed ? - type_to_string(tmpctx, - struct bitcoin_tx, - trimmed) - : "NONE"); - } - tx = trimmed; - } - - status_trace("Received fee offer %"PRIu64, received_fee); - - /* BOLT #2: - * - * Otherwise, the recipient MUST fail the connection if - * `fee_satoshis` is greater than the base fee of the final - * commitment transaction as calculated in [BOLT #3] */ - if (received_fee > maxfee) - peer_failed(PEER_FD, &cs, &channel_id, - "Bad closing_signed fee %"PRIu64" > %"PRIu64, - received_fee, maxfee); - - /* Is fee reasonable? Tell master. */ - if (received_fee < minfee) { - status_trace("Fee too low, below %"PRIu64, minfee); - limit_fee = minfee; + whose_turn = funder; + for (size_t i = 0; i < 2; i++, whose_turn = !whose_turn) { + if (whose_turn == LOCAL) { + send_offer(&cs, &channel_id, funding_pubkey, + funding_wscript, + scriptpubkey, &funding_txid, funding_txout, + funding_satoshi, satoshi_out, funder, + our_dust_limit, &secrets, offer[LOCAL]); } else { - status_trace("Fee accepted."); - msg = towire_closing_received_signature(tmpctx, - &sig, tx); - if (!wire_sync_write(REQ_FD, take(msg))) - status_failed(STATUS_FAIL_MASTER_IO, - "Writing received to master: %s", - strerror(errno)); - msg = wire_sync_read(tmpctx, REQ_FD); - if (!fromwire_closing_received_signature_reply(msg,NULL)) - master_badmsg(WIRE_CLOSING_RECEIVED_SIGNATURE_REPLY, - msg); - limit_fee = received_fee; + offer[REMOTE] + = receive_offer(&cs, &channel_id, funding_pubkey, + funding_wscript, + scriptpubkey, &funding_txid, + funding_txout, funding_satoshi, + satoshi_out, funder, + our_dust_limit, + min_fee_to_accept); } + } - /* BOLT #2: - * - * If `fee_satoshis` is equal to its previously sent - * `fee_satoshis`, the receiver SHOULD sign and broadcast the - * final closing transaction and MAY close the connection. - */ - if (received_fee == sent_fee) - break; - - /* BOLT #2: - * - * the recipient SHOULD fail the connection if `fee_satoshis` - * is not strictly between its last-sent `fee_satoshis` and - * its previously-received `fee_satoshis`, unless it has - * reconnected since then. */ - if (last_received_fee != -1) { - bool previous_dir = sent_fee < last_received_fee; - bool dir = received_fee < last_received_fee; - bool next_dir = sent_fee < received_fee; - - /* They went away from our offer? */ - if (dir != previous_dir) - peer_failed(PEER_FD, &cs, &channel_id, - "Their fee went %" - PRIu64" to %"PRIu64 - " when ours was %"PRIu64, - last_received_fee, - received_fee, - sent_fee); - - /* They jumped over our offer? */ - if (next_dir != previous_dir) - peer_failed(PEER_FD, &cs, &channel_id, - "Their fee jumped %" - PRIu64" to %"PRIu64 - " when ours was %"PRIu64, - last_received_fee, - received_fee, - sent_fee); + /* Now we have first two points, we can init fee range. */ + init_feerange(&feerange, commitment_fee, offer); + + /* Now apply the one constraint from above (other is inside loop). */ + adjust_feerange(&cs, &channel_id, &feerange, + offer[!whose_turn], !whose_turn); + + /* Now any extra rounds required. */ + while (offer[LOCAL] != offer[REMOTE]) { + /* If they differ, adjust feerate. */ + adjust_feerange(&cs, &channel_id, &feerange, + offer[whose_turn], whose_turn); + + /* Now its the other side's turn. */ + whose_turn = !whose_turn; + + if (whose_turn == LOCAL) { + offer[LOCAL] = adjust_offer(&cs, &channel_id, + &feerange, offer[REMOTE], + min_fee_to_accept); + send_offer(&cs, &channel_id, funding_pubkey, + funding_wscript, + scriptpubkey, &funding_txid, funding_txout, + funding_satoshi, satoshi_out, funder, + our_dust_limit, &secrets, offer[LOCAL]); + } else { + offer[REMOTE] + = receive_offer(&cs, &channel_id, funding_pubkey, + funding_wscript, + scriptpubkey, &funding_txid, + funding_txout, funding_satoshi, + satoshi_out, funder, + our_dust_limit, + min_fee_to_accept); } - - /* BOLT #2: - * - * ...otherwise it MUST propose a value strictly between the - * received `fee_satoshis` and its previously-sent - * `fee_satoshis`. - */ - - /* We do it by bisection, with twists: - * 1. Don't go outside limits, or reach them immediately: - * treat out-of-limit offers as on-limit offers. - * 2. Round towards the target, otherwise we can't close - * a final 1-satoshi gap. - * - * Note: Overflow impossible here, since fee <= funder amount */ - new_fee = one_towards(limit_fee, limit_fee + sent_fee) / 2; - - /* If we didn't move, give up (we're ~ at min/max). */ - if (new_fee == sent_fee) - peer_failed(PEER_FD, &cs, &channel_id, - "Final fee %"PRIu64" vs %"PRIu64 - " at limits %"PRIu64"-%"PRIu64, - sent_fee, received_fee, - minfee, maxfee); - - last_received_fee = received_fee; - sent_fee = new_fee; - tal_free(tmpctx); } /* We're done! */ From 6b99dd0d32217dccb6bcaf1813b8ce286725c6a3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:25:06 +1030 Subject: [PATCH 7/9] closingd: don't punish peers who can't negotiate properly. This is a transitional patch so we can still close channels cleanly; for want of a better option, I hooked it into --deprecated-apis. Signed-off-by: Rusty Russell --- closingd/closing.c | 27 ++++++++++++++++++--------- closingd/closing_wire.csv | 2 ++ lightningd/peer_control.c | 3 ++- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/closingd/closing.c b/closingd/closing.c index 323c69d98c0f..6533cb1403a6 100644 --- a/closingd/closing.c +++ b/closingd/closing.c @@ -343,11 +343,14 @@ static uint64_t receive_offer(struct crypto_state *cs, struct feerange { enum side higher_side; u64 min, max; + + bool allow_mistakes; }; static void init_feerange(struct feerange *feerange, u64 commitment_fee, - const u64 offer[NUM_SIDES]) + const u64 offer[NUM_SIDES], + bool allow_mistakes) { feerange->min = 0; @@ -374,13 +377,17 @@ static void adjust_feerange(struct crypto_state *cs, struct feerange *feerange, u64 offer, enum side side) { - if (offer < feerange->min || offer > feerange->max) - peer_failed(PEER_FD, cs, channel_id, - "%s offer %"PRIu64 - " not between %"PRIu64" and %"PRIu64, - side == LOCAL ? "local" : "remote", - offer, feerange->min, feerange->max); + if (offer < feerange->min || offer > feerange->max) { + if (!feerange->allow_mistakes || side != REMOTE) + peer_failed(PEER_FD, cs, channel_id, + "%s offer %"PRIu64 + " not between %"PRIu64" and %"PRIu64, + side == LOCAL ? "local" : "remote", + offer, feerange->min, feerange->max); + status_trace("Allowing deprecated out-of-range fee"); + return; + } /* BOLT #2: * @@ -444,6 +451,7 @@ int main(int argc, char *argv[]) u64 next_index[NUM_SIDES], revocations_received; u64 gossip_index; enum side whose_turn; + bool deprecated_api; subdaemon_setup(argc, argv); @@ -466,7 +474,8 @@ int main(int argc, char *argv[]) &reconnected, &next_index[LOCAL], &next_index[REMOTE], - &revocations_received)) + &revocations_received, + &deprecated_api)) master_badmsg(WIRE_CLOSING_INIT, msg); status_trace("satoshi_out = %"PRIu64"/%"PRIu64, @@ -512,7 +521,7 @@ int main(int argc, char *argv[]) } /* Now we have first two points, we can init fee range. */ - init_feerange(&feerange, commitment_fee, offer); + init_feerange(&feerange, commitment_fee, offer, deprecated_api); /* Now apply the one constraint from above (other is inside loop). */ adjust_feerange(&cs, &channel_id, &feerange, diff --git a/closingd/closing_wire.csv b/closingd/closing_wire.csv index 8c68c1b70de4..2394238978c9 100644 --- a/closingd/closing_wire.csv +++ b/closingd/closing_wire.csv @@ -24,6 +24,8 @@ closing_init,,reconnected,bool closing_init,,next_index_local,u64 closing_init,,next_index_remote,u64 closing_init,,revocations_received,u64 +# This means we allow closing negotiations out of bounds. +closing_init,,deprecated_api,bool # We received an offer, save signature. closing_received_signature,2002 diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 1835b384a354..6f613fdfd25e 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1976,7 +1976,8 @@ static void peer_start_closingd(struct peer *peer, reconnected, peer->next_index[LOCAL], peer->next_index[REMOTE], - num_revocations); + num_revocations, + deprecated_apis); /* We don't expect a response: it will give us feedback on * signatures sent and received, then closing_complete. */ From 53a9e4f7c4e5269b12dce5dd2a4307d5381688f4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:25:06 +1030 Subject: [PATCH 8/9] test_lightning.py: speed up test_closing_different_fees It was taking over 10 minutes under valgrind, causing Travis to time it out. This shrinks it to its essential tests, and also batches. Signed-off-by: Rusty Russell --- tests/test_lightningd.py | 73 +++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 386c2d13ba4a..7cfcd820d31e 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -982,38 +982,51 @@ def test_closing(self): def test_closing_different_fees(self): l1 = self.node_factory.get_node() - self.give_funds(l1, (10**6) * 5 + 1000000) # Default feerate = 15000/7500/1000 - # It will accept between upper and lower feerate, starting at normal. - for feerates in [ [ 20000, 15000, 7400 ], - [ 15000, 8000, 1000 ], - [ 15000, 6000, 1000 ], - [ 8000, 7500, 1000 ], - [ 8000, 1200, 100 ] ]: - # With and without dust - for pamount in [ 0, 545999, 546000, 546001, 10**6 // 2 ]: - l2 = self.node_factory.get_node(options=['--override-fee-rates={}/{}/{}' - .format(feerates[0], - feerates[1], - feerates[2])]) - - l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) - - self.fund_channel(l1, l2, 10**6) - if pamount > 0: - self.pay(l1, l2, pamount) - - l1.rpc.close(l2.info['id']) - l1.daemon.wait_for_log(' to CHANNELD_SHUTTING_DOWN') - l2.daemon.wait_for_log(' to CHANNELD_SHUTTING_DOWN') - - l1.daemon.wait_for_log(' to CLOSINGD_COMPLETE') - l2.daemon.wait_for_log(' to CLOSINGD_COMPLETE') - - bitcoind.generate_block(1) - l1.daemon.wait_for_log(' to ONCHAIND_MUTUAL') - l2.daemon.wait_for_log(' to ONCHAIND_MUTUAL') + # It will start at the second number, accepting anything above the first. + feerates=[ [ 20000, 15000, 7400 ], + [ 8000, 1001, 100 ] ] + amounts = [ 0, 545999, 546000 ] + num_peers = len(feerates) * len(amounts) + self.give_funds(l1, (10**6) * num_peers + 10000 * num_peers) + + # Create them in a batch, but only valgrind one in three, for speed! + peers = [] + for feerate in feerates: + for amount in amounts: + p = self.node_factory.get_node(options=['--override-fee-rates={}/{}/{}' + .format(feerate[0], + feerate[1], + feerate[2])]) + p.feerate = feerate + p.amount = amount + l1.rpc.connect(p.info['id'], 'localhost', p.info['port']) + peers.append(p) + + for p in peers: + l1.rpc.fundchannel(p.info['id'], 10**6) + + bitcoind.generate_block(6) + + # Now wait for them all to hit normal state, do payments + for p in peers: + p.daemon.wait_for_log('to CHANNELD_NORMAL') + if p.amount != 0: + self.pay(l1,p,100000000) + + # Now close + for p in peers: + l1.rpc.close(p.info['id']) + + for p in peers: + p.daemon.wait_for_log(' to CLOSINGD_COMPLETE') + + bitcoind.generate_block(1) + for p in peers: + p.daemon.wait_for_log(' to ONCHAIND_MUTUAL') + + l1.daemon.wait_for_logs([' to ONCHAIND_MUTUAL'] * num_peers) @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") def test_permfail(self): From 89fb7505ffad3d0472eb26a0e46a066735ad0ca0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:25:06 +1030 Subject: [PATCH 9/9] pay: remove cmd pointer from htlc_out. Maintaining it was always fraught, since the command could go away if the JSON RPC died. Most recently, it was broken again on shutdown (see below). In future we may allow pay commands to block on previous payments, so it won't even be a 1:1 mapping. Generalize it: keep commands in a simple list and do a lookup when a payment fails/succeeds. Valgrind error file: valgrind-errors.5732 ==5732== Invalid read of size 8 ==5732== at 0x4149FD: remove_cmd_from_hout (pay.c:292) ==5732== by 0x468BAB: notify (tal.c:237) ==5732== by 0x469077: del_tree (tal.c:400) ==5732== by 0x4690C7: del_tree (tal.c:410) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x40F1EA: main (lightningd.c:362) ==5732== Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd ==5732== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x469150: del_tree (tal.c:421) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x4198F2: free_htlcs (peer_control.c:1281) ==5732== by 0x40EBA9: shutdown_subdaemons (lightningd.c:209) ==5732== by 0x40F1DE: main (lightningd.c:360) ==5732== Block was alloc'd at ==5732== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x468C30: allocate (tal.c:250) ==5732== by 0x4691F7: tal_alloc_ (tal.c:448) ==5732== by 0x40A279: new_htlc_out (htlc_end.c:143) ==5732== by 0x41FD64: send_htlc_out (peer_htlcs.c:397) ==5732== by 0x41511C: send_payment (pay.c:388) ==5732== by 0x41589E: json_sendpay (pay.c:513) ==5732== by 0x40D9B1: parse_request (jsonrpc.c:600) ==5732== by 0x40DCAC: read_json (jsonrpc.c:667) ==5732== by 0x45C706: next_plan (io.c:59) ==5732== by 0x45D1DD: do_plan (io.c:387) ==5732== by 0x45D21B: io_ready (io.c:397) Signed-off-by: Rusty Russell --- lightningd/htlc_end.c | 6 +-- lightningd/htlc_end.h | 6 +-- lightningd/lightningd.c | 1 + lightningd/lightningd.h | 3 ++ lightningd/pay.c | 90 ++++++++++++++++++++++++++++------------- lightningd/peer_htlcs.c | 6 +-- lightningd/peer_htlcs.h | 1 - wallet/wallet.c | 1 - 8 files changed, 70 insertions(+), 44 deletions(-) diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index 6de06e33f6a3..ca6ecc72af0f 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -125,8 +125,6 @@ struct htlc_out *htlc_out_check(const struct htlc_out *hout, htlc_state_name(hout->hstate)); else if (hout->failuremsg && hout->preimage) return corrupt(hout, abortstr, "Both failed and succeeded"); - else if (hout->cmd && hout->in) - return corrupt(hout, abortstr, "Both local and forwarded"); return cast_const(struct htlc_out *, hout); } @@ -137,8 +135,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, u64 msatoshi, u32 cltv_expiry, const struct sha256 *payment_hash, const u8 *onion_routing_packet, - struct htlc_in *in, - struct command *cmd) + struct htlc_in *in) { struct htlc_out *hout = tal(ctx, struct htlc_out); @@ -150,7 +147,6 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, hout->msatoshi = msatoshi; hout->cltv_expiry = cltv_expiry; hout->payment_hash = *payment_hash; - hout->cmd = cmd; memcpy(hout->onion_routing_packet, onion_routing_packet, sizeof(hout->onion_routing_packet)); diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index c6901c998e9f..1d2a8c9c7b74 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -75,9 +75,6 @@ struct htlc_out { /* Where it's from, if not going to us. */ struct htlc_in *in; - - /* Otherwise, this MAY be non-null if there's a pay command waiting */ - struct command *cmd; }; static inline const struct htlc_key *keyof_htlc_in(const struct htlc_in *in) @@ -132,8 +129,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, u64 msatoshi, u32 cltv_expiry, const struct sha256 *payment_hash, const u8 *onion_routing_packet, - struct htlc_in *in, - struct command *cmd); + struct htlc_in *in); void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin); void connect_htlc_out(struct htlc_out_map *map, struct htlc_out *hout); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 6de9f185c8b4..25a3fca512ba 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -62,6 +62,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx, ld->alias = NULL; ld->rgb = NULL; list_head_init(&ld->connects); + list_head_init(&ld->pay_commands); ld->wireaddrs = tal_arr(ld, struct wireaddr, 0); ld->portnum = DEFAULT_PORT; timers_init(&ld->timers, time_mono()); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index eebfdb794e13..86d5a065e319 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -128,6 +128,9 @@ struct lightningd { struct wallet *wallet; + /* Outstanding sendpay/pay commands. */ + struct list_head pay_commands; + /* Maintained by invoices.c */ struct invoices *invoices; diff --git a/lightningd/pay.c b/lightningd/pay.c index b9645910f77f..d52221ee69f7 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -18,28 +18,74 @@ #include #include -static void json_pay_success(struct command *cmd, const struct preimage *rval) +/* pay/sendpay command */ +struct pay_command { + struct list_node list; + + struct sha256 payment_hash; + struct command *cmd; +}; + +static void destroy_pay_command(struct pay_command *pc) { - struct json_result *response; + list_del(&pc->list); +} - /* Can be NULL if JSON RPC goes away. */ - if (!cmd) - return; +/* Owned by cmd */ +static struct pay_command *new_pay_command(struct command *cmd, + const struct sha256 *payment_hash, + struct lightningd *ld) +{ + struct pay_command *pc = tal(cmd, struct pay_command); + + pc->payment_hash = *payment_hash; + pc->cmd = cmd; + list_add(&ld->pay_commands, &pc->list); + tal_add_destructor(pc, destroy_pay_command); + return pc; +} + +static void json_pay_command_success(struct command *cmd, + const struct preimage *payment_preimage) +{ + struct json_result *response; response = new_json_result(cmd); json_object_start(response, NULL); - json_add_hex(response, "preimage", rval, sizeof(*rval)); + json_add_hex(response, "preimage", + payment_preimage, sizeof(*payment_preimage)); json_object_end(response); command_success(cmd, response); } -static void json_pay_failed(struct command *cmd, +static void json_pay_success(struct lightningd *ld, + const struct sha256 *payment_hash, + const struct preimage *payment_preimage) +{ + struct pay_command *pc, *next; + + list_for_each_safe(&ld->pay_commands, pc, next, list) { + if (!structeq(payment_hash, &pc->payment_hash)) + continue; + + /* Deletes itself. */ + json_pay_command_success(pc->cmd, payment_preimage); + } +} + +static void json_pay_failed(struct lightningd *ld, + const struct sha256 *payment_hash, enum onion_type failure_code, const char *details) { - /* Can be NULL if JSON RPC goes away. */ - if (cmd) { - command_fail(cmd, "Failed: %s (%s)", + struct pay_command *pc, *next; + + list_for_each_safe(&ld->pay_commands, pc, next, list) { + if (!structeq(payment_hash, &pc->payment_hash)) + continue; + + /* Deletes itself. */ + command_fail(pc->cmd, "failed: %s (%s)", onion_type_name(failure_code), details); } } @@ -49,10 +95,7 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout, { wallet_payment_set_status(ld->wallet, &hout->payment_hash, PAYMENT_COMPLETE, rval); - /* Can be NULL if JSON RPC goes away. */ - if (hout->cmd) - json_pay_success(hout->cmd, rval); - hout->cmd = NULL; + json_pay_success(ld, &hout->payment_hash, rval); } /* Return NULL if the wrapped onion error message has no @@ -282,17 +325,10 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, * and payment again. */ (void) retry_plausible; - json_pay_failed(hout->cmd, failcode, failmsg); + json_pay_failed(ld, &hout->payment_hash, failcode, failmsg); tal_free(tmpctx); } -/* When JSON RPC goes away, cmd is freed: detach from the hout */ -static void remove_cmd_from_hout(struct command *cmd, struct htlc_out *hout) -{ - assert(hout->cmd == cmd); - hout->cmd = NULL; -} - /* Returns true if it's still pending. */ static bool send_payment(struct command *cmd, const struct sha256 *rhash, @@ -362,7 +398,8 @@ static bool send_payment(struct command *cmd, &payment->destination)); return false; } - json_pay_success(cmd, payment->payment_preimage); + json_pay_command_success(cmd, + payment->payment_preimage); return false; } wallet_payment_delete(cmd->ld->wallet, rhash); @@ -387,8 +424,7 @@ static bool send_payment(struct command *cmd, failcode = send_htlc_out(peer, route[0].amount, base_expiry + route[0].delay, - rhash, onion, NULL, cmd, - &hout); + rhash, onion, NULL, &hout); if (failcode) { command_fail(cmd, "First peer not ready: %s", onion_type_name(failcode)); @@ -416,9 +452,7 @@ static bool send_payment(struct command *cmd, /* We write this into db when HTLC is actually sent. */ wallet_payment_setup(cmd->ld->wallet, payment); - /* If we fail, remove cmd ptr from htlc_out. */ - tal_add_destructor2(cmd, remove_cmd_from_hout, hout); - + new_pay_command(cmd, rhash, cmd->ld); tal_free(tmpctx); return true; } diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index f295937bc241..b1fb0bfd042c 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -375,7 +375,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct command *cmd, struct htlc_out **houtp) { struct htlc_out *hout; @@ -395,8 +394,7 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, /* Make peer's daemon own it, catch if it dies. */ hout = new_htlc_out(out->owner, out, amount, cltv, - payment_hash, onion_routing_packet, in, - cmd); + payment_hash, onion_routing_packet, in); tal_add_destructor(hout, hout_subd_died); msg = towire_channel_offer_htlc(out, amount, cltv, payment_hash, @@ -488,7 +486,7 @@ static void forward_htlc(struct htlc_in *hin, failcode = send_htlc_out(next, amt_to_forward, outgoing_cltv_value, &hin->payment_hash, - next_onion, hin, NULL, NULL); + next_onion, hin, NULL); if (!failcode) return; diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index 5f46297c5655..585d16667cdb 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -39,7 +39,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct command *cmd, struct htlc_out **houtp); struct htlc_out *find_htlc_out_by_ripemd(const struct peer *peer, diff --git a/wallet/wallet.c b/wallet/wallet.c index c881728d81a5..b9e983b32a89 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1059,7 +1059,6 @@ static bool wallet_stmt2htlc_out(const struct wallet_channel *channel, out->failuremsg = NULL; out->failcode = 0; - out->cmd = NULL; /* Need to defer wiring until we can look up all incoming * htlcs, will wire using origin_htlc_id */