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)) { diff --git a/closingd/closing.c b/closingd/closing.c index 94f5561cc6e2..6533cb1403a6 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,288 @@ 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; + + bool allow_mistakes; +}; + +static void init_feerange(struct feerange *feerange, + u64 commitment_fee, + const u64 offer[NUM_SIDES], + bool allow_mistakes) +{ + 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) { + 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: + * + * ...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 +441,17 @@ 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; - 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; + bool deprecated_api; subdaemon_setup(argc, argv); @@ -193,19 +467,21 @@ 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, &next_index[LOCAL], &next_index[REMOTE], - &revocations_received)) + &revocations_received, + &deprecated_api)) master_badmsg(WIRE_CLOSING_INIT, msg); 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 +495,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, deprecated_api); + + /* 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! */ diff --git a/closingd/closing_wire.csv b/closingd/closing_wire.csv index 4e4ad01d4498..2394238978c9 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 @@ -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/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_control.c b/lightningd/peer_control.c index f373460c7e18..6f613fdfd25e 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1764,14 +1764,16 @@ 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; 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 +1782,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,7 +1803,17 @@ 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); - return (new_diff < old_diff); + /* 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" + : new_diff > old_diff + ? "further from" + : "same distance to", + ideal_fee); + + return new_diff <= old_diff; } static void peer_received_closing_signature(struct peer *peer, const u8 *msg) @@ -1868,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; @@ -1916,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) @@ -1954,13 +1970,14 @@ 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, 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. */ 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/tests/test_lightningd.py b/tests/test_lightningd.py index 52c59993337d..7cfcd820d31e 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -980,6 +980,54 @@ def test_closing(self): wait_forget_channels(l1) wait_forget_channels(l2) + def test_closing_different_fees(self): + l1 = self.node_factory.get_node() + + # Default feerate = 15000/7500/1000 + # 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): l1,l2 = self.connect() 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 */