diff --git a/CHANGELOG.md b/CHANGELOG.md index 572442db37e4..ce73baec3494 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,74 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). +## [26.06.10] - 2026-11-05: "Quantum-Resistant Lightning Channel VII" + +This point release is recommended for all users. + +### Added + + - offers: create a CLN policy to constraint recurrence parameters to reasonable values, to avoid user footguns and computation overflows. + - docs: offers and fetchinvoice documentation mention the limits set on recurrence variables stablished by CLN's policy. + +### Changed + + - xpay: computes a budget for the number of hops in the payment depending on the final payload and blinded paths, it can fail early if the onion packet doesn't fit + - askrene-getroutes: penalizes every extra hop in every route with an homogeneous and constant probability of failure of 1%, as a consequence askrene will slightly prefer smaller routes and fewer payment parts. + - Plugins: a hook handler that fails or panics (pyln-client, cln-plugin) now gets a safe default reply instead of crashing or hanging lightningd; commitment_revocation and db_write still stop the node. + - bookkeeper: `bkpr-dumpincomecsv` `csv_file` must now be a plain filename ending in `.csv`; it is written in the node's network directory, and never through a symlink. + - `connect` with a host name now resolves it asynchronously in connectd, and fails with `--disable-dns`. + - currencyrate: IPv6 proxy addresses are rejected because bitreq does not support them + +### Fixed + + - xpay: Clamp the length of the payment routes using askrene's maxhops option aiming at producing routes that fit the data limits in the onion packet. + - JSON-RPC: `createinvoice` now returns error 901 for an already-used preimage instead of crashing the node. + - offers: no longer crashes on an offer with an empty offer_paths. + - channeld no longer crashes when a peer sends tx_signatures instead of commitment_signed during splice finalization. + - Protocol: a connected node could overwrite our peer's channel_update for a public channel before it was announced. + - JSON-RPC: `signpsbt` no longer crashes the node when given a PSBT it cannot sign (e.g. one with no outputs); it returns an error instead. + - dual-funding: a peer stalling a channel open no longer keeps our contributed UTXOs reserved. + - dual-funding: a peer stalling an RBF can no longer keep our channel from confirming. + - dualopend: a v2 channel open requesting 'option_zeroconf' is now refused unless we're configured to accept 0-confirmation channels from that peer, as openingd already does. + - Avoid closingd crashes on invalid legacy closing fee offers. + - runes: `pname` restrictions can no longer be bypassed with duplicate or punctuation-variant parameter names. + - onchaind: no longer crashes when an unexpected transaction spends a revoked HTLC output with too few outputs + - Protocol: onion messages with an empty (0-length) per-hop payload are now accepted, as BOLT 4 allows, instead of being silently dropped + - commando: a peer can no longer crash the node via a use-after-free by reusing a command id after a failed command + - dual-funding: `close` on a channel still being negotiated no longer crashes the node. + - dualopend: reject a dual-funded channel whose peer dust_limit exceeds the channel reserve. + - a peer can no longer open a channel with a dust_limit_satoshis above 5000sat, which let it trim our balance into fees on force close. + - Plugins: `commitment_revocation` is now called for each splice candidate while a splice is pending, so watchtowers can penalize whichever revoked commitment gets broadcast. + - lightningd: a self-payment completing while another payment was in flight read freed memory. + - openingd: a dual-funding peer could crash our Elements dualopend with an out-of-range prevtx_vout. + - Protocol: a peer could crash lightningd by committing an HTLC before the funding tx confirmed, then sending an error. + - lightningd: a channel peer flooding us with channel_updates no longer causes a db write for each one. + - lightningd: replay htlc_accepted for HTLCs committed before shutdown or on-chain instead of failing them on restart without calling the hook. + - lightningd: no longer crashes if a client hangs up while an `rpc_command` hook holds its `getlog` or `check` command. + - connectd: when out of file descriptors, close incoming connections which haven't sent init before disconnecting peers. + - connectd: no longer crashes when accepting a websocket connection fails (e.g. out of file descriptors). + - Plugins: hooks pending on an important plugin that dies are now called again on restart, instead of continuing as if the plugin had answered (an `htlc_accepted` hook could fail the HTLC). + - gossipd: more efficient handling of large reply_channel_range replies. + - lightningd: fix a crash on cooperative close of an unconfirmed dual-funded channel. + - gossipd: ignore gossip messages far larger than any valid one. + - gossipd: stale, duplicate and unknown-node gossip is now rejected before signature verification, so peers can no longer force needless ECDSA work + - gossipd: the gossip_store is also compacted based on its size, not just its record count. + - connectd: gossip messages far larger than any valid one are dropped before they are queued for gossipd. + - lightningd: a channel with an unconfirmed splice now notices if its funding output gets spent after a restart. + - connectd: a stalling DNS name in a peer's announced address no longer freezes all peer traffic; resolution is now done asynchronously. + - cln-grpc no longer panics on malformed request values. + - onchaind: Fixed a race between preimage return from downstream to upstream + - onchaind: Don't wait forever if the peer takes back an HTLC we were trying to claim with a late preimage + - bookkeeper: An incoming HTLC we claim with a late preimage after it timed out is no longer booked both to the peer and to us + - lightningd: unilateral close of a channel with a pending splice now works when the peer used a new funding key for it. + - splice_init: RBF of a pending splice now uses at least 25/24 of its feerate, which peers such as Eclair require. + - splice_init: a feerate above the most we'll pay no longer leaves the channel unable to splice until channeld restarts. + - abort_channels no longer fails the channel when a splice we already signed is pending. + - Protocol: channeld now rejects a 'start_batch' with 'batch_size' above 20, as the spec requires, so a peer can no longer stall the channel or use up memory + - dualopen and splicing: added tx_signatures verification according to BOLT-2 + - onchaind: an HTLC removed just before a unilateral close, with the same payment hash, amount and cltv as a live one, could be mistaken for it onchain, causing us to fail the live HTLC upstream while the peer could still claim it. + - commando and rune RPCs reject oversized runes instead of spending quadratic time decoding them. + ## [26.06.9] - 2026-10-06: "Quantum-Resistant Lightning Channel VI" This point release is recommended for all users. diff --git a/channeld/channeld.c b/channeld/channeld.c index c4cc4d816138..165184a73075 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -61,6 +61,8 @@ #define SAT_MIN(a, b) (amount_sat_less((a), (b)) ? (a) : (b)) +#define MAX_START_BATCH_SIZE 20 + struct peer { struct per_peer_state *pps; bool channel_ready[NUM_SIDES]; @@ -1306,10 +1308,6 @@ static u8 *send_commit_part(const tal_t *ctx, " for funding %s", remote_index, fmt_bitcoin_outpoint(tmpctx, funding)); - htlc_sigs = - calc_commitsigs(tmpctx, peer, txs, funding_wscript, htlc_map, - remote_index, remote_per_commit, &commit_sig, - remote_funding_pubkey); if (direct_outputs[LOCAL] != NULL) { pbase = penalty_base_new(tmpctx, remote_index, @@ -1343,6 +1341,8 @@ static u8 *send_commit_part(const tal_t *ctx, if (notify_master) { status_debug("Telling master we're about to commit..."); /* Tell master to save this next commit to database, then wait. + * Must happen before signing: a stateful signer (e.g. VLS) won't + * sign a different commitment at this index after a crash. */ msg = sending_commitsig_msg(NULL, remote_index, pbase, peer->channel->fee_states, @@ -1351,10 +1351,16 @@ static u8 *send_commit_part(const tal_t *ctx, /* Message is empty; receiving it is the point. */ master_wait_sync_reply(tmpctx, peer, take(msg), WIRE_CHANNELD_SENDING_COMMITSIG_REPLY); + } + htlc_sigs = + calc_commitsigs(tmpctx, peer, txs, funding_wscript, htlc_map, + remote_index, remote_per_commit, &commit_sig, + remote_funding_pubkey); + + if (notify_master) status_debug("Sending commit_sig with %zu htlc sigs", tal_count(htlc_sigs)); - } msg = towire_commitment_signed(ctx, &peer->channel_id, &commit_sig.s, @@ -2608,6 +2614,19 @@ static void handle_peer_start_batch(struct peer *peer, const u8 *msg) "below 2"); } + /* BOLT #2: + * A receiving node: + *... + * - If `batch_size` is strictly greater than 20: + * - MUST send a `warning` and close the connection, or send an `error` and + * fail the channel. + */ + if (batch_size > MAX_START_BATCH_SIZE) + peer_failed_warn(peer->pps, &peer->channel_id, + "start_batch batch_size %"PRIu16 + " exceeds maximum of %u", + batch_size, MAX_START_BATCH_SIZE); + handle_peer_commit_sig_batch(peer, peer_read(tmpctx, peer->pps), peer->channel->funding_pubkey[REMOTE], NULL, 0, 0, @@ -2618,23 +2637,6 @@ static void handle_peer_start_batch(struct peer *peer, const u8 *msg) } -/* Pops the penalty base for the given commitnum from our internal list. There - * may not be one, in which case we return NULL and leave the list - * unmodified. */ -static struct penalty_base * -penalty_base_by_commitnum(const tal_t *ctx, struct peer *peer, u64 commitnum) -{ - struct penalty_base *res = NULL; - for (size_t i = 0; i < tal_count(peer->pbases); i++) { - if (peer->pbases[i]->commitment_num == commitnum) { - res = tal_steal(ctx, peer->pbases[i]); - tal_arr_remove(&peer->pbases, i); - break; - } - } - return res; -} - static u8 *got_revoke_msg(struct peer *peer, u64 revoke_num, const struct secret *per_commitment_secret, const struct pubkey *next_per_commit_point, @@ -2643,9 +2645,9 @@ static u8 *got_revoke_msg(struct peer *peer, u64 revoke_num, const struct height_states *blockheight_states) { u8 *msg; - struct penalty_base *pbase; + struct penalty_base *pbases = tal_arr(tmpctx, struct penalty_base, 0); + const struct bitcoin_tx **ptxs = tal_arr(tmpctx, const struct bitcoin_tx *, 0); struct changed_htlc *changed = tal_arr(tmpctx, struct changed_htlc, 0); - const struct bitcoin_tx *ptx = NULL; for (size_t i = 0; i < tal_count(changed_htlcs); i++) { struct changed_htlc c; @@ -2660,23 +2662,36 @@ static u8 *got_revoke_msg(struct peer *peer, u64 revoke_num, tal_arr_expand(&changed, c); } - pbase = penalty_base_by_commitnum(tmpctx, peer, revoke_num); + /* During a splice we signed a commitment at revoke_num for the + * current funding and one for each splice candidate: the peer can + * broadcast whichever ends up valid, so each needs a penalty tx. */ + for (size_t i = 0; i < tal_count(peer->pbases);) { + const struct penalty_base *pbase = peer->pbases[i]; + const struct bitcoin_tx *ptx; + + if (pbase->commitment_num != revoke_num) { + i++; + continue; + } - if (pbase) { - /* DTODO we need penalty tx's per splice candidate */ ptx = penalty_tx_create( - NULL, peer->channel, peer->feerate_penalty, + ptxs, peer->channel, peer->feerate_penalty, peer->final_index, peer->final_ext_key, peer->final_scriptpubkey, per_commitment_secret, &pbase->txid, pbase->outnum, pbase->amount, HSM_FD); + if (ptx) { + tal_arr_expand(&pbases, *pbase); + tal_arr_expand(&ptxs, ptx); + } + tal_free(peer->pbases[i]); + tal_arr_remove(&peer->pbases, i); } msg = towire_channeld_got_revoke(peer, revoke_num, per_commitment_secret, next_per_commit_point, fee_states, blockheight_states, changed, - pbase, ptx); - tal_free(ptx); + pbases, ptxs); return msg; } @@ -3423,39 +3438,6 @@ static size_t calc_weight(enum tx_role role, const struct wally_psbt *psbt, return weight; } -/* Get the fundee amount in the channel after the splice */ -static struct amount_msat -relative_splice_balance_fundee(struct peer *peer, - enum tx_role our_role, - const struct wally_psbt *psbt, - int chan_output_index, - int chan_input_index) -{ - /* Relative fundee channel balance */ - u64 push_value; - - /* We calculcate the `push_value` to send to the - * hsmd, that is the remote amount in the channel - * after the splice. */ - switch (our_role) { - case TX_INITIATOR: - /* push_value is the fundee relative value so if we open the channel - * fundee is the remote node. */ - push_value = peer->splicing->accepter_relative; - break; - case TX_ACCEPTER: - /* push_value is the fundee relative value so if the remote node open the channel - * fundee in this case is the opener. */ - push_value = peer->splicing->opener_relative; - break; - default: - /* This should never happen. Help us to early catch the tx_role change */ - abort(); - } - - return amount_msat(push_value); -} - static struct amount_sat calc_balance(struct peer *peer) { struct amount_sat funding_amount_res; @@ -3969,12 +3951,7 @@ static void resume_splice_negotiation(struct peer *peer, check_tx_abort(peer, msg_received, &inflight->outpoint.txid); - if (their_commit) { - if (inflight->last_tx != their_commit->tx) - inflight->last_tx = tal_free(inflight->last_tx); - inflight->last_tx = tal_steal(inflight, their_commit->tx); - inflight->last_sig = their_commit->commit_signature; - + if (splice_record_commitment(inflight, their_commit)) { msg = towire_channeld_update_inflight(NULL, current_psbt, their_commit->tx, &their_commit->commit_signature, @@ -4287,11 +4264,38 @@ static struct inflight *inflights_new(struct peer *peer) return inf; } -static void update_hsmd_with_splice(struct peer *peer, struct inflight *inflight, - const enum tx_role our_role, - const struct amount_msat push_val) +/* hsmd_setup_channel's push_value is what the side which did *not* open the + * channel has in it (as openingd and dualopend send it). After a splice, + * that's its current balance plus its share of the splice, which is what + * channel_update_funding() will apply once the splice locks. */ +static struct amount_msat splice_push_value(const struct peer *peer, + const struct inflight *inflight) +{ + enum side fundee = peer->channel->opener == LOCAL ? REMOTE : LOCAL; + s64 funding_diff = sats_diff(inflight->amnt, + peer->channel->funding_sats); + s64 relative = fundee == LOCAL ? inflight->splice_amnt + : funding_diff - inflight->splice_amnt; + struct amount_msat push; + + if (!amount_msat_add_sat_s64(&push, + peer->channel->view[LOCAL].owed[fundee], + relative)) + peer_failed_err(peer->pps, &peer->channel_id, + "Splice would make %s balance negative", + fundee == LOCAL ? "our" : "your"); + return push; +} + +static void update_hsmd_with_splice(struct peer *peer, struct inflight *inflight) { u8 *msg; + struct amount_msat push_val = splice_push_value(peer, inflight); + + status_debug("Splice setup_channel to hsmd: channel_value %s," + " push_value %s", + fmt_amount_sat(tmpctx, inflight->amnt), + fmt_amount_msat(tmpctx, push_val)); /* local_upfront_shutdown_script, local_upfront_shutdown_wallet_index, * remote_upfront_shutdown_script aren't allowed to change, so we @@ -4352,7 +4356,6 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg) struct inflight *new_inflight; struct wally_psbt_output *new_chan_output; struct bitcoin_outpoint outpoint; - struct amount_msat current_push_val; const enum tx_role our_role = TX_ACCEPTER; u8 *abort_msg; enum peer_wire type; @@ -4556,9 +4559,7 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg) new_inflight->locked_scid = NULL; new_inflight->i_sent_sigs = false; - current_push_val = relative_splice_balance_fundee(peer, our_role,ictx->current_psbt, - outpoint.n, splice_funding_index); - update_hsmd_with_splice(peer, new_inflight, our_role, current_push_val); + update_hsmd_with_splice(peer, new_inflight); update_view_from_inflights(peer); @@ -4732,7 +4733,6 @@ static void splice_initiator_user_finalized(struct peer *peer) struct bitcoin_txid current_psbt_txid; struct amount_sat both_amount; struct commitsig *their_commit; - struct amount_msat current_push_val; const enum tx_role our_role = TX_INITIATOR; u8 *abort_msg; const u8* msg; @@ -4866,11 +4866,7 @@ static void splice_initiator_user_finalized(struct peer *peer) * to it. */ new_inflight->psbt = clone_psbt(new_inflight, ictx->current_psbt); - current_push_val = relative_splice_balance_fundee(peer, our_role, - new_inflight->psbt, - chan_output_index, - splice_funding_index); - update_hsmd_with_splice(peer, new_inflight, our_role, current_push_val); + update_hsmd_with_splice(peer, new_inflight); update_view_from_inflights(peer); @@ -4880,9 +4876,9 @@ static void splice_initiator_user_finalized(struct peer *peer) our_role, last_inflight_index(peer), true, true, NULL, 0); - - new_inflight->last_tx = tal_steal(new_inflight, their_commit->tx); - new_inflight->last_sig = their_commit->commit_signature; + if (!splice_record_commitment(new_inflight, their_commit)) + peer_failed_err(peer->pps, &peer->channel_id, + "Splice finalize: peer sent no commitment_signed"); outmsg = towire_channeld_update_inflight(NULL, new_inflight->psbt, their_commit->tx, @@ -5283,10 +5279,31 @@ static void handle_stfu_req(struct peer *peer, const u8 *inmsg) static void handle_abort_req(struct peer *peer, const u8 *inmsg) { + struct inflight *inflight; + if (!fromwire_channeld_abort(inmsg)) master_badmsg(WIRE_CHANNELD_ABORT, inmsg); - splice_abort(peer, last_inflight(peer), "requested by user"); + /* tx_abort is only for a negotiation, or to leave quiescence: once a + * splice is signed we are neither, and the peer may be sending us + * updates that splice_abort() would throw away. */ + if (!peer->splicing && !is_stfu_active(peer)) { + wire_sync_write(MASTER_FD, + take(towire_channeld_splice_state_error(NULL, + "No splice negotiation to abort"))); + return; + } + + /* BOLT #2: + * A sending node: + * - MUST NOT have already transmitted `tx_signatures` + */ + /* So keep a splice we have signed: abort only this negotiation. */ + inflight = last_inflight(peer); + if (inflight && inflight->i_sent_sigs) + inflight = NULL; + + splice_abort(peer, inflight, "requested by user"); } static void peer_in(struct peer *peer, const u8 *msg) @@ -5317,8 +5334,8 @@ static void peer_in(struct peer *peer, const u8 *msg) } } - /* Must get channel_ready before almost anything. */ - if (!peer->channel_ready[REMOTE]) { + /* Must exchange channel_ready before almost anything. */ + if (!peer->channel_ready[LOCAL] || !peer->channel_ready[REMOTE]) { if (type != WIRE_CHANNEL_READY && type != WIRE_SHUTDOWN /* We expect these for v2 !! */ diff --git a/channeld/channeld_wire.csv b/channeld/channeld_wire.csv index cb8a20717add..416b05230c63 100644 --- a/channeld/channeld_wire.csv +++ b/channeld/channeld_wire.csv @@ -205,8 +205,11 @@ msgdata,channeld_got_revoke,fee_states,fee_states, msgdata,channeld_got_revoke,blockheight_states,height_states, msgdata,channeld_got_revoke,num_changed,u16, msgdata,channeld_got_revoke,changed,changed_htlc,num_changed -msgdata,channeld_got_revoke,pbase,?penalty_base, -msgdata,channeld_got_revoke,penalty_tx,?bitcoin_tx, +# One for each commitment we signed at revokenum which has a penalty tx: the +# current funding's, and during a splice each splice candidate's. +msgdata,channeld_got_revoke,num_penalties,u16, +msgdata,channeld_got_revoke,pbases,penalty_base,num_penalties +msgdata,channeld_got_revoke,penalty_txs,bitcoin_tx,num_penalties # Wait for reply, to make sure it's on disk before we continue # (eg. if we sent another commitment_signed, that would implicitly ack). msgtype,channeld_got_revoke_reply,1122 diff --git a/channeld/splice.c b/channeld/splice.c index e5356d01af23..f554fca5fb3e 100644 --- a/channeld/splice.c +++ b/channeld/splice.c @@ -1,6 +1,20 @@ #include "config.h" +#include #include +bool splice_record_commitment(struct inflight *inflight, + struct commitsig *commit) +{ + if (!commit) + return false; + + if (inflight->last_tx != commit->tx) + inflight->last_tx = tal_free(inflight->last_tx); + inflight->last_tx = tal_steal(inflight, commit->tx); + inflight->last_sig = commit->commit_signature; + return true; +} + struct splice_state *splice_state_new(const tal_t *ctx) { struct splice_state *splice_state = tal(ctx, struct splice_state); diff --git a/channeld/splice.h b/channeld/splice.h index 47050aee041b..897f7690f9e2 100644 --- a/channeld/splice.h +++ b/channeld/splice.h @@ -25,6 +25,12 @@ struct splice_state { u32 count; }; +struct commitsig; + +/* Returns false without changing inflight if the peer sent no commitment. */ +bool splice_record_commitment(struct inflight *inflight, + struct commitsig *commit); + /* Sets `splice_state` items to default values */ struct splice_state *splice_state_new(const tal_t *ctx); diff --git a/closingd/closingd.c b/closingd/closingd.c index bd4e575b90ed..76a33343fc70 100644 --- a/closingd/closingd.c +++ b/closingd/closingd.c @@ -1059,6 +1059,21 @@ int main(int argc, char *argv[]) wrong_funding, &closing_txid, their_feerange); + /* BOLT #2: + * - otherwise, if `fee_satoshis` is not strictly between its last-sent + * `fee_satoshis` and its previously-received `fee_satoshis`, UNLESS it + * has since reconnected: + * - SHOULD send a `warning` and close the connection, or send an `error` + * and fail the channel. + */ + if (!amount_sat_eq(offer[REMOTE], offer[LOCAL]) + && (amount_sat_less(offer[REMOTE], feerange.min) + || amount_sat_greater(offer[REMOTE], feerange.max))) + peer_failed_warn(pps, &channel_id, + "Fee offer %s outside negotiation range %s-%s", + fmt_amount_sat(tmpctx, offer[REMOTE]), + fmt_amount_sat(tmpctx, feerange.min), + fmt_amount_sat(tmpctx, feerange.max)); } whose_turn = !whose_turn; diff --git a/closingd/test/Makefile b/closingd/test/Makefile new file mode 100644 index 000000000000..185a2732d9bb --- /dev/null +++ b/closingd/test/Makefile @@ -0,0 +1,13 @@ +CLOSINGD_TEST_SRC := $(wildcard closingd/test/run-*.c) +CLOSINGD_TEST_OBJS := $(CLOSINGD_TEST_SRC:.c=.o) +CLOSINGD_TEST_PROGRAMS := $(CLOSINGD_TEST_OBJS:.o=) + +ALL_C_SOURCES += $(CLOSINGD_TEST_SRC) +ALL_TEST_PROGRAMS += $(CLOSINGD_TEST_PROGRAMS) + +$(CLOSINGD_TEST_PROGRAMS): libcommon.a +$(CLOSINGD_TEST_OBJS): $(CLOSINGD_SRC) $(CLOSINGD_HEADERS) + +closingd/test/run-closingd-negotiation: closingd/closingd_wiregen.o + +check-units: $(CLOSINGD_TEST_PROGRAMS:%=unittest/%) diff --git a/closingd/test/run-closingd-negotiation.c b/closingd/test/run-closingd-negotiation.c new file mode 100644 index 000000000000..183c26460d90 --- /dev/null +++ b/closingd/test/run-closingd-negotiation.c @@ -0,0 +1,301 @@ +#include "config.h" +int closingd_main(int argc, char *argv[]); +#define main closingd_main +#define subdaemon_setup test_subdaemon_setup +#define daemon_shutdown test_daemon_shutdown +#define status_setup_sync test_status_setup_sync +#define status_fmt test_status_fmt +#define status_failed test_status_failed +#define peer_billboard test_peer_billboard +#define peer_failed_warn test_peer_failed_warn +#define peer_read test_peer_read +#define peer_write test_peer_write +#define wire_sync_read test_wire_sync_read +#define wire_sync_write test_wire_sync_write +#include "../closingd.c" +#undef main +#include +#include + +/* AUTOGENERATED MOCKS START */ +/* AUTOGENERATED MOCKS END */ + +enum reply { ZERO, REPEAT, AWAY, CROSS, LOWER_BOUND, UPPER_BOUND, AGREE }; +enum initial_offer { WIDE, ADJACENT, EQUAL, INITIAL_ZERO }; + +static enum side funder; +static enum reply reply; +static enum initial_offer initial_offer; +static bool expect_warning, high_remote, use_quickclose; +static size_t received, sent, completed; +static struct channel_id cid; +static struct bitcoin_outpoint funding; +static struct privkey privkey[NUM_SIDES]; +static struct pubkey pubkey[NUM_SIDES]; +static u8 *scripts[NUM_SIDES], *wscript, *initmsg; +static struct amount_sat first_remote, last_local; +static struct bitcoin_signature hsm_sig; +static struct bitcoin_txid saved_txid; +static const struct amount_sat outputs[NUM_SIDES] = { + AMOUNT_SAT(500000), AMOUNT_SAT(500000) +}; + +static void cleanup(void) +{ + tal_free(wscript); + tal_free(scripts[LOCAL]); + tal_free(scripts[REMOTE]); + common_shutdown(); + assert(tal_first(NULL) == NULL); +} + +bool test_subdaemon_setup(int argc UNUSED, char *argv[] UNUSED) +{ + return false; +} + +void test_daemon_shutdown(void) +{ +} + +void test_status_setup_sync(int fd UNUSED) +{ +} + +void test_status_fmt(enum log_level level UNUSED, + const struct node_id *peer UNUSED, + const char *fmt UNUSED, ...) +{ +} + +void test_peer_billboard(bool perm UNUSED, const char *fmt UNUSED, ...) +{ +} + +void test_status_failed(enum status_failreason reason UNUSED, + const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + vfprintf(stderr, fmt, ap); + va_end(ap); + abort(); +} + +void test_peer_failed_warn(struct per_peer_state *pps, + const struct channel_id *channel_id, + const char *fmt, ...) +{ + assert(expect_warning); + assert(memcmp(channel_id, &cid, sizeof(cid)) == 0); + assert(strstr(fmt, "outside negotiation range")); + assert(received == 2); + assert(completed == 0); + tal_free(tal_parent(pps)); + cleanup(); + exit(0); +} + +u8 *towire_hsmd_sign_mutual_close_tx(const tal_t *ctx, + const struct bitcoin_tx *tx, + const struct pubkey *remote_funding_key UNUSED) +{ + sign_tx_input(tx, 0, NULL, wscript, &privkey[LOCAL], &pubkey[LOCAL], + SIGHASH_ALL, &hsm_sig); + return tal_arr(ctx, u8, 0); +} + +bool fromwire_hsmd_sign_tx_reply(const void *p UNUSED, + struct bitcoin_signature *sig) +{ + *sig = hsm_sig; + return true; +} + +u8 *test_wire_sync_read(const tal_t *ctx, int fd) +{ + if (fd == HSM_FD) + return tal_arr(ctx, u8, 0); + assert(fd == REQ_FD); + if (initmsg) { + u8 *msg = tal_steal(ctx, initmsg); + initmsg = NULL; + return msg; + } + return towire_closingd_received_signature_reply(ctx, &saved_txid, true); +} + +bool test_wire_sync_write(int fd, const void *msg TAKES) +{ + if (fd == REQ_FD) { + switch (fromwire_peektype(msg)) { + case WIRE_CLOSINGD_RECEIVED_SIGNATURE: { + struct bitcoin_signature sig; + struct bitcoin_tx *tx; + assert(fromwire_closingd_received_signature(tmpctx, msg, &sig, &tx)); + bitcoin_txid(tx, &saved_txid); + break; + } + case WIRE_CLOSINGD_COMPLETE: + completed++; + break; + case WIRE_CLOSINGD_NOTIFICATION: + break; + default: + abort(); + } + } else + assert(fd == HSM_FD); + tal_free_if_taken(msg); + return true; +} + +void test_peer_write(struct per_peer_state *pps UNUSED, const void *msg TAKES) +{ + struct channel_id channel_id; + secp256k1_ecdsa_signature sig; + struct tlv_closing_signed_tlvs *tlvs; + assert(fromwire_closing_signed(tmpctx, msg, &channel_id, &last_local, + &sig, &tlvs)); + assert((tlvs && tlvs->fee_range) == use_quickclose); + sent++; + tal_free_if_taken(msg); +} + +static struct amount_sat offset(struct amount_sat fee, bool increase) +{ + if (increase) + assert(amount_sat_add(&fee, fee, AMOUNT_SAT(1))); + else + assert(amount_sat_sub(&fee, fee, AMOUNT_SAT(1))); + return fee; +} + +u8 *test_peer_read(const tal_t *ctx, struct per_peer_state *pps) +{ + struct amount_sat fee; + struct bitcoin_signature sig; + struct bitcoin_tx *tx; + + assert(received < 3); + if (received == 0) + fee = first_remote; + else if (received == 2 || reply == AGREE) + fee = last_local; + else { + switch (reply) { + case ZERO: + fee = AMOUNT_SAT(0); + break; + case REPEAT: + fee = first_remote; + break; + case AWAY: + fee = offset(first_remote, high_remote); + break; + case CROSS: + fee = offset(last_local, !high_remote); + break; + case LOWER_BOUND: + fee = offset(high_remote ? last_local : first_remote, true); + break; + case UPPER_BOUND: + fee = offset(high_remote ? first_remote : last_local, false); + break; + default: + abort(); + } + } + received++; + tx = close_tx(ctx, chainparams, pps, &cid, NULL, NULL, scripts, + &funding, AMOUNT_SAT(1000000), wscript, outputs, funder, + fee, AMOUNT_SAT(546), NULL); + sign_tx_input(tx, 0, NULL, wscript, &privkey[REMOTE], &pubkey[REMOTE], + SIGHASH_ALL, &sig); + return towire_closing_signed(ctx, &cid, fee, &sig.s, NULL); +} + +static void run_case(void) +{ + struct amount_sat min, initial, max; + char *argv[] = { "closingd", NULL }; + + calc_fee_bounds(closing_tx_weight_estimate(scripts, wscript, outputs, + AMOUNT_SAT(1000000), AMOUNT_SAT(546), + NULL, NULL), + 1000, 3750, 200000, AMOUNT_SAT(1000000), funder, + &min, &initial, &max); + first_remote = high_remote ? AMOUNT_SAT(50000) : AMOUNT_SAT(1000); + if (initial_offer == ADJACENT) + first_remote = offset(initial, high_remote); + else if (initial_offer == EQUAL) + first_remote = initial; + else if (initial_offer == INITIAL_ZERO) + first_remote = AMOUNT_SAT(0); + initmsg = towire_closingd_init(NULL, chainparams, &cid, &funding, + AMOUNT_SAT(1000000), &pubkey[LOCAL], &pubkey[REMOTE], + funder, outputs[LOCAL], outputs[REMOTE], AMOUNT_SAT(546), + 1000, 3750, 200000, NULL, NULL, scripts[LOCAL], scripts[REMOTE], + 50, CLOSING_FEE_NEGOTIATION_STEP_UNIT_PERCENTAGE, use_quickclose, NULL); + assert(closingd_main(1, argv) == 0); + assert(!expect_warning); + assert(completed == 1); + assert(received >= 1 && sent >= 1); +} + +static void check_case_once(void) +{ + pid_t pid = fork(); + int status; + + assert(pid >= 0); + if (pid == 0) { + run_case(); + cleanup(); + exit(0); + } + assert(waitpid(pid, &status, 0) == pid); + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) + fprintf(stderr, "funder=%d high=%d initial=%d reply=%d quickclose=%d status=%d\n", + funder, high_remote, initial_offer, reply, use_quickclose, status); + assert(WIFEXITED(status) && WEXITSTATUS(status) == 0); +} + +static void check_case(void) +{ + use_quickclose = false; + check_case_once(); + use_quickclose = true; + check_case_once(); +} + +int main(int argc UNUSED, char *argv[]) +{ + common_setup(argv[0]); + chainparams = chainparams_for_network("regtest"); + memset(&funding.txid, 1, sizeof(funding.txid)); + for (size_t i = 0; i < NUM_SIDES; i++) { + privkey[i].secret.data[31] = i + 1; + assert(pubkey_from_privkey(&privkey[i], &pubkey[i])); + scripts[i] = scriptpubkey_p2wpkh(NULL, &pubkey[i]); + } + wscript = bitcoin_redeem_2of2(NULL, &pubkey[LOCAL], &pubkey[REMOTE]); + + /* Exercise the actual main loop with signed offers; only IPC is mocked. */ + for (funder = LOCAL; funder <= REMOTE; funder++) { + for (size_t high = 0; high < 2; high++) { + high_remote = (high == 0); + initial_offer = WIDE; + for (reply = ZERO; reply <= AGREE; reply++) { + expect_warning = reply <= CROSS; + check_case(); + } + reply = AGREE; + expect_warning = false; + for (initial_offer = ADJACENT; initial_offer <= INITIAL_ZERO; initial_offer++) + check_case(); + } + } + cleanup(); + return 0; +} diff --git a/common/bolt12.c b/common/bolt12.c index ace3ff853595..6003d884cb61 100644 --- a/common/bolt12.c +++ b/common/bolt12.c @@ -8,8 +8,219 @@ #include #include #include +#include #include +static char *check_recurrence_policy_fields(const tal_t *ctx, + const struct recurrence *recurr, + const struct recurrence_base *base, + const struct recurrence_paywindow *paywindow, + const u32 *limit) +{ + u64 max_period; + + /* No recurrence -> nothing to check (BOLT already rejects stray + * recurrence_* fields without a recurrence descriptor). */ + if (!recurr) + return NULL; + + /* time_unit / period==0 are BOLT MUST checks; still reject here so + * create-path gets the same errors without going through decode. */ + if (recurr->time_unit != 0 && recurr->time_unit != 1 && + recurr->time_unit != 2) + return tal_fmt(ctx, "invalid recurrence time_unit %u", + recurr->time_unit); + + if (recurr->period == 0) + return tal_fmt(ctx, "invalid recurrence period %u", + recurr->period); + + switch (recurr->time_unit) { + case 0: + max_period = OFFER_RECURRENCE_MAX_PERIOD_SECONDS; + break; + case 1: + max_period = OFFER_RECURRENCE_MAX_PERIOD_DAYS; + break; + case 2: + max_period = OFFER_RECURRENCE_MAX_PERIOD_MONTHS; + break; + default: + /* Unreachable given the check above. */ + max_period = 0; + break; + } + if (recurr->period > max_period) + return tal_fmt( + ctx, + "recurrence period %u exceeds CLN maximum %" PRIu64 + " for time_unit %u", + recurr->period, max_period, recurr->time_unit); + + if (base && base->basetime > OFFER_RECURRENCE_MAX_BASETIME) + return tal_fmt(ctx, + "recurrence_base basetime %" PRIu64 + " exceeds CLN maximum %" PRIu64, + base->basetime, + (u64)OFFER_RECURRENCE_MAX_BASETIME); + + if (limit) { + if (*limit == 0) + return tal_fmt(ctx, "invalid recurrence limit %u", + *limit); + if (*limit > OFFER_RECURRENCE_MAX_LIMIT) + return tal_fmt( + ctx, + "recurrence_limit %u exceeds CLN maximum %" PRIu64, + *limit, (u64)OFFER_RECURRENCE_MAX_LIMIT); + } + + if (paywindow) { + if (paywindow->seconds_before > + OFFER_RECURRENCE_MAX_PAYWINDOW_SECONDS) + return tal_fmt( + ctx, + "recurrence_paywindow seconds_before %u exceeds " + "CLN maximum %" PRIu64, + paywindow->seconds_before, + (u64)OFFER_RECURRENCE_MAX_PAYWINDOW_SECONDS); + if (paywindow->seconds_after > + OFFER_RECURRENCE_MAX_PAYWINDOW_SECONDS) + return tal_fmt( + ctx, + "recurrence_paywindow seconds_after %u exceeds CLN " + "maximum %" PRIu64, + paywindow->seconds_after, + (u64)OFFER_RECURRENCE_MAX_PAYWINDOW_SECONDS); + } + + return NULL; +} + +char *offer_check_recurrence_policy(const tal_t *ctx, + const struct tlv_offer *offer) +{ + return check_recurrence_policy_fields( + ctx, offer_recurrence(offer), offer->offer_recurrence_base, + offer->offer_recurrence_paywindow, offer->offer_recurrence_limit); +} + +char *invreq_check_recurrence_policy(const tal_t *ctx, + const struct tlv_invoice_request *invreq) +{ + char *err; + u64 period_idx; + + err = check_recurrence_policy_fields( + ctx, invreq_recurrence(invreq), invreq->offer_recurrence_base, + invreq->offer_recurrence_paywindow, invreq->offer_recurrence_limit); + if (err) + return err; + + if (!invreq_recurrence(invreq)) + return NULL; + + /* Peer-controlled period index: counter [+ start]. */ + period_idx = 0; + if (invreq->invreq_recurrence_counter) { + if (*invreq->invreq_recurrence_counter > + OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt(ctx, + "invreq_recurrence_counter %u exceeds " + "CLN maximum %" PRIu64, + *invreq->invreq_recurrence_counter, + (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + period_idx = *invreq->invreq_recurrence_counter; + } + if (invreq->invreq_recurrence_start) { + if (*invreq->invreq_recurrence_start > + OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt(ctx, + "invreq_recurrence_start %u exceeds CLN " + "maximum %" PRIu64, + *invreq->invreq_recurrence_start, + (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + if (add_overflows_u64(period_idx, + *invreq->invreq_recurrence_start)) + return tal_fmt( + ctx, + "invreq_recurrence_start (%u) + counter (%" PRIu64 + ") overflows", + *invreq->invreq_recurrence_start, period_idx); + period_idx += *invreq->invreq_recurrence_start; + } + if (period_idx > OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt( + ctx, + "period index %" PRIu64 " exceeds CLN maximum %" PRIu64, + period_idx, (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + + return NULL; +} + + +char *invoice_check_recurrence_policy(const tal_t *ctx, + const struct tlv_invoice *invoice) +{ + char *err; + u64 period_idx; + + err = check_recurrence_policy_fields( + ctx, invoice_recurrence(invoice), invoice->offer_recurrence_base, + invoice->offer_recurrence_paywindow, invoice->offer_recurrence_limit); + if (err) + return err; + + if (!invoice_recurrence(invoice)) + return NULL; + + /* Copied from invreq (and matched on reply): counter [+ start]. */ + period_idx = 0; + if (invoice->invreq_recurrence_counter) { + if (*invoice->invreq_recurrence_counter > + OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt(ctx, + "invreq_recurrence_counter %u exceeds " + "CLN maximum %" PRIu64, + *invoice->invreq_recurrence_counter, + (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + period_idx = *invoice->invreq_recurrence_counter; + } + if (invoice->invreq_recurrence_start) { + if (*invoice->invreq_recurrence_start > + OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt(ctx, + "invreq_recurrence_start %u exceeds CLN " + "maximum %" PRIu64, + *invoice->invreq_recurrence_start, + (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + if (add_overflows_u64(period_idx, + *invoice->invreq_recurrence_start)) + return tal_fmt( + ctx, + "invreq_recurrence_start (%u) + counter (%" PRIu64 + ") overflows", + *invoice->invreq_recurrence_start, period_idx); + period_idx += *invoice->invreq_recurrence_start; + } + if (period_idx > OFFER_RECURRENCE_MAX_PERIOD_IDX) + return tal_fmt( + ctx, + "period index %" PRIu64 " exceeds CLN maximum %" PRIu64, + period_idx, (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + + /* Issuer-chosen; outside invoice_matches_request ranges. */ + if (invoice->invoice_recurrence_basetime && + *invoice->invoice_recurrence_basetime > OFFER_RECURRENCE_MAX_BASETIME) + return tal_fmt(ctx, + "invoice_recurrence_basetime %" PRIu64 + " exceeds CLN maximum %" PRIu64, + *invoice->invoice_recurrence_basetime, + (u64)OFFER_RECURRENCE_MAX_BASETIME); + + return NULL; +} + /* If chains is NULL, max_num_chains is ignored */ bool bolt12_chains_match(const struct bitcoin_blkid *chains, size_t max_num_chains, @@ -258,6 +469,13 @@ struct tlv_offer *offer_decode(const tal_t *ctx, return tal_free(offer); } + /* An empty offer_paths has no path to use, but fetchinvoice assumes a + * non-NULL offer_paths has one. Reject it, as we do empty offer_chains. */ + if (offer->offer_paths && tal_count(offer->offer_paths) == 0) { + *fail = tal_strdup(ctx, "Offer contains an empty offer_paths"); + return tal_free(offer); + } + /* BOLT #12: *... * - if `num_hops` is 0 in any `blinded_path` in `offer_paths`: @@ -369,6 +587,12 @@ struct tlv_invoice_request *invrequest_decode(const tal_t *ctx, return tal_free(invrequest); } + /* Likewise, sendinvoice assumes a non-NULL invreq_paths has a path. */ + if (invrequest->invreq_paths && tal_count(invrequest->invreq_paths) == 0) { + *fail = tal_strdup(ctx, "Invoice request contains an empty invreq_paths"); + return tal_free(invrequest); + } + /* BOLT #12: * - if `num_hops` is 0 in any `blinded_path` in `invreq_paths`: * - MUST reject the invoice request. @@ -436,32 +660,49 @@ struct tlv_invoice *invoice_decode_minimal(const tal_t *ctx, return invoice; } -static void add_days(struct tm *tm, u32 number) +static bool add_days(struct tm *tm, u32 number) { - tm->tm_mday += number; + s64 x = number + (s64)tm->tm_mday; + if (x > INT_MAX) + return false; + tm->tm_mday = (int)x; + return true; } -static void add_months(struct tm *tm, u32 number) +static bool add_months(struct tm *tm, u32 number) { - tm->tm_mon += number; + s64 x = number + (s64)tm->tm_mon; + if (x > INT_MAX) + return false; + tm->tm_mon = (int)x; + return true; } -static u64 time_change(u64 prevstart, u32 number, - void (*add_time)(struct tm *tm, u32 number), - bool day_const) +static bool time_change(u64 *new_start, u64 prevstart, u32 number, + bool (*add_time)(struct tm *tm, u32 number), + bool day_const) { + struct tm *tm_ptr; struct tm tm; time_t prev = prevstart, ret; - tm = *gmtime(&prev); + tm_ptr = gmtime(&prev); + /* Early exit, prev cannot be represented by struct tm. */ + if (!tm_ptr) + return false; + tm = *tm_ptr; for (;;) { struct tm new_tm = tm; - add_time(&new_tm, number); + + /* The individual time components are int and can overflow. */ + if (!add_time(&new_tm, number)) + return false; + ret = mktime(&new_tm); if (ret == (time_t)-1) - return 0; + return false; /* If we overflowed that month, try one less. */ if (!day_const || new_tm.tm_mday == tm.tm_mday) @@ -469,40 +710,62 @@ static u64 time_change(u64 prevstart, u32 number, tm.tm_mday--; } - return ret; + *new_start = ret; + return true; } -u64 offer_period_start(u64 basetime, size_t n, - const struct recurrence *recur) +bool offer_period_start(u64 *start, u64 basetime, size_t n, + const struct recurrence *recur) { /* BOLT-recurrence #12: * 1. A `time_unit` defining 0 (seconds), 1 (days), 2 (months). */ + u64 n_periods; + u32 n_periods_u32; + if (mul_overflows_u64(recur->period, n)) + return false; + n_periods = recur->period * (u64)n; + switch (recur->time_unit) { case 0: - return basetime + recur->period * n; + if (add_overflows_u64(basetime, n_periods)) + return false; + *start = basetime + n_periods; + break; case 1: - return time_change(basetime, recur->period * n, add_days, false); + if (!assign_overflow_u32(&n_periods_u32, n_periods) || + !time_change(start, basetime, n_periods_u32, add_days, + false)) + return false; + break; case 2: - return time_change(basetime, recur->period * n, add_months, true); + if (!assign_overflow_u32(&n_periods_u32, n_periods) || + !time_change(start, basetime, n_periods_u32, add_months, + true)) + return false; + break; default: /* This is our offer, how did we get here? */ - return 0; + return false; } + return true; } -void offer_period_paywindow(const struct recurrence *recurrence, +bool offer_period_paywindow(const struct recurrence *recurrence, const struct recurrence_paywindow *recurrence_paywindow, const struct recurrence_base *recurrence_base, u64 basetime, u64 period_idx, u64 *start, u64 *end) { + u64 pstart; /* BOLT-recurrence #12: * - if `offer_recurrence_paywindow` is present: */ if (recurrence_paywindow) { - u64 pstart = offer_period_start(basetime, period_idx, - recurrence); + if (!offer_period_start(&pstart, basetime, period_idx, + recurrence)) + return false; + /* BOLT-recurrence #12: * - if `recurrence_basetime` is present or * `recurrence_counter` is non-zero: @@ -511,15 +774,24 @@ void offer_period_paywindow(const struct recurrence *recurrence, * - SHOULD NOT send an `invoice_request` for a period later * than `seconds_after` seconds past that period start. */ + if (pstart < recurrence_paywindow->seconds_before) + return false; *start = pstart - recurrence_paywindow->seconds_before; + + if (add_overflows_u64(pstart, + recurrence_paywindow->seconds_after)) + return false; *end = pstart + recurrence_paywindow->seconds_after; /* First payment without recurrence_base, we give * ourselves 60 seconds, since period will start * now */ - if (!recurrence_base && period_idx == 0 - && recurrence_paywindow->seconds_after < 60) + if (!recurrence_base && period_idx == 0 && + recurrence_paywindow->seconds_after < 60) { + if (add_overflows_u64(pstart, 60)) + return false; *end = pstart + 60; + } } else { /* BOLT-recurrence #12: * - otherwise: @@ -529,17 +801,28 @@ void offer_period_paywindow(const struct recurrence *recurrence, */ if (period_idx == 0) *start = 0; - else - *start = offer_period_start(basetime, period_idx-1, - recurrence); + else { + + /* we use the return value 0 to indicate an error */ + if (!offer_period_start(start, basetime, period_idx - 1, + recurrence)) + return false; + } /* BOLT-recurrence #12: * - SHOULD NOT send an `invoice_request` for a period which * has already passed. */ - *end = offer_period_start(basetime, period_idx+1, - recurrence) - 1; + + /* we use the return value 0 to indicate an error */ + if (!offer_period_start(end, basetime, period_idx + 1, + recurrence)) + return false; + + /* the end of the current is the start of the next minus 1 */ + *end -= 1; } + return true; } struct tlv_invoice *invoice_decode(const tal_t *ctx, diff --git a/common/bolt12.h b/common/bolt12.h index 1f9b61f93d8f..2ad4cb372679 100644 --- a/common/bolt12.h +++ b/common/bolt12.h @@ -15,6 +15,47 @@ struct feature_set; */ #define BOLT12_DEFAULT_REL_EXPIRY 7200 +/* CLN recurrence policy limits: stricter than BOLT, keep values in a domain + * where period math (gmtime/mktime, u64 adds) stays well-defined and offers + * remain practical. Overflow checks in offer_period_start remain as a + * fail-closed backstop. */ + +/* Basetime must be gmtime-representable on typical platforms. Stay under + * year 10000 UTC (well below the range where glibc gmtime returns NULL). + * MAX_BASETIME = 9999-12-31 23:59:59 */ +#define OFFER_RECURRENCE_MAX_BASETIME 253402300799ULL + +/* Cap on offer_recurrence_limit (max_period_index). */ +#define OFFER_RECURRENCE_MAX_LIMIT 1000000000ULL + +/* Cap on invreq_recurrence_counter / invreq_recurrence_start and their sum. */ +#define OFFER_RECURRENCE_MAX_PERIOD_IDX 1000000000ULL + +/* One recurrence period should not exceed ~10 years (per time_unit). */ +#define OFFER_RECURRENCE_MAX_PERIOD_SECONDS (10ULL * 365 * 24 * 3600) +#define OFFER_RECURRENCE_MAX_PERIOD_DAYS (10ULL * 365) +#define OFFER_RECURRENCE_MAX_PERIOD_MONTHS (10ULL * 12) + +/* Pay window sides should not exceed ~1 year. */ +#define OFFER_RECURRENCE_MAX_PAYWINDOW_SECONDS (365ULL * 24 * 3600) + +/* CLN's policy on offers recurrence. This is stricter than the specification. + * It helps keep values inside a reasonable domain to avoid user footguns and + * internal value overflows. + * + * Returns NULL if OK, otherwise an allocated error string. + */ +char *offer_check_recurrence_policy(const tal_t *ctx, + const struct tlv_offer *offer); + +/* Same policy applied to recurrence fields carried in an invoice_request */ +char *invreq_check_recurrence_policy(const tal_t *ctx, + const struct tlv_invoice_request *invreq); + +/* Same policy applied to recurrence fields carried in an invoice */ +char *invoice_check_recurrence_policy(const tal_t *ctx, + const struct tlv_invoice *invoice); + /** * offer_encode - encode this complete bolt12 offer TLV into text. */ @@ -111,11 +152,12 @@ bool bolt12_chains_match(const struct bitcoin_blkid *chains, const struct chainparams *must_be_chain); /* Given a basetime, when does period N start? */ -u64 offer_period_start(u64 basetime, size_t n, - const struct recurrence *recurrence); +bool offer_period_start(u64 *start, u64 basetime, size_t n, + const struct recurrence *recurrence); -/* Get the start and end of the payment window for period N. */ -void offer_period_paywindow(const struct recurrence *recurrence, +/* Get the start and end of the payment window for period N. + * Returns a boolean to indicate success or failure. */ +bool offer_period_paywindow(const struct recurrence *recurrence, const struct recurrence_paywindow *recurrence_paywindow, const struct recurrence_base *recurrence_base, u64 basetime, u64 period_idx, diff --git a/common/channel_config.h b/common/channel_config.h index b796dc1aa96a..9f38573c0b51 100644 --- a/common/channel_config.h +++ b/common/channel_config.h @@ -81,6 +81,26 @@ struct channel_config { struct amount_msat max_dust_htlc_exposure_msat; }; +/* Bounds on the peer's dust_limit_satoshis, checked at negotiation. + * max_dust_htlc_exposure_msat above caps trimmed HTLCs; this caps the + * dust limit itself, so our balance output can't be trimmed too. */ + +/* BOLT #3: + * + * Bitcoin Core defines the following dust thresholds: + * + * - pay to pubkey hash (p2pkh): 546 satoshis + * - pay to script hash (p2sh): 540 satoshis + * - pay to witness pubkey hash (p2wpkh): 294 satoshis + * - pay to witness script hash (p2wsh): 330 satoshis + * - pay to anchor (p2a): 240 satoshis + * - unknown segwit versions: 354 satoshis + * - `OP_RETURN` outputs: these are never dust + */ +#define MIN_REMOTE_DUST_LIMIT AMOUNT_SAT(354) + +#define MAX_REMOTE_DUST_LIMIT AMOUNT_SAT(5000) + void towire_channel_config(u8 **pptr, const struct channel_config *config); void fromwire_channel_config(const u8 **ptr, size_t *max, struct channel_config *config); diff --git a/common/configdir.c b/common/configdir.c index 8a68f5d108cf..e855b12a64bc 100644 --- a/common/configdir.c +++ b/common/configdir.c @@ -191,7 +191,8 @@ static void parse_configvars(struct configvar **cvs, bool developer) { for (size_t i = 0; i < tal_count(cvs); i++) { - const char *problem; + const char *problem, *line; + const struct opt_table *ot; bool should_know; should_know = full_knowledge; @@ -208,12 +209,18 @@ static void parse_configvars(struct configvar **cvs, if (!problem) continue; + /* Don't print secrets */ + ot = configvar_unparsed(cvs[i]); + if (ot && (ot->type & OPT_CONCEAL) && cvs[i]->optarg) + line = tal_fmt(tmpctx, "%s=...", cvs[i]->optvar); + else + line = cvs[i]->configline; + if (cvs[i]->file) { errx(opt_exitcode, "Config file %s line %u: %s: %s", - cvs[i]->file, cvs[i]->linenum, - cvs[i]->configline, problem); + cvs[i]->file, cvs[i]->linenum, line, problem); } else { - errx(opt_exitcode, "--%s: %s", cvs[i]->configline, problem); + errx(opt_exitcode, "--%s: %s", line, problem); } } } diff --git a/common/gossip_constants.h b/common/gossip_constants.h index b11b4db4cbff..09f335592606 100644 --- a/common/gossip_constants.h +++ b/common/gossip_constants.h @@ -97,6 +97,10 @@ enum query_option_flags { #define GOSSIP_TOKEN_TIME(dev_fast_gossip_flag) \ DEV_FAST_GOSSIP(dev_fast_gossip_flag, 1, 3600) +/* How often we save a peer's channel_update for our channel to the db. */ +#define PEER_UPDATE_SAVE_INTERVAL(dev_fast_gossip_flag) \ + DEV_FAST_GOSSIP(dev_fast_gossip_flag, 5, 60) + /* This is where we keep our gossip */ #define GOSSIP_STORE_FILENAME "gossip_store" diff --git a/common/gossmap.c b/common/gossmap.c index aaa4e12c2bbe..1d4943993ae4 100644 --- a/common/gossmap.c +++ b/common/gossmap.c @@ -92,6 +92,9 @@ struct gossmap { /* How many live and dead records? */ size_t num_live, num_dead; + /* Bytes in records which were already deleted when we read them. */ + u64 deleted_bytes_read; + /* Optional logging callback */ void (*logcb)(void *cbarg, enum log_level level, @@ -790,6 +793,7 @@ static bool reopen_store(struct gossmap *map, u64 ended_off) /* This isn't quite true, as there may be deleted ones, but not many. */ map->num_dead = 0; + map->deleted_bytes_read = 0; /* Now do reload. */ map_catchup(map, NULL, NULL, false, &changed); @@ -843,6 +847,7 @@ static bool map_catchup(struct gossmap *map, if (flags & GOSSIP_STORE_DELETED_BIT) { map->num_dead++; + map->deleted_bytes_read += reclen; continue; } @@ -1418,6 +1423,7 @@ struct gossmap *gossmap_load_(const tal_t *ctx, map->fname = tal_strdup(map, filename); map->fd = open(map->fname, O_RDONLY); map->num_live = map->num_dead = 0; + map->deleted_bytes_read = 0; if (map->fd < 0) return tal_free(map); if (logcb) @@ -2033,3 +2039,8 @@ void gossmap_stats(const struct gossmap *map, u64 *num_live, u64 *num_dead) *num_live = map->num_live; *num_dead = map->num_dead; } + +u64 gossmap_deleted_bytes_read(const struct gossmap *map) +{ + return map->deleted_bytes_read; +} diff --git a/common/gossmap.h b/common/gossmap.h index 9b34be9341e8..37b937af6fc3 100644 --- a/common/gossmap.h +++ b/common/gossmap.h @@ -312,6 +312,11 @@ void gossmap_iter_end(const struct gossmap *map, struct gossmap_iter *iter); /* How dense is this? */ void gossmap_stats(const struct gossmap *map, u64 *num_live, u64 *num_dead); +/* Bytes in records which were already deleted when we read them: right + * after loading, that's every deleted record in the file. Records deleted + * after we've read them are not counted. */ +u64 gossmap_deleted_bytes_read(const struct gossmap *map); + /* For debugging: returns length read, and total known length of file */ u64 gossmap_lengths(const struct gossmap *map, u64 *total); diff --git a/common/onion_message_parse.c b/common/onion_message_parse.c index 8f09edbd2514..6dedd6caca22 100644 --- a/common/onion_message_parse.c +++ b/common/onion_message_parse.c @@ -117,7 +117,7 @@ const char *onion_message_parse(const tal_t *ctx, /* Now get onion shared secret and parse it. */ ecdh(&ephemeral, &onion_ss); - rs = process_onionpacket(tmpctx, op, &onion_ss, NULL, 0); + rs = process_onionmsg_packet(tmpctx, op, &onion_ss); if (!rs) { return tal_fmt(ctx, "onion_message_parse: can't process onionpacket ss=%s", fmt_secret(tmpctx, &onion_ss)); diff --git a/common/sphinx.c b/common/sphinx.c index bd4647a99d3c..9b3378c86695 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -643,13 +643,12 @@ bool dev_fail_process_onionpacket; * Given an onionpacket msg extract the information for the current * node and unwrap the remainder so that the node can forward it. */ -struct route_step *process_onionpacket( - const tal_t *ctx, - const struct onionpacket *msg, - const struct secret *shared_secret, - const u8 *assocdata, - const size_t assocdatalen - ) +static struct route_step *process_onion(const tal_t *ctx, + const struct onionpacket *msg, + const struct secret *shared_secret, + const u8 *assocdata, + const size_t assocdatalen, + bool is_onion_message) { struct route_step *step = talz(ctx, struct route_step); struct hmac hmac; @@ -715,7 +714,15 @@ struct route_step *process_onionpacket( * Since no `payload` TLV value can ever be shorter than 2 bytes, `length` values of 0 and 1 are * reserved. (`0` indicated a legacy format no longer supported, and `1` is reserved for future * use). */ - if (payload_size < 2 || !cursor) + if (!cursor) + return tal_free(step); + + /* BOLT #4: + * The `onionmsg_payloads` themselves are the same + * as the `hop_payloads` format, except there is no "legacy" length: a 0 + * `length` would mean an empty `onionmsg_payload` + */ + if (payload_size < 2 && !(is_onion_message && payload_size == 0)) return tal_free(step); /* This includes length field and hmac */ @@ -737,6 +744,25 @@ struct route_step *process_onionpacket( return step; } +struct route_step *process_onionpacket( + const tal_t *ctx, + const struct onionpacket *msg, + const struct secret *shared_secret, + const u8 *assocdata, + const size_t assocdatalen + ) +{ + return process_onion(ctx, msg, shared_secret, + assocdata, assocdatalen, false); +} + +struct route_step *process_onionmsg_packet(const tal_t *ctx, + const struct onionpacket *msg, + const struct secret *shared_secret) +{ + return process_onion(ctx, msg, shared_secret, NULL, 0, true); +} + unsigned dev_onion_reply_length = 256; struct onionreply *create_onionreply(const tal_t *ctx, diff --git a/common/sphinx.h b/common/sphinx.h index 0dd60926f64d..1c3c8fc130f2 100644 --- a/common/sphinx.h +++ b/common/sphinx.h @@ -111,6 +111,10 @@ struct route_step *process_onionpacket( const u8 *assocdata, const size_t assocdatalen); +struct route_step *process_onionmsg_packet(const tal_t *ctx, + const struct onionpacket *packet, + const struct secret *shared_secret); + /** * serialize_onionpacket - Serialize an onionpacket to a buffer. * diff --git a/common/test/run-blindedpath_onion.c b/common/test/run-blindedpath_onion.c index f52fa65ed056..0eac81c5f5a1 100644 --- a/common/test/run-blindedpath_onion.c +++ b/common/test/run-blindedpath_onion.c @@ -146,7 +146,7 @@ static u8 *next_onion(const tal_t *ctx, u8 *omsg, assert(unblind_onion(&path_key, ecdh, &ephemeral, &ss)); ecdh(&ephemeral, &onion_ss); - rs = process_onionpacket(tmpctx, op, &onion_ss, NULL, 0); + rs = process_onionmsg_packet(tmpctx, op, &onion_ss); assert(rs); /* The raw payload is prepended with length in the modern onion. */ diff --git a/common/test/run-bolt12-encode-test.c b/common/test/run-bolt12-encode-test.c index 60fab44303a8..249b0811b446 100644 --- a/common/test/run-bolt12-encode-test.c +++ b/common/test/run-bolt12-encode-test.c @@ -463,6 +463,14 @@ int main(int argc, char *argv[]) print_invalid_offer(offer, "Second offer_path is empty"); offer->offer_paths = NULL; + /* An offer_paths with no entries is invalid. */ + offer->offer_issuer_id = tal(offer, struct pubkey); + assert(pubkey_from_secret(&alice, offer->offer_issuer_id)); + offer->offer_paths = tal_arr(offer, struct blinded_path *, 0); + print_invalid_offer(offer, "offer_paths with zero entries"); + offer->offer_paths = NULL; + offer->offer_issuer_id = NULL; + offer->offer_chains = tal_arr(offer, struct bitcoin_blkid, 0); print_invalid_offer(offer, "offer_chains with zero entries"); offer->offer_chains = NULL; diff --git a/common/test/run-bolt12_period.c b/common/test/run-bolt12_period.c index 28dd706f9866..0c0c6f756642 100644 --- a/common/test/run-bolt12_period.c +++ b/common/test/run-bolt12_period.c @@ -8,6 +8,9 @@ #include #include +#include +#include + /* AUTOGENERATED MOCKS START */ /* Generated stub for amount_asset_is_main */ bool amount_asset_is_main(struct amount_asset *asset UNNEEDED) @@ -167,6 +170,176 @@ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNE { fprintf(stderr, "towire_u8_array called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ +/* Find a basetime where gmtime() fails (unrepresentable as struct tm). */ +static u64 find_gmtime_unrepresentable_basetime(void) +{ + /* Well past year 2^31 on typical glibc 64-bit time_t. */ + u64 candidate = 67768036191676800ULL; + time_t t = (time_t)candidate; + + if ((u64)t == candidate && gmtime(&t) == NULL) + return candidate; + + candidate = (u64)INT64_MAX; + t = (time_t)candidate; + if ((u64)t == candidate && gmtime(&t) == NULL) + return candidate; + + /* Skip platform-specific case rather than false-fail. */ + return 0; +} + +static void test_offer_period_start_overflows(void) +{ + struct recurrence rec; + u64 start, period_idx, basetime; + + /* Seconds: period * n overflows u64. */ + rec.time_unit = 0; + rec.period = 1e9; + period_idx = 1e11; + basetime = 0; + assert(!offer_period_start(&start, basetime, period_idx, &rec)); + + /* Seconds: basetime + period*n overflows. */ + rec.time_unit = 0; + rec.period = 1e9; + period_idx = 1e9; + basetime = 1.8e19; + assert(!offer_period_start(&start, basetime, period_idx, &rec)); + + /* Days: period * n does not fit in u32 for time_change. */ + rec.time_unit = 1; + rec.period = 1e4; + period_idx = 3e5; + basetime = 0; + assert(!offer_period_start(&start, basetime, period_idx, &rec)); + + /* Months: same u32 assign path. */ + rec.time_unit = 2; + rec.period = 1e4; + period_idx = 3e5; + basetime = 0; + assert(!offer_period_start(&start, basetime, period_idx, &rec)); + + /* Days/months: basetime unrepresentable by gmtime -> time_change fails. */ + { + u64 bad_base = find_gmtime_unrepresentable_basetime(); + if (bad_base) { + rec.time_unit = 1; + rec.period = 1; + assert(!offer_period_start(&start, bad_base, 0, &rec)); + assert(!offer_period_start(&start, bad_base, 1, &rec)); + + rec.time_unit = 2; + assert(!offer_period_start(&start, bad_base, 0, &rec)); + } + } + + /* Happy path still works (seconds). */ + rec.time_unit = 0; + rec.period = 60; + assert(offer_period_start(&start, 1000, 3, &rec)); + assert(start == 1000 + 180); +} + +static void test_offer_period_paywindow_overflows(void) +{ + struct recurrence rec; + struct recurrence_paywindow pw; + u64 pstart, pend; + + rec.time_unit = 0; + rec.period = 60; + pw.seconds_before = 10; + pw.seconds_after = 20; + + /* Normal paywindow (period_idx > 0 so no first-period 60s bump). */ + assert(offer_period_paywindow(&rec, &pw, NULL, 1000, 1, &pstart, + &pend)); + assert(pstart == 1000 + 60 - 10); + assert(pend == 1000 + 60 + 20); + + /* seconds_before larger than period start -> fail. */ + pw.seconds_before = 2000; + assert(!offer_period_paywindow(&rec, &pw, NULL, 1000, 0, &pstart, + &pend)); + + /* seconds_after overflows end. */ + pw.seconds_before = 0; + pw.seconds_after = 1; + assert(!offer_period_paywindow(&rec, &pw, NULL, UINT64_MAX, 0, + &pstart, &pend)); +} + +static void test_recurrence_policy_helpers(void) +{ + /* Zero-init so optional pointers are NULL without wiregen tlv_*_new. */ + struct tlv_offer offer; + struct tlv_invoice inv; + struct tlv_invoice_request ir; + struct recurrence rec; + struct recurrence_base base; + u32 limit, counter; + u64 inv_basetime; + char *err; + + memset(&offer, 0, sizeof(offer)); + memset(&inv, 0, sizeof(inv)); + memset(&ir, 0, sizeof(ir)); + + /* No recurrence: OK. */ + assert(offer_check_recurrence_policy(tmpctx, &offer) == NULL); + assert(invreq_check_recurrence_policy(tmpctx, &ir) == NULL); + assert(invoice_check_recurrence_policy(tmpctx, &inv) == NULL); + + /* Valid small recurrence. */ + rec.time_unit = 0; + rec.period = 60; + offer.offer_recurrence_compulsory = &rec; + assert(offer_check_recurrence_policy(tmpctx, &offer) == NULL); + + /* Period too large (seconds: > ~10 years). */ + rec.period = (u32)OFFER_RECURRENCE_MAX_PERIOD_SECONDS + 1; + err = offer_check_recurrence_policy(tmpctx, &offer); + assert(err); + assert(strstr(err, "period")); + + /* Basetime too large. */ + rec.period = 60; + base.proportional_amount = 0; + base.basetime = OFFER_RECURRENCE_MAX_BASETIME + 1; + offer.offer_recurrence_base = &base; + err = offer_check_recurrence_policy(tmpctx, &offer); + assert(err); + assert(strstr(err, "basetime")); + offer.offer_recurrence_base = NULL; + + /* Limit too large. */ + limit = (u32)OFFER_RECURRENCE_MAX_LIMIT + 1; + offer.offer_recurrence_limit = &limit; + err = offer_check_recurrence_policy(tmpctx, &offer); + assert(err); + assert(strstr(err, "limit")); + offer.offer_recurrence_limit = NULL; + + /* Invoice basetime policy (issuer-only field). */ + inv.offer_recurrence_compulsory = &rec; + inv_basetime = OFFER_RECURRENCE_MAX_BASETIME + 1; + inv.invoice_recurrence_basetime = &inv_basetime; + err = invoice_check_recurrence_policy(tmpctx, &inv); + assert(err); + assert(strstr(err, "invoice_recurrence_basetime")); + + /* Invreq counter too large. */ + ir.offer_recurrence_compulsory = &rec; + counter = (u32)OFFER_RECURRENCE_MAX_PERIOD_IDX + 1; + ir.invreq_recurrence_counter = &counter; + err = invreq_check_recurrence_policy(tmpctx, &ir); + assert(err); + assert(strstr(err, "invreq_recurrence_counter")); +} + int main(int argc, char *argv[]) { char *json; @@ -177,6 +350,12 @@ int main(int argc, char *argv[]) common_setup(argv[0]); + /* Standalone overflow / policy cases (no external JSON). */ + setenv("TZ", "", 1); + test_offer_period_start_overflows(); + test_offer_period_paywindow_overflows(); + test_recurrence_policy_helpers(); + if (argv[1]) json = grab_file_str(tmpctx, argv[1]); else { @@ -186,7 +365,7 @@ int main(int argc, char *argv[]) dir ? dir : ".tmp.lightningrfc", "bolt12/offer-period-test.json")); if (!json) { - printf("test file not found, skipping\n"); + printf("test file not found, skipping bolt vectors\n"); common_shutdown(); exit(0); } @@ -202,7 +381,7 @@ int main(int argc, char *argv[]) struct recurrence recurrence; int unit; const jsmntok_t *exp; - u64 base, secs, n; + u64 base, secs, n, start; time_t time; char *timestring, *tstr, **tparts; @@ -230,7 +409,8 @@ int main(int argc, char *argv[]) /* Now each part should appear. */ for (i = 0; tparts[i]; i++) assert(strstr(timestring, tparts[i])); - assert(offer_period_start(base, n, &recurrence) == secs); + assert(offer_period_start(&start, base, n, &recurrence)); + assert(start == secs); } common_shutdown(); return 0; diff --git a/common/test/run-onion-message-test.c b/common/test/run-onion-message-test.c index 299a0c5e9dda..9ba9fb967001 100644 --- a/common/test/run-onion-message-test.c +++ b/common/test/run-onion-message-test.c @@ -230,6 +230,72 @@ static u8 *add_hop(const char *name, return encrypted_recipient_data; } +/* Single-hop onion message to @id, whose per-hop payload is @payload */ +static u8 *single_hop_onionmsg(const struct pubkey *id, + const struct privkey *path_key, + const u8 *payload, + struct secret **path_secrets) +{ + struct secret ss; + struct pubkey alias; + struct sphinx_path *sp; + struct onionpacket *op; + + mykey = path_key; + ecdh(id, &ss); + assert(blindedpath_get_alias(&ss, id, &alias)); + + sp = sphinx_path_new(tmpctx, NULL, 0); + sphinx_add_hop(sp, &alias, payload); + op = create_onionpacket(tmpctx, sp, ROUTING_INFO_SIZE, path_secrets); + return serialize_onionpacket(tmpctx, op); +} + +/* Per bolt 4, a 0 'length' would mean an empty 'onionmsg_payload' */ +static void test_empty_onionmsg_payload(const struct privkey *privkey, + const struct pubkey *id) +{ + struct privkey path_key; + struct pubkey path_key_pub; + struct secret *path_secrets, *final_path_id; + struct sciddir_or_pubkey next_node; + struct tlv_onionmsg_tlv *final_om; + struct pubkey final_alias; + struct onionpacket *op; + u8 *packet, *next_onion_msg; + const u8 one_byte_payload[] = { 0x01 }; + + memset(&path_key, 42, sizeof(path_key)); + assert(pubkey_from_privkey(&path_key, &path_key_pub)); + + packet = single_hop_onionmsg(id, &path_key, + tal_arr(tmpctx, u8, 0), &path_secrets); + + op = parse_onionpacket(tmpctx, packet, tal_bytelen(packet), NULL); + assert(!process_onionpacket(tmpctx, op, &path_secrets[0], NULL, 0)); + + mykey = privkey; + assert(onion_message_parse(tmpctx, packet, &path_key_pub, id, + &next_onion_msg, &next_node, + &final_om, &final_alias, + &final_path_id) == NULL); + assert(!next_onion_msg); + assert(final_om); + assert(tal_count(final_om->fields) == 0); + assert(pubkey_eq(&final_alias, id)); + assert(!final_path_id); + + packet = single_hop_onionmsg(id, &path_key, + tal_dup_arr(tmpctx, u8, one_byte_payload, + sizeof(one_byte_payload), 0), + &path_secrets); + mykey = privkey; + assert(onion_message_parse(tmpctx, packet, &path_key_pub, id, + &next_onion_msg, &next_node, + &final_om, &final_alias, + &final_path_id) != NULL); +} + int main(int argc, char *argv[]) { const char *alias[] = {"Alice", "Bob", "Carol", "Dave"}; @@ -388,5 +454,7 @@ int main(int argc, char *argv[]) json_end(']'); json_end('}'); json_end('}'); + + test_empty_onionmsg_payload(&privkey[3], &id[3]); common_shutdown(); } diff --git a/connectd/Makefile b/connectd/Makefile index 4cec6fdde40f..6f6d68967c52 100644 --- a/connectd/Makefile +++ b/connectd/Makefile @@ -3,6 +3,7 @@ CONNECTD_HEADERS := connectd/connectd_wiregen.h \ connectd/connectd_gossipd_wiregen.h \ connectd/connectd.h \ + connectd/dns.h \ connectd/peer_exchange_initmsg.h \ connectd/handshake.h \ connectd/gossip_store.h \ diff --git a/connectd/connectd.c b/connectd/connectd.c index f1e08db71aeb..bb43bf6378b1 100644 --- a/connectd/connectd.c +++ b/connectd/connectd.c @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -485,6 +486,27 @@ static void conn_timeout(struct io_conn *conn) io_close(conn); } +/* Incoming connection which hasn't sent init yet. */ +struct pending_in { + struct list_node list; + struct io_conn *conn; + struct wireaddr_internal addr; +}; + +static void destroy_pending_in(struct pending_in *pin) +{ + list_del(&pin->list); +} + +static void conn_in_timeout(struct pending_in *pin) +{ + struct io_conn *conn = pin->conn; + + /* timer_expired() moved us to tmpctx, but conn is about to go. */ + tal_free(pin); + conn_timeout(conn); +} + /*~ So, where are you from? */ static bool get_remote_address(struct io_conn *conn, struct wireaddr_internal *addr) @@ -552,12 +574,22 @@ static struct io_plan *conn_in(struct io_conn *conn, struct conn_in *conn_in_arg) { struct daemon *daemon = conn_in_arg->daemon; + struct pending_in *pin; struct oneshot *timeout; + pin = tal(conn, struct pending_in); + pin->conn = conn; + pin->addr = conn_in_arg->addr; + list_add_tail(&daemon->pending_in, &pin->list); + tal_add_destructor(pin, destroy_pending_in); + /* If they don't complete handshake in reasonable time, we hang up */ timeout = new_reltimer(&daemon->timers, conn, time_from_sec(daemon->timeout_secs), - conn_timeout, conn); + conn_in_timeout, pin); + + /* Timer is freed when they send init: then they're not pending. */ + tal_steal(timeout, pin); /*~ The crypto handshake differs depending on whether you received or * initiated the socket connection, so there are two entry points. @@ -595,6 +627,17 @@ void close_random_connection(struct daemon *daemon) struct connecting *c; struct connecting_htable_iter cit; bool closed_connect_attempt = false; + struct pending_in *pin; + + /* Cheapest to lose: the oldest incoming connection without init. */ + pin = list_top(&daemon->pending_in, struct pending_in, list); + if (pin) { + status_debug("due to stress, closing incoming connection from %s" + " which has not sent init", + fmt_wireaddr_internal(tmpctx, &pin->addr)); + io_close(pin->conn); + return; + } /* First, close all transient connection attempts in-flight */ for (c = connecting_htable_first(daemon->connecting, &cit); @@ -646,6 +689,19 @@ void close_random_connection(struct daemon *daemon) io_close(best_peer->to_peer); } +/*~ We enabled io_set_extended_errors(), so when accept() fails, our + * listener callback is called with a NULL conn. */ +static struct io_plan *accept_failed(struct daemon *daemon) +{ + static bool accept_logged = false; + status_broken_once(&accept_logged, + "accepting incoming fd failed: %s", + strerror(errno)); + /* Maybe free up some fds by closing something. */ + close_random_connection(daemon); + return NULL; +} + /*~ When we get a direct connection in we set up its network address * then call handshake.c to set up the crypto state. */ static struct io_plan *connection_in(struct io_conn *conn, @@ -654,15 +710,8 @@ static struct io_plan *connection_in(struct io_conn *conn, struct conn_in conn_in_arg; /* Did we fail to accept? */ - if (!conn) { - static bool accept_logged = false; - status_broken_once(&accept_logged, - "accepting incoming fd failed: %s", - strerror(errno)); - /* Maybe free up some fds by closing something. */ - close_random_connection(daemon); - return NULL; - } + if (!conn) + return accept_failed(daemon); conn_in_arg.addr.u.wireaddr.is_websocket = false; if (!get_remote_address(conn, &conn_in_arg.addr)) @@ -688,6 +737,10 @@ static struct io_plan *websocket_connection_in(struct io_conn *conn, int err; struct conn_in conn_in_arg; + /* Did we fail to accept? */ + if (!conn) + return accept_failed(daemon); + conn_in_arg.addr.u.wireaddr.is_websocket = true; if (!get_remote_address(conn, &conn_in_arg.addr)) return io_close(conn); @@ -1053,6 +1106,33 @@ static struct io_plan *conn_proxy_init(struct io_conn *conn, connect); } +/*~ Resolving a DNS address used to be a synchronous getaddrinfo() right + * here: a slow or unresponsive nameserver would then hold up every peer + * on the box for the duration of the lookup. Now the lookup is forked + * off, and we come back here from the io loop when it's done: append the + * results (they go after any remaining announced addresses, exactly as the + * old code's appendings did) and move on. */ +static void dns_resolved(void *arg, const struct wireaddr *addrs, + const char *err) +{ + struct connecting *connect = arg; + const struct wireaddr_internal *addr = &connect->addrs[connect->addrnum]; + + if (err) { + tal_append_fmt(&connect->errors, + "%s: %s. ", + fmt_wireaddr_internal(tmpctx, addr), err); + } else { + for (size_t i = 0; i < tal_count(addrs); i++) + append_addr(&connect->addrs, &addrs[i]); + } + + /* This causes the same very limited recursion as `goto next` below, + * from a safe (timer) context. */ + connect->addrnum++; + try_connect_one_addr(connect); +} + /*~ This is the routine which tries to connect. */ static void try_connect_one_addr(struct connecting *connect) { @@ -1061,10 +1141,6 @@ static void try_connect_one_addr(struct connecting *connect) const struct wireaddr_internal *addr = &connect->addrs[connect->addrnum]; struct io_conn *conn; bool use_dns = connect->daemon->use_dns; - struct addrinfo hints, *ais, *aii; - int gai_err; - struct sockaddr_in *sa4; - struct sockaddr_in6 *sa6; assert(!connect->conn); assert(!connect->waiting); @@ -1132,49 +1208,18 @@ static void try_connect_one_addr(struct connecting *connect) fmt_wireaddr_internal(tmpctx, addr)); goto next; } - /* Resolve with getaddrinfo */ - memset(&hints, 0, sizeof(hints)); - hints.ai_socktype = SOCK_STREAM; - hints.ai_family = AF_UNSPEC; - hints.ai_protocol = 0; - /* AI_ADDRCONFIG speeds up connects on single-stack hosts by - * pruning unreachable address families. */ - hints.ai_flags = AI_ADDRCONFIG; - - gai_err = getaddrinfo((char *)addr->u.wireaddr.wireaddr.addr, - tal_fmt(tmpctx, "%d", - addr->u.wireaddr.wireaddr.port), - &hints, &ais); - if (gai_err != 0) { - tal_append_fmt(&connect->errors, - "%s: getaddrinfo error '%s'. ", - fmt_wireaddr_internal(tmpctx, addr), - gai_strerror(gai_err)); - goto next; - } - /* create new addrhints on-the-fly per result ... */ - for (aii = ais; aii; aii = aii->ai_next) { - struct wireaddr wa; - if (aii->ai_family == AF_INET) { - sa4 = (struct sockaddr_in *) aii->ai_addr; - wireaddr_from_ipv4(&wa, - &sa4->sin_addr, - addr->u.wireaddr.wireaddr.port); - } else if (aii->ai_family == AF_INET6) { - sa6 = (struct sockaddr_in6 *) aii->ai_addr; - wireaddr_from_ipv6(&wa, - &sa6->sin6_addr, - addr->u.wireaddr.wireaddr.port); - } else { - /* skip unsupported ai_family */ - continue; - } - append_addr(&connect->addrs, &wa); - /* don't forget to update convenience pointer */ - addr = &connect->addrs[connect->addrnum]; - } - freeaddrinfo(ais); - goto next; + /* Resolve in a forked child so a stalling + * nameserver cannot block the loop: dns_resolved() + * will pick this back up from the io loop. */ + async_dns_resolve(connect, + &connect->daemon->timers, + tal_strndup(tmpctx, + (const char *)addr->u.wireaddr.wireaddr.addr, + addr->u.wireaddr.wireaddr.addrlen), + addr->u.wireaddr.wireaddr.port, + dns_resolved, connect); + connect->connstate = "Resolving dns"; + return; } } @@ -2559,6 +2604,7 @@ int main(int argc, char *argv[]) daemon->peers = new_htable(daemon, peer_htable); daemon->listeners = tal_arr(daemon, struct io_listener *, 0); daemon->connecting = new_htable(daemon, connecting_htable); + list_head_init(&daemon->pending_in); daemon->important_ids = new_htable(daemon, important_id_htable); timers_init(&daemon->timers, time_mono()); daemon->gossmap_raw = NULL; diff --git a/connectd/connectd.h b/connectd/connectd.h index 6b6503df3746..b3c972bef99d 100644 --- a/connectd/connectd.h +++ b/connectd/connectd.h @@ -3,6 +3,7 @@ #include "config.h" #include #include +#include #include #include #include @@ -315,6 +316,9 @@ struct daemon { /* Peers we are trying to reach right now. */ struct connecting_htable *connecting; + /* Incoming connections which haven't sent init, oldest first */ + struct list_head pending_in; + /* Important (non-transient) peers we should reconnect to */ struct important_id_htable *important_ids; diff --git a/connectd/dns.c b/connectd/dns.c new file mode 100644 index 000000000000..de6b7b202cdb --- /dev/null +++ b/connectd/dns.c @@ -0,0 +1,476 @@ +#include "config.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/*~ Name resolution must never run on connectd's single io thread: with a + * slow or unresponsive nameserver, glibc's getaddrinfo() would sit in the + * loop for ~10s, holding up every peer on the machine. libc helpers are + * no help: getaddrinfo_a() is GNU-only (musl, i.e. our static builds, has + * nothing), and threads are against this daemon's grain (tal is not + * threadsafe). + * + * So instead we fork() a child to run getaddrinfo() and serialize the results + * down a pipe; the parent never blocks: it simply reads the pipe from the io + * loop, and runs the caller's callback from a loop callback. fork() is safe + * here because connectd is single-threaded, and since connectd sets + * SIGCHLD to SIG_IGN the children reap themselves: pipe EOF is all we need + * to know a child is finished. + * + * Doing the lookup in libc inside the child, rather than parsing DNS + * ourselves, keeps /etc/hosts, search domains, systemd-resolved and + * AI_ADDRCONFIG ordering behaving exactly as before, and keeps DNS packet + * parsing out of connectd itself. */ + +#define DNS_TOTAL_TIMEOUT_SECS 30 +/* One child per lookup, and we don't want a gossip churn (which can fire + * off hundreds of connects) to fork hundreds of resolvers: excess lookups + * queue FIFO and start as a child exits. The 30 second cap spans queue + * time too, so no lookup waits longer than that. */ +#define DNS_MAX_CONCURRENT_LOOKUPS 10 +#define DNS_MAX_ADDRS 64 +/* One serialized address: type byte (1=v4, 2=v6), 16 zero-padded address + * bytes, then 2 raw network-order port bytes. */ +#define DNS_RECORD_LEN 19 +#define DNS_PIPE_MAX (DNS_MAX_ADDRS * DNS_RECORD_LEN) +#define DNS_READ_BUF 512 + +/* How many resolvers we have running, and those waiting for a slot + * (connectd is single-threaded, so plain statics are fine). */ +static unsigned int dns_num_active; +static struct list_head dns_queue = LIST_HEAD_INIT(dns_queue); + +struct dns_lookup { + struct timers *timers; + dns_result_cb cb; + void *cb_arg; + + char *hostname; + u16 port; + + /* Results: once finished, exactly one is meaningful: addrs + * (non-empty) or err (a string). */ + struct wireaddr *addrs; + const char *err; + bool finished; + + /* Queued for a child slot / actually has a child+pipe? */ + bool queued; + bool started; + struct list_node list; + + pid_t child; /* 0 once we know it's gone. */ + struct io_conn *conn; /* Pipe read end, NULL once gone. */ + struct oneshot *total_timer; + struct oneshot *call_timer; + + /* Bytes accumulated from the child, and bytes of the last read. */ + u8 *in; + size_t inode; + u8 readbuf[DNS_READ_BUF]; + size_t readlen; +}; + +static void dns_start(struct dns_lookup *lookup); +static void dns_pump_queue(void); +static void dns_resolve_in_child(int fd, const char *hostname, u16 port); +static void dns_conn_destroy(struct io_conn *conn, struct dns_lookup *lookup); +static struct io_plan *dns_read_init(struct io_conn *conn, + struct dns_lookup *lookup); + +/* Defer the callback onto the io loop: we never call it synchronously, and + * it must be the last thing we do (it may free lookup, and thus us). */ +static void dns_call_cb(struct dns_lookup *lookup) +{ + lookup->call_timer = NULL; + status_debug("dns: lookup of %s %s", + lookup->hostname, + lookup->err ? lookup->err : "succeeded"); + /* Callback may free lookup: the lookups above are all we do first. */ + if (lookup->err) + lookup->cb(lookup->cb_arg, NULL, lookup->err); + else + lookup->cb(lookup->cb_arg, lookup->addrs, NULL); +} + +/* Stop everything and schedule the callback. Safe from timer or normal + * loop context; from inside an io callback on lookup->conn, detach the conn + * (lookup->conn = NULL) and return io_close() rather than using this. */ +static void dns_finish(struct dns_lookup *lookup) +{ + struct io_conn *conn; + + if (lookup->finished) + return; + lookup->finished = true; + + lookup->total_timer = tal_free(lookup->total_timer); + if (lookup->queued) { + list_del(&lookup->list); + lookup->queued = false; + } + if (lookup->child > 0) { + /* No EOF seen yet, so the child must still be resolving: + * any exit would have closed the pipe and woken us. Safe + * to kill: its pid cannot have been recycled yet. */ + kill(lookup->child, SIGKILL); + lookup->child = 0; + } + if (lookup->conn) { + conn = lookup->conn; + lookup->conn = NULL; /* Destructor checks this. */ + tal_free(conn); + } + lookup->call_timer = new_reltimer(lookup->timers, lookup, + time_from_sec(0), dns_call_cb, + lookup); + /* Our child (if we had one) is gone now, so a queued lookup may + * have a slot: hand it over (after scheduling our own callback, + * for fairness). */ + if (lookup->started) { + lookup->started = false; + dns_num_active--; + dns_pump_queue(); + } +} + +/* Hand the lookup its pipe and child: called directly if a slot is free, + * else from dns_pump_queue when one exits. */ +static void dns_start(struct dns_lookup *lookup) +{ + int fds[2]; + + assert(!lookup->started); + + if (pipe(fds) != 0) { + lookup->err = tal_fmt(lookup, "dns pipe failed: %s", + strerror(errno)); + dns_finish(lookup); + return; + } + /* Not strictly needed (we don't exec), but NSS backends can + * subprocess, and leaking the write end there would pin EOF. */ + fcntl(fds[0], F_SETFD, FD_CLOEXEC); + fcntl(fds[1], F_SETFD, FD_CLOEXEC); + + lookup->child = fork(); + if (lookup->child < 0) { + close(fds[0]); + close(fds[1]); + lookup->err = tal_fmt(lookup, "dns fork failed: %s", + strerror(errno)); + dns_finish(lookup); + return; + } + + if (lookup->child == 0) { + close(fds[0]); + dns_resolve_in_child(fds[1], lookup->hostname, lookup->port); + /* Not reached. */ + } + + close(fds[1]); + + lookup->conn = io_new_conn(lookup, fds[0], dns_read_init, lookup); + if (!lookup->conn) { /* Out of poll fds, etc. */ + close_noerr(fds[0]); + /* child > 0, so dns_finish will kill it. */ + lookup->err = tal_fmt(lookup, "dns pipe setup failed"); + dns_finish(lookup); + return; + } + tal_add_destructor2(lookup->conn, dns_conn_destroy, lookup); + + status_debug("dns: async lookup of %s started", lookup->hostname); + dns_num_active++; + lookup->started = true; +} + +/* Start queued lookups while we have child slots free. A failed start + * finishes its lookup (with an error, via callback) without ever taking a + * slot, so the loop simply moves on to the next one. */ +static void dns_pump_queue(void) +{ + while (dns_num_active < DNS_MAX_CONCURRENT_LOOKUPS + && !list_empty(&dns_queue)) { + struct dns_lookup *lookup + = list_pop(&dns_queue, struct dns_lookup, list); + lookup->queued = false; + dns_start(lookup); + } +} + +/*~ Parsing the child's output: strictly, and fail-closed on anything + * unexpected. */ +static void dns_parse_results(struct dns_lookup *lookup) +{ + const u8 *p = lookup->in; + size_t n = lookup->inode; + + if (n == 0) { + lookup->err = tal_fmt(lookup, "dns lookup of %s failed", + lookup->hostname); + return; + } + if (n % DNS_RECORD_LEN != 0) { + lookup->err = tal_fmt(lookup, + "dns resolver returned malformed data"); + return; + } + + for (size_t off = 0; off < n; off += DNS_RECORD_LEN) { + struct wireaddr wa; + /* The records are packed at arbitrary byte offsets, so we + * must copy the address into an aligned struct, not cast + * the buffer pointer. */ + struct in_addr ip4; + struct in6_addr ip6; + u16 port = ((u16)p[off + 17] << 8) | p[off + 18]; + + if (p[off] == 1) { + memcpy(&ip4, p + off + 1, sizeof(ip4)); + wireaddr_from_ipv4(&wa, &ip4, port); + } else if (p[off] == 2) { + memcpy(&ip6, p + off + 1, sizeof(ip6)); + wireaddr_from_ipv6(&wa, &ip6, port); + } else { + lookup->err = tal_fmt(lookup, "dns resolver returned" + " unexpected address type %u", + p[off]); + tal_free(lookup->addrs); + lookup->addrs = tal_arr(lookup, struct wireaddr, 0); + return; + } + + /* getaddrinfo can hand us repeats: dedup. */ + if (wireaddr_arr_contains(lookup->addrs, &wa)) + continue; + tal_arr_expand(&lookup->addrs, wa); + } + + if (tal_count(lookup->addrs) == 0) + lookup->err = tal_fmt(lookup, "dns lookup of %s returned no" + " addresses", lookup->hostname); +} + +/* The pipe closed: normally that means the child exited (EOF fails the + * conn rather than calling our read callback), and we parse the bytes it + * managed to send. Any io error which closes the conn lands here too: + * either way, don't trust the pid to kill: the child may already have been + * reaped (SIGCHLD is ignored) and its pid recycled into another child + * entirely. */ +static void dns_conn_destroy(struct io_conn *conn, struct dns_lookup *lookup) +{ + if (lookup->finished) + return; + + lookup->conn = NULL; /* Being freed: dns_finish mustn't. */ + lookup->child = 0; + dns_parse_results(lookup); + dns_finish(lookup); +} + +/*~ io callbacks: must not free conn, so anything terminal detaches conn and + * returns io_close(). EOF itself is *not* seen here (that fails the conn and + * runs its destructor): we're only ever called with lookup->readlen > 0. */ +static struct io_plan *dns_read_reply(struct io_conn *conn, + struct dns_lookup *lookup) +{ + size_t newlen = lookup->inode + lookup->readlen; + + if (newlen > DNS_PIPE_MAX) { + lookup->err = tal_fmt(lookup, "dns resolver returned too many" + " addresses"); + lookup->child = 0; /* Closing will EPIPE it out. */ + lookup->conn = NULL; + dns_finish(lookup); + return io_close(conn); + } + + tal_resize(&lookup->in, newlen); + memcpy(lookup->in + lookup->inode, lookup->readbuf, lookup->readlen); + lookup->inode = newlen; + + return io_read_partial(conn, lookup->readbuf, + sizeof(lookup->readbuf), &lookup->readlen, + dns_read_reply, lookup); +} + +static struct io_plan *dns_read_init(struct io_conn *conn, + struct dns_lookup *lookup) +{ + return io_read_partial(conn, lookup->readbuf, + sizeof(lookup->readbuf), &lookup->readlen, + dns_read_reply, lookup); +} + +static void dns_total_timeout(struct dns_lookup *lookup) +{ + lookup->total_timer = NULL; + lookup->err = tal_fmt(lookup, "dns lookup timed out"); + dns_finish(lookup); /* Kills the child, if it had one. */ +} + +/* If ctx goes away, we vanish too: main job is leaving the queue if we + * were still sitting in it. */ +static void destroy_dns_lookup(struct dns_lookup *lookup) +{ + if (lookup->queued) { + list_del(&lookup->list); + lookup->queued = false; + } +} + +/*~ Child side: we're a copy of connectd, so first shed every fd we + * inherited (see dns_resolve_in_child), then touch nothing shared, make no + * status_ calls (they'd impersonate connectd to lightningd) and never + * return: only _exit(). */ +static bool write_all(int fd, const void *p, size_t len) +{ + while (len != 0) { + ssize_t n = write(fd, p, len); + if (n <= 0) { + if (n < 0 && errno == EINTR) + continue; + return false; + } + p += n; + len -= n; + } + return true; +} + +static void dns_resolve_in_child(int fd, const char *hostname, u16 port) +{ + struct addrinfo hints, *res, *rp; + char portstr[7]; + unsigned int num = 0; + + /* We're a fork, so we inherited every fd connectd holds: peer + * sockets, the lightningd wires, and the *other* lookups' pipes: + * a child which then wedges would pin those pipes' EOF until its + * own alarm() fired. Move our pipe to a fixed low fd and drop + * everything else: from here we hold exactly one fd, and + * status_ calls through stdout can't even leak to lightningd. */ + if (fd != STDIN_FILENO) { + if (dup2(fd, STDIN_FILENO) == -1) + _exit(1); + close(fd); + fd = STDIN_FILENO; + } + /* dup2() clears close-on-exec, and NSS backends can subprocess: + * a stray inheritor of the write end would pin the parent's EOF + * just as well as a wedged child would. */ + fcntl(fd, F_SETFD, FD_CLOEXEC); + closefrom(STDIN_FILENO + 1); + + /* In case libc itself wedges, don't outlive the parent's cap. */ + alarm(DNS_TOTAL_TIMEOUT_SECS); + signal(SIGPIPE, SIG_IGN); + + memset(&hints, 0, sizeof(hints)); + hints.ai_socktype = SOCK_STREAM; + hints.ai_family = AF_UNSPEC; + hints.ai_protocol = 0; + /* AI_ADDRCONFIG speeds up connects on single-stack hosts by + * pruning unreachable address families. */ + hints.ai_flags = AI_ADDRCONFIG; + + snprintf(portstr, sizeof(portstr), "%u", port); + if (getaddrinfo(hostname, portstr, &hints, &res) != 0) + _exit(0); /* Write nothing: parent sees EOF. */ + + for (rp = res; rp; rp = rp->ai_next) { + u8 rec[DNS_RECORD_LEN]; + + if (num == DNS_MAX_ADDRS) + break; + memset(rec, 0, sizeof(rec)); + if (rp->ai_family == AF_INET) { + const struct sockaddr_in *s4 = (void *)rp->ai_addr; + if (rp->ai_addrlen < sizeof(*s4)) + continue; + rec[0] = 1; + memcpy(rec + 1, &s4->sin_addr, 4); + memcpy(rec + 17, &s4->sin_port, 2); + } else if (rp->ai_family == AF_INET6) { + const struct sockaddr_in6 *s6 = (void *)rp->ai_addr; + if (rp->ai_addrlen < sizeof(*s6)) + continue; + rec[0] = 2; + memcpy(rec + 1, &s6->sin6_addr, 16); + memcpy(rec + 17, &s6->sin6_port, 2); + } else { + continue; + } + /* On EPIPE the parent is gone: stop. */ + if (!write_all(fd, rec, sizeof(rec))) + break; + num++; + } + _exit(0); +} + +struct dns_lookup *async_dns_resolve(const tal_t *ctx, + struct timers *timers, + const char *hostname, + u16 port, + dns_result_cb cb, + void *cb_arg) +{ + struct dns_lookup *lookup = tal(ctx, struct dns_lookup); + + lookup->timers = timers; + lookup->cb = cb; + lookup->cb_arg = cb_arg; + lookup->hostname = tal_strdup(lookup, hostname); + lookup->port = port; + lookup->addrs = tal_arr(lookup, struct wireaddr, 0); + lookup->err = NULL; + lookup->finished = false; + lookup->queued = false; + lookup->started = false; + list_node_init(&lookup->list); + lookup->child = 0; + lookup->conn = NULL; + lookup->call_timer = NULL; + lookup->in = tal_arr(lookup, u8, 0); + lookup->inode = 0; + lookup->readlen = 0; + /* If ctx dies while we're queued, we must leave the queue. We + * deliberately don't kill a running child here: it may already have + * exited and its pid been recycled (SIGCHLD is ignored), and it + * will die on EPIPE or its own alarm regardless. */ + tal_add_destructor(lookup, destroy_dns_lookup); + + /* The cap spans queueing too: a lookup never takes longer than + * DNS_TOTAL_TIMEOUT_SECS from ask to callback. */ + lookup->total_timer = new_reltimer(timers, lookup, + time_from_sec(DNS_TOTAL_TIMEOUT_SECS), + dns_total_timeout, lookup); + + if (dns_num_active < DNS_MAX_CONCURRENT_LOOKUPS) { + dns_start(lookup); + } else { + status_debug("dns: lookup of %s queued, %u running", + hostname, dns_num_active); + list_add_tail(&dns_queue, &lookup->list); + lookup->queued = true; + } + return lookup; +} diff --git a/connectd/dns.h b/connectd/dns.h new file mode 100644 index 000000000000..809736acb263 --- /dev/null +++ b/connectd/dns.h @@ -0,0 +1,38 @@ +#ifndef LIGHTNING_CONNECTD_DNS_H +#define LIGHTNING_CONNECTD_DNS_H +#include "config.h" +#include +#include +#include + +struct timers; +struct wireaddr; + +/* What we hand to the callback: exactly one of `addrs` (non-empty, allocated + * off tmpctx) or `err` (a human-readable string off tmpctx) is set. */ +typedef void (*dns_result_cb)(void *cb_arg, + const struct wireaddr *addrs, + const char *err); + +struct dns_lookup; + +/*~ Resolve `hostname`:`port` without ever blocking the io loop: the lookup + * runs in a forked child which reports the results back down a pipe, which + * we read from the main loop. cb is called from an io loop callback, once + * per lookup, as long as @ctx (our parent) is still around: if @ctx is + * freed first, cb never runs (the child may linger until its own timeout, + * but its results go nowhere, and having closed every other fd it + * inherited it holds nothing else open meanwhile). + * + * At most DNS_MAX_CONCURRENT_LOOKUPS (10) resolvers run at once: extra + * lookups queue FIFO and start as earlier children exit. A lookup not + * finished within 30 seconds (queueing included) is failed; if it already + * has a child, that child is killed. Returns the lookup (a start failure + * is reported through cb, never by returning NULL). */ +struct dns_lookup *async_dns_resolve(const tal_t *ctx, + struct timers *timers, + const char *hostname, + u16 port, + dns_result_cb cb, + void *cb_arg); +#endif /* LIGHTNING_CONNECTD_DNS_H */ diff --git a/connectd/multiplex.c b/connectd/multiplex.c index b5fec97b0d90..b02f1c6d0378 100644 --- a/connectd/multiplex.c +++ b/connectd/multiplex.c @@ -920,6 +920,16 @@ static void handle_gossip_in(struct peer *peer, const u8 *msg) if (daemon_conn_queue_length(peer->daemon->gossipd) > 500000) return; + /* Don't queue gossip larger than any valid message: gossipd would + * only ignore it. */ + if (is_oversized_gossip_msg(msg)) { + status_peer_debug(&peer->id, + "Ignoring oversized gossip %s (%zu bytes)", + peer_wire_name(fromwire_peektype(msg)), + tal_bytelen(msg)); + return; + } + gmsg = towire_gossipd_recv_gossip(NULL, &peer->id, msg); /* gossipd doesn't log IO, so we log it here. */ @@ -1600,16 +1610,29 @@ static struct io_plan *read_body_from_peer_done(struct io_conn *peer_conn, peer_wire_name(t)); return io_close(peer_conn); } - status_peer_debug(&peer->id, "Activating for message %s", - peer_wire_name(t)); - subd = new_subd(peer, &channel_id); - /* We tell lightningd to fire up a subdaemon to handle this! */ + + /* Don't allocate a subd for these: nobody would claim it. */ + if (t != WIRE_ERROR && t != WIRE_WARNING) { + status_peer_debug(&peer->id, "Activating for message %s", + peer_wire_name(t)); + subd = new_subd(peer, &channel_id); + } else { + status_peer_io(LOG_IO_IN, &peer->id, decrypted); + status_peer_debug(&peer->id, "Received %s: %s", + peer_wire_name(t), + sanitize_error(tmpctx, decrypted, NULL)); + } + + /* We tell lightningd, which fires up a subdaemon to handle + * this, or deals with an error/warning itself. */ daemon_conn_send(peer->daemon->master, take(towire_connectd_peer_spoke(NULL, &peer->id, peer->counter, t, &channel_id, is_peer_error(tmpctx, decrypted)))); + if (!subd) + return next_read(peer_conn, peer); } /* Even if we just created it, call this to catch open_channel2 */ diff --git a/contrib/msggen/msggen/schema.json b/contrib/msggen/msggen/schema.json index 4cf64ff1474e..189e2ff85a3d 100644 --- a/contrib/msggen/msggen/schema.json +++ b/contrib/msggen/msggen/schema.json @@ -3533,7 +3533,7 @@ "rpc": "bkpr-dumpincomecsv", "title": "Command to emit a CSV of income events", "description": [ - "The **bkpr-dumpincomcsv** RPC command writes a CSV file to disk at *csv_file* location. This is a formatted output of the **listincome** RPC command." + "The **bkpr-dumpincomcsv** RPC command writes a CSV file named *csv_file* in the node's network directory. This is a formatted output of the **listincome** RPC command." ], "request": { "required": [ @@ -3550,7 +3550,7 @@ "csv_file": { "type": "string", "description": [ - "On-disk destination of the generated CSV file." + "Filename of the generated CSV file: a plain name ending in `.csv`, without a directory. It is written in the node's network directory (e.g. *~/.lightning/bitcoin*), replacing an existing file of that name, but never through a symlink." ] }, "consolidate_fees": { @@ -6822,7 +6822,7 @@ "type": "feerate" }, "description": [ - "An optional array [ *min*, *max* ], indicating the minimum and maximum feerates to offer: the peer will obey these if it supports the quick-close protocol. *slow* and *unilateral_close* are the defaults. Note that the maximum fee will be capped at the final commitment transaction fee (unless the experimental anchor-outputs option is negotiated)." + "An optional array [ *min*, *max* ], indicating the minimum and maximum feerates to offer (*min* must not exceed *max*): the peer will obey these if it supports the quick-close protocol. *slow* and *unilateral_close* are the defaults. Note that the maximum fee will be capped at the final commitment transaction fee (unless the experimental anchor-outputs option is negotiated)." ] } } @@ -7543,6 +7543,7 @@ "", "- -1: Catchall nonspecific error.", "- 900: An invoice with the given *label* already exists.", + "- 901: An invoice with the given *preimage* already exists.", "- 907: The offer associated with the invoice is not active." ], "author": [ @@ -7952,7 +7953,7 @@ " * per: how often the rune can be used, with suffix \"sec\" (default), \"min\", \"hour\", \"day\" or \"msec\", \"usec\" or \"nsec\". e.g. `per=5sec`.", " * rate: the rate limit, per minute, e.g. `rate=60` is equivalent to `per=1sec`.", " * pnum: the number of parameters. e.g. `pnum<2`.", - " * pnameX: the parameter named X e.g. `pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`. NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed.", + " * pnameX: the parameter named X e.g. `pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`. NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed. If several parameters match X (duplicate names, or names which only differ in punctuation like `amount_msat` and `amountmsat`), any restriction on X (including `pinvX_N`) fails.", " * parrN: the N'th parameter. e.g. `parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`.", " * pinvX_N: parse the parameter X as an invoice (bolt11 or bolt12) and extract field N for comparison. Fails if parameter X is not present, does not parse, or N is not one of the following field names:", " * amount", @@ -14619,13 +14620,15 @@ "recurrence_counter": { "type": "u64", "description": [ - "Required if the offer specifies recurrence, otherwise it is not allowed. recurrence_counter should first be set to 0, and incremented for each successive invoice in a given series." + "Required if the offer specifies recurrence, otherwise it is not allowed. recurrence_counter should first be set to 0, and incremented for each successive invoice in a given series.", + "It should be a number no greater than 1000000000." ] }, "recurrence_start": { "type": "number", "description": [ - "Indicates what period number to start at (usually 0)." + "Indicates what period number to start at (usually 0).", + "It should be a number no greater than 1000000000." ] }, "recurrence_label": { @@ -31363,26 +31366,30 @@ "recurrence": { "type": "string", "description": [ - "An invoice is expected at regular intervals. The argument is a positive number followed by one of `seconds`, `minutes`, `hours`, `days`, `weeks`, or `months` (variants without the trailing `s` are also permitted). This is encoded in the offer. The semantics of recurrence is fairly predictable, but fully documented in BOLT 12. e.g. `4weeks`." + "An invoice is expected at regular intervals. The argument is a positive number followed by one of `seconds`, `minutes`, `hours`, `days`, `weeks`, or `months` (variants without the trailing `s` are also permitted). This is encoded in the offer. The semantics of recurrence is fairly predictable, but fully documented in BOLT 12. e.g. `4weeks`.", + "Periods greater than 10 years are not accepted." ] }, "recurrence_base": { "type": "integer", "description": [ - "Time in seconds since the first day of 1970 UTC. This indicates when the first period begins; without this, the recurrence periods start from the first invoice." + "Time in seconds since the first day of 1970 UTC. This indicates when the first period begins; without this, the recurrence periods start from the first invoice.", + "This number must correspond to a time not beyond 9999-12-31 23:59:59 UTC." ] }, "recurrence_paywindow": { "type": "string", "description": [ - "Argument of form `-time+time`. The first time is the number of seconds before the start of a period in which an invoice and payment is valid, the second time is the number of seconds after the start of the period. For example *-604800+86400* means you can fetch an pay the invoice 4 weeks before the given period starts, and up to 1 day afterwards. The optional *%* indicates that the amount of the invoice will be scaled by the time remaining in the period. This is encoded in the offer." + "Argument of form `-time+time`. The first time is the number of seconds before the start of a period in which an invoice and payment is valid, the second time is the number of seconds after the start of the period. For example *-604800+86400* means you can fetch an pay the invoice 4 weeks before the given period starts, and up to 1 day afterwards. The optional *%* indicates that the amount of the invoice will be scaled by the time remaining in the period. This is encoded in the offer.", + "Either side of the pay window must not be greater than 1 year." ], "default": "that payment is allowed during the current and previous periods" }, "recurrence_limit": { "type": "u32", "description": [ - "To indicate the maximum period which exists. eg. `12` means there are 13 periods, from 0 to 12 inclusive. This is encoded in the offer." + "To indicate the maximum period which exists. eg. `12` means there are 13 periods, from 0 to 12 inclusive. This is encoded in the offer.", + "The limit cannot be 0 nor greater than 1000000000." ] }, "single_use": { @@ -43429,6 +43436,8 @@ "The `commitment_revocation` hook is used to inform the plugin about the state transition being completed, and deliver the penalty transaction.", "The penalty transaction could then be sent to a watchtower that automatically reacts in case one party attempts to settle using a revoked commitment.", "", + "While a splice is pending, each state has a commitment transaction for the current funding and one for each splice candidate, any of which could be broadcast: the hook is then called once for each of them, with the same `commitnum` and a different `commitment_txid`.", + "", "This is a chained hook: multiple plugins may be registered." ], "request": { @@ -44511,7 +44520,7 @@ "dust_limit_msat": { "type": "msat", "description": [ - "Dust limit for outputs." + "Dust limit for outputs on the peer's commitment transaction. Opens where this is below 354 sat or above 5000 sat are rejected before the hook is called." ] }, "max_htlc_value_in_flight_msat": { @@ -44748,7 +44757,7 @@ "dust_limit_msat": { "type": "msat", "description": [ - "Minimum output value below which outputs are considered dust." + "Minimum output value below which outputs are considered dust on the peer's commitment transaction. Opens where this is below 354 sat or above 5000 sat are rejected before the hook is called." ] }, "max_htlc_value_in_flight_msat": { diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 5dfbaa208d75..3d47b7a286a4 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -164,8 +164,6 @@ def set_exception(self, exc: Union[Exception, RpcException]) -> None: "error": { "code": code, "message": message, - # 'data' field "may be omitted." - "traceback": traceback.format_exc(), }, }) self.state = RequestState.FAILED @@ -265,6 +263,13 @@ def json(self) -> Dict[str, Any]: 'openchannel': {'result': 'reject'}, 'rpc_command': {'result': 'continue'}, 'custommsg': {'result': 'continue'}, + 'openchannel2': {'result': 'reject'}, + 'openchannel2_changed': {'result': 'reject'}, + 'openchannel2_sign': {'result': 'reject'}, + 'rbf_channel': {'result': 'reject'}, + 'onion_message_recv': {'result': 'continue'}, + 'onion_message_recv_secret': {'result': 'continue'}, + 'recover': {'result': 'continue'}, } @@ -718,7 +723,7 @@ def _dispatch_request(self, request: Request) -> None: # return a result or raise an exception. request.set_result(result) except Exception as e: - if name in hook_fallbacks: + if method.mtype == MethodType.HOOK and name in hook_fallbacks: response = hook_fallbacks[name] self.log(( "Hook handler for {name} failed with an exception. " diff --git a/contrib/pyln-client/tests/test_plugin.py b/contrib/pyln-client/tests/test_plugin.py index 9493a20cc78b..a585e0a1f97d 100644 --- a/contrib/pyln-client/tests/test_plugin.py +++ b/contrib/pyln-client/tests/test_plugin.py @@ -454,3 +454,60 @@ def some_method(some_arg: str = None): assert "some_method" in usage assert "some_arg" in usage assert "some description" in usage + + +def test_hook_fallbacks_and_exception_redaction(): + from pyln.client.plugin import hook_fallbacks + + # Test all hook fallbacks are defined properly + expected_hooks = { + 'htlc_accepted': {'result': 'fail', 'failure_message': '2002'}, + 'peer_connected': {'result': 'continue'}, + 'invoice_payment': {'result': 'continue'}, + 'openchannel': {'result': 'reject'}, + 'rpc_command': {'result': 'continue'}, + 'custommsg': {'result': 'continue'}, + 'openchannel2': {'result': 'reject'}, + 'openchannel2_changed': {'result': 'reject'}, + 'openchannel2_sign': {'result': 'reject'}, + 'rbf_channel': {'result': 'reject'}, + 'onion_message_recv': {'result': 'continue'}, + 'onion_message_recv_secret': {'result': 'continue'}, + 'recover': {'result': 'continue'}, + } + for hook, fallback in expected_hooks.items(): + assert hook in hook_fallbacks + assert hook_fallbacks[hook] == fallback + + # commitment_revocation and db_write must not have fallbacks (crash-only) + assert 'commitment_revocation' not in hook_fallbacks + assert 'db_write' not in hook_fallbacks + + p = Plugin(autopatch=False) + results = {} + + def fake_write_result(resultdict): + results['last'] = resultdict + + for hook, fallback in expected_hooks.items(): + @p.hook(hook) + def failing_hook(**kwargs): + raise RuntimeError("hook failure simulation") + + req = Request(p, 1, hook, {}) + req._write_result = fake_write_result + p._dispatch_request(req) + assert results['last']['result'] == fallback + del p.methods[hook] + + # Verify an RPC method named 'recover' is treated as an RPC method, not a hook + @p.method("recover") + def rpc_recover(): + raise RuntimeError("recover rpc failed") + + req = Request(p, 3, "recover", {}) + req._write_result = fake_write_result + p._dispatch_request(req) + assert 'error' in results['last'] + assert 'traceback' not in results['last']['error'] + assert results['last']['error']['code'] == -32600 diff --git a/doc/getting-started/getting-started/configuration.md b/doc/getting-started/getting-started/configuration.md index c7c1557e41fb..d6af4b4b7de8 100644 --- a/doc/getting-started/getting-started/configuration.md +++ b/doc/getting-started/getting-started/configuration.md @@ -452,7 +452,9 @@ If a 'DNS' hostname was given that resolves to a local interface, the daemon wil - **disable-dns** - Disable the DNS bootstrapping mechanism to find a node by its node ID. + Disable DNS lookups: the DNS bootstrapping mechanism to find a node by its + node ID, resolving DNS addresses announced by peers, and resolving a + *host* name passed to `connect`. - **tor-service-password**=_PASSWORD_ diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index b05e73225483..de10ab6d8b29 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -342,6 +342,8 @@ connections. Default is 9736. This will fail if the `hsm_secret` file exists. Your node will start the node in offline mode, for manual recovery. The secret can be extracted from the `hsm_secret` using lightning-hsmtool(8)'s `getsecret`. + Other users can see command line arguments (e.g. with `ps`) until `lightningd` wipes the secret at startup, and your shell may save it in its history. The lightning-recover(7) command avoids both. + * **alias**=*NAME* Up to 32 bytes of UTF-8 characters to tag your node. Completely silly, since @@ -721,7 +723,9 @@ all DNS lookups, to avoid leaking information. * **disable-dns** - Disable the DNS bootstrapping mechanism to find a node by its node ID. + Disable DNS lookups: the DNS bootstrapping mechanism to find a node by its + node ID, resolving DNS addresses announced by peers, and resolving a + *host* name passed to `connect`. * **message-padding**=*BOOL* diff --git a/doc/schemas/bkpr-dumpincomecsv.json b/doc/schemas/bkpr-dumpincomecsv.json index 461cb6818d11..0249957e6ae7 100644 --- a/doc/schemas/bkpr-dumpincomecsv.json +++ b/doc/schemas/bkpr-dumpincomecsv.json @@ -4,7 +4,7 @@ "rpc": "bkpr-dumpincomecsv", "title": "Command to emit a CSV of income events", "description": [ - "The **bkpr-dumpincomcsv** RPC command writes a CSV file to disk at *csv_file* location. This is a formatted output of the **listincome** RPC command." + "The **bkpr-dumpincomcsv** RPC command writes a CSV file named *csv_file* in the node's network directory. This is a formatted output of the **listincome** RPC command." ], "request": { "required": [ @@ -21,7 +21,7 @@ "csv_file": { "type": "string", "description": [ - "On-disk destination of the generated CSV file." + "Filename of the generated CSV file: a plain name ending in `.csv`, without a directory. It is written in the node's network directory (e.g. *~/.lightning/bitcoin*), replacing an existing file of that name, but never through a symlink." ] }, "consolidate_fees": { diff --git a/doc/schemas/close.json b/doc/schemas/close.json index a3212cdd4d67..871a9ea0b10f 100644 --- a/doc/schemas/close.json +++ b/doc/schemas/close.json @@ -66,7 +66,7 @@ "type": "feerate" }, "description": [ - "An optional array [ *min*, *max* ], indicating the minimum and maximum feerates to offer: the peer will obey these if it supports the quick-close protocol. *slow* and *unilateral_close* are the defaults. Note that the maximum fee will be capped at the final commitment transaction fee (unless the experimental anchor-outputs option is negotiated)." + "An optional array [ *min*, *max* ], indicating the minimum and maximum feerates to offer (*min* must not exceed *max*): the peer will obey these if it supports the quick-close protocol. *slow* and *unilateral_close* are the defaults. Note that the maximum fee will be capped at the final commitment transaction fee (unless the experimental anchor-outputs option is negotiated)." ] } } diff --git a/doc/schemas/createinvoice.json b/doc/schemas/createinvoice.json index a490c490851f..a30a4c7ad03b 100644 --- a/doc/schemas/createinvoice.json +++ b/doc/schemas/createinvoice.json @@ -189,6 +189,7 @@ "", "- -1: Catchall nonspecific error.", "- 900: An invoice with the given *label* already exists.", + "- 901: An invoice with the given *preimage* already exists.", "- 907: The offer associated with the invoice is not active." ], "author": [ diff --git a/doc/schemas/createrune.json b/doc/schemas/createrune.json index 5e09188decce..796a4f701049 100644 --- a/doc/schemas/createrune.json +++ b/doc/schemas/createrune.json @@ -35,7 +35,7 @@ " * per: how often the rune can be used, with suffix \"sec\" (default), \"min\", \"hour\", \"day\" or \"msec\", \"usec\" or \"nsec\". e.g. `per=5sec`.", " * rate: the rate limit, per minute, e.g. `rate=60` is equivalent to `per=1sec`.", " * pnum: the number of parameters. e.g. `pnum<2`.", - " * pnameX: the parameter named X e.g. `pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`. NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed.", + " * pnameX: the parameter named X e.g. `pnamedestination=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`. NOTE: until v24.05, X had to remove underscores from the parameter name (e.g. `pnameamount_msat` had to be specified as `pnameamountmsat`) but that is now fixed. If several parameters match X (duplicate names, or names which only differ in punctuation like `amount_msat` and `amountmsat`), any restriction on X (including `pinvX_N`) fails.", " * parrN: the N'th parameter. e.g. `parr0=1RustyRX2oai4EYYDpQGWvEL62BBGqN9T`.", " * pinvX_N: parse the parameter X as an invoice (bolt11 or bolt12) and extract field N for comparison. Fails if parameter X is not present, does not parse, or N is not one of the following field names:", " * amount", diff --git a/doc/schemas/fetchinvoice.json b/doc/schemas/fetchinvoice.json index 31584b7ebe1a..724358c060f7 100644 --- a/doc/schemas/fetchinvoice.json +++ b/doc/schemas/fetchinvoice.json @@ -35,13 +35,15 @@ "recurrence_counter": { "type": "u64", "description": [ - "Required if the offer specifies recurrence, otherwise it is not allowed. recurrence_counter should first be set to 0, and incremented for each successive invoice in a given series." + "Required if the offer specifies recurrence, otherwise it is not allowed. recurrence_counter should first be set to 0, and incremented for each successive invoice in a given series.", + "It should be a number no greater than 1000000000." ] }, "recurrence_start": { "type": "number", "description": [ - "Indicates what period number to start at (usually 0)." + "Indicates what period number to start at (usually 0).", + "It should be a number no greater than 1000000000." ] }, "recurrence_label": { diff --git a/doc/schemas/hook/commitment_revocation.json b/doc/schemas/hook/commitment_revocation.json index 3766ebfefd7b..752f354c2c8c 100644 --- a/doc/schemas/hook/commitment_revocation.json +++ b/doc/schemas/hook/commitment_revocation.json @@ -15,6 +15,8 @@ "The `commitment_revocation` hook is used to inform the plugin about the state transition being completed, and deliver the penalty transaction.", "The penalty transaction could then be sent to a watchtower that automatically reacts in case one party attempts to settle using a revoked commitment.", "", + "While a splice is pending, each state has a commitment transaction for the current funding and one for each splice candidate, any of which could be broadcast: the hook is then called once for each of them, with the same `commitnum` and a different `commitment_txid`.", + "", "This is a chained hook: multiple plugins may be registered." ], "request": { diff --git a/doc/schemas/hook/openchannel.json b/doc/schemas/hook/openchannel.json index 51d368500754..0c00623cc50e 100644 --- a/doc/schemas/hook/openchannel.json +++ b/doc/schemas/hook/openchannel.json @@ -61,7 +61,7 @@ "dust_limit_msat": { "type": "msat", "description": [ - "Dust limit for outputs." + "Dust limit for outputs on the peer's commitment transaction. Opens where this is below 354 sat or above 5000 sat are rejected before the hook is called." ] }, "max_htlc_value_in_flight_msat": { diff --git a/doc/schemas/hook/openchannel2.json b/doc/schemas/hook/openchannel2.json index 51a10c6ae432..7fef750aa393 100644 --- a/doc/schemas/hook/openchannel2.json +++ b/doc/schemas/hook/openchannel2.json @@ -72,7 +72,7 @@ "dust_limit_msat": { "type": "msat", "description": [ - "Minimum output value below which outputs are considered dust." + "Minimum output value below which outputs are considered dust on the peer's commitment transaction. Opens where this is below 354 sat or above 5000 sat are rejected before the hook is called." ] }, "max_htlc_value_in_flight_msat": { diff --git a/doc/schemas/offer.json b/doc/schemas/offer.json index 1316acea476d..9c37bb901b39 100644 --- a/doc/schemas/offer.json +++ b/doc/schemas/offer.json @@ -60,26 +60,30 @@ "recurrence": { "type": "string", "description": [ - "An invoice is expected at regular intervals. The argument is a positive number followed by one of `seconds`, `minutes`, `hours`, `days`, `weeks`, or `months` (variants without the trailing `s` are also permitted). This is encoded in the offer. The semantics of recurrence is fairly predictable, but fully documented in BOLT 12. e.g. `4weeks`." + "An invoice is expected at regular intervals. The argument is a positive number followed by one of `seconds`, `minutes`, `hours`, `days`, `weeks`, or `months` (variants without the trailing `s` are also permitted). This is encoded in the offer. The semantics of recurrence is fairly predictable, but fully documented in BOLT 12. e.g. `4weeks`.", + "Periods greater than 10 years are not accepted." ] }, "recurrence_base": { "type": "integer", "description": [ - "Time in seconds since the first day of 1970 UTC. This indicates when the first period begins; without this, the recurrence periods start from the first invoice." + "Time in seconds since the first day of 1970 UTC. This indicates when the first period begins; without this, the recurrence periods start from the first invoice.", + "This number must correspond to a time not beyond 9999-12-31 23:59:59 UTC." ] }, "recurrence_paywindow": { "type": "string", "description": [ - "Argument of form `-time+time`. The first time is the number of seconds before the start of a period in which an invoice and payment is valid, the second time is the number of seconds after the start of the period. For example *-604800+86400* means you can fetch an pay the invoice 4 weeks before the given period starts, and up to 1 day afterwards. The optional *%* indicates that the amount of the invoice will be scaled by the time remaining in the period. This is encoded in the offer." + "Argument of form `-time+time`. The first time is the number of seconds before the start of a period in which an invoice and payment is valid, the second time is the number of seconds after the start of the period. For example *-604800+86400* means you can fetch an pay the invoice 4 weeks before the given period starts, and up to 1 day afterwards. The optional *%* indicates that the amount of the invoice will be scaled by the time remaining in the period. This is encoded in the offer.", + "Either side of the pay window must not be greater than 1 year." ], "default": "that payment is allowed during the current and previous periods" }, "recurrence_limit": { "type": "u32", "description": [ - "To indicate the maximum period which exists. eg. `12` means there are 13 periods, from 0 to 12 inclusive. This is encoded in the offer." + "To indicate the maximum period which exists. eg. `12` means there are 13 periods, from 0 to 12 inclusive. This is encoded in the offer.", + "The limit cannot be 0 nor greater than 1000000000." ] }, "single_use": { diff --git a/gossipd/gossmap_manage.c b/gossipd/gossmap_manage.c index 3676fa9caaa6..feecde663517 100644 --- a/gossipd/gossmap_manage.c +++ b/gossipd/gossmap_manage.c @@ -28,6 +28,7 @@ #include #include #include +#include #define PENDING_LIMIT 10000 #define GOSSIP_STORE_COMPACT_FILENAME "gossip_store.compact" @@ -54,6 +55,7 @@ struct pending_cupdate { struct pending_nannounce { struct node_id node_id; + secp256k1_ecdsa_signature signature; u32 timestamp; const u8 *nannounce; const struct node_id *source_peer; @@ -117,6 +119,11 @@ struct gossmap_manage { /* Non-NULL if a compactd is running. */ struct compactd *compactd; + + /* Roughly how much of gossip_store is live: its length right after + * our last compaction, or at startup, its length minus deleted + * records. */ + u64 compacted_len; }; /* Timer recursion */ @@ -171,6 +178,7 @@ static void enqueue_cupdate(struct pending_cupdate ***queue, static void enqueue_nannounce(struct pending_nannounce ***queue, const struct node_id *node_id, + const secp256k1_ecdsa_signature *signature, u32 timestamp, const u8 *nannounce TAKES, const struct node_id *source_peer TAKES) @@ -193,6 +201,7 @@ static void enqueue_nannounce(struct pending_nannounce ***queue, pna = tal(*queue, struct pending_nannounce); pna->node_id = *node_id; + pna->signature = *signature; pna->timestamp = timestamp; pna->nannounce = tal_dup_talarr(pna, u8, nannounce); pna->source_peer = tal_dup_or_null(pna, struct node_id, source_peer); @@ -549,6 +558,7 @@ struct gossmap_manage *gossmap_manage_new(const tal_t *ctx, struct daemon *daemon) { struct gossmap_manage *gm = tal(ctx, struct gossmap_manage); + u64 deleted; if (!setup_gossmap(gm, daemon)) { tal_free(gm->dying_channels); @@ -561,6 +571,11 @@ struct gossmap_manage *gossmap_manage_new(const tal_t *ctx, assert(gm->raw_gossmap); gm->daemon = daemon; gm->compactd = NULL; + deleted = gossmap_deleted_bytes_read(gm->raw_gossmap); + if (deleted < gossip_store_len_written(gm->gs)) + gm->compacted_len = gossip_store_len_written(gm->gs) - deleted; + else + gm->compacted_len = 0; map_init(&gm->pending_ann_map, "pending announcements"); gm->pending_cupdates = tal_arr(gm, struct pending_cupdate *, 0); @@ -641,6 +656,14 @@ const char *gossmap_manage_channel_announcement(const tal_t *ctx, if (taken(announce)) tal_steal(tmpctx, announce); + /* Ignore messages larger than any valid one. */ + if (is_oversized_gossip_msg(announce)) { + status_debug("Ignoring oversized channel_announcement (%zu bytes)", + tal_bytelen(announce)); + tal_free_if_taken(source_peer); + return NULL; + } + if (!fromwire_channel_announcement(tmpctx, announce, &node_signature_1, &node_signature_2, &bitcoin_signature_1, &bitcoin_signature_2, &features, &chain_hash, &scid, &node_id_1, &node_id_2, &bitcoin_key_1, &bitcoin_key_2)) { @@ -688,6 +711,16 @@ const char *gossmap_manage_channel_announcement(const tal_t *ctx, || map_get(&gm->early_ann_map, scid)) return NULL; + /* dont expect to be more than 12 blocks behind! */ + if (!known_amount + && blockheight != 0 + && short_channel_id_blocknum(scid) > blockheight + 12) { + return tal_fmt(ctx, + "Bad gossip order: ignoring channel_announcement %s at blockheight %u", + fmt_short_channel_id(tmpctx, scid), + blockheight); + } + warn = sigcheck_channel_announcement(ctx, &node_id_1, &node_id_2, &bitcoin_key_1, &bitcoin_key_2, &node_signature_1, &node_signature_2, @@ -722,15 +755,6 @@ const char *gossmap_manage_channel_announcement(const tal_t *ctx, /* Don't know blockheight yet, or not yet deep enough? Don't even ask */ if (!is_scid_depth_announceable(scid, blockheight)) { - /* Don't expect to be more than 12 blocks behind! */ - if (blockheight != 0 - && short_channel_id_blocknum(scid) > blockheight + 12) { - return tal_fmt(ctx, - "Bad gossip order: ignoring channel_announcement %s at blockheight %u", - fmt_short_channel_id(tmpctx, scid), - blockheight); - } - if (!map_add(&gm->early_ann_map, scid, pca)) { /* Already pending? Ignore */ tal_free(pca); @@ -920,6 +944,29 @@ static const char *process_channel_update(const tal_t *ctx, return NULL; } + /* BOLT #7: + * - if the channel output has been spent: + * - MUST ignore `channel_update`s, unless they have the `disable` bit set to 1. + */ + if (gossmap_chan_is_dying(gossmap, chan) + && !(channel_flags & ROUTING_FLAGS_DISABLED)) { + status_peer_trace(source_peer, + "Ignoring enabled update for dying channel %s", + fmt_short_channel_id(tmpctx, scid)); + return NULL; + } + + if (gossmap_chan_set(chan, dir)) { + u32 prev_timestamp + = gossip_store_get_timestamp(gm->gs, chan->cupdate_off[dir]); + if (prev_timestamp >= timestamp) { + if (timestamp < prev_timestamp) + status_trace("Too-old update for %s", + fmt_short_channel_id(tmpctx, scid)); + return NULL; + } + } + /* Now we know node, we can check signature. */ gossmap_node_get_id(gossmap, gossmap_nth_node(gossmap, chan, dir), @@ -935,24 +982,10 @@ static const char *process_channel_update(const tal_t *ctx, fmt_short_channel_id(tmpctx, scid)); } - /* Do we have same or earlier update? */ - if (gossmap_chan_set(chan, dir)) { - u32 prev_timestamp - = gossip_store_get_timestamp(gm->gs, chan->cupdate_off[dir]); - if (prev_timestamp >= timestamp) { - /* Don't spam the logs for duplicates! */ - if (timestamp < prev_timestamp) - status_trace("Too-old update for %s", - fmt_short_channel_id(tmpctx, scid)); - /* Too old / redundant, ignore */ - return NULL; - } - } else { - /* Is this the first update in either direction? If so, - * rewrite channel_announcement so timestamp is correct. */ - if (!gossmap_chan_set(chan, !dir)) - gossip_store_set_timestamp(gm->gs, chan->cann_off, timestamp); - } + /* Is this the first update in either direction? If so, + * rewrite channel_announcement so timestamp is correct. */ + if (!gossmap_chan_set(chan, dir) && !gossmap_chan_set(chan, !dir)) + gossip_store_set_timestamp(gm->gs, chan->cann_off, timestamp); /* OK, apply the new one */ offset = gossip_store_add(gm->gs, update, timestamp); @@ -974,7 +1007,11 @@ static const char *process_channel_update(const tal_t *ctx, gossmap_nth_node(gossmap, chan, !dir), &remote_id); if (node_id_eq(&remote_id, &gm->daemon->id)) { - tell_lightningd_peer_update(gm->daemon, source_peer, + /* We can get public channel_updates from other than + * the direct peer, but we've checked the signature + * against the channel_announcement, so tell lightningd + * to trust us (NULL source). */ + tell_lightningd_peer_update(gm->daemon, NULL, scid, fee_base_msat, fee_proportional_millionths, cltv_expiry_delta, htlc_minimum_msat, @@ -1033,6 +1070,13 @@ const char *gossmap_manage_channel_update(const tal_t *ctx, if (taken(source_peer)) tal_steal(tmpctx, source_peer); + /* Ignore messages larger than any valid one. */ + if (is_oversized_gossip_msg(update)) { + status_debug("Ignoring oversized channel_update (%zu bytes)", + tal_bytelen(update)); + return NULL; + } + if (!fromwire_channel_update(update, &signature, &chain_hash, &scid, ×tamp, &message_flags, @@ -1123,15 +1167,19 @@ const char *gossmap_manage_channel_update(const tal_t *ctx, timestamp, update, source_peer); } -static void process_node_announcement(struct gossmap_manage *gm, - struct gossmap *gossmap, - const struct gossmap_node *node, - u32 timestamp, - const struct node_id *node_id, - const u8 *nannounce, - const struct node_id *source_peer) +/* This is called both from when we receive the node announcement, and if we had to defer. */ +static const char *process_node_announcement(const tal_t *ctx, + struct gossmap_manage *gm, + struct gossmap *gossmap, + const struct gossmap_node *node, + const secp256k1_ecdsa_signature *signature, + u32 timestamp, + const struct node_id *node_id, + const u8 *nannounce, + const struct node_id *source_peer) { u64 offset; + const char *err; /* Do we have a later one? If so, ignore */ if (gossmap_node_announced(node)) { @@ -1139,10 +1187,15 @@ static void process_node_announcement(struct gossmap_manage *gm, = gossip_store_get_timestamp(gm->gs, node->nann_off); if (prev_timestamp >= timestamp) { /* Too old, ignore */ - return; + return NULL; } } + /* only now do the (expensive) signature check */ + err = sigcheck_node_announcement(ctx, node_id, signature, nannounce); + if (err) + return err; + /* OK, apply the new one */ offset = gossip_store_add(gm->gs, nannounce, timestamp); /* If all channels are dying, make sure this is marked too. */ @@ -1162,6 +1215,7 @@ static void process_node_announcement(struct gossmap_manage *gm, status_peer_trace(source_peer, "Received node_announcement for node %s", fmt_node_id(tmpctx, node_id)); + return NULL; } const char *gossmap_manage_node_announcement(const tal_t *ctx, @@ -1178,7 +1232,6 @@ const char *gossmap_manage_node_announcement(const tal_t *ctx, struct wireaddr *wireaddrs; struct tlv_node_ann_tlvs *na_tlv; struct gossmap_node *node; - const char *err; struct gossmap *gossmap = gossmap_manage_get_gossmap(gm); if (taken(nannounce)) @@ -1187,6 +1240,13 @@ const char *gossmap_manage_node_announcement(const tal_t *ctx, if (taken(source_peer)) tal_steal(tmpctx, source_peer); + /* Ignore messages larger than any valid one. */ + if (is_oversized_gossip_msg(nannounce)) { + status_debug("Ignoring oversized node_announcement (%zu bytes)", + tal_bytelen(nannounce)); + return NULL; + } + if (!fromwire_node_announcement(tmpctx, nannounce, &signature, &features, ×tamp, &node_id, rgb_color, alias, @@ -1218,11 +1278,7 @@ const char *gossmap_manage_node_announcement(const tal_t *ctx, tal_hex(tmpctx, nannounce)); } - err = sigcheck_node_announcement(ctx, &node_id, &signature, - nannounce); - if (err) - return err; - + /* We check the signature in process_node_announcement, after cheaper checks for unknown node and stale timestamp. */ node = gossmap_find_node(gossmap, &node_id); if (!node) { /* Still waiting for some channel_announcement? */ @@ -1230,6 +1286,7 @@ const char *gossmap_manage_node_announcement(const tal_t *ctx, || !map_empty(&gm->early_ann_map)) { enqueue_nannounce(&gm->pending_nannounces, &node_id, + &signature, timestamp, take(nannounce), source_peer); @@ -1247,8 +1304,9 @@ const char *gossmap_manage_node_announcement(const tal_t *ctx, return NULL; } - process_node_announcement(gm, gossmap, node, timestamp, &node_id, nannounce, source_peer); - return NULL; + return process_node_announcement(ctx, gm, gossmap, node, &signature, + timestamp, &node_id, nannounce, + source_peer); } static void process_pending_cupdate(struct gossmap_manage *gm, @@ -1340,6 +1398,7 @@ static void reprocess_queued_msgs(struct gossmap_manage *gm) for (size_t i = 0; i < tal_count(pnas); i++) { struct gossmap_node *node; struct gossmap *gossmap = gossmap_manage_get_gossmap(gm); + const char *err; node = gossmap_find_node(gossmap, &pnas[i]->node_id); if (!node) { @@ -1355,11 +1414,15 @@ static void reprocess_queued_msgs(struct gossmap_manage *gm) continue; } - process_node_announcement(gm, gossmap, node, - pnas[i]->timestamp, - &pnas[i]->node_id, - pnas[i]->nannounce, - pnas[i]->source_peer); + err = process_node_announcement(tmpctx, gm, gossmap, node, + &pnas[i]->signature, + pnas[i]->timestamp, + &pnas[i]->node_id, + pnas[i]->nannounce, + pnas[i]->source_peer); + if (err) + peer_warning(gm, pnas[i]->source_peer, + "node_announcement: %s", err); } /* Won't add any new ones */ @@ -1724,6 +1787,7 @@ static void compactd_done(struct io_conn *unused, struct gossmap_manage *gm) status_failed(STATUS_FAIL_INTERNAL_ERROR, "Error renaming gossip store: %s", strerror(errno)); + gm->compacted_len = st.st_size; /* Now append record to old one, so everyone will switch */ gossip_store_add(gm->gs, @@ -1733,8 +1797,12 @@ static void compactd_done(struct io_conn *unused, struct gossmap_manage *gm) if (gm->compactd->dev_compact) daemon_conn_send(gm->daemon->master, take(towire_gossipd_dev_compact_store_reply(NULL, ""))); + gm->compactd = tal_free(gm->compactd); + return; failed: + /* Don't retry on every message: wait until it grows 5x again. */ + gm->compacted_len = gossip_store_len_written(gm->gs); gm->compactd = tal_free(gm->compactd); } @@ -1867,8 +1935,12 @@ void gossmap_manage_maybe_compact(struct gossmap_manage *gm) if (gossip_store_len_written(gm->gs) < 10000000) return; - /* Compact when the density would be 5x better */ - if (num_dead < 4 * num_live) + /* Compact when the density would be 5x better. Records vary in size + * (a node_announcement can be ten times a channel_update), so counting + * records alone lets large dead records pile up: also compact once the + * store is 5x its (roughly live) size after the last compaction. */ + if (num_dead < 4 * num_live + && gossip_store_len_written(gm->gs) < 5 * gm->compacted_len) return; compact_started = gossmap_compact(gm, false); diff --git a/gossipd/queries.c b/gossipd/queries.c index e106f47dfef6..72a0c8f22691 100644 --- a/gossipd/queries.c +++ b/gossipd/queries.c @@ -7,6 +7,8 @@ #include #include +#define MAX_RANGE_REPLIES 1000000 + /* BOLT #7: * * There are several messages which contain a long array of @@ -144,7 +146,7 @@ static u8 *append_range_reply(struct peer *peer, const struct tlv_reply_channel_range_tlvs_timestamps_tlv *timestamps_tlv) { - u16 i, old_num, added; + size_t old_num, added; const struct channel_update_timestamps *ts; /* Zero means "no timestamp" */ const static struct channel_update_timestamps zero_ts = { 0, 0 }; @@ -166,8 +168,8 @@ static u8 *append_range_reply(struct peer *peer, old_num = tal_count(peer->range_replies); added = tal_count(scids); - for (i = 0; i < added; i++) { - tal_resize(&peer->range_replies, old_num + i + 1); + tal_resize(&peer->range_replies, old_num + added); + for (size_t i = 0; i < added; i++) { peer->range_replies[old_num + i].scid = scids[i]; if (ts) peer->range_replies[old_num + i].ts = ts[i]; @@ -228,6 +230,14 @@ const u8 *handle_reply_channel_range(struct peer *peer, const u8 *msg) tal_hex(tmpctx, encoded)); } + if (tal_count(peer->range_replies) + tal_count(scids) + > MAX_RANGE_REPLIES) { + peer->range_replies = tal_free(peer->range_replies); + peer->query_channel_range_cb = NULL; + return towire_warningfmt(peer, NULL, + "reply_channel_range too many scids"); + } + status_peer_debug(&peer->id, "reply_channel_range %u+%u (of %u+%u) %zu scids", first_blocknum, number_of_blocks, diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index 2dfe79f0d297..9e48a362a4db 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -551,8 +551,9 @@ static void hsm_key_for_utxo(struct privkey *privkey, struct pubkey *pubkey, } /* Find our inputs by the pubkey associated with the inputs, and - * add a partial sig for each */ -static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) + * add a partial sig for each. Returns NULL, or why libwally refused. */ +static const char *sign_our_inputs(struct hsm_utxo **utxos, + struct wally_psbt *psbt) { bool is_cache_enabled = false; for (size_t i = 0; i < tal_count(utxos); i++) { @@ -608,11 +609,11 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) tal_wally_end(psbt); /* Converting to v0 for log consumption */ psbt_set_version(psbt, 0); - hsmd_status_failed(STATUS_FAIL_INTERNAL_ERROR, - "Received wally_err attempting to " - "sign input %zu with key %s. PSBT: %s", - j, fmt_pubkey(tmpctx, &pubkey), - fmt_wally_psbt(tmpctx, psbt)); + return tal_fmt(tmpctx, + "Received wally_err attempting to " + "sign input %zu with key %s. PSBT: %s", + j, fmt_pubkey(tmpctx, &pubkey), + fmt_wally_psbt(tmpctx, psbt)); } if (dev_warn_on_overgrind && needed_sig @@ -625,6 +626,7 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) tal_wally_end(psbt); } } + return NULL; } static void check_overgrind(const struct bitcoin_signature *sig) @@ -1397,13 +1399,20 @@ static u8 *handle_get_per_commitment_point(struct hsmd_client *c, const u8 *msg_ static u8 *handle_sign_withdrawal_tx(struct hsmd_client *c, const u8 *msg_in) { struct hsm_utxo **utxos; - struct wally_psbt *psbt; + struct wally_psbt *psbt, *unsigned_psbt; + const char *err; if (!fromwire_hsmd_sign_withdrawal(tmpctx, msg_in, &utxos, &psbt)) return hsmd_status_malformed_request(c, msg_in); - sign_our_inputs(utxos, psbt); + /* From signpsbt, so not our bug: return it unsigned rather than die. */ + unsigned_psbt = clone_psbt(tmpctx, psbt); + err = sign_our_inputs(utxos, psbt); + if (err) { + hsmd_status_fmt(LOG_UNUSUAL, NULL, "Not signing PSBT: %s", err); + psbt = unsigned_psbt; + } return towire_hsmd_sign_withdrawal_reply(NULL, psbt); } @@ -1535,23 +1544,21 @@ static u8 *do_sign_local_htlc_tx(struct hsmd_client *c, return hsmd_status_bad_request_fmt(c, msg_in, "bad pubkey_from_privkey"); - if (tx->wtx->num_inputs != 1) + /* With anchors, lightningd adds our wallet inputs to pay the fee */ + if (!option_anchor_outputs && tx->wtx->num_inputs != 1) return hsmd_status_bad_request_fmt(c, msg_in, "bad txinput count"); /* FIXME: Check that output script is correct! */ - /* BOLT #3: - * ## HTLC-Timeout and HTLC-Success Transactions - *... - * * if `option_anchors` applies to this commitment transaction, - * `SIGHASH_SINGLE|SIGHASH_ANYONECANPAY` is used as described in [BOLT #5] + /* BOLT #5: + * Otherwise, `SIGHASH_SINGLE|SIGHASH_ANYONECANPAY` MUST be used on the + * HTLC signatures received from the peer, as this allows HTLC transactions to be combined with + * other transactions. The local signature MUST use `SIGHASH_ALL`, otherwise + * anyone can attach additional inputs and outputs to the tx */ sign_tx_input(tx, input_num, NULL, wscript, &htlc_privkey, &htlc_pubkey, - option_anchor_outputs - ? (SIGHASH_SINGLE|SIGHASH_ANYONECANPAY) - : SIGHASH_ALL, - &sig); + SIGHASH_ALL, &sig); check_overgrind(&sig); return towire_hsmd_sign_tx_reply(NULL, &sig); @@ -1778,6 +1785,7 @@ static u8 *handle_sign_anchorspend(struct hsmd_client *c, const u8 *msg_in) struct secret seed; struct pubkey local_funding_pubkey; struct secrets secrets; + const char *err; int ret; /* FIXME: Check output goes to us. */ @@ -1786,7 +1794,9 @@ static u8 *handle_sign_anchorspend(struct hsmd_client *c, const u8 *msg_in) return hsmd_status_malformed_request(c, msg_in); /* Sign all the UTXOs */ - sign_our_inputs(utxos, psbt); + err = sign_our_inputs(utxos, psbt); + if (err) + hsmd_status_failed(STATUS_FAIL_INTERNAL_ERROR, "%s", err); get_channel_seed(&peer_id, dbid, &seed); derive_basepoints(&seed, &local_funding_pubkey, NULL, &secrets, NULL); @@ -1825,15 +1835,18 @@ static u8 *handle_sign_htlc_tx_mingle(struct hsmd_client *c, const u8 *msg_in) u64 dbid; struct hsm_utxo **utxos; struct wally_psbt *psbt; + const char *err; /* FIXME: Check output goes to us. */ if (!fromwire_hsmd_sign_htlc_tx_mingle(tmpctx, msg_in, &peer_id, &dbid, &utxos, &psbt)) return hsmd_status_malformed_request(c, msg_in); - /* Sign all the UTXOs (htlc_inout input is already signed with - * SIGHASH_SINGLE|SIGHASH_ANYONECANPAY) */ - sign_our_inputs(utxos, psbt); + /* Sign all the UTXOs (lightningd gets the htlc input signed + * separately, using hsmd_sign_any_local_htlc_tx) */ + err = sign_our_inputs(utxos, psbt); + if (err) + hsmd_status_failed(STATUS_FAIL_INTERNAL_ERROR, "%s", err); return towire_hsmd_sign_htlc_tx_mingle_reply(NULL, psbt); } @@ -2010,8 +2023,7 @@ static u8 *do_sign_remote_htlc_to_us(struct hsmd_client *c, u64 channel_dbid, const struct pubkey *remote_per_commitment_point, struct bitcoin_tx *tx, - const u8 *wscript, - bool option_anchor_outputs) + const u8 *wscript) { struct secret channel_seed, htlc_basepoint_secret; struct pubkey htlc_basepoint; @@ -2032,16 +2044,10 @@ static u8 *do_sign_remote_htlc_to_us(struct hsmd_client *c, return hsmd_status_bad_request(c, msg_in, "Failed deriving htlc privkey"); - /* BOLT #3: - * ## HTLC-Timeout and HTLC-Success Transactions - *... - * * if `option_anchors` applies to this commitment transaction, - * `SIGHASH_SINGLE|SIGHASH_ANYONECANPAY` is used as described in [BOLT #5] - */ - return handle_sign_to_us_tx( - c, msg_in, input_num, tx, &privkey, wscript, - option_anchor_outputs ? (SIGHASH_SINGLE | SIGHASH_ANYONECANPAY) - : SIGHASH_ALL); + /* This is a direct sweep, not an HTLC tx, and we're the only + * signer, so we use SIGHASH_ALL */ + return handle_sign_to_us_tx(c, msg_in, input_num, tx, &privkey, wscript, + SIGHASH_ALL); } /*~ When called by onchaind */ @@ -2060,8 +2066,7 @@ static u8 *handle_sign_remote_htlc_to_us(struct hsmd_client *c, return do_sign_remote_htlc_to_us(c, msg_in, 0, &c->id, c->dbid, &remote_per_commitment_point, - tx, wscript, - option_anchor_outputs); + tx, wscript); } /*~ When called by lightningd */ @@ -2083,8 +2088,7 @@ static u8 *handle_sign_any_remote_htlc_to_us(struct hsmd_client *c, return do_sign_remote_htlc_to_us(c, msg_in, input_num, &peer_id, dbid, &remote_per_commitment_point, - tx, wscript, - option_anchor_outputs); + tx, wscript); } /*~ When we send a commitment transaction onchain (unilateral close), there's diff --git a/lightningd/channel.c b/lightningd/channel.c index dfe48ee45583..0ffafcd676df 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -1212,6 +1212,17 @@ void channel_fail_forget(struct channel *channel, const char *fmt, ...) why = tal_vfmt(tmpctx, fmt, ap); va_end(ap); + /* A peer can only make this channel forgettable before commitment + * updates are possible. Keep the channel if an older or buggy + * channeld nevertheless attached an HTLC to it. */ + if (channel_has_htlc_out(channel) || channel_has_htlc_in(channel)) { + channel_fail_permanent(channel, REASON_PROTOCOL, + "Cannot forget channel with HTLCs: %s", + why); + tal_free(why); + return; + } + log_unusual(channel->log, "Peer permanent failure in %s: %s, " "forget channel", channel_state_name(channel), why); diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 246d827c5037..35b6d22c53e4 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -1816,12 +1816,10 @@ bool peer_start_channeld(struct channel *channel, hsmfd = hsm_get_client_fd(ld, &channel->peer->id, channel->dbid, - HSM_PERM_SIGN_GOSSIP - | HSM_PERM_ECDH + HSM_PERM_ECDH | HSM_PERM_COMMITMENT_POINT | HSM_PERM_SIGN_REMOTE_TX | HSM_PERM_SIGN_ONCHAIN_TX - | HSM_PERM_SIGN_CLOSING_TX | HSM_PERM_SIGN_SPLICE_TX | HSM_PERM_LOCK_OUTPOINT); if (hsmfd < 0) { @@ -2122,6 +2120,12 @@ is_fundee_should_forget(struct lightningd *ld, if (!amount_msat_is_zero(channel->msat_to_us_max)) return false; + /* A channel with committed HTLCs is not safe to forget, even if a + * buggy peer managed to add them before the funding transaction was + * confirmed. */ + if (channel_has_htlc_out(channel) || channel_has_htlc_in(channel)) + return false; + /* Ah forget it! */ return true; } @@ -2397,6 +2401,7 @@ static struct command_result *json_splice_init(struct command *cmd, struct wally_psbt *initialpsbt; s64 *relative_amount; u32 *feerate_per_kw; + u32 last_feerate, next_feerate_min; bool *force_feerate, *skip_stfu; u8 *msg; @@ -2416,13 +2421,53 @@ static struct command_result *json_splice_init(struct command *cmd, "Currently waiting on previous splice" " command to finish."); + /* With a splice pending, channeld sends this as tx_init_rbf. + * + * BOLT #2: + * The sender: + * - MUST set `feerate` greater than or equal to 25/24 times the + * `feerate` of the previously constructed transaction, rounded + * down. + */ + last_feerate = channel_last_funding_feerate(channel); + if (!last_feerate) + next_feerate_min = 0; + else if (!next_funding_feerate(last_feerate, &next_feerate_min)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Can't calculate the next feerate: the" + " pending splice's feerate (%u) is out" + " of range", + last_feerate); + + if (feerate_per_kw && *feerate_per_kw < next_feerate_min) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Next feerate must be at least 1/24th" + " greater than the pending splice's. Min" + " req %u, you proposed %u", + next_feerate_min, *feerate_per_kw); + if (command_check_only(cmd)) return command_check_done(cmd); if (!feerate_per_kw) { feerate_per_kw = tal(cmd, u32); *feerate_per_kw = splice_feerate(cmd->ld->topology, cmd->ld); - } + if (*feerate_per_kw < next_feerate_min) + *feerate_per_kw = next_feerate_min; + } + + /* channeld would refuse this too, but only after marking a splice in + * progress, which then blocks the next splice_init. */ + if (!*force_feerate + && !channel->ignore_fee_limits + && !cmd->ld->config.ignore_fee_limits + && *feerate_per_kw > our_feerate_max(cmd->ld, NULL)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Feerate %u is above the most we'll pay" + " (%u); the pending splice is at %u", + *feerate_per_kw, + our_feerate_max(cmd->ld, NULL), + last_feerate); if (!initialpsbt) initialpsbt = create_psbt(cmd, 0, 0, 0); diff --git a/lightningd/channel_gossip.c b/lightningd/channel_gossip.c index 8eea0cd9bd83..ef148942e5be 100644 --- a/lightningd/channel_gossip.c +++ b/lightningd/channel_gossip.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -191,6 +192,11 @@ struct channel_gossip { /* Details of latest channel_update sent by peer */ const struct peer_update *peer_update; + /* The peer can send updates as fast as it likes, so once we've + * saved one, we only save the latest when this timer expires. */ + struct oneshot *peer_update_timer; + bool peer_update_unsaved; + /* To avoid a storm, we only respond to announcement_signatures with * our own signatures once */ bool sent_sigs; @@ -940,6 +946,8 @@ void channel_gossip_init(struct channel *channel, cg->cupdate = NULL; cg->refresh_timer = NULL; cg->peer_update = tal_dup_or_null(channel, struct peer_update, remote_update); + cg->peer_update_timer = NULL; + cg->peer_update_unsaved = false; cg->remote_sigs = NULL; cg->sent_sigs = false; @@ -1337,8 +1345,45 @@ static struct channel *lookup_by_peer_remote_alias(struct lightningd *ld, return NULL; } +static bool peer_update_eq(const struct peer_update *a, + const struct peer_update *b) +{ + return short_channel_id_eq(a->scid, b->scid) + && a->fee_base == b->fee_base + && a->fee_ppm == b->fee_ppm + && a->cltv_delta == b->cltv_delta + && amount_msat_eq(a->htlc_minimum_msat, b->htlc_minimum_msat) + && amount_msat_eq(a->htlc_maximum_msat, b->htlc_maximum_msat); +} + +static void save_peer_update(struct channel *channel); + +static void peer_update_timer_expired(struct channel *channel) +{ + struct channel_gossip *cg = channel->channel_gossip; + + cg->peer_update_timer = NULL; + if (cg->peer_update_unsaved) + save_peer_update(channel); +} + +static void save_peer_update(struct channel *channel) +{ + struct lightningd *ld = channel->peer->ld; + struct channel_gossip *cg = channel->channel_gossip; + + log_debug(channel->log, "Saving peer's channel_update"); + wallet_channel_save(ld->wallet, channel); + cg->peer_update_unsaved = false; + cg->peer_update_timer + = new_reltimer(ld->timers, cg, + time_from_sec(PEER_UPDATE_SAVE_INTERVAL(ld->dev_fast_gossip)), + peer_update_timer_expired, channel); +} + /* A peer sent gossipd an update_channel message for one of our channels. - * Gossipd checked the signature. + * Gossipd checked the signature: against the channel_announcement if + * source is NULL, otherwise only that source signed it. */ void channel_gossip_set_remote_update(struct lightningd *ld, const struct peer_update *update TAKES, @@ -1368,12 +1413,11 @@ void channel_gossip_set_remote_update(struct lightningd *ld, return; } - /* For public channels, it could come from anywhere. Private - * channels must come from gossipd itself (the old store - * migration!) or the correct peer. */ - if (is_private(channel) - && source - && !node_id_eq(source, &channel->peer->id)) { + /* gossipd doesn't know about our unannounced channels (even + * public ones which aren't announced yet), so unless it checked + * the update against the channel_announcement (NULL source), it + * must come from the correct peer. */ + if (source && !node_id_eq(source, &channel->peer->id)) { log_unusual(ld->log, "Bad gossip order: %s sent us a channel update for a " "channel owned by %s (%s)", fmt_node_id(tmpctx, source), @@ -1382,11 +1426,20 @@ void channel_gossip_set_remote_update(struct lightningd *ld, return; } + /* Nothing changed? Don't let them spam us with db writes. */ + if (cg->peer_update && peer_update_eq(cg->peer_update, update)) + return; + log_debug(ld->log, "updating channel %s with inbound settings", fmt_short_channel_id(tmpctx, update->scid)); tal_free(cg->peer_update); cg->peer_update = tal_dup(cg, struct peer_update, update); - wallet_channel_save(ld->wallet, channel); + + /* Saved one recently? Timer will save this one. */ + if (cg->peer_update_timer) + cg->peer_update_unsaved = true; + else + save_peer_update(channel); } const struct peer_update *channel_gossip_get_remote_update(const struct channel *channel) @@ -1398,6 +1451,28 @@ const struct peer_update *channel_gossip_get_remote_update(const struct channel return cg->peer_update; } +void channel_gossip_shutdown(struct lightningd *ld) +{ + struct peer *peer; + struct peer_node_id_map_iter it; + + db_begin_transaction(ld->wallet->db); + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { + struct channel *channel; + list_for_each(&peer->channels, channel, list) { + struct channel_gossip *cg = channel->channel_gossip; + + if (!cg || !cg->peer_update_unsaved) + continue; + wallet_channel_save(ld->wallet, channel); + cg->peer_update_unsaved = false; + } + } + db_commit_transaction(ld->wallet->db); +} + static bool has_announced_channels(struct lightningd *ld) { struct peer *peer; diff --git a/lightningd/channel_gossip.h b/lightningd/channel_gossip.h index b6fae02a204e..d8e232744f99 100644 --- a/lightningd/channel_gossip.h +++ b/lightningd/channel_gossip.h @@ -61,4 +61,9 @@ const u8 *channel_gossip_update_for_error(const tal_t *ctx, /* Get the peer's last-sent channel_update info, if any. */ const struct peer_update *channel_gossip_get_remote_update(const struct channel *channel); +/* We're shutting down: save anything we were going to save later. + * Call before shutdown_plugins(), so db_write plugins see it. Any + * update after this is lost, but the peer sends it again on reconnect. */ +void channel_gossip_shutdown(struct lightningd *ld); + #endif /* LIGHTNING_LIGHTNINGD_CHANNEL_GOSSIP_H */ diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 64987c14f2a3..dd7ec391c318 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -625,6 +625,12 @@ static struct command_result *param_feerate_range(struct command *cmd, if (ret) return ret; (*feerate_range)[1] = *rate; + + /* closingd would silently lower min to max, but we check the + * peer's closing tx against min: don't let them disagree. */ + if ((*feerate_range)[0] > (*feerate_range)[1]) + return command_fail_badparam(cmd, name, buffer, tok, + "minimum feerate must not exceed maximum"); return NULL; } @@ -642,12 +648,14 @@ static bool channel_state_can_close(enum channel_state state) case CHANNELD_AWAITING_SPLICE: case CHANNELD_AWAITING_LOCKIN: case DUALOPEND_AWAITING_LOCKIN: - case DUALOPEND_OPEN_INIT: case DUALOPEND_OPEN_COMMIT_READY: case DUALOPEND_OPEN_COMMITTED: case CLOSINGD_SIGEXCHANGE: case CHANNELD_SHUTTING_DOWN: return true; + /* Not in the db yet, so not a real channel: param_channel_or_peer + * hands it back as unsaved_channel instead. */ + case DUALOPEND_OPEN_INIT: case CLOSINGD_COMPLETE: case AWAITING_UNILATERAL: case FUNDING_SPEND_SEEN: diff --git a/lightningd/connect_control.c b/lightningd/connect_control.c index 99e417118a1b..aeba212c2ad3 100644 --- a/lightningd/connect_control.c +++ b/lightningd/connect_control.c @@ -187,9 +187,11 @@ static struct command_result *json_connect(struct command *cmd, if (id_addr.host) { u16 port = id_addr.port ? *id_addr.port : chainparams_get_ln_port(chainparams); addr = tal(cmd, struct wireaddr_internal); + /* Never resolve names here: getaddrinfo() would block + * lightningd's main loop for as long as the nameserver takes. + * Names come back unresolved, and connectd deals with them. */ err_msg = parse_wireaddr_internal(tmpctx, id_addr.host, port, - !cmd->ld->always_use_proxy - && !cmd->ld->pure_tor_setup, addr); + false, addr); if (err_msg) { return command_fail(cmd, LIGHTNINGD, "Host %s:%u not valid: %s", @@ -199,8 +201,26 @@ static struct command_result *json_connect(struct command *cmd, switch (addr->itype) { case ADDR_INTERNAL_SOCKNAME: case ADDR_INTERNAL_WIREADDR: - /* Can happen if we're disable DNS */ + break; case ADDR_INTERNAL_FORPROXY: + /* With a proxy, it resolves the name for us. */ + if (cmd->ld->always_use_proxy || cmd->ld->pure_tor_setup) + break; + /* Otherwise connectd resolves it off its io loop, + * just like a DNS address from a node_announcement + * (and honours --disable-dns). */ + err_msg = parse_wireaddr(tmpctx, + tal_fmt(tmpctx, "dns:%s", + addr->u.unresolved.name), + addr->u.unresolved.port, NULL, + &addr->u.wireaddr.wireaddr); + if (err_msg) { + return command_fail(cmd, LIGHTNINGD, + "Host %s:%u not valid: %s", + id_addr.host, port, err_msg); + } + addr->itype = ADDR_INTERNAL_WIREADDR; + addr->u.wireaddr.is_websocket = false; break; case ADDR_INTERNAL_ALLPROTO: case ADDR_INTERNAL_AUTOTOR: diff --git a/lightningd/dual_open_control.c b/lightningd/dual_open_control.c index 7455de3b6268..e273c25280e0 100644 --- a/lightningd/dual_open_control.c +++ b/lightningd/dual_open_control.c @@ -12,8 +12,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -509,6 +511,19 @@ static void rbf_channel_remove_dualopend(struct subd *dualopend, payload->dualopend = NULL; } +/* The channel itself is fine (its funding tx is signed and broadcast), so + * only the RBF attempt is dropped. dualopend only reads the peer while + * negotiating, so it can't act on the funding tx confirming until we + * disconnect to unblock it; channel_cleanup_commands fires + * channel_open_failed so funder cleans up its side of the attempt. */ +static void accepter_rbf_timeout(struct channel *channel) +{ + channel_disconnect(channel, LOG_INFORM, true, + tal_fmt(tmpctx, + "Peer did not complete RBF within %u seconds", + channel->peer->ld->dev_max_dualopen_negotiation_secs)); +} + static void rbf_channel_hook_cb(struct rbf_channel_payload *payload STEALS) { struct subd *dualopend = payload->dualopend; @@ -548,6 +563,11 @@ static void rbf_channel_hook_cb(struct rbf_channel_payload *payload STEALS) /* Update channel with new open attempt. */ channel->open_attempt = new_channel_open_attempt(channel); + /* Bound how long the peer can stall us, as for the initial open. + * Freed with open_attempt at commitment_signed. */ + notleak(new_reltimer(dualopend->ld->timers, channel->open_attempt, + time_from_sec(dualopend->ld->dev_max_dualopen_negotiation_secs), + accepter_rbf_timeout, channel)); msg = towire_dualopend_got_rbf_offer_reply(NULL, payload->our_funding, payload->psbt); @@ -653,6 +673,31 @@ static void openchannel2_remove_dualopend(struct subd *dualopend, payload->dualopend = NULL; } +static void forget_channel_open(struct channel *channel, const char *why); + +/* dualopend is blocked reading the peer and can't hear a dualopend_fail, + * so we tell the peer and kill it. channel_open_failed is what makes + * funder unreserve. */ +static void accepter_negotiation_timeout(struct channel *channel) +{ + struct lightningd *ld = channel->peer->ld; + const char *why = tal_fmt(tmpctx, + "Peer did not complete channel open" + " within %u seconds", + ld->dev_max_dualopen_negotiation_secs); + + subd_send_msg(ld->connectd, + take(towire_connectd_peer_send_msg(NULL, + &channel->peer->id, + channel->peer->connectd_counter, + towire_errorfmt(tmpctx, + &channel->cid, + "%s", why)))); + channel_cleanup_commands(channel, why); + channel_set_owner(channel, NULL); + forget_channel_open(channel, why); +} + static void openchannel2_hook_cb(struct openchannel2_payload *payload STEALS) { @@ -704,6 +749,11 @@ openchannel2_hook_cb(struct openchannel2_payload *payload STEALS) channel->cid = payload->channel_id; channel->opener = REMOTE; channel->open_attempt = new_channel_open_attempt(channel); + /* Bound how long the peer can hold what our hook reserved. + * Freed with open_attempt at commitment_signed. */ + notleak(new_reltimer(dualopend->ld->timers, channel->open_attempt, + time_from_sec(dualopend->ld->dev_max_dualopen_negotiation_secs), + accepter_negotiation_timeout, channel)); channel->req_confirmed_ins[REMOTE] = payload->req_confirmed_ins_remote; msg = towire_dualopend_got_offer_reply(NULL, diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 97ab154c47b8..15691fe7e51c 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -1694,6 +1694,15 @@ static struct command_result *json_createinvoice(struct command *cmd, return command_param_failed(); sha256(&payment_hash, preimage, sizeof(*preimage)); + + /* invoices_create() only catches duplicate labels (reported via + * fail_exists() below), but payment_hash is UNIQUE in the db too. */ + if (!invoices_find_by_label(cmd->ld->wallet->invoices, &inv_dbid, label) + && invoices_find_by_rhash(cmd->ld->wallet->invoices, + &inv_dbid, &payment_hash)) + return command_fail(cmd, INVOICE_PREIMAGE_ALREADY_EXISTS, + "preimage already used"); + b11 = bolt11_decode_nosig(cmd, invstring, cmd->ld->our_features, NULL, chainparams, &hash, &sig, &have_n, &fail); diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index c9f61c4fa182..c373f4da44d7 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -319,7 +319,7 @@ static struct command_result *json_recover(struct command *cmd, move_prerecover_files(dir); - /* Top level with add --recover=... here */ + /* Top level hands this to our re-exec'd self */ cmd->ld->recover_secret = tal_steal(cmd->ld, hsm_secret); cmd->ld->try_reexec = true; return prepare_stop_conn(cmd, "Recovery restart in progress"); @@ -706,8 +706,12 @@ void json_stream_log_suppress_for_cmd(struct json_stream *js, const struct command *cmd) { const char *nm = cmd->json_cmd->name; - const char *s = tal_fmt(tmpctx, "Suppressing logging of %s command", nm); - log_io(cmd->jcon->log, LOG_IO_OUT, NULL, take(s), NULL, 0); + + /* Catch the case where they sent this command then hung up. */ + if (cmd->jcon) { + const char *s = tal_fmt(tmpctx, "Suppressing logging of %s command", nm); + log_io(cmd->jcon->log, LOG_IO_OUT, NULL, take(s), NULL, 0); + } /* Really shouldn't be used for anything else */ assert(streq(nm, "getlog")); @@ -1490,7 +1494,8 @@ static struct command_result *param_command(struct command *cmd, const jsmntok_t *tok, const jsmntok_t **out) { - cmd->json_cmd = find_cmd(cmd->jcon->ld->jsonrpc, buffer, tok); + /* Not cmd->jcon: they may have hung up while a hook held us. */ + cmd->json_cmd = find_cmd(cmd->ld->jsonrpc, buffer, tok); if (cmd->json_cmd) { *out = tok; return NULL; diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 52d9bb92c65e..e59888e44df9 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -77,6 +77,7 @@ #include #include #include +#include #include #include #include @@ -141,6 +142,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_no_htlc_timeout = false; ld->dev_no_version_checks = false; ld->dev_max_funding_unconfirmed = 2016; + ld->dev_max_dualopen_negotiation_secs = 600; ld->dev_low_prio_anchor_blocks = 2016; ld->dev_ignore_modern_onion = false; ld->dev_disable_commit = -1; @@ -1184,6 +1186,91 @@ static bool bwatch_enabled(const struct lightningd *ld) return configvar_first(ld->configvars, names) != NULL; } +/* Is argv[i] --name[=val]? Returns how many args it takes (0-2). */ +static size_t opt_args(char *argv[], size_t i, const char *name, char **val) +{ + size_t len = strlen(name); + + if (!strstarts(argv[i], name)) + return 0; + if (argv[i][len] == '=') { + *val = argv[i] + len + 1; + return 1; + } + if (argv[i][len] != '\0') + return 0; + /* Same guess as initial_config_opts() */ + if (argv[i+1] && !strstarts(argv[i+1], "-")) { + *val = argv[i+1]; + return 2; + } + *val = NULL; + return 1; +} + +/*~ We store a copy of our arguments before parsing mangles them, so we can + * re-exec if versions of subdaemons change. Minus --recover, which would + * only fail now: we stay offline instead. */ +static char **reexec_args(const tal_t *ctx, int argc, char *argv[]) +{ + char **args = tal_arr(ctx, char *, 0); + bool recovered = false, offline = false; + + /*~ Turn argv[0] into an absolute path (if not already) */ + tal_arr_expand(&args, path_join(args, take(path_cwd(NULL)), argv[0])); + for (size_t i = 1; i < argc; i++) { + char *val; + size_t n = opt_args(argv, i, "--recover", &val); + + if (!n) + n = opt_args(argv, i, "--recover-fd", &val); + if (n) { + recovered = true; + i += n - 1; + continue; + } + if (streq(argv[i], "--offline")) + offline = true; + tal_arr_expand(&args, tal_strdup(args, argv[i])); + } + if (recovered && !offline) + tal_arr_expand(&args, tal_strdup(args, "--offline")); + tal_arr_expand(&args, NULL); + return args; +} + +/* Anyone can read argv (e.g. ps), so wipe the secret once parsed. */ +static void scrub_recover_secret(char *argv[]) +{ + for (size_t i = 1; argv[i]; i++) { + char *val; + + if (opt_args(argv, i, "--recover", &val) && val) + sodium_memzero(val, strlen(val)); + } +} + +/* The recover command's secret goes to our re-exec'd self through a pipe + * on this fd, which is free after closefrom(). */ +#define RECOVER_FD (STDERR_FILENO + 1) + +static void pipe_recover_secret(const char *secret) +{ + int fds[2]; + + if (pipe(fds) != 0) + err(1, "Creating pipe for recover"); + /* Tiny, so it fits in the pipe */ + if (!write_all(fds[1], secret, strlen(secret))) + err(1, "Writing recover secret"); + close(fds[1]); + if (fds[0] != RECOVER_FD) { + if (dup2(fds[0], RECOVER_FD) != RECOVER_FD) + err(1, "Moving recover pipe"); + close(fds[0]); + } +} + int main(int argc, char *argv[]) { struct lightningd *ld; @@ -1195,7 +1282,8 @@ int main(int argc, char *argv[]) int sigchld_rfd; struct io_conn *sigchld_conn = NULL; int exit_code = 0; - char **orig_argv; + char **orig_argv, **args_to_scrub; + const char *recover_secret = NULL; bool try_reexec; size_t num_channels; @@ -1236,17 +1324,9 @@ int main(int argc, char *argv[]) ld->state = LD_STATE_INITIALIZING; log_info(ld->log, "%s", version()); - /*~ We store an copy of our arguments before parsing mangles them, so - * we can re-exec if versions of subdaemons change. Note the use of - * notleak() since our leak-detector can't find orig_argv on the - * stack. */ - orig_argv = notleak(tal_arr(ld, char *, argc + 1)); - for (size_t i = 1; i < argc; i++) - orig_argv[i] = tal_strdup(orig_argv, argv[i]); - - /*~ Turn argv[0] into an absolute path (if not already) */ - orig_argv[0] = path_join(orig_argv, take(path_cwd(NULL)), argv[0]); - orig_argv[argc] = NULL; + /*~ Note the use of notleak() since our leak-detector can't find + * orig_argv on the stack. */ + orig_argv = notleak(reexec_args(ld, argc, argv)); /* Figure out where our subdaemons are first. */ find_subdaemons_and_plugins(ld, argv[0]); @@ -1257,8 +1337,12 @@ int main(int argc, char *argv[]) /*~ Handle early options; this moves us into --lightning-dir. * Plugins may add new options, which is why we are splitting * between early args (including --plugin registration) and - * non-early opts. This also forks if they say --daemon. */ + * non-early opts. This also forks if they say --daemon. + * Parsing removes args from argv, so keep them to scrub after. */ + args_to_scrub = tal_dup_arr(NULL, char *, argv, argc + 1, 0); handle_early_opts(ld, argc, argv); + scrub_recover_secret(args_to_scrub); + tal_free(args_to_scrub); /*~ Set the default portnum according to the used network * similarly to what Bitcoin Core does to ports by default. */ @@ -1558,6 +1642,10 @@ int main(int argc, char *argv[]) /* Get rid of per-channel subdaemons. */ subd_shutdown_nonglobals(ld); + /* Save peers' channel_updates we held back, while db_write + * plugins can still see them. */ + channel_gossip_shutdown(ld); + /* Tell plugins we're shutting down, use force if necessary. */ shutdown_plugins(ld); @@ -1582,11 +1670,16 @@ int main(int argc, char *argv[]) /* Gather these before we free ld! */ try_reexec = ld->try_reexec; if (try_reexec) { - /* Maybe we reexec with --recover, due to recover command */ + /* Maybe we reexec with the secret, due to recover command */ if (ld->recover_secret) { - tal_arr_insert(&orig_argv, argc, - tal_fmt(orig_argv, "--recover=%s", - ld->recover_secret)); + /* Before the NULL: tal_arr_insert() evaluates this + * after resizing */ + size_t end = tal_count(orig_argv) - 1; + + recover_secret = tal_steal(orig_argv, ld->recover_secret); + tal_arr_insert(&orig_argv, end, + tal_fmt(orig_argv, "--recover-fd=%i", + RECOVER_FD)); } tal_steal(NULL, orig_argv); } @@ -1615,6 +1708,8 @@ int main(int argc, char *argv[]) /* Close all filedescriptors except stdin/stdout/stderr */ closefrom(STDERR_FILENO + 1); + if (recover_secret) + pipe_recover_secret(recover_secret); execv(orig_argv[0], orig_argv); err(1, "Failed to re-exec ourselves after version change/recover"); } diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 75759e21efa5..d52c743bd4c9 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -348,6 +348,10 @@ struct lightningd { * if we are the fundee. */ u32 dev_max_funding_unconfirmed; + /* Seconds a peer's dual-funded open (or RBF) has to reach + * commitment_signed. */ + u32 dev_max_dualopen_negotiation_secs; + /* Special switches to test onion compatibility */ bool dev_ignore_modern_onion; @@ -422,7 +426,7 @@ struct lightningd { /* Should we re-exec ourselves instead of just exiting? */ bool try_reexec; - /* If set, we are to restart with --recover=... */ + /* If set, we are to restart and recover with this secret */ const char *recover_secret; /* Array of (even) TLV types that we should allow. This is required diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 53e3aabef928..a7e0a0ff7d6b 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -128,13 +128,96 @@ static bool tell_if_missing(const struct channel *channel, return true; } +/* Is this HTLC in the commitment tx which went onchain? + * + * The stubs cover a range of commitment numbers, so they include HTLCs + * removed just before this commitment: those can be indistinguishable + * onchain from a live HTLC with the same payment hash, amount and cltv. + * We know the exact state of every HTLC we still track, so we can tell + * onchaind which stubs are really in this tx (as best we know). */ +static bool htlc_in_commitment(const struct channel *channel, + const struct htlc_stub *stub, + u64 commit_num, + bool our_commitment) +{ + enum htlc_state hstate; + int flags; + enum side side; + bool current; + + if (stub->owner == LOCAL) { + const struct htlc_out *hout; + hout = find_htlc_out(channel->peer->ld->htlcs_out, channel, + stub->id); + if (!hout) + return false; + hstate = hout->hstate; + } else { + const struct htlc_in *hin; + hin = find_htlc_in(channel->peer->ld->htlcs_in, channel, + stub->id); + if (!hin) + return false; + hstate = hin->hstate; + } + + /* The only commitment of ours we ever broadcast is the latest. */ + if (our_commitment) { + side = LOCAL; + current = true; + } else { + u64 nrev; + + side = REMOTE; + nrev = revocations_received(&channel->their_shachain.chain); + /* next_index[REMOTE] advances when we send commitment_signed, + * so next_index - 2 is their previous commitment. That one + * is still unrevoked only while we're waiting for + * revoke_and_ack (revocations_received == next_index - 2). + * Once they have revoked it, onchaind takes the cheat path + * and sweeps every output, so the present bit doesn't matter. */ + if (commit_num + 1 == channel->next_index[REMOTE]) + current = true; + else if (nrev + 2 == channel->next_index[REMOTE] + && commit_num + 2 == channel->next_index[REMOTE]) + current = false; + else + /* A revoked (or unknown) commitment: we take + * everything anyway, so being here is good enough. */ + return true; + } + + flags = htlc_state_flags(hstate); + + /* HTLC_F_COMMITTED/HTLC_F_REVOKED refer to the change in progress + * (adding or removing), not to the HTLC itself. */ + if (flags & HTLC_ADDING) { + if (current) + return flags & HTLC_FLAG(side, HTLC_F_COMMITTED); + /* In the previous commitment iff it was added before + * that one, i.e. the add has been revoked in. */ + return flags & HTLC_FLAG(side, HTLC_F_REVOKED); + } + if (flags & HTLC_REMOVING) { + if (current) + return flags & HTLC_FLAG(side, HTLC_F_COMMITTED); + /* In the previous commitment unless the removal has + * already been revoked in. */ + return !(flags & HTLC_FLAG(side, HTLC_F_REVOKED)); + } + + /* Fully added, not being removed: in every valid commitment. */ + return true; +} + static void handle_onchain_init_reply(struct channel *channel, const u8 *msg) { struct htlc_stub *stubs; u64 commit_num; - bool *tell, *tell_immediate; + bool our_commitment; + bool *tell, *tell_immediate, *present; - if (!fromwire_onchaind_init_reply(msg, &commit_num)) { + if (!fromwire_onchaind_init_reply(msg, &commit_num, &our_commitment)) { channel_internal_error(channel, "Invalid onchaind_init_reply %s", tal_hex(tmpctx, msg)); return; @@ -153,12 +236,16 @@ static void handle_onchain_init_reply(struct channel *channel, const u8 *msg) commit_num); tell = tal_arr(stubs, bool, tal_count(stubs)); tell_immediate = tal_arr(stubs, bool, tal_count(stubs)); + present = tal_arr(stubs, bool, tal_count(stubs)); for (size_t i = 0; i < tal_count(stubs); i++) { tell[i] = tell_if_missing(channel, &stubs[i], &tell_immediate[i]); + present[i] = htlc_in_commitment(channel, &stubs[i], + commit_num, our_commitment); } - msg = towire_onchaind_htlcs(channel, stubs, tell, tell_immediate); + msg = towire_onchaind_htlcs(channel, stubs, tell, tell_immediate, + present); subd_send_msg(channel->owner, take(msg)); /* Tell it about any preimages we know. */ @@ -1043,6 +1130,31 @@ static u8 **sign_and_get_witness(const tal_t *ctx, info->wscript); } +/* Our HTLC tx input is always input 0 */ +static u8 **sign_and_get_htlc_witness(const tal_t *ctx, + struct lightningd *ld, + const struct bitcoin_tx *tx, + const struct onchain_signing_info *info) +{ + const u8 *msg; + struct bitcoin_signature sig; + + msg = hsm_sync_req(tmpctx, ld, take(info->sign(NULL, tx, info))); + if (!fromwire_hsmd_sign_tx_reply(msg, &sig)) + fatal("Reading sign_tx_reply: %s", tal_hex(tmpctx, msg)); + + if (info->msgtype == WIRE_ONCHAIND_SPEND_HTLC_SUCCESS) + return bitcoin_witness_htlc_success_tx(ctx, &sig, + &info->u.htlc_success.remote_htlc_sig, + &info->u.htlc_success.preimage, + info->wscript); + + assert(info->msgtype == WIRE_ONCHAIND_SPEND_HTLC_TIMEOUT); + return bitcoin_witness_htlc_timeout_tx(ctx, &sig, + &info->u.htlc_timeout.remote_htlc_sig, + info->wscript); +} + /* Always sets *welements, returns tx. Sets *worthwhile to false if * it wasn't worthwhile at the given feerate (and it had to drop feerate). * Returns NULL iff it called channel_internal_error(). @@ -1232,6 +1344,15 @@ static bool consider_onchain_htlc_tx_rebroadcast(struct channel *channel, assert(newtx->wtx); newtx->psbt = tal_steal(newtx, psbt); + /* BOLT #5: + * The local signature MUST use `SIGHASH_ALL`, otherwise + * anyone can attach additional inputs and outputs to the tx + */ + /* So our signature from raw_htlc_tx is not valid here - re-sign */ + bitcoin_tx_input_set_witness(newtx, 0, + take(sign_and_get_htlc_witness(NULL, ld, + newtx, info))); + bitcoin_txid(*tx, &oldtxid); bitcoin_txid(newtx, &newtxid); @@ -1488,7 +1609,6 @@ static void handle_onchaind_spend_htlc_success(struct channel *channel, u8 *htlc_wscript; struct bitcoin_tx *tx; u8 **witness; - struct bitcoin_signature sig; const struct onchain_witness_element **welements; const bool option_anchor_outputs = channel_has(channel, OPT_ANCHOR_OUTPUTS_DEPRECATED); const bool option_anchors_zero_fee_htlc_tx = channel_has(channel, OPT_ANCHORS_ZERO_FEE_HTLC_TX); @@ -1527,14 +1647,8 @@ static void handle_onchaind_spend_htlc_success(struct channel *channel, info->deadline_block = htlc_incoming_deadline(channel, htlc_id); /* Now sign, and set witness */ - msg = hsm_sync_req(tmpctx, ld, take(sign_htlc_success(NULL, tx, info))); - if (!fromwire_hsmd_sign_tx_reply(msg, &sig)) - fatal("Reading sign_tx_reply: %s", tal_hex(tmpctx, msg)); - - witness = bitcoin_witness_htlc_success_tx(NULL, &sig, - &info->u.htlc_success.remote_htlc_sig, - &info->u.htlc_success.preimage, - info->wscript); + info->sign = sign_htlc_success; + witness = sign_and_get_htlc_witness(NULL, ld, tx, info); welements = onchain_witness_htlc_tx(tmpctx, witness); bitcoin_tx_input_set_witness(tx, 0, take(witness)); @@ -1569,7 +1683,6 @@ static void handle_onchaind_spend_htlc_timeout(struct channel *channel, u8 *htlc_wscript; struct bitcoin_tx *tx; u8 **witness; - struct bitcoin_signature sig; const struct onchain_witness_element **welements; const bool option_anchor_outputs = channel_has(channel, OPT_ANCHOR_OUTPUTS_DEPRECATED); const bool option_anchors_zero_fee_htlc_tx = channel_has(channel, OPT_ANCHORS_ZERO_FEE_HTLC_TX); @@ -1610,13 +1723,8 @@ static void handle_onchaind_spend_htlc_timeout(struct channel *channel, info->minblock = cltv_expiry + 1; /* Now sign, and set witness */ - msg = hsm_sync_req(tmpctx, ld, take(sign_htlc_timeout(NULL, tx, info))); - if (!fromwire_hsmd_sign_tx_reply(msg, &sig)) - fatal("Reading sign_tx_reply: %s", tal_hex(tmpctx, msg)); - - witness = bitcoin_witness_htlc_timeout_tx(NULL, &sig, - &info->u.htlc_timeout.remote_htlc_sig, - info->wscript); + info->sign = sign_htlc_timeout; + witness = sign_and_get_htlc_witness(NULL, ld, tx, info); welements = onchain_witness_htlc_tx(tmpctx, witness); bitcoin_tx_input_set_witness(tx, 0, take(witness)); diff --git a/lightningd/options.c b/lightningd/options.c index 3443518fea02..f276d4564646 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -23,6 +24,7 @@ #include #include #include +#include #include #include @@ -844,6 +846,13 @@ static void dev_register_opts(struct lightningd *ld) "Maximum number of blocks we wait for a channel " "funding transaction to confirm, if we are the " "fundee."); + clnopt_witharg("--dev-max-dualopen-negotiation-secs", + OPT_DEV|OPT_SHOWINT, + opt_set_u32, opt_show_u32, + &ld->dev_max_dualopen_negotiation_secs, + "Maximum seconds a peer opening or RBFing a " + "dual-funded channel with us gets to reach " + "commitment_signed."); clnopt_witharg("--dev-force-tmp-channel-id", OPT_DEV, opt_force_tmp_channel_id, NULL, ld, "Force the temporary channel id, instead of random"); @@ -1441,6 +1450,27 @@ static char *opt_set_hsm_secret(const char *arg, struct lightningd *ld) return NULL; } +/* The recover command re-execs us with the secret in this pipe */ +static char *opt_set_recover_fd(const char *arg, struct lightningd *ld) +{ + int fd; + char *secret, *err; + + err = opt_set_intval(arg, &fd); + if (err) + return err; + + secret = grab_fd_str(tmpctx, fd); + if (!secret) + return tal_fmt(tmpctx, "Reading recover secret from fd %i: %s", + fd, strerror(errno)); + close(fd); + + err = opt_set_hsm_secret(secret, ld); + sodium_memzero(secret, strlen(secret)); + return err; +} + static void register_opts(struct lightningd *ld) { /* This happens before plugins started */ @@ -1495,6 +1525,8 @@ static void register_opts(struct lightningd *ld) ld, "Populate hsm_secret with the given codex32/hex/mnemonic secret" " and starts the node in `offline` mode."); + clnopt_witharg("--recover-fd", OPT_EARLY|OPT_CONCEAL, + opt_set_recover_fd, NULL, ld, opt_hidden); /* This affects our features, so set early. */ opt_register_early_noarg("--large-channels|--wumbo", diff --git a/lightningd/pay.c b/lightningd/pay.c index 9576e490816c..068941f8954b 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -303,7 +303,6 @@ json_sendpay_in_progress(struct command *cmd, } static void tell_waiters_failed(struct lightningd *ld, - const struct sha256 *payment_hash, const struct wallet_payment *payment, enum jsonrpc_errcode pay_errcode, const struct onionreply *onionreply, @@ -315,9 +314,11 @@ static void tell_waiters_failed(struct lightningd *ld, const char *errmsg = sendpay_errmsg_fmt(tmpctx, pay_errcode, fail, details); - /* Careful: ->fail deletes cmd */ + /* Careful: ->fail deletes cmd, which may own the payment_hash our + * caller was given (injectpaymentonion's selfpay), so match on + * payment's own copy. */ list_for_each_safe(&ld->waitsendpay_commands, pc, next, list) { - if (!sha256_eq(payment_hash, &pc->payment_hash)) + if (!sha256_eq(&payment->payment_hash, &pc->payment_hash)) continue; if (payment->partid != pc->partid) continue; @@ -336,15 +337,14 @@ static void tell_waiters_failed(struct lightningd *ld, } static void tell_waiters_success(struct lightningd *ld, - const struct sha256 *payment_hash, struct wallet_payment *payment) { struct waitsendpay_command *pc; struct waitsendpay_command *next; - /* Careful: sendpay_success deletes cmd */ + /* Careful: sendpay_success deletes cmd (see tell_waiters_failed) */ list_for_each_safe(&ld->waitsendpay_commands, pc, next, list) { - if (!sha256_eq(payment_hash, &pc->payment_hash)) + if (!sha256_eq(&payment->payment_hash, &pc->payment_hash)) continue; if (payment->partid != pc->partid) continue; @@ -374,7 +374,7 @@ void payment_succeeded(struct lightningd *ld, if (payment->local_invreq_id) wallet_invoice_request_mark_used(ld->wallet->db, payment->local_invreq_id); - tell_waiters_success(ld, payment_hash, payment); + tell_waiters_success(ld, payment); } /* Return a struct routing_failure for an immediate failure @@ -655,7 +655,7 @@ void payment_failed(struct lightningd *ld, fail ? fail->channel_dir : 0, fail ? fail->msg : NULL); - tell_waiters_failed(ld, payment_hash, payment, pay_errcode, + tell_waiters_failed(ld, payment, pay_errcode, failonion, fail, failstr); } @@ -1553,7 +1553,7 @@ static struct command_result *self_payment(struct lightningd *ld, 0, NULL); /* We do this even though there really can't be any waiters, * since we didn't block. */ - tell_waiters_failed(ld, rhash, payment, PAY_DESTINATION_PERM_FAIL, + tell_waiters_failed(ld, payment, PAY_DESTINATION_PERM_FAIL, NULL, fail, err); return sendpay_fail(cmd, payment, PAY_DESTINATION_PERM_FAIL, NULL, fail, err, NULL); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index f1ae4af167b0..d9ea97555402 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -248,7 +248,8 @@ u8 *p2tr_for_keyidx(const tal_t *ctx, struct lightningd *ld, u64 keyidx) static struct bitcoin_tx *sign_last_tx(const tal_t *ctx, const struct channel *channel, const struct bitcoin_tx *last_tx, - const struct bitcoin_signature *last_sig) + const struct bitcoin_signature *last_sig, + const struct pubkey *remote_fundingkey) { struct lightningd *ld = channel->peer->ld; struct bitcoin_signature sig; @@ -262,8 +263,7 @@ static struct bitcoin_tx *sign_last_tx(const tal_t *ctx, &channel->peer->id, channel->dbid, tx, - &channel->channel_info - .remote_fundingkey, + remote_fundingkey, commit_index); msg = hsm_sync_req(tmpctx, ld, take(msg)); @@ -273,13 +273,23 @@ static struct bitcoin_tx *sign_last_tx(const tal_t *ctx, witness = bitcoin_witness_2of2(tx, last_sig, - &sig, &channel->channel_info.remote_fundingkey, + &sig, remote_fundingkey, &channel->local_funding_pubkey); bitcoin_tx_input_set_witness(tx, 0, take(witness)); return tx; } +/* The peer may use a new funding key for a splice: that inflight's + * commitment spends the splice output, not the current funding. */ +static const struct pubkey * +inflight_remote_fundingkey(const struct channel_inflight *inflight) +{ + if (inflight->funding->splice_remote_funding) + return inflight->funding->splice_remote_funding; + return &inflight->channel->channel_info.remote_fundingkey; +} + bool invalid_last_tx(const struct bitcoin_tx *tx) { /* This problem goes back further, but was discovered just before the @@ -318,13 +328,15 @@ static struct bitcoin_tx *sign_and_send_last(const tal_t *ctx, struct channel *channel, const char *cmd_id, const struct bitcoin_tx *last_tx, - const struct bitcoin_signature *last_sig) + const struct bitcoin_signature *last_sig, + const struct pubkey *remote_fundingkey) { struct bitcoin_txid txid; struct anchor_details *adet; struct bitcoin_tx *tx; - tx = sign_last_tx(ctx, channel, last_tx, last_sig); + tx = sign_last_tx(ctx, channel, last_tx, last_sig, + remote_fundingkey); bitcoin_txid(tx, &txid); wallet_transaction_add(ld->wallet, tx->wtx, 0, 0); wallet_extract_owned_outputs(ld->wallet, tx->wtx, false, NULL, NULL); @@ -500,18 +512,21 @@ void drop_to_chain(struct lightningd *ld, struct channel *channel, channel, cmd_id, inflight->last_tx, - &inflight->last_sig)); + &inflight->last_sig, + inflight_remote_fundingkey(inflight))); } if (!have_current) tal_arr_expand(&txs, sign_and_send_last(tmpctx, ld, channel, cmd_id, channel->last_tx, - &channel->last_sig)); + &channel->last_sig, + &channel->channel_info.remote_fundingkey)); } else { tal_arr_expand(&txs, sign_and_send_last(tmpctx, ld, channel, cmd_id, channel->last_tx, - &channel->last_sig)); + &channel->last_sig, + &channel->channel_info.remote_fundingkey)); } resolve_close_command(ld, channel, cooperative, txs); @@ -523,13 +538,19 @@ void drop_to_chain(struct lightningd *ld, struct channel *channel, * force close. */ if (cooperative) { list_for_each(&channel->inflights, inflight, list) { + const struct pubkey *remote_funding; + const u8 *funding_wscript; + if (bitcoin_outpoint_eq(&inflight->funding->outpoint, &channel->funding)) { continue; } - const u8 *funding_wscript = bitcoin_redeem_2of2(tmpctx, - &channel->local_funding_pubkey, - inflight->funding->splice_remote_funding); + remote_funding = inflight->funding->splice_remote_funding; + if (!remote_funding) + remote_funding = &channel->channel_info.remote_fundingkey; + funding_wscript = bitcoin_redeem_2of2(tmpctx, + &channel->local_funding_pubkey, + remote_funding); watch_scriptpubkey(inflight, ld->topology, take(scriptpubkey_p2wsh(NULL, funding_wscript)), &inflight->funding->outpoint, @@ -2328,6 +2349,20 @@ void handle_peer_spoke(struct lightningd *ld, const u8 *msg) "Channel is closed and forgotten"); goto send_error; } + break; + + /* BOLT #1: + * + * - if no existing channel is referred to by `channel_id`: + * - MUST ignore the message. + */ + case WIRE_ERROR: + case WIRE_WARNING: + log_peer_debug(ld->log, &peer->id, + "Ignoring %s for unknown channel %s", + peer_wire_name(msgtype), + fmt_channel_id(tmpctx, &channel_id)); + return; } /* Weird message? Log and reply with error. */ @@ -3172,8 +3207,10 @@ static void setup_peer(struct peer *peer) watch_opening_inflight(ld, inflight); break; - /* We need to watch all inflights which may splice */ + /* We need to watch all inflights which may splice, and the + * current funding, which is live until one of them does. */ case CHANNELD_AWAITING_SPLICE: + channel_watch_funding(ld, channel); list_for_each(&channel->inflights, inflight, list) watch_splice_inflight(ld, inflight); break; @@ -4115,7 +4152,8 @@ static struct command_result *json_sign_last_tx(struct command *cmd, log_debug(channel->log, "dev-sign-last-tx: signing tx with %zu outputs", channel->last_tx->wtx->num_outputs); - tx = sign_last_tx(cmd, channel, channel->last_tx, &channel->last_sig); + tx = sign_last_tx(cmd, channel, channel->last_tx, &channel->last_sig, + &channel->channel_info.remote_fundingkey); json_add_tx(response, "tx", tx); /* If we've got inflights, return them */ @@ -4127,7 +4165,8 @@ static struct command_result *json_sign_last_tx(struct command *cmd, if (inflight->splice_locked_memonly) continue; tx = sign_last_tx(cmd, channel, inflight->last_tx, - &inflight->last_sig); + &inflight->last_sig, + inflight_remote_fundingkey(inflight)); json_object_start(response, NULL); json_add_txid(response, "funding_txid", &inflight->funding->outpoint.txid); diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 9f6e73ee8aaf..c14bff6435da 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -2575,6 +2575,16 @@ void peer_got_commitsig(struct channel *channel, const u8 *msg) tal_count(failed), tal_count(changed), tal_count(inflight_commit_sigs)); + /* We may still forget a channel awaiting lockin, and forgetting + * one with HTLCs is fatal: channeld shouldn't let this happen. */ + if (tal_count(added) != 0 + && channel->state == CHANNELD_AWAITING_LOCKIN) { + channel_internal_error(channel, + "Got commitsig adding HTLCs in state %s", + channel_state_name(channel)); + return; + } + i = 0; list_for_each(&channel->inflights, inflight, list) { if (!inflight->splice_locked_memonly) @@ -2731,9 +2741,8 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) struct lightningd *ld = channel->peer->ld; struct fee_states *fee_states; struct height_states *blockheight_states; - struct penalty_base *pbase; - struct commitment_revocation_payload *payload; - struct bitcoin_tx *penalty_tx; + struct penalty_base *pbases; + struct bitcoin_tx **penalty_txs; if (!fromwire_channeld_got_revoke(msg, msg, &revokenum, &per_commitment_secret, @@ -2741,8 +2750,8 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) &fee_states, &blockheight_states, &changed, - &pbase, - &penalty_tx) + &pbases, + &penalty_txs) || !fee_states_valid(fee_states, channel->opener) || !height_states_valid(blockheight_states, channel->opener)) { channel_internal_error(channel, "bad fromwire_channeld_got_revoke %s", @@ -2754,30 +2763,6 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) "got revoke %"PRIu64": %zu changed", revokenum, tal_count(changed)); - /* Save any immediate failures for after we reply. */ - badonions = tal_arrz(msg, enum onion_wire, tal_count(changed)); - failmsgs = tal_arrz(msg, u8 *, tal_count(changed)); - for (i = 0; i < tal_count(changed); i++) { - /* BOLT #2: - * A node: - * - until an incoming HTLC has been irrevocably committed: - * - MUST NOT offer the corresponding outgoing HTLC - * (`update_add_htlc`) in response to that incoming HTLC. - */ - /* If we're doing final accept, we need to forward */ - if (changed[i].newstate == RCVD_ADD_ACK_REVOCATION) { - peer_accepted_htlc(failmsgs, - channel, changed[i].id, false, - &badonions[i], &failmsgs[i]); - } else { - if (!changed_htlc(channel, &changed[i])) { - channel_internal_error(channel, - "got_revoke: update failed"); - return; - } - } - } - if (revokenum >= (1ULL << 48)) { channel_internal_error(channel, "got_revoke: too many txs %"PRIu64, revokenum); @@ -2810,6 +2795,31 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) return; } + /* Store the revocation secret before acting on the HTLCs it + * locks in. */ + badonions = tal_arrz(msg, enum onion_wire, tal_count(changed)); + failmsgs = tal_arrz(msg, u8 *, tal_count(changed)); + for (i = 0; i < tal_count(changed); i++) { + /* BOLT #2: + * A node: + * - until an incoming HTLC has been irrevocably committed: + * - MUST NOT offer the corresponding outgoing HTLC + * (`update_add_htlc`) in response to that incoming HTLC. + */ + /* If we're doing final accept, we need to forward */ + if (changed[i].newstate == RCVD_ADD_ACK_REVOCATION) { + peer_accepted_htlc(failmsgs, + channel, changed[i].id, false, + &badonions[i], &failmsgs[i]); + } else { + if (!changed_htlc(channel, &changed[i])) { + channel_internal_error(channel, + "got_revoke: update failed"); + return; + } + } + } + tal_free(channel->fee_states); channel->fee_states = tal_steal(channel, fee_states); @@ -2869,17 +2879,20 @@ void peer_got_revoke(struct channel *channel, const u8 *msg) /* Maybe graceful wants to know? */ check_graceful_shutdown(ld); - if (penalty_tx == NULL) - return; + /* During a splice there is one per splice candidate, as well as the + * one for the current funding: the peer could broadcast any of them. */ + for (i = 0; i < tal_count(penalty_txs); i++) { + struct commitment_revocation_payload *payload; - payload = tal(tmpctx, struct commitment_revocation_payload); - payload->commitment_txid = pbase->txid; - payload->penalty_tx = tal_steal(payload, penalty_tx); - payload->wallet = ld->wallet; - payload->channel_dbid = channel->dbid; - payload->commitnum = pbase->commitment_num; - payload->channel_id = channel->cid; - plugin_hook_call_commitment_revocation(ld, NULL, payload); + payload = tal(tmpctx, struct commitment_revocation_payload); + payload->commitment_txid = pbases[i].txid; + payload->penalty_tx = tal_steal(payload, penalty_txs[i]); + payload->wallet = ld->wallet; + payload->channel_dbid = channel->dbid; + payload->commitnum = pbases[i].commitment_num; + payload->channel_id = channel->cid; + plugin_hook_call_commitment_revocation(ld, NULL, payload); + } } diff --git a/lightningd/plugin_hook.c b/lightningd/plugin_hook.c index 5c561e1f7f3e..a5fbef716e38 100644 --- a/lightningd/plugin_hook.c +++ b/lightningd/plugin_hook.c @@ -267,8 +267,19 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks, ecode = 0; json_scan(tmpctx, buffer, toks, "{error:{code:%}}", JSON_SCAN(json_to_jsonrpc_errcode, &ecode)); - if (ecode == PLUGIN_TERMINATED) + if (ecode == PLUGIN_TERMINATED) { + /* An important plugin dying takes us down with it. Don't act + * as if it said "continue": leave the hook unanswered, like at + * shutdown, so it's called again when we restart. */ + if (h && h->plugin->important) { + log_unusual(ph_req->ld->log, + "Important plugin %s died from %s hook call:" + " leaving it for restart", + h->plugin->shortname, ph_req->hook->name); + return; + } h = NULL; + } /* We really only handle plugins dying: other errors are fatal. */ if (h) { diff --git a/lightningd/runes.c b/lightningd/runes.c index 947c74c84862..86cc25f45791 100644 --- a/lightningd/runes.c +++ b/lightningd/runes.c @@ -17,6 +17,9 @@ /* "640k should be enough for anybody!" */ #define MAX_BLACKLIST_NUM 100000000 +/* Real runes are tiny; rune decoding is quadratic in length, so refuse huge ones. */ +#define MAX_RUNE_LEN (64 * 1024) + static const u64 sec_per_nsec = 1000000000; struct cond_info { @@ -225,6 +228,10 @@ static struct command_result *param_rune(struct command *cmd, const char *name, { u64 uid; + if (tok->end - tok->start > MAX_RUNE_LEN) + return command_fail_badparam(cmd, name, buffer, tok, + "is too long"); + *rune_and_string = tal(cmd, struct rune_and_string); (*rune_and_string)->runestr = json_strdup(*rune_and_string, buffer, tok); (*rune_and_string)->rune = rune_from_base64(cmd, (*rune_and_string)->runestr); @@ -703,6 +710,23 @@ static const struct json_command destroyrune_command = { }; AUTODATA(json_command, &destroyrune_command); +/* Several parameters map to this name, and commands differ in which one + * they'd use, so any restriction on it fails. */ +static const jsmntok_t ambiguous_param; + +static void cache_param(struct cond_info *cinfo, const char *name, + const jsmntok_t *val) +{ + const jsmntok_t *old = strmap_get(&cinfo->cached_params, name); + + if (!old) { + strmap_add(&cinfo->cached_params, name, val); + } else if (old != val) { + strmap_del(&cinfo->cached_params, name, NULL); + strmap_add(&cinfo->cached_params, name, &ambiguous_param); + } +} + /* Returns the parameter name and set *invf, or NULL */ static const char *match_inv_condition(const tal_t *ctx, const char *fieldname, @@ -800,6 +824,8 @@ static const char *check_inv_condition(const tal_t *ctx, tal_fmt(tmpctx, "pname%s", param)); if (!ptok) return tal_fmt(ctx, "Unknown invoice parameter %s", param); + if (ptok == &ambiguous_param) + return tal_fmt(ctx, "Ambiguous invoice parameter %s", param); invstr = json_strdup(tmpctx, cinfo->buf, ptok); if (strstarts(invstr, "lightning:") || strstarts(invstr, "LIGHTNING:")) @@ -872,8 +898,7 @@ static const char *check_condition(const tal_t *ctx, size_t off = strlen("pname"); /* First, add version with underscores intact. */ - strmap_add(&cinfo->cached_params, - tal_strdup(tmpctx, pmemname), t+1); + cache_param(cinfo, tal_strdup(tmpctx, pmemname), t+1); /* Now with punctuation removed: */ for (size_t n = off; pmemname[n]; n++) { @@ -882,7 +907,7 @@ static const char *check_condition(const tal_t *ctx, pmemname[off++] = pmemname[n]; } pmemname[off++] = '\0'; - strmap_add(&cinfo->cached_params, pmemname, t+1); + cache_param(cinfo, pmemname, t+1); } } else if (cinfo->params->type == JSMN_ARRAY) { json_for_each_arr(i, t, cinfo->params) { @@ -899,6 +924,8 @@ static const char *check_condition(const tal_t *ctx, ptok = strmap_get(&cinfo->cached_params, alt->fieldname); if (!ptok) return rune_alt_single_missing(ctx, alt); + if (ptok == &ambiguous_param) + return tal_fmt(ctx, "%s is ambiguous", alt->fieldname); /* Pass through valid integers as integers. */ if (ptok->type == JSMN_PRIMITIVE) { diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index ffd6cbe7b2f1..5dd7c0583b1e 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -15,6 +15,9 @@ void begin_topology(struct chain_topology *topo UNNEEDED) /* Generated stub for channel_gossip_notify_new_block */ void channel_gossip_notify_new_block(struct lightningd *ld UNNEEDED) { fprintf(stderr, "channel_gossip_notify_new_block called!\n"); abort(); } +/* Generated stub for channel_gossip_shutdown */ +void channel_gossip_shutdown(struct lightningd *ld UNNEEDED) +{ fprintf(stderr, "channel_gossip_shutdown called!\n"); abort(); } /* Generated stub for channel_notify_new_block */ void channel_notify_new_block(struct lightningd *ld UNNEEDED) { fprintf(stderr, "channel_notify_new_block called!\n"); abort(); } diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index 75cabfbac09b..ce7532cdd347 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -94,6 +94,22 @@ struct resolution { enum tx_type tx_type; }; +/* What we currently believe about an HTLC they offered us. This is + * deliberately separate from tracked_output->resolved, which cannot express + * "we've written it off, but it could still become ours": a preimage can + * arrive at any time until we exit, and we only tell the bookkeeper where + * the money went once that can no longer change. */ +enum their_htlc_fate { + /* We may still claim it (also: it's not a THEIR_HTLC at all). */ + THEIR_HTLC_UNDECIDED, + /* We stopped trying to claim it (it timed out, or our claim wasn't + * worth the fee): nothing recorded yet, and a preimage still + * revives it. */ + THEIR_HTLC_ABANDONED, + /* Final: it's theirs, and we've told the bookkeeper. */ + THEIR_HTLC_THEIRS, +}; + struct tracked_output { enum tx_type tx_type; struct bitcoin_outpoint outpoint; @@ -116,6 +132,9 @@ struct tracked_output { /* If it is resolved. */ struct resolution *resolved; + /* For a THEIR_HTLC: where we currently think it's going. */ + enum their_htlc_fate fate; + /* stashed so we can pass it along to the coin ledger */ struct sha256 payment_hash; }; @@ -253,6 +272,17 @@ static void record_external_deposit(const struct tracked_output *out, record_external_output(&out->outpoint, out->sat, blockheight, tags); } +/* An HTLC they offered us has ended up back with them. Only call this once + * that's final: the bookkeeper has no way of taking it back, and a preimage + * can revive an abandoned HTLC right up until we exit. */ +static void record_htlc_to_them(struct tracked_output *out) +{ + assert(out->fate != THEIR_HTLC_THEIRS); + record_external_deposit(out, out->tx_blockheight, + mk_mvt_tags(MVT_HTLC_TIMEOUT)); + out->fate = THEIR_HTLC_THEIRS; +} + static void record_mutual_close(const struct tx_parts *tx, const u8 *remote_scriptpubkey, u32 blockheight) @@ -598,6 +628,7 @@ new_tracked_output(struct tracked_output ***outs, out->output_type = output_type; out->proposal = NULL; out->resolved = NULL; + out->fate = THEIR_HTLC_UNDECIDED; if (htlc) out->htlc = *htlc; out->wscript = tal_steal(out, wscript); @@ -635,8 +666,16 @@ static void handle_spend_created(struct tracked_output *out, const u8 *msg) tal_steal(out->proposal, witness)); /* Did it decide it's not worth it? Don't wait for it. */ - if (!worthwhile) + if (!worthwhile) { ignore_output(out); + /* If that was our claim on their HTLC, we've given up on it + * just as if it had timed out: it's theirs unless a preimage + * (or a better fee) turns up, and the bookkeeper hears + * about it once that can't happen any more. */ + if (out->output_type == THEIR_HTLC + && out->fate == THEIR_HTLC_UNDECIDED) + out->fate = THEIR_HTLC_ABANDONED; + } } static struct proposed_resolution *new_proposed_resolution(struct tracked_output *out, @@ -706,6 +745,11 @@ static void propose_ignore(struct tracked_output *out, out->proposal = new_proposed_resolution(out, block_required, tx_type); out->proposal->welements = NULL; + /* We've given up on this one, though a preimage can still revive it: + * see handle_preimage(). */ + if (tx_type == THEIR_HTLC_TIMEOUT_TO_THEM) + out->fate = THEIR_HTLC_ABANDONED; + /* Can we immediately ignore? */ if (out->proposal->depth_required == 0) ignore_output(out); @@ -749,9 +793,9 @@ static bool onchain_witness_element_matches(const struct onchain_witness_element return true; } -/* This simple case: true if this was resolved by our proposal. */ -static bool resolved_by_proposal(struct tracked_output *out, - const struct tx_parts *tx_parts) +/* Does this tx spend the output the way our proposal would? */ +static bool spent_by_proposal(const struct tracked_output *out, + const struct tx_parts *tx_parts) { const struct wally_tx_input *input; @@ -762,7 +806,14 @@ static bool resolved_by_proposal(struct tracked_output *out, input = which_input_spends(tx_parts, &out->outpoint); if (!input || !input->witness) return false; - if (!onchain_witness_element_matches(out->proposal->welements, input)) + return onchain_witness_element_matches(out->proposal->welements, input); +} + +/* This simple case: true if this was resolved by our proposal. */ +static bool resolved_by_proposal(struct tracked_output *out, + const struct tx_parts *tx_parts) +{ + if (!spent_by_proposal(out, tx_parts)) return false; out->resolved = tal(out, struct resolution); @@ -892,12 +943,15 @@ static bool is_local_commitment(const struct bitcoin_txid *txid, * once the remote's *resolving* transaction is included in a block at least 100 * deep, on the most-work blockchain. */ +#define IRREVOCABLY_RESOLVED_DEPTH 100 + static size_t num_not_irrevocably_resolved(struct tracked_output **outs) { size_t i, num = 0; for (i = 0; i < tal_count(outs); i++) { - if (!outs[i]->resolved || outs[i]->resolved->depth < 100) + if (!outs[i]->resolved + || outs[i]->resolved->depth < IRREVOCABLY_RESOLVED_DEPTH) num++; } return num; @@ -963,8 +1017,9 @@ static void billboard_update(struct tracked_output **outs) "All outputs resolved:" " waiting %u more blocks before forgetting" " channel", - best->resolved->depth < 100 - ? 100 - best->resolved->depth : 0); + best->resolved->depth < IRREVOCABLY_RESOLVED_DEPTH + ? IRREVOCABLY_RESOLVED_DEPTH - best->resolved->depth + : 0); return; } @@ -1140,10 +1195,23 @@ static void steal_htlc_tx(struct tracked_output *out, struct amount_asset asset; struct amount_sat htlc_out_amt; const u8 *msg; + u8 *wscript; + + if (htlc_outpoint->n >= tal_count(htlc_tx->outputs)) { + status_unusual("%s/%s spent by input %u of %s, which has" + " only %zu outputs: cannot steal", + tx_type_name(out->tx_type), + output_type_name(out->output_type), + htlc_outpoint->n, + fmt_bitcoin_txid(tmpctx, &htlc_tx->txid), + tal_count(htlc_tx->outputs)); + resolved_by_other(out, &htlc_tx->txid, htlc_tx_type); + return; + } - u8 *wscript = bitcoin_wscript_htlc_tx(htlc_tx, to_self_delay[REMOTE], - &keyset->self_revocation_key, - &keyset->self_delayed_payment_key); + wscript = bitcoin_wscript_htlc_tx(htlc_tx, to_self_delay[REMOTE], + &keyset->self_revocation_key, + &keyset->self_delayed_payment_key); asset = wally_tx_output_get_amount(htlc_tx->outputs[htlc_outpoint->n]); assert(amount_asset_is_main(&asset)); @@ -1209,8 +1277,46 @@ static bool output_spent(struct tracked_output ***outs, interesting = true; - if (out->resolved) + if (out->resolved) { + if (out->resolved->tx_type == SELF + && out->proposal + && spent_by_proposal(out, tx_parts)) { + /* We stopped waiting for this spend of our + * own, since lightningd didn't think it worth + * much (see handle_spend_created()), but it + * confirmed anyway. Book it as ours, and + * re-anchor the resolution on this tx: it's + * what actually resolved the output, so the + * clock for irrevocable resolution restarts + * from its confirmation, not the commitment's. + * A lowball tx can mine long after the + * commitment is 100 deep itself, and with + * one confirmation it could still be + * reorged out. */ + status_debug("Resolved %s/%s by our lowballed" + " proposal %s (%s) after all", + tx_type_name(out->tx_type), + output_type_name(out->output_type), + tx_type_name(out->proposal->tx_type), + fmt_bitcoin_txid(tmpctx, + &tx_parts->txid)); + out->resolved->tx_type = out->proposal->tx_type; + out->resolved->txid = tx_parts->txid; + out->resolved->depth = 0; + out->fate = THEIR_HTLC_UNDECIDED; + record_coin_movements(out, tx_blockheight, + &tx_parts->txid); + } else if (out->output_type == THEIR_HTLC + && out->resolved->tx_type == SELF + && out->fate == THEIR_HTLC_ABANDONED) { + /* If we'd abandoned this HTLC (assuming it + * would go back to them), this spend proves + * it: it's really theirs now, and no preimage + * can change that. */ + record_htlc_to_them(out); + } continue; + } /* Was this our resolution? */ if (resolved_by_proposal(out, tx_parts)) { @@ -1253,11 +1359,20 @@ static bool output_spent(struct tracked_output ***outs, THEIR_HTLC_TIMEOUT_TO_THEM, &htlc_outpoint); } else { - /* We ignore this timeout tx, since we should - * resolve by ignoring once we reach depth. */ onchain_annotate_txout( &htlc_outpoint, TX_CHANNEL_HTLC_TIMEOUT | TX_THEIRS); + + /* They took it back. We usually resolve this + * by ignoring it once we reach depth, but if + * a preimage arrived we replaced that + * proposal with a spend which can now never + * happen: resolve it here, or we'd wait + * forever. (out->fate stops handle_preimage + * undoing this again). */ + ignore_output(out); + if (out->fate != THEIR_HTLC_THEIRS) + record_htlc_to_them(out); } break; @@ -1432,11 +1547,10 @@ static void tx_new_depth(struct tracked_output **outs, if (outs[i]->proposal && depth >= outs[i]->proposal->depth_required && !outs[i]->proposal->welements) { + /* Note: we don't tell the bookkeeper an HTLC went + * back to them here! A preimage can still turn up + * and let us claim it: see record_htlc_to_them(). */ ignore_output(outs[i]); - - if (outs[i]->proposal->tx_type == THEIR_HTLC_TIMEOUT_TO_THEM) - record_external_deposit(outs[i], outs[i]->tx_blockheight, - mk_mvt_tags(MVT_HTLC_TIMEOUT)); } } } @@ -1486,15 +1600,23 @@ static void handle_preimage(struct tracked_output **outs, if (!ripemd160_eq(&outs[i]->htlc.ripemd, &ripemd)) continue; + /* The only reason not to try to claim it is that we can't: + * they spent it, so there's nothing left to spend. */ + if (outs[i]->fate == THEIR_HTLC_THEIRS) { + status_unusual("HTLC already went to them" + " when we found preimage"); + continue; + } + /* If HTLC has timed out, we will already have * proposed a "ignore this, it's their problem". But * now try this proposal instead! */ if (outs[i]->resolved) { if (outs[i]->resolved->tx_type != SELF) { - status_broken("HTLC already resolved by %s" - " when we found preimage", - tx_type_name(outs[i]->resolved->tx_type)); - return; + status_unusual("HTLC already resolved by %s" + " when we found preimage", + tx_type_name(outs[i]->resolved->tx_type)); + continue; } } @@ -1504,6 +1626,9 @@ static void handle_preimage(struct tracked_output **outs, /* Discard any previous resolution. Could be a timeout, * could be due to multiple identical rhashes in tx. */ outs[i]->proposal = tal_free(outs[i]->proposal); + outs[i]->resolved = tal_free(outs[i]->resolved); + /* It's up for grabs again! */ + outs[i]->fate = THEIR_HTLC_UNDECIDED; /* BOLT #5: * @@ -1707,6 +1832,13 @@ static void wait_for_resolved(struct tracked_output **outs) clean_tmpctx(); } + /* We're done: no preimage can arrive now, so any HTLC we abandoned + * really did go back to them. Tell the bookkeeper. */ + for (size_t i = 0; i < tal_count(outs); i++) { + if (outs[i]->fate == THEIR_HTLC_ABANDONED) + record_htlc_to_them(outs[i]); + } + wire_sync_write(REQ_FD, take(towire_onchaind_all_irrevocably_resolved(outs))); } @@ -1715,11 +1847,12 @@ struct htlcs_info { struct htlc_stub *htlcs; bool *tell_if_missing; bool *tell_immediately; + bool *present; }; struct htlc_with_tells { struct htlc_stub htlc; - bool tell_if_missing, tell_immediately; + bool tell_if_missing, tell_immediately, present; }; static int cmp_htlc_with_tells_cltv(const struct htlc_with_tells *a, @@ -1729,10 +1862,18 @@ static int cmp_htlc_with_tells_cltv(const struct htlc_with_tells *a, return -1; else if (a->htlc.cltv_expiry > b->htlc.cltv_expiry) return 1; + + /* lightningd hands us every HTLC which might be in this commitment, + * including ones removed just before it. Such an HTLC can have the + * same payment hash, amount and cltv as a live one, and so the same + * output script: make sure the live one gets the output. */ + if (a->present != b->present) + return a->present ? -1 : 1; return 0; } -static struct htlcs_info *init_reply(const tal_t *ctx, const char *what) +static struct htlcs_info *init_reply(const tal_t *ctx, const char *what, + bool our_commitment) { struct htlcs_info *htlcs_info = tal(ctx, struct htlcs_info); const u8 *msg; @@ -1743,7 +1884,8 @@ static struct htlcs_info *init_reply(const tal_t *ctx, const char *what) /* Send init_reply first, so billboard gets credited to ONCHAIND */ wire_sync_write(REQ_FD, - take(towire_onchaind_init_reply(NULL, commit_num))); + take(towire_onchaind_init_reply(NULL, commit_num, + our_commitment))); peer_billboard(true, "%s", what); @@ -1752,7 +1894,8 @@ static struct htlcs_info *init_reply(const tal_t *ctx, const char *what) if (!fromwire_onchaind_htlcs(htlcs_info, msg, &htlcs_info->htlcs, &htlcs_info->tell_if_missing, - &htlcs_info->tell_immediately)) + &htlcs_info->tell_immediately, + &htlcs_info->present)) master_badmsg(WIRE_ONCHAIND_HTLCS, msg); /* One convenient structure, so we sort them together! */ @@ -1761,19 +1904,23 @@ static struct htlcs_info *init_reply(const tal_t *ctx, const char *what) htlcs[i].htlc = htlcs_info->htlcs[i]; htlcs[i].tell_if_missing = htlcs_info->tell_if_missing[i]; htlcs[i].tell_immediately = htlcs_info->tell_immediately[i]; + htlcs[i].present = htlcs_info->present[i]; } - /* Sort by CLTV, so matches are in CLTV order (and easy to skip dups) */ + /* Sort by CLTV, so matches are in CLTV order (and easy to skip dups), + * with the ones which are really in this tx first. */ asort(htlcs, tal_count(htlcs), cmp_htlc_with_tells_cltv, NULL); /* Now put them back (prev were allocated off tmpctx) */ htlcs_info->htlcs = tal_arr(htlcs_info, struct htlc_stub, tal_count(htlcs)); htlcs_info->tell_if_missing = tal_arr(htlcs_info, bool, tal_count(htlcs)); htlcs_info->tell_immediately = tal_arr(htlcs_info, bool, tal_count(htlcs)); + htlcs_info->present = tal_arr(htlcs_info, bool, tal_count(htlcs)); for (size_t i = 0; i < tal_count(htlcs); i++) { htlcs_info->htlcs[i] = htlcs[i].htlc; htlcs_info->tell_if_missing[i] = htlcs[i].tell_if_missing; htlcs_info->tell_immediately[i] = htlcs[i].tell_immediately; + htlcs_info->present[i] = htlcs[i].present; } return htlcs_info; @@ -1783,7 +1930,7 @@ static void handle_mutual_close(struct tracked_output **outs, const struct tx_parts *tx) { /* In this case, we don't care about htlcs: there are none. */ - init_reply(tmpctx, "Tracking mutual close transaction"); + init_reply(tmpctx, "Tracking mutual close transaction", false); /* Annotate the first input as close. We can currently only have a * single input for these. */ @@ -2195,7 +2342,7 @@ static void handle_our_unilateral(const struct tx_parts *tx, size_t i; struct htlcs_info *htlcs_info; - htlcs_info = init_reply(tx, "Tracking our own unilateral close"); + htlcs_info = init_reply(tx, "Tracking our own unilateral close", true); onchain_annotate_txin(&tx->txid, 0, TX_CHANNEL_UNILATERAL); /* BOLT #5: @@ -2646,7 +2793,8 @@ static void handle_their_cheat(const struct tx_parts *tx, struct htlcs_info *htlcs_info; htlcs_info = init_reply(tx, - "Tracking their illegal close: taking all funds"); + "Tracking their illegal close: taking all funds", + false); onchain_annotate_txin( &tx->txid, 0, TX_CHANNEL_UNILATERAL | TX_CHANNEL_CHEAT | TX_THEIRS); @@ -2980,7 +3128,7 @@ static void handle_their_unilateral(const struct tx_parts *tx, size_t i; struct htlcs_info *htlcs_info; - htlcs_info = init_reply(tx, "Tracking their unilateral close"); + htlcs_info = init_reply(tx, "Tracking their unilateral close", false); onchain_annotate_txin(&tx->txid, 0, TX_CHANNEL_UNILATERAL | TX_THEIRS); /* HSM can't derive this. */ @@ -3392,12 +3540,14 @@ static void handle_unknown_commitment(const struct tx_parts *tx, if (to_us_output == -1) { status_broken("FUNDS LOST. Unknown commitment #%"PRIu64"!", commit_num); - htlcs_info = init_reply(tx, "ERROR: FUNDS LOST. Unknown commitment!"); + htlcs_info = init_reply(tx, "ERROR: FUNDS LOST. Unknown commitment!", + false); } else { status_broken("ERROR: Unknown commitment #%"PRIu64 ", recovering our funds!", commit_num); - htlcs_info = init_reply(tx, "ERROR: Unknown commitment, recovering our funds!"); + htlcs_info = init_reply(tx, "ERROR: Unknown commitment, recovering our funds!", + false); } /* Tell master to give up on HTLCs immediately. */ diff --git a/onchaind/onchaind_wire.csv b/onchaind/onchaind_wire.csv index 3c9c76bbaf5a..f918ee439539 100644 --- a/onchaind/onchaind_wire.csv +++ b/onchaind/onchaind_wire.csv @@ -52,6 +52,8 @@ msgdata,onchaind_init,min_relay_feerate,u32, # This says we're ready; give us htlcs and preimages. msgtype,onchaind_init_reply,5101 msgdata,onchaind_init_reply,commit_num,u64, +# Is this our own commitment tx (otherwise it's one of theirs)? +msgdata,onchaind_init_reply,our_commitment,bool, #include msgtype,onchaind_htlcs,5002 @@ -60,6 +62,9 @@ msgdata,onchaind_htlcs,htlc,htlc_stub,num_htlcs # If it's not in the commitment tx, tell us (immediately or htlc_missing_depth) msgdata,onchaind_htlcs,tell_if_missing,bool,num_htlcs msgdata,onchaind_htlcs,tell_immediately,bool,num_htlcs +# Does lightningd believe this htlc is in this commitment tx? Stubs cover +# a range of commitments, so a removed htlc can look just like a live one. +msgdata,onchaind_htlcs,present,bool,num_htlcs # master->onchaind: Notifier that an output has been spent by input_num of tx. msgtype,onchaind_spent,5004 diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 1acff25e440b..8545d1e776ec 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -25,7 +25,7 @@ bool fromwire_onchaind_depth(const void *p UNNEEDED, struct bitcoin_txid *txid U bool fromwire_onchaind_dev_memleak(const void *p UNNEEDED) { fprintf(stderr, "fromwire_onchaind_dev_memleak called!\n"); abort(); } /* Generated stub for fromwire_onchaind_htlcs */ -bool fromwire_onchaind_htlcs(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct htlc_stub **htlc UNNEEDED, bool **tell_if_missing UNNEEDED, bool **tell_immediately UNNEEDED) +bool fromwire_onchaind_htlcs(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct htlc_stub **htlc UNNEEDED, bool **tell_if_missing UNNEEDED, bool **tell_immediately UNNEEDED, bool **present UNNEEDED) { fprintf(stderr, "fromwire_onchaind_htlcs called!\n"); abort(); } /* Generated stub for fromwire_onchaind_init */ bool fromwire_onchaind_init(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct shachain *shachain UNNEEDED, const struct chainparams **chainparams UNNEEDED, struct amount_sat *funding_amount_satoshi UNNEEDED, struct amount_msat *our_msat UNNEEDED, struct pubkey *old_remote_per_commitment_point UNNEEDED, struct pubkey *remote_per_commitment_point UNNEEDED, u32 *local_to_self_delay UNNEEDED, u32 *remote_to_self_delay UNNEEDED, struct amount_sat *local_dust_limit_satoshi UNNEEDED, struct bitcoin_txid *our_broadcast_txid UNNEEDED, u8 **local_scriptpubkey UNNEEDED, u8 **remote_scriptpubkey UNNEEDED, enum side *opener UNNEEDED, struct basepoints *local_basepoints UNNEEDED, struct basepoints *remote_basepoints UNNEEDED, struct tx_parts **tx_parts UNNEEDED, u32 *locktime UNNEEDED, u32 *tx_blockheight UNNEEDED, u32 *reasonable_depth UNNEEDED, struct bitcoin_signature **htlc_signature UNNEEDED, u32 *min_possible_feerate UNNEEDED, u32 *max_possible_feerate UNNEEDED, struct pubkey *local_funding_pubkey UNNEEDED, struct pubkey *remote_funding_pubkey UNNEEDED, u64 *local_static_remotekey_start UNNEEDED, u64 *remote_static_remotekey_start UNNEEDED, bool *option_anchor_outputs UNNEEDED, bool *option_anchors_zero_fee_htlc_tx UNNEEDED, u32 *min_relay_feerate UNNEEDED) @@ -67,7 +67,7 @@ u8 *towire_onchaind_extracted_preimage(const tal_t *ctx UNNEEDED, const struct p u8 *towire_onchaind_htlc_timeout(const tal_t *ctx UNNEEDED, const struct htlc_stub *htlc UNNEEDED) { fprintf(stderr, "towire_onchaind_htlc_timeout called!\n"); abort(); } /* Generated stub for towire_onchaind_init_reply */ -u8 *towire_onchaind_init_reply(const tal_t *ctx UNNEEDED, u64 commit_num UNNEEDED) +u8 *towire_onchaind_init_reply(const tal_t *ctx UNNEEDED, u64 commit_num UNNEEDED, bool our_commitment UNNEEDED) { fprintf(stderr, "towire_onchaind_init_reply called!\n"); abort(); } /* Generated stub for towire_onchaind_missing_htlc_output */ u8 *towire_onchaind_missing_htlc_output(const tal_t *ctx UNNEEDED, const struct htlc_stub *htlc UNNEEDED) diff --git a/openingd/common.c b/openingd/common.c index 7d1ea1844e13..e0b3c6999ea9 100644 --- a/openingd/common.c +++ b/openingd/common.c @@ -1,7 +1,6 @@ #include "config.h" #include #include -#include #include #include #include @@ -168,6 +167,67 @@ bool check_config_bounds(const tal_t *ctx, return true; } +bool check_dust_limit(const tal_t *ctx, + const struct channel_config *remoteconf, + const struct channel_config *localconf, + bool allow_dust_reserve, + char **err_reason) +{ + /* BOLT #2: + * + * The receiving node MUST fail the channel if: + *... + * - `dust_limit_satoshis` is smaller than `354 satoshis` (see [BOLT 3](03-transactions.md#dust-limits)). + */ + if (amount_sat_less(remoteconf->dust_limit, MIN_REMOTE_DUST_LIMIT)) { + *err_reason = tal_fmt(ctx, + "dust_limit_satoshis %s too small" + " (minimum %s)", + fmt_amount_sat(ctx, remoteconf->dust_limit), + fmt_amount_sat(ctx, MIN_REMOTE_DUST_LIMIT)); + return false; + } + + /* Everything we hold below their dust limit is trimmed from + * *their* commitment transaction and goes to miners when they + * force close. When they open, we derive the reserve from their + * dust limit, so without an absolute ceiling a peer could set it + * to half the channel, pay us up to just under it, and burn (or, + * with a miner, steal) our whole balance. */ + /* BOLT #2: + * + * The receiving node MAY fail the channel if: + *... + * - it considers `dust_limit_satoshis` too large (see [BOLT 3](03-transactions.md#dust-limits)). + */ + if (amount_sat_greater(remoteconf->dust_limit, MAX_REMOTE_DUST_LIMIT)) { + *err_reason = tal_fmt(ctx, + "dust_limit_satoshis %s too large" + " (maximum %s)", + fmt_amount_sat(ctx, remoteconf->dust_limit), + fmt_amount_sat(ctx, MAX_REMOTE_DUST_LIMIT)); + return false; + } + + /* BOLT #2: + * + * The receiving node MUST fail the channel if: + *... + * - `dust_limit_satoshis` is greater than `channel_reserve_satoshis`. + */ + if (!allow_dust_reserve + && amount_sat_greater(remoteconf->dust_limit, + localconf->channel_reserve)) { + *err_reason = tal_fmt(ctx, + "dust_limit_satoshis %s" + " would be above our reserve %s", + fmt_amount_sat(ctx, remoteconf->dust_limit), + fmt_amount_sat(ctx, localconf->channel_reserve)); + return false; + } + return true; +} + u8 *no_upfront_shutdown_script(const tal_t *ctx, bool developer, struct feature_set *our_features, diff --git a/openingd/common.h b/openingd/common.h index 443ee487eea8..532ce9ae76b4 100644 --- a/openingd/common.h +++ b/openingd/common.h @@ -2,12 +2,11 @@ #define LIGHTNING_OPENINGD_COMMON_H #include "config.h" +#include #include -struct amount_sat; struct bitcoin_tx; struct bitcoin_signature; -struct channel_config; bool check_config_bounds(const tal_t *ctx, @@ -20,6 +19,16 @@ bool check_config_bounds(const tal_t *ctx, bool option_anchors_zero_fee_htlc_tx, char **err_reason); +/* Check their dust_limit_satoshis is one we're willing to live with: + * not below MIN_REMOTE_DUST_LIMIT, not above MAX_REMOTE_DUST_LIMIT + * (see common/channel_config.h), and (unless allow_dust_reserve) not + * above the reserve we hold them to. */ +bool check_dust_limit(const tal_t *ctx, + const struct channel_config *remoteconf, + const struct channel_config *localconf, + bool allow_dust_reserve, + char **err_reason); + bool anchors_negotiated(struct feature_set *our_features, const u8 *their_features); diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 8dca662f11a8..962ed10b7b87 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -1988,8 +1988,13 @@ static bool run_tx_interactive(struct state *state, asset = amount_sat_to_asset(&amt, chainparams->fee_asset_tag); /* FIXME: persist nonces */ + /* The asset belongs to the input we just + * appended, not the prevtx output index: + * outpoint.n is bounded only by the peer's + * prevtx and can be well past our PSBT's + * inputs. */ psbt_elements_input_set_asset(psbt, - outpoint.n, + psbt->num_inputs - 1, &asset); } psbt_input_set_serial_id(psbt, in, serial_id); @@ -2571,7 +2576,6 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) * The receiving node MUST fail the channel if: *... * - the `channel_type` is not suitable. - * - the `channel_type` includes `option_zeroconf` and it does not trust the sender to open an unconfirmed channel. */ if (!open_tlv->channel_type) { negotiation_failed(state, @@ -2598,6 +2602,18 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) } } + /* BOLT #2: + * The receiving node MUST fail the channel if: + *... + * - the `channel_type` includes `option_zeroconf` and it does not trust the + * sender to open an unconfirmed channel. + */ + if (channel_type_has(state->channel_type, OPT_ZEROCONF) && + state->minimum_depth > 0) { + negotiation_failed(state, "You required zeroconf, but you're not on our allowlist"); + return; + } + /* The commitment feerate is a different matter: too low and the * commitment we are signing cannot be relayed when we need it. This * has to wait for channel_type above, since what counts as too low @@ -2795,7 +2811,9 @@ static void accepter_start(struct state *state, const u8 *oc2_msg) &tx_state->remoteconf, &tx_state->localconf, channel_type_has(state->channel_type, OPT_ANCHORS_ZERO_FEE_HTLC_TX), - &err_reason)) { + &err_reason) + || !check_dust_limit(tmpctx, &tx_state->remoteconf, + &tx_state->localconf, false, &err_reason)) { negotiation_failed(state, "%s", err_reason); return; } @@ -3449,7 +3467,9 @@ static void opener_start(struct state *state, u8 *msg) &tx_state->remoteconf, &tx_state->localconf, channel_type_has(state->channel_type, OPT_ANCHORS_ZERO_FEE_HTLC_TX), - &err_reason)) { + &err_reason) + || !check_dust_limit(tmpctx, &tx_state->remoteconf, + &tx_state->localconf, false, &err_reason)) { negotiation_failed(state, "%s", err_reason); return; } @@ -3758,7 +3778,9 @@ static void rbf_local_start(struct state *state, u8 *msg) &tx_state->remoteconf, &tx_state->localconf, channel_type_has(state->channel_type, OPT_ANCHORS_ZERO_FEE_HTLC_TX), - &err_reason)) { + &err_reason) + || !check_dust_limit(tmpctx, &tx_state->remoteconf, + &tx_state->localconf, false, &err_reason)) { open_abort(state, "%s", err_reason); return; } @@ -3916,7 +3938,9 @@ static void rbf_remote_start(struct state *state, const u8 *rbf_msg) &tx_state->remoteconf, &tx_state->localconf, channel_type_has(state->channel_type, OPT_ANCHORS_ZERO_FEE_HTLC_TX), - &err_reason)) { + &err_reason) + || !check_dust_limit(tmpctx, &tx_state->remoteconf, + &tx_state->localconf, false, &err_reason)) { negotiation_failed(state, "%s", err_reason); goto free_rbf_ctx; } diff --git a/openingd/openingd.c b/openingd/openingd.c index f7d3cfd3a647..a682594fc93a 100644 --- a/openingd/openingd.c +++ b/openingd/openingd.c @@ -446,14 +446,9 @@ static u8 *funder_channel_start(struct state *state, u8 channel_flags, fmt_channel_id(msg, &state->channel_id)); - if (!state->allowdustreserve && - amount_sat_greater(state->remoteconf.dust_limit, - state->localconf.channel_reserve)) { - negotiation_failed(state, - "dust limit %s" - " would be above our reserve %s", - fmt_amount_sat(tmpctx, state->remoteconf.dust_limit), - fmt_amount_sat(tmpctx, state->localconf.channel_reserve)); + if (!check_dust_limit(tmpctx, &state->remoteconf, &state->localconf, + state->allowdustreserve, &err_reason)) { + negotiation_failed(state, "%s", err_reason); return NULL; } @@ -980,7 +975,9 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) return NULL; } - /* This reserves 1% of the channel (rounded up) */ + /* This reserves 1% of the channel (rounded up), or their dust + * limit if that's higher: so check_dust_limit()'s ceiling is what + * actually bounds their dust limit here. */ set_reserve(state, state->remoteconf.dust_limit); /* Pending proposal to remove these limits. */ @@ -993,14 +990,9 @@ static u8 *fundee_channel(struct state *state, const u8 *open_channel_msg) * - MUST set `dust_limit_satoshis` less than or equal to * `channel_reserve_satoshis` from the `open_channel` message. */ - if (!state->allowdustreserve && - amount_sat_greater(state->remoteconf.dust_limit, - state->localconf.channel_reserve)) { - negotiation_failed(state, - "Our channel reserve %s" - " would be below their dust %s", - fmt_amount_sat(tmpctx, state->localconf.channel_reserve), - fmt_amount_sat(tmpctx, state->remoteconf.dust_limit)); + if (!check_dust_limit(tmpctx, &state->remoteconf, &state->localconf, + state->allowdustreserve, &err_reason)) { + negotiation_failed(state, "%s", err_reason); return NULL; } diff --git a/openingd/test/Makefile b/openingd/test/Makefile new file mode 100644 index 000000000000..505ddb95bf19 --- /dev/null +++ b/openingd/test/Makefile @@ -0,0 +1,14 @@ +# Note that these actually #include everything they need, except ccan/ and bitcoin/. +# That allows for unit testing of statics, and special effects. +OPENINGD_TEST_SRC := $(wildcard openingd/test/run-*.c) +OPENINGD_TEST_OBJS := $(OPENINGD_TEST_SRC:.c=.o) +OPENINGD_TEST_PROGRAMS := $(OPENINGD_TEST_OBJS:.o=) + +ALL_C_SOURCES += $(OPENINGD_TEST_SRC) +ALL_TEST_PROGRAMS += $(OPENINGD_TEST_PROGRAMS) + +$(OPENINGD_TEST_PROGRAMS): libcommon.a + +$(OPENINGD_TEST_OBJS): $(OPENINGD_HEADERS) $(OPENINGD_SRC) openingd/test/Makefile + +check-units: $(OPENINGD_TEST_PROGRAMS:%=unittest/%) diff --git a/openingd/test/run-check_dust_limit.c b/openingd/test/run-check_dust_limit.c new file mode 100644 index 000000000000..863cbb2c8b2c --- /dev/null +++ b/openingd/test/run-check_dust_limit.c @@ -0,0 +1,77 @@ +#include "config.h" +#include "../common.c" +#include +#include +#include + +/* AUTOGENERATED MOCKS START */ +/* Generated stub for fromwire_hsmd_validate_commitment_tx_reply */ +bool fromwire_hsmd_validate_commitment_tx_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct secret **old_commitment_secret UNNEEDED, struct pubkey *next_per_commitment_point UNNEEDED) +{ fprintf(stderr, "fromwire_hsmd_validate_commitment_tx_reply called!\n"); abort(); } +/* Generated stub for towire_hsmd_validate_commitment_tx */ +u8 *towire_hsmd_validate_commitment_tx(const tal_t *ctx UNNEEDED, const struct bitcoin_tx *tx UNNEEDED, const struct hsm_htlc *htlcs UNNEEDED, u64 commit_num UNNEEDED, u32 feerate UNNEEDED, const struct bitcoin_signature *sig UNNEEDED, const struct bitcoin_signature *htlc_sigs UNNEEDED) +{ fprintf(stderr, "towire_hsmd_validate_commitment_tx called!\n"); abort(); } +/* AUTOGENERATED MOCKS END */ + +/* Returns NULL if accepted, otherwise the error. */ +static const char *dust_err(u64 their_dust, u64 our_reserve, + bool allow_dust_reserve) +{ + struct channel_config remoteconf, localconf; + char *err; + + memset(&remoteconf, 0, sizeof(remoteconf)); + memset(&localconf, 0, sizeof(localconf)); + remoteconf.dust_limit = amount_sat(their_dust); + localconf.channel_reserve = amount_sat(our_reserve); + + if (check_dust_limit(tmpctx, &remoteconf, &localconf, + allow_dust_reserve, &err)) + return NULL; + return err; +} + +static bool dust_ok(u64 their_dust, u64 our_reserve) +{ + return dust_err(their_dust, our_reserve, false) == NULL; +} + +static bool dust_fails_with(u64 their_dust, u64 our_reserve, + bool allow_dust_reserve, const char *substr) +{ + const char *err = dust_err(their_dust, our_reserve, allow_dust_reserve); + return err && strstr(err, substr); +} + +int main(int argc, char *argv[]) +{ + common_setup(argv[0]); + + /* Normal cases. */ + assert(dust_ok(354, 10000)); + assert(dust_ok(546, 10000)); + assert(dust_ok(5000, 10000)); + assert(dust_ok(5000, 5000)); + + /* Above our reserve (#125): spec says MUST fail. */ + assert(dust_fails_with(1001, 1000, false, "would be above our reserve")); + /* ...unless we deliberately allow sub-dust reserves. */ + assert(dust_err(1001, 1000, true) == NULL); + + /* Below the BOLT #3 floor. */ + assert(dust_fails_with(353, 10000, false, "too small")); + assert(dust_fails_with(0, 10000, false, "too small")); + + /* Above our ceiling (#201): rejected even when the reserve has + * been inflated to cover it, and even with allow_dust_reserve. */ + assert(dust_fails_with(5001, 5001, false, "too large")); + assert(dust_fails_with(5001, 5001, true, "too large")); + assert(dust_fails_with(400000, 400000, false, "too large")); + assert(dust_fails_with(2100000000000000ULL, 10000, false, "too large")); + + /* The ceiling is reported before the reserve check. */ + assert(dust_fails_with(10001, 10000, false, "too large")); + + common_shutdown(); + return 0; +} diff --git a/plugins/bcli.c b/plugins/bcli.c index caa0618e79ce..0bba7b561379 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -491,7 +491,7 @@ static struct command_result *getchaininfo(struct command *cmd, return command_finished(cmd, response); } -/* Add a feerate, but don't publish one that bitcoind won't accept. */ +/* Add a feerate, respecting the floor within the supported u32 range. */ static void json_add_feerate(struct json_stream *result, const char *fieldname, struct command *cmd, u64 perkb_floor, @@ -543,6 +543,13 @@ static struct command_result *get_feerate_floor(struct command *cmd, return command_err(cmd, res, tal_fmt(tmpctx, "bad JSON: %s", err)); *perkb_floor = max_u64(mempoolfee, relayfee); + /* Bound the floor before it can also raise the per-target feerates. */ + if (*perkb_floor > 0xFFFFFFFF) { + plugin_log(cmd->plugin, LOG_UNUSUAL, + "Feerate floor %"PRIu64" is ridiculous: trimming to 32 bits", + *perkb_floor); + *perkb_floor = 0xFFFFFFFF; + } return NULL; } diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c index 40968e9c84a2..e299745dc10b 100644 --- a/plugins/bkpr/bookkeeper.c +++ b/plugins/bkpr/bookkeeper.c @@ -467,6 +467,30 @@ static struct command_result *do_dump_income(struct command *cmd, return command_finished(cmd, res); } +/* The CSV is written in our (network) directory, next to the node's own + * files, so only allow a plain *.csv filename there: no directory, no + * hidden name, nothing which could name one of those files. */ +static struct command_result *param_csv_filename(struct command *cmd, + const char *name, + const char *buffer, + const jsmntok_t *tok, + const char **filename) +{ + struct command_result *ret; + const char *f; + + ret = param_string(cmd, name, buffer, tok, &f); + if (ret) + return ret; + + if (strchr(f, '/') || f[0] == '.' || !strends(f, ".csv")) + return command_fail_badparam(cmd, name, buffer, tok, + "must be a filename ending in .csv," + " without a directory"); + *filename = f; + return NULL; +} + static struct command_result *json_dump_income(struct command *cmd, const char *buf, const jsmntok_t *params) @@ -475,7 +499,7 @@ static struct command_result *json_dump_income(struct command *cmd, if (!param(cmd, buf, params, p_req("csv_format", param_csv_format, &info->csv_fmt), - p_opt("csv_file", param_string, &info->filename), + p_opt("csv_file", param_csv_filename, &info->filename), p_opt_def("consolidate_fees", param_bool, &info->consolidate_fees, true), p_opt_def("start_time", param_u64, &info->start_time, 0), diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c index 470fc5d16eab..1e95ad367a25 100644 --- a/plugins/bkpr/incomestmt.c +++ b/plugins/bkpr/incomestmt.c @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include #include #include @@ -20,6 +22,7 @@ #include #include #include +#include #define ONCHAIN_FEE "onchain_fee" @@ -846,10 +849,18 @@ char *csv_print_income_events(const tal_t *ctx, struct income_event **evs) { FILE *csvf; - - csvf = fopen(filename, "w"); - if (!csvf) + int fd; + + /* Like fopen(.., "w"), but don't follow a symlink. */ + fd = open(filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW, 0666); + if (fd < 0) + return tal_fmt(ctx, "Failed to open csv file %s: %s", + filename, strerror(errno)); + csvf = fdopen(fd, "w"); + if (!csvf) { + close(fd); return tal_fmt(ctx, "Failed to open csv file %s", filename); + } csvfmt->emit_header(csvf); for (size_t i = 0; i < tal_count(evs); i++) diff --git a/plugins/commando.c b/plugins/commando.c index e7eb9b3a57d2..7a08b0930308 100644 --- a/plugins/commando.c +++ b/plugins/commando.c @@ -18,6 +18,9 @@ /* Specifically: bad/missing rune */ #define COMMANDO_ERROR_REMOTE_AUTH 0x4c51 +/* Real runes are tiny; rune decoding is quadratic in length, so refuse huge ones. */ +#define MAX_RUNE_LEN (64 * 1024) + enum commando_msgtype { /* Requests are split across multiple CONTINUES, then TERM. */ COMMANDO_MSG_CMD_CONTINUES = 0x4c4d, @@ -373,6 +376,9 @@ static struct command_result *try_command(struct command *cmd, struct rune *rune; struct out_req *req; + destroy_commando(incoming, &incoming_commands); + tal_del_destructor2(incoming, destroy_commando, &incoming_commands); + toks = json_parse_simple(incoming, buf, tal_bytelen(buf)); if (!toks) { return commando_error(cmd, incoming, COMMANDO_ERROR_REMOTE, @@ -404,6 +410,10 @@ static struct command_result *try_command(struct command *cmd, return commando_error(cmd, incoming, COMMANDO_ERROR_REMOTE, "Missing rune"); } + if (runetok->end - runetok->start > MAX_RUNE_LEN) { + return commando_error(cmd, incoming, COMMANDO_ERROR_REMOTE, + "Rune too long"); + } rune = rune_from_base64n(tmpctx, buf + runetok->start, runetok->end - runetok->start); if (!rune) { @@ -413,10 +423,6 @@ static struct command_result *try_command(struct command *cmd, /* Gather all the info we need to execute this command (steals toks). */ cinfo = new_cond_info(incoming, incoming, toks, method, params, id, filter); - /* Don't count this towards incomings anymore */ - destroy_commando(incoming, &incoming_commands); - tal_del_destructor2(incoming, destroy_commando, &incoming_commands); - req = jsonrpc_request_start(cmd, "checkrune", checkrune_done, checkrune_failed, cinfo); @@ -551,8 +557,9 @@ static struct command_result *handle_reply(struct node_id *peer, } result = json_get_member(replystr, toks, "result"); - if (!result) - return command_fail(ocmd->cmd, COMMANDO_ERROR_LOCAL, "Reply had no result"); + if (!result || result->type != JSMN_OBJECT) + return command_fail(ocmd->cmd, COMMANDO_ERROR_LOCAL, + "Reply had no valid result"); res = jsonrpc_stream_success(ocmd->cmd); diff --git a/plugins/fetchinvoice.c b/plugins/fetchinvoice.c index 48a52a051f39..185dc4cb572e 100644 --- a/plugins/fetchinvoice.c +++ b/plugins/fetchinvoice.c @@ -171,6 +171,11 @@ static struct command_result *handle_invreq_response(struct command *cmd, const char *badfield; u64 *expected_amount; const struct recurrence *recurrence; + u64 starttime, endtime; + u64 next_counter, next_period_idx; + u64 paywindow_start, paywindow_end; + bool have_next_period = false; + char *error; invtok = json_get_member(buf, om, "invoice"); if (!invtok) { @@ -294,6 +299,64 @@ static struct command_result *handle_invreq_response(struct command *cmd, goto badinv; } + /* CLN's recurrence policy */ + if (inv->invoice_recurrence_basetime && + *inv->invoice_recurrence_basetime > OFFER_RECURRENCE_MAX_BASETIME) { + plugin_log(cmd->plugin, LOG_DBG, + "invoice_recurrence_basetime (%" PRIu64 + ") exceeds CLN maximum %" PRIu64, + *inv->invoice_recurrence_basetime, + (u64)OFFER_RECURRENCE_MAX_BASETIME); + badfield = "invoice_recurrence_basetime"; + goto badinv; + } + + error = invoice_check_recurrence_policy(tmpctx, inv); + if (error) { + plugin_log(cmd->plugin, LOG_DBG, + "invoice recurrence policy check failed: %s", error); + badfield = "recurrence"; + goto badinv; + } + + if (recurrence) { + next_counter = *inv->invreq_recurrence_counter + 1; + + if (inv->invreq_recurrence_start) + next_period_idx = *inv->invreq_recurrence_start + + next_counter; + else + next_period_idx = next_counter; + + if (!inv->offer_recurrence_limit || + next_period_idx <= *inv->offer_recurrence_limit) { + have_next_period = true; + + if (!offer_period_start( + &starttime, *inv->invoice_recurrence_basetime, + next_period_idx, recurrence) || + !offer_period_start( + &endtime, *inv->invoice_recurrence_basetime, + next_period_idx + 1, recurrence)) { + badfield = "invoice_recurrence_basetime or " + "next_period_idx"; + goto badinv; + } + endtime -= 1; + + if (!offer_period_paywindow( + recurrence, inv->offer_recurrence_paywindow, + inv->offer_recurrence_base, + *inv->invoice_recurrence_basetime, + next_period_idx, &paywindow_start, + &paywindow_end)) { + badfield = "invoice_recurrence_basetime or " + "next_period_idx"; + goto badinv; + } + } + } + out = jsonrpc_stream_success(sent->cmd); json_add_string(out, "invoice", invoice_encode(tmpctx, inv)); json_object_start(out, "changes"); @@ -311,36 +374,14 @@ static struct command_result *handle_invreq_response(struct command *cmd, /* We tell them about next period at this point, if any. */ if (recurrence) { - u64 next_counter, next_period_idx; - u64 paywindow_start, paywindow_end; - - next_counter = *inv->invreq_recurrence_counter + 1; - if (inv->invreq_recurrence_start) - next_period_idx = *inv->invreq_recurrence_start - + next_counter; - else - next_period_idx = next_counter; - /* If this was the last, don't tell them about a next! */ - if (!inv->offer_recurrence_limit - || next_period_idx <= *inv->offer_recurrence_limit) { + if (have_next_period) { json_object_start(out, "next_period"); json_add_u64(out, "counter", next_counter); - json_add_u64(out, "starttime", - offer_period_start(*inv->invoice_recurrence_basetime, - next_period_idx, - recurrence)); - json_add_u64(out, "endtime", - offer_period_start(*inv->invoice_recurrence_basetime, - next_period_idx + 1, - recurrence) - 1); - - offer_period_paywindow(recurrence, - inv->offer_recurrence_paywindow, - inv->offer_recurrence_base, - *inv->invoice_recurrence_basetime, - next_period_idx, - &paywindow_start, &paywindow_end); + + json_add_u64(out, "starttime", starttime); + json_add_u64(out, "endtime", endtime); + json_add_u64(out, "paywindow_start", paywindow_start); json_add_u64(out, "paywindow_end", paywindow_end); json_object_end(out); @@ -865,6 +906,7 @@ struct command_result *json_fetchinvoice(struct command *cmd, u32 *timeout; u64 *quantity; u32 *recurrence_counter, *recurrence_start; + char *error; if (!param_check(cmd, buffer, params, p_req("offer", param_offer, &sent->offer), @@ -882,6 +924,13 @@ struct command_result *json_fetchinvoice(struct command *cmd, NULL)) return command_param_failed(); + error = offer_check_recurrence_policy(tmpctx, sent->offer); + if (error) { + return command_fail( + cmd, JSONRPC2_INVALID_PARAMS, + "offer doesn't satisfy CLN's recurrence policy: %s", error); + } + sent->wait_timeout = *timeout; sent->their_paths = sent->offer->offer_paths; if (sent->their_paths) @@ -906,6 +955,32 @@ struct command_result *json_fetchinvoice(struct command *cmd, return command_fail(cmd, OFFER_EXPIRED, "Offer expired"); } + if (recurrence_counter && + *recurrence_counter > OFFER_RECURRENCE_MAX_PERIOD_IDX) { + return command_fail( + cmd, JSONRPC2_INVALID_PARAMS, + "recurrence_counter %u exceeds CLN maximum %" PRIu64, + *recurrence_counter, (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + } + if (recurrence_start && + *recurrence_start > OFFER_RECURRENCE_MAX_PERIOD_IDX) { + return command_fail( + cmd, JSONRPC2_INVALID_PARAMS, + "recurrence_start %u exceeds CLN maximum %" PRIu64, + *recurrence_start, (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + } + if (recurrence_counter && recurrence_start) { + u64 period_idx = *recurrence_counter + (u64)*recurrence_start; + if (period_idx > OFFER_RECURRENCE_MAX_PERIOD_IDX) { + return command_fail( + cmd, JSONRPC2_INVALID_PARAMS, + "recurrence_start (%u) + recurrence_counter (%u) " + "exceeds CLN maximum %" PRIu64, + *recurrence_start, *recurrence_counter, + (u64)OFFER_RECURRENCE_MAX_PERIOD_IDX); + } + } + /* BOLT #12: * The writer: * - if it is responding to an offer: @@ -1015,6 +1090,14 @@ struct command_result *json_fetchinvoice(struct command *cmd, = recurrence_invreq_metadata(invreq, invreq, &od->nodealias_base, rec_label); + + error = invreq_check_recurrence_policy(tmpctx, invreq); + if (error) { + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "invoice request doesn't satisfy " + "CLN's recurrence policy: %s", + error); + } } else { /* BOLT-recurrence #12: * - otherwise: @@ -1084,6 +1167,14 @@ struct command_result *json_fetchinvoice(struct command *cmd, strlen(payer_note), 0); + error = invreq_check_recurrence_policy(tmpctx, invreq); + if (error) { + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Invoice request does not satisfy CLN " + "recurrence policy: %s", + error); + } + /* If only checking, we're done now */ if (command_check_only(cmd)) return command_check_done(cmd); diff --git a/plugins/funder.c b/plugins/funder.c index 6ce91b6959de..cee6b9ab6616 100644 --- a/plugins/funder.c +++ b/plugins/funder.c @@ -22,6 +22,12 @@ /* In-progress channel opens */ static struct list_head pending_opens; +/* openchannel2 and rbf_channel requests evaluate policy against the wallet + * one at a time, so each sees the utxos reserved by the one before. The head + * is in progress (from listfunds until fundpsbt/utxopsbt returns), the rest + * wait. */ +static struct list_head funding_queue; + /* Current set policy */ static struct funder_policy *current_policy; @@ -386,6 +392,10 @@ json_openchannel2_changed_call(struct command *cmd, /* Tiny struct to pass info to callback for fundpsbt */ struct open_info { + /* Our entry in funding_queue */ + struct list_node list; + struct command *cmd; + struct channel_id cid; struct node_id id; struct amount_sat our_funding; @@ -421,6 +431,35 @@ static struct open_info *new_open_info(const tal_t *ctx) return info; } +static struct command_result *send_listfunds(struct command *cmd, + struct open_info *info); + +static struct command_result *funding_queue_join(struct command *cmd, + struct open_info *info) +{ + bool idle = list_empty(&funding_queue); + + info->cmd = cmd; + list_add_tail(&funding_queue, &info->list); + if (!idle) + return command_still_pending(cmd); + + return send_listfunds(cmd, info); +} + +/* Our utxos are reserved (or we're not funding): let the next one go. */ +static void funding_queue_done(struct open_info *info) +{ + struct open_info *next; + + /* Only the head of the queue gets this far. */ + assert(list_top(&funding_queue, struct open_info, list) == info); + list_del_from(&funding_queue, &info->list); + next = list_top(&funding_queue, struct open_info, list); + if (next) + send_listfunds(next->cmd, next); +} + static struct command_result * psbt_funded(struct command *cmd, const char *method, @@ -464,6 +503,7 @@ psbt_funded(struct command *cmd, if (current_policy->rates && !amount_sat_is_zero(info->requested_lease)) json_add_lease_rates(response, current_policy->rates); + funding_queue_done(info); return command_finished(cmd, response); } @@ -486,6 +526,7 @@ psbt_fund_failed(struct command *cmd, json_tok_full_len(error), json_tok_full(buf, error)); + funding_queue_done(info); return command_hook_success(cmd); } @@ -712,8 +753,10 @@ listfunds_success(struct command *cmd, fmt_amount_sat(tmpctx, info->our_funding), funding_err ? funding_err : ""); - if (amount_sat_is_zero(info->our_funding)) + if (amount_sat_is_zero(info->our_funding)) { + funding_queue_done(info); return command_hook_success(cmd); + } plugin_log(cmd->plugin, LOG_DBG, "Funding channel %s with %s (their input %s)", @@ -730,8 +773,8 @@ listfunds_success(struct command *cmd, committed_funds, avail_utxos); json_add_bool(req->js, "reservedok", true); - /* We don't re-reserve any UTXOS :) */ - json_add_num(req->js, "reserve", 0); + /* Reserve like fundpsbt: new utxos mustn't look free to the + * next open, and unreserve_psbt() undoes the whole psbt. */ } else { req = jsonrpc_request_start(cmd, "fundpsbt", @@ -777,9 +820,23 @@ listfunds_failed(struct command *cmd, json_tok_full_len(error), json_tok_full(buf, error)); + funding_queue_done(info); return command_hook_success(cmd); } +static struct command_result *send_listfunds(struct command *cmd, + struct open_info *info) +{ + struct out_req *req; + + req = jsonrpc_request_start(cmd, + "listfunds", + &listfunds_success, + &listfunds_failed, + info); + return send_outreq(req); +} + static struct command_result * json_openchannel2_call(struct command *cmd, const char *buf, @@ -792,7 +849,6 @@ json_openchannel2_call(struct command *cmd, u32 to_self_delay, max_accepted_htlcs; u16 channel_flags; const char *err; - struct out_req *req; err = json_scan(tmpctx, buf, params, "{openchannel2:" @@ -903,13 +959,7 @@ json_openchannel2_call(struct command *cmd, } /* Figure out what our funds are */ - req = jsonrpc_request_start(cmd, - "listfunds", - &listfunds_success, - &listfunds_failed, - info); - - return send_outreq(req); + return funding_queue_join(cmd, info); } static struct command_result * @@ -919,8 +969,6 @@ datastore_list_fail(struct command *cmd, const jsmntok_t *error, struct open_info *info) { - struct out_req *req; - /* Oops, something's broken */ plugin_log(cmd->plugin, LOG_BROKEN, "`datastore` list failed: %.*s", @@ -929,12 +977,7 @@ datastore_list_fail(struct command *cmd, /* Figure out what our funds are... same flow * as with openchannel2 callback. */ - req = jsonrpc_request_start(cmd, - "listfunds", - &listfunds_success, - &listfunds_failed, - info); - return send_outreq(req); + return funding_queue_join(cmd, info); } static struct command_result * @@ -944,7 +987,6 @@ datastore_list_success(struct command *cmd, const jsmntok_t *result, struct open_info *info) { - struct out_req *req; const char *key, *err; const u8 *utxos_bin; size_t len, i; @@ -999,12 +1041,7 @@ datastore_list_success(struct command *cmd, tal_arr_expand(&info->prev_outs, outpoint); } - req = jsonrpc_request_start(cmd, - "listfunds", - &listfunds_success, - &listfunds_failed, - info); - return send_outreq(req); + return funding_queue_join(cmd, info); } /* Peer has asked us to RBF */ @@ -1201,8 +1238,10 @@ static struct command_result *json_channel_open_failed(struct command *cmd, if (open) unreserve_psbt(cmd, open); - /* Also clean up datastore for this channel */ - return delete_channel_from_datastore(cmd, &cid); + /* Keep the channel's saved utxos: a failed RBF attempt also ends up + * here, and the next RBF still builds on them. They're removed when + * the channel leaves AWAITING_LOCKIN (json_channel_state_changed). */ + return notification_handled(cmd); } static void json_add_policy(struct json_stream *stream, @@ -1499,6 +1538,7 @@ static void tell_lightningd_lease_rates(struct command *init_cmd, static void memleak_mark(struct plugin *p, struct htable *memtable) { memleak_scan_list_head(memtable, &pending_opens); + memleak_scan_list_head(memtable, &funding_queue); memleak_scan_obj(memtable, current_policy); } @@ -1508,6 +1548,7 @@ static const char *init(struct command *init_cmd, const char *b, const jsmntok_t struct amount_msat msat; list_head_init(&pending_opens); + list_head_init(&funding_queue); err = funder_check_policy(current_policy); if (err) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 2527ec9cc0a9..8b6eec15668a 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1392,6 +1392,18 @@ handle_final_failure(struct command *cmd, } +/* gossipd ignores updates for dying channels: don't retry with stale ones */ +static void disable_if_dying(struct payment *p, const struct route_hop *errchan) +{ + const struct gossmap *gossmap = get_raw_gossmap(p); + const struct gossmap_chan *c = gossmap_find_chan(gossmap, &errchan->scid); + + if (c && gossmap_chan_is_dying(gossmap, c)) + channel_hints_update(p, errchan->scid, errchan->direction, + false, false, NULL, errchan->capacity, + NULL); +} + static struct command_result * handle_intermediate_failure(struct command *cmd, struct payment *p, @@ -1460,6 +1472,7 @@ handle_intermediate_failure(struct command *cmd, case WIRE_INCORRECT_CLTV_EXPIRY: report_tampering(p, errchan - p->route, "cltv"); + disable_if_dying(p, errchan); goto error; case WIRE_INVALID_ONION_VERSION: @@ -1475,8 +1488,11 @@ handle_intermediate_failure(struct command *cmd, case WIRE_AMOUNT_BELOW_MINIMUM: case WIRE_FEE_INSUFFICIENT: - case WIRE_EXPIRY_TOO_FAR: case WIRE_EXPIRY_TOO_SOON: + disable_if_dying(p, errchan); + goto error; + + case WIRE_EXPIRY_TOO_FAR: goto error; } diff --git a/plugins/offers_invreq_hook.c b/plugins/offers_invreq_hook.c index e54013a2a24c..e78461d03ba3 100644 --- a/plugins/offers_invreq_hook.c +++ b/plugins/offers_invreq_hook.c @@ -531,11 +531,17 @@ static struct command_result *check_period(struct command *cmd, period_idx); } - offer_period_paywindow(invreq_recurrence(ir->invreq), - ir->invreq->offer_recurrence_paywindow, - ir->invreq->offer_recurrence_base, - basetime, period_idx, - &paywindow_start, &paywindow_end); + if (!offer_period_paywindow(invreq_recurrence(ir->invreq), + ir->invreq->offer_recurrence_paywindow, + ir->invreq->offer_recurrence_base, basetime, + period_idx, &paywindow_start, + &paywindow_end)) { + return fail_invreq(cmd, ir, + "pay window computation overflows, some " + "values may be too big: basetime=%" PRIu64 + ", period_idx=%" PRIu64 "", + basetime, period_idx); + } if (*ir->inv->invoice_created_at < paywindow_start) { return fail_invreq(cmd, ir, "period_index %"PRIu64 @@ -563,11 +569,18 @@ static struct command_result *check_period(struct command *cmd, */ if (ir->invreq->offer_recurrence_base && ir->invreq->offer_recurrence_base->proportional_amount == 1) { - u64 start = offer_period_start(basetime, period_idx, - invreq_recurrence(ir->invreq)); - u64 end = offer_period_start(basetime, period_idx + 1, - invreq_recurrence(ir->invreq)); - + u64 start, end; + if (!offer_period_start(&start, basetime, period_idx, + invreq_recurrence(ir->invreq)) || + !offer_period_start(&end, basetime, period_idx + 1, + invreq_recurrence(ir->invreq))) { + return fail_invreq(cmd, ir, + "basetime %" PRIu64 + " and period_index %" PRIu64 + " are too great, the period start " + "computation overflows", + basetime, period_idx); + } if (end <= start) { return fail_invreq(cmd, ir, "period_index %"PRIu64" bad period", @@ -1199,6 +1212,7 @@ struct command_result *handle_invoice_request(struct command *cmd, size_t len = tal_count(invreqbin); const u8 *cursor = invreqbin; struct invreq *ir = tal(cmd, struct invreq); + char *err; ir->reply_path = tal_steal(ir, reply_path); ir->secret = tal_dup_or_null(ir, struct secret, secret); @@ -1290,6 +1304,15 @@ struct command_result *handle_invoice_request(struct command *cmd, return fail_invreq(cmd, ir, "Not based on an offer"); } + /* CLN policy: reject absurd recurrence values from remote peers early. + */ + err = invreq_check_recurrence_policy(tmpctx, ir->invreq); + if (err) { + return fail_invreq( + cmd, ir, + "invreq doesn't satisfy CLN's recurrence policy: %s", err); + } + /* BOLT #12: * * - if `offer_issuer_id` or `offer_paths` are present (response to an offer): diff --git a/plugins/offers_offer.c b/plugins/offers_offer.c index 0bd87ad726ba..85f9decd5978 100644 --- a/plugins/offers_offer.c +++ b/plugins/offers_offer.c @@ -83,7 +83,7 @@ static struct command_result *param_recurrence(struct command *cmd, const jsmntok_t *tok, struct recurrence **recurrence) { - u32 mul; + u32 mul, period; const struct time_string *ts; ts = json_to_time(buffer, tok, &mul); @@ -91,9 +91,14 @@ static struct command_result *param_recurrence(struct command *cmd, return command_fail_badparam(cmd, name, buffer, tok, "not a valid time"); + if (!assign_overflow_u32(&period, ts->mul * (u64)mul)) + return command_fail_badparam( + cmd, name, buffer, tok, + "period is too large to fit in 32 bits"); + *recurrence = tal(cmd, struct recurrence); (*recurrence)->time_unit = ts->unit; - (*recurrence)->period = ts->mul * mul; + (*recurrence)->period = period; return NULL; } @@ -422,6 +427,7 @@ struct command_result *json_offer(struct command *cmd, struct offer_info *offinfo = tal(cmd, struct offer_info); struct path **paths; bool *proportional, *optional_recurrence; + char *error; offinfo->offer = offer = tlv_offer_new(offinfo); @@ -527,6 +533,13 @@ struct command_result *json_offer(struct command *cmd, offer->offer_recurrence_compulsory = NULL; } + error = offer_check_recurrence_policy(tmpctx, offer); + if (error) { + return command_fail( + cmd, JSONRPC2_INVALID_PARAMS, + "offer doesn't satisfy CLN's recurrence policy: %s", error); + } + if (command_check_only(cmd)) return command_check_done(cmd); diff --git a/plugins/src/lib.rs b/plugins/src/lib.rs index ddc79c4c0666..50dfb25a6859 100644 --- a/plugins/src/lib.rs +++ b/plugins/src/lib.rs @@ -23,6 +23,7 @@ use crate::codec::{JsonCodec, JsonRpcCodec}; pub use anyhow::anyhow; use anyhow::{Context, Result}; +use futures::FutureExt; use futures::sink::SinkExt; use serde::Serialize; use serde::de::DeserializeOwned; @@ -250,7 +251,8 @@ where /// Add a hook subscription for `hookname` with typed request and response. The request is /// deserialized from JSON into `Req` and the response is serialized from `Resp` back to JSON - /// automatically. If deserialization of the request fails, the hook returns an error to CLN. + /// automatically. If deserialization of the request fails, an error is returned and handled + /// according to the hook fallback policy. /// Use [`Builder::hook_from_builder`] with [`HookBuilder::new_typed`] if you additionally need /// to configure `before`, `after`, or `filters`. pub fn hook_typed(mut self, hookname: &str, callback: C) -> Self @@ -267,19 +269,18 @@ where Hook { name: hookname.clone(), callback: Box::new(move |p, r| { - let typed_req = serde_json::from_value(r).unwrap_or_else(|e| { - let error = format!( - "cln-plugin: hook '{hookname}' received a request that doesn't match \ - the expected schema. Error: {e}" - ); - println!( - "{}", - serde_json::json!({"jsonrpc": "2.0", - "method": "log", - "params": {"level":"warn", "message":error}}) - ); - std::process::exit(1); - }); + let hookname = hookname.clone(); + let typed_req = match serde_json::from_value(r) { + Ok(req) => req, + Err(e) => { + let error = format!( + "cln-plugin: hook '{hookname}' received a request that doesn't \ + match the expected schema. Error: {e}" + ); + log::warn!("{error}"); + return Box::pin(async move { Err(anyhow!("{error}")) }); + } + }; let fut = callback(p, typed_req); Box::pin(async move { let typed_resp = fut.await?; @@ -427,11 +428,10 @@ where } }; - // TODO Split the two hashmaps once we fill in the hook - // payload structs in messages.rs - let mut rpcmethods: HashMap> = + let rpcmethods: HashMap> = HashMap::from_iter(self.rpcmethods.drain().map(|(k, v)| (k, v.callback))); - rpcmethods.extend(self.hooks.drain().map(|(k, v)| (k, v.callback))); + let hooks: HashMap> = + HashMap::from_iter(self.hooks.drain().map(|(k, v)| (k, v.callback))); let subscriptions = HashMap::from_iter(self.subscriptions.drain().map(|(k, v)| (k, v.callback))); @@ -452,7 +452,7 @@ where options: self.options, option_values: self.option_values, configuration, - hooks: HashMap::new(), + hooks, })) } @@ -584,19 +584,18 @@ where Self { name: hookname.clone(), callback: Box::new(move |p, r| { - let typed_req = serde_json::from_value(r).unwrap_or_else(|e| { - let error = format!( - "cln-plugin: hook '{hookname}' received a request that doesn't match \ - the expected schema. Error: {e}" - ); - println!( - "{}", - serde_json::json!({"jsonrpc": "2.0", - "method": "log", - "params": {"level":"warn", "message":error}}) - ); - std::process::exit(1); - }); + let hookname = hookname.clone(); + let typed_req = match serde_json::from_value(r) { + Ok(req) => req, + Err(e) => { + let error = format!( + "cln-plugin: hook '{hookname}' received a request that doesn't match \ + the expected schema. Error: {e}" + ); + log::warn!("{error}"); + return Box::pin(async move { Err(anyhow!("{error}")) }); + } + }; let fut = callback(p, typed_req); Box::pin(async move { let typed_resp = fut.await?; @@ -895,6 +894,29 @@ where } } +fn hook_fallback(hookname: &str) -> Option { + match hookname { + "htlc_accepted" => Some(json!({ + "result": "fail", + "failure_message": "2002" + })), + "peer_connected" + | "invoice_payment" + | "rpc_command" + | "custommsg" + | "onion_message_recv" + | "onion_message_recv_secret" + | "recover" => Some(json!({"result": "continue"})), + "openchannel" + | "openchannel2" + | "openchannel2_changed" + | "openchannel2_sign" + | "rbf_channel" => Some(json!({"result": "reject"})), + // commitment_revocation and db_write cannot recover from failing, let lightningd crash + _ => None, + } +} + impl PluginDriver where S: Send + Clone, @@ -961,46 +983,147 @@ where .context("Missing 'method' in request")? .as_str() .context("'method' is not a string")?; - let callback = match method { - name if name.eq("setconfig") => { - self.setconfig_callback.as_ref().ok_or_else(|| { - anyhow!("No handler for method '{}' registered", method) - })? - } - _ => self.rpcmethods.get(method).with_context(|| { - anyhow!("No handler for method '{}' registered", method) - })?, - }; let params = request .get("params") .context("Missing 'params' field in request")? .clone(); let plugin = plugin.clone(); - let call = callback(plugin.clone(), params); - - tokio::spawn(async move { - match call.await { - Ok(v) => plugin - .sender - .send(json!({ - "jsonrpc": "2.0", - "id": id, - "result": v - })) - .await - .context("returning custom response"), - Err(e) => plugin - .sender - .send(json!({ - "jsonrpc": "2.0", - "id": id, - "error": parse_error(&e), - })) - .await - .context("returning custom error"), - } - }); + let method_str = method.to_string(); + + if let Some(callback) = self.hooks.get(method) { + let call = callback(plugin.clone(), params); + tokio::spawn(async move { + let res = std::panic::AssertUnwindSafe(call).catch_unwind().await; + match res { + Ok(Ok(v)) => { + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "result": v + })) + .await + .context("returning custom response"); + } + Ok(Err(e)) => { + if let Some(fallback) = hook_fallback(&method_str) { + log::error!( + "Hook handler for '{method_str}' failed with \ + error: {e}. Returning safe fallback response to \ + avoid crashing the main daemon." + ); + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "result": fallback + })) + .await + .context("returning hook fallback response"); + } else { + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "error": parse_error(&e), + })) + .await + .context("returning custom error"); + } + } + Err(_) => { + if let Some(fallback) = hook_fallback(&method_str) { + log::error!( + "Hook handler for '{method_str}' panicked. \ + Returning safe fallback response to avoid hanging \ + the main daemon." + ); + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "result": fallback + })) + .await + .context("returning hook fallback response"); + } else { + log::error!( + "Hook handler for '{method_str}' panicked." + ); + let err = + anyhow!("Hook handler for '{method_str}' panicked"); + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "error": parse_error(&err), + })) + .await + .context("returning hook panic error"); + } + } + } + }); + } else { + let callback = match method { + name if name.eq("setconfig") => { + self.setconfig_callback.as_ref().ok_or_else(|| { + anyhow!("No handler for method '{method}' registered") + })? + } + _ => self.rpcmethods.get(method).with_context(|| { + anyhow!("No handler for method '{method}' registered") + })?, + }; + + let call = callback(plugin.clone(), params); + tokio::spawn(async move { + let res = std::panic::AssertUnwindSafe(call).catch_unwind().await; + match res { + Ok(Ok(v)) => { + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "result": v + })) + .await + .context("returning custom response"); + } + Ok(Err(e)) => { + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "error": parse_error(&e), + })) + .await + .context("returning custom error"); + } + Err(_) => { + log::error!("RPC handler for '{method_str}' panicked."); + let err = anyhow!("RPC method '{method_str}' panicked"); + let _ = plugin + .sender + .send(json!({ + "jsonrpc": "2.0", + "id": id, + "error": parse_error(&err), + })) + .await + .context("returning rpc panic error"); + } + } + }); + } Ok(()) } messages::JsonRpc::CustomNotification(request) => { @@ -1018,17 +1141,20 @@ where // Send to notification to the wildcard // subscription "*" it it exists - match &self.wildcard_subscription { - Some(cb) => { - let call = cb(plugin.clone(), params.clone()); - tokio::spawn(async move { - if let Err(e) = call.await { - log::warn!("Wildcard notification handler error: '{}'", e) + if let Some(cb) = &self.wildcard_subscription { + let call = cb(plugin.clone(), params.clone()); + tokio::spawn(async move { + match std::panic::AssertUnwindSafe(call).catch_unwind().await { + Ok(Err(e)) => { + log::warn!("Wildcard notification handler error: '{e}'"); } - }); - } - None => {} - }; + Err(_) => { + log::error!("Wildcard notification handler panicked"); + } + Ok(Ok(())) => {} + } + }); + } // Find the appropriate callback and process it // We'll log a warning if no handler is defined @@ -1036,8 +1162,16 @@ where Some(cb) => { let call = cb(plugin.clone(), params.clone()); tokio::spawn(async move { - if let Err(e) = call.await { - log::warn!("Notification handler error: '{}'", e) + match std::panic::AssertUnwindSafe(call).catch_unwind().await { + Ok(Err(e)) => { + log::warn!("Notification handler error: '{e}'"); + } + Err(_) => { + log::error!( + "Notification handler error: handler panicked" + ); + } + Ok(Ok(())) => {} } }); } @@ -1181,4 +1315,275 @@ mod test { ); assert_eq!(rpc_error.data, None); } + + fn dummy_configuration() -> Configuration { + Configuration { + lightning_dir: "".into(), + rpc_file: "".into(), + startup: false, + network: "".into(), + feature_set: HashMap::new(), + proxy: None, + torv3_enabled: None, + always_use_proxy: None, + } + } + + #[tokio::test] + async fn test_hook_error_returns_safe_fallback() { + let (mut client_write, server_read) = tokio::io::duplex(1024); + let mut framed_in = FramedRead::new(server_read, JsonRpcCodec::default()); + let (sender, mut receiver) = tokio::sync::mpsc::channel(4); + let (wait_handle, _) = tokio::sync::broadcast::channel(1); + + let plugin = Plugin { + state: (), + options: HashMap::new(), + option_values: Arc::new(std::sync::Mutex::new(HashMap::new())), + configuration: dummy_configuration(), + wait_handle, + sender, + }; + + let mut hooks: HashMap> = HashMap::new(); + hooks.insert( + "openchannel2".to_string(), + Box::new(|_p, _r| Box::pin(async { Err(anyhow!("something broke in openchannel2")) })), + ); + + let driver = PluginDriver { + plugin: plugin.clone(), + rpcmethods: HashMap::new(), + setconfig_callback: None, + hooks, + subscriptions: HashMap::new(), + wildcard_subscription: None, + }; + + client_write + .write_all(b"{\"jsonrpc\": \"2.0\", \"id\": 42, \"method\": \"openchannel2\", \"params\": {}}\n\n") + .await + .unwrap(); + + driver + .dispatch_one(&mut framed_in, &driver.plugin) + .await + .unwrap(); + + let response = receiver.recv().await.unwrap(); + assert_eq!(response["id"], 42); + assert_eq!(response["result"], json!({"result": "reject"})); + } + + #[tokio::test] + async fn test_hook_panic_returns_safe_fallback() { + let (mut client_write, server_read) = tokio::io::duplex(1024); + let mut framed_in = FramedRead::new(server_read, JsonRpcCodec::default()); + let (sender, mut receiver) = tokio::sync::mpsc::channel(4); + let (wait_handle, _) = tokio::sync::broadcast::channel(1); + + let plugin = Plugin { + state: (), + options: HashMap::new(), + option_values: Arc::new(std::sync::Mutex::new(HashMap::new())), + configuration: dummy_configuration(), + wait_handle, + sender, + }; + + let mut hooks: HashMap> = HashMap::new(); + hooks.insert( + "htlc_accepted".to_string(), + Box::new(|_p, _r| { + Box::pin(async { + panic!("hook handler explicitly panicked!"); + }) + }), + ); + + let driver = PluginDriver { + plugin: plugin.clone(), + rpcmethods: HashMap::new(), + setconfig_callback: None, + hooks, + subscriptions: HashMap::new(), + wildcard_subscription: None, + }; + + client_write + .write_all(b"{\"jsonrpc\": \"2.0\", \"id\": 99, \"method\": \"htlc_accepted\", \"params\": {}}\n\n") + .await + .unwrap(); + + driver + .dispatch_one(&mut framed_in, &driver.plugin) + .await + .unwrap(); + + let response = receiver.recv().await.unwrap(); + assert_eq!(response["id"], 99); + assert_eq!( + response["result"], + json!({"result": "fail", "failure_message": "2002"}) + ); + } + + #[tokio::test] + async fn test_typed_hook_schema_mismatch_fallback() { + #[derive(serde::Deserialize)] + struct ExpectedPayload { + #[allow(dead_code)] + mandatory_field: String, + } + + #[derive(serde::Serialize)] + struct DummyResponse { + result: String, + } + + let builder = Builder::new(tokio::io::empty(), tokio::io::sink()).hook_typed( + "peer_connected", + |_p: Plugin<()>, _req: ExpectedPayload| async move { + Ok(DummyResponse { + result: "continue".to_string(), + }) + }, + ); + + let (mut client_write, server_read) = tokio::io::duplex(1024); + let mut framed_in = FramedRead::new(server_read, JsonRpcCodec::default()); + let (sender, mut receiver) = tokio::sync::mpsc::channel(4); + let (wait_handle, _) = tokio::sync::broadcast::channel(1); + + let plugin = Plugin { + state: (), + options: HashMap::new(), + option_values: Arc::new(std::sync::Mutex::new(HashMap::new())), + configuration: dummy_configuration(), + wait_handle, + sender, + }; + + let hooks: HashMap> = + HashMap::from_iter(builder.hooks.into_iter().map(|(k, v)| (k, v.callback))); + + let driver = PluginDriver { + plugin: plugin.clone(), + rpcmethods: HashMap::new(), + setconfig_callback: None, + hooks, + subscriptions: HashMap::new(), + wildcard_subscription: None, + }; + + // Send invalid payload (missing mandatory_field) + client_write + .write_all(b"{\"jsonrpc\": \"2.0\", \"id\": 101, \"method\": \"peer_connected\", \"params\": {}}\n\n") + .await + .unwrap(); + + driver + .dispatch_one(&mut framed_in, &driver.plugin) + .await + .unwrap(); + + let response = receiver.recv().await.unwrap(); + assert_eq!(response["id"], 101); + assert_eq!(response["result"], json!({"result": "continue"})); + } + + #[tokio::test] + async fn test_crash_only_hook_returns_error() { + let (mut client_write, server_read) = tokio::io::duplex(1024); + let mut framed_in = FramedRead::new(server_read, JsonRpcCodec::default()); + let (sender, mut receiver) = tokio::sync::mpsc::channel(4); + let (wait_handle, _) = tokio::sync::broadcast::channel(1); + + let plugin = Plugin { + state: (), + options: HashMap::new(), + option_values: Arc::new(std::sync::Mutex::new(HashMap::new())), + configuration: dummy_configuration(), + wait_handle, + sender, + }; + + let mut hooks: HashMap> = HashMap::new(); + hooks.insert( + "db_write".to_string(), + Box::new(|_p, _r| Box::pin(async { Err(anyhow!("db disk full")) })), + ); + + let driver = PluginDriver { + plugin: plugin.clone(), + rpcmethods: HashMap::new(), + setconfig_callback: None, + hooks, + subscriptions: HashMap::new(), + wildcard_subscription: None, + }; + + client_write + .write_all(b"{\"jsonrpc\": \"2.0\", \"id\": 777, \"method\": \"db_write\", \"params\": {}}\n\n") + .await + .unwrap(); + + driver + .dispatch_one(&mut framed_in, &driver.plugin) + .await + .unwrap(); + + let response = receiver.recv().await.unwrap(); + assert_eq!(response["id"], 777); + assert!(response.get("error").is_some()); + } + + #[tokio::test] + async fn test_rpc_method_named_recover_returns_error_not_fallback() { + let (mut client_write, server_read) = tokio::io::duplex(1024); + let mut framed_in = FramedRead::new(server_read, JsonRpcCodec::default()); + let (sender, mut receiver) = tokio::sync::mpsc::channel(4); + let (wait_handle, _) = tokio::sync::broadcast::channel(1); + + let plugin = Plugin { + state: (), + options: HashMap::new(), + option_values: Arc::new(std::sync::Mutex::new(HashMap::new())), + configuration: dummy_configuration(), + wait_handle, + sender, + }; + + let mut rpcmethods: HashMap> = HashMap::new(); + rpcmethods.insert( + "recover".to_string(), + Box::new(|_p, _r| Box::pin(async { Err(anyhow!("recover rpc failed")) })), + ); + + let driver = PluginDriver { + plugin: plugin.clone(), + rpcmethods, + setconfig_callback: None, + hooks: HashMap::new(), + subscriptions: HashMap::new(), + wildcard_subscription: None, + }; + + client_write + .write_all( + b"{\"jsonrpc\": \"2.0\", \"id\": 888, \"method\": \"recover\", \"params\": {}}\n\n", + ) + .await + .unwrap(); + + driver + .dispatch_one(&mut framed_in, &driver.plugin) + .await + .unwrap(); + + let response = receiver.recv().await.unwrap(); + assert_eq!(response["id"], 888); + assert!(response.get("error").is_some()); + assert_eq!(response["error"]["message"], "recover rpc failed"); + } } diff --git a/tests/test_closing.py b/tests/test_closing.py index 00069b4c387c..67aa70db703f 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -3041,14 +3041,26 @@ def test_onchain_all_dust(node_factory, bitcoind, executor): bitcoind.generate_block(5) # 100 deep and l2 forgets. - bitcoind.generate_block(93, wait_for_mempool=txid) + mined = bitcoind.generate_block(93, wait_for_mempool=txid) sync_blockheight(bitcoind, [l1, l2]) assert not l2.daemon.is_in_log('onchaind complete, forgetting peer') assert not l1.daemon.is_in_log('onchaind complete, forgetting peer') + + # l1 must rebuild its "we gave up on this, but it got mined anyway" + # state from replay: the bookkeeping must not double up and the depth + # must be measured from the spend's confirmation all the same. + l1.restart() + bitcoind.generate_block(1) l2.daemon.wait_for_log('onchaind complete, forgetting peer') - # l1 does not wait for ignored payment. + # l1 does wait for the ignored-but-mined payment now: its 100 blocks + # count from when it was mined, not from the commitment tx, so it + # trails l2 by however long the lowball fee sat in the mempool. + assert not l1.daemon.is_in_log('onchaind complete, forgetting peer') + # The test elementsd runs without -txindex: look the spend up in its block. + confs = bitcoind.rpc.getrawtransaction(txid, True, mined[0])['confirmations'] + bitcoind.generate_block(100 - confs) l1.daemon.wait_for_log('onchaind complete, forgetting peer') assert account_balance(l1, channel_id) == 0 diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 37256871ea85..196311097000 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -2047,6 +2047,10 @@ def test_addgossip(node_factory): else: badupdate = badupdate[:-1] + 'f' + # Newer than the one we have, or it's ignored before the signature check. + # type(2) signature(64) chain_hash(32) scid(8) timestamp(4) + timestamp = int(badupdate[212:220], 16) + 1 + badupdate = badupdate[:212] + format(timestamp, '08x') + badupdate[220:] with pytest.raises(RpcError, match='Bad signature'): l3.rpc.addgossip(badupdate) diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 46a85d762106..185d70cc5ed1 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -261,16 +261,6 @@ void fatal(const char *fmt UNNEEDED, ...) /* Generated stub for fatal_vfmt */ void fatal_vfmt(const char *fmt UNNEEDED, va_list ap UNNEEDED) { fprintf(stderr, "fatal_vfmt called!\n"); abort(); } -/* Generated stub for find_txwatch_ */ -struct txwatch *find_txwatch_(struct chain_topology *topo UNNEEDED, - const struct bitcoin_txid *txid UNNEEDED, - enum watch_result (*cb)(struct lightningd *ld UNNEEDED, - const struct bitcoin_txid * UNNEEDED, - const struct bitcoin_tx * UNNEEDED, - unsigned int depth UNNEEDED, - void *arg) UNNEEDED, - void *arg UNNEEDED) -{ fprintf(stderr, "find_txwatch_ called!\n"); abort(); } /* Generated stub for force_peer_disconnect */ void force_peer_disconnect(struct lightningd *ld UNNEEDED, const struct peer *peer UNNEEDED, @@ -307,7 +297,7 @@ bool fromwire_channeld_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNE bool fromwire_channeld_got_commitsig(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *commitnum UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct bitcoin_signature *signature UNNEEDED, struct bitcoin_signature **htlc_signature UNNEEDED, struct added_htlc ***added UNNEEDED, struct fulfilled_htlc **fulfilled UNNEEDED, struct failed_htlc ***failed UNNEEDED, struct changed_htlc **changed UNNEEDED, struct bitcoin_tx **tx UNNEEDED, struct commitsig ***inflight_commitsigs UNNEEDED) { fprintf(stderr, "fromwire_channeld_got_commitsig called!\n"); abort(); } /* Generated stub for fromwire_channeld_got_revoke */ -bool fromwire_channeld_got_revoke(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *revokenum UNNEEDED, struct secret *per_commitment_secret UNNEEDED, struct pubkey *next_per_commit_point UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct changed_htlc **changed UNNEEDED, struct penalty_base **pbase UNNEEDED, struct bitcoin_tx **penalty_tx UNNEEDED) +bool fromwire_channeld_got_revoke(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *revokenum UNNEEDED, struct secret *per_commitment_secret UNNEEDED, struct pubkey *next_per_commit_point UNNEEDED, struct fee_states **fee_states UNNEEDED, struct height_states **blockheight_states UNNEEDED, struct changed_htlc **changed UNNEEDED, struct penalty_base **pbases UNNEEDED, struct bitcoin_tx ***penalty_txs UNNEEDED) { fprintf(stderr, "fromwire_channeld_got_revoke called!\n"); abort(); } /* Generated stub for fromwire_channeld_offer_htlc_reply */ bool fromwire_channeld_offer_htlc_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, u64 *id UNNEEDED, u8 **failuremsg UNNEEDED, wirestring **failurestr UNNEEDED, u8 *reason UNNEEDED) @@ -820,11 +810,6 @@ struct txowatch *watch_txo(const tal_t *ctx UNNEEDED, size_t input_num UNNEEDED, const struct block *block)) { fprintf(stderr, "watch_txo called!\n"); abort(); } -/* Generated stub for watchman_unwatch_outpoint */ -void watchman_unwatch_outpoint(struct lightningd *ld UNNEEDED, - const char *owner UNNEEDED, - const struct bitcoin_outpoint *outpoint UNNEEDED) -{ fprintf(stderr, "watchman_unwatch_outpoint called!\n"); abort(); } /* Generated stub for watchman_watch_outpoint */ void watchman_watch_outpoint(struct lightningd *ld UNNEEDED, const char *owner UNNEEDED, diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index 4a0e2385fd85..16993539346f 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -15,6 +15,7 @@ #include #include #include +#include #include /* May return NULL if encoding error occurs. */ @@ -773,6 +774,28 @@ static bool match_psbt_outputs_to_wallet(struct wally_psbt *psbt, return ok; } +static bool input_spends_utxo(const struct wally_psbt_input *input, + struct utxo **utxos) +{ + for (size_t i = 0; i < tal_count(utxos); i++) { + if (wally_psbt_input_spends(input, &utxos[i]->outpoint)) + return true; + } + return false; +} + +static bool psbt_input_signed(const struct wally_psbt *psbt, size_t in) +{ + size_t taproot_sig_len; + + if (psbt->inputs[in].signatures.num_items != 0 + || psbt->inputs[in].final_witness) + return true; + return wally_psbt_get_input_taproot_signature_len(psbt, in, + &taproot_sig_len) == WALLY_OK + && taproot_sig_len != 0; +} + static struct command_result *param_input_numbers(struct command *cmd, const char *name, const char *buffer, @@ -848,6 +871,11 @@ static struct command_result *json_signpsbt(struct command *cmd, " added inputs to this PSBT? If not, then" " there is no need to sign it."); + /* libwally can't compute a sighash without outputs. */ + if (psbt->num_outputs == 0) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "PSBT has no outputs"); + if (command_check_only(cmd)) return command_check_done(cmd); @@ -856,8 +884,6 @@ static struct command_result *json_signpsbt(struct command *cmd, return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Could not add keypaths to PSBT?"); - /* FIXME: hsm will sign almost anything, but it should really - * fail cleanly (not abort!) and let us report the error here. */ hsm_utxos = utxos_to_hsm_utxos(tmpctx, utxos); u8 *msg = towire_hsmd_sign_withdrawal(cmd, hsm_utxos, psbt); @@ -884,6 +910,16 @@ static struct command_result *json_signpsbt(struct command *cmd, fmt_wally_psbt(tmpctx, signed_psbt)); } + /* hsmd returns the PSBT unsigned if libwally refused it. */ + for (size_t i = 0; i < combined_psbt->num_inputs; i++) { + if (!input_spends_utxo(&combined_psbt->inputs[i], utxos)) + continue; + if (!psbt_input_signed(combined_psbt, i)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "HSM could not sign input %zu" + " (see logs)", i); + } + if (!psbt_set_version(combined_psbt, psbt_version)) { return command_fail(cmd, LIGHTNINGD, "Signed PSBT unable to have version set: %s", diff --git a/wire/peer_wire.c b/wire/peer_wire.c index 2357073491a1..d35eb6330ff6 100644 --- a/wire/peer_wire.c +++ b/wire/peer_wire.c @@ -62,6 +62,20 @@ static bool unknown_type(enum peer_wire t) return true; } +bool is_oversized_gossip_msg(const u8 *msg) +{ + switch ((enum peer_wire)fromwire_peektype(msg)) { + case WIRE_CHANNEL_ANNOUNCEMENT: + return tal_bytelen(msg) > MAX_CHANNEL_ANNOUNCEMENT_LEN; + case WIRE_CHANNEL_UPDATE: + return tal_bytelen(msg) > MAX_CHANNEL_UPDATE_LEN; + case WIRE_NODE_ANNOUNCEMENT: + return tal_bytelen(msg) > MAX_NODE_ANNOUNCEMENT_LEN; + default: + return false; + } +} + bool is_msg_for_gossipd(const u8 *cursor) { switch ((enum peer_wire)fromwire_peektype(cursor)) { diff --git a/wire/peer_wire.h b/wire/peer_wire.h index 760e06230293..5670a99a845b 100644 --- a/wire/peer_wire.h +++ b/wire/peer_wire.h @@ -18,6 +18,15 @@ bool is_unknown_msg_discardable(const u8 *cursor); /* Return true if it's a message for gossipd. */ bool is_msg_for_gossipd(const u8 *cursor); + +/* Generous upper bounds on a well-formed gossip message: real ones are a + * few hundred bytes. We ignore anything larger. */ +#define MAX_CHANNEL_ANNOUNCEMENT_LEN 1024 +#define MAX_CHANNEL_UPDATE_LEN 1024 +#define MAX_NODE_ANNOUNCEMENT_LEN 2048 + +/* Return true if it's a gossip message larger than any valid one. */ +bool is_oversized_gossip_msg(const u8 *msg); /* Returns true if the message type should be treated as a custommsg */ bool peer_wire_is_internal(enum peer_wire type); /* Extract channel_id from various packets, return true if possible. */ diff --git a/wire/test/run-tlvstream.c b/wire/test/run-tlvstream.c index 5aa609336299..88e4b9c88c6e 100644 --- a/wire/test/run-tlvstream.c +++ b/wire/test/run-tlvstream.c @@ -455,6 +455,37 @@ static bool ignored_fields(const struct tlv_n1 *tlv_n1) && tlv_n1->tlv4 == NULL; } +/* tlvstream_set_raw() appends fields we have no tlv_record_type for: they + * must look unknown (meta == NULL) to towire_tlv() and tlv_update_fields(), + * otherwise those silently drop them. */ +static void test_tlvstream_set_raw(void) +{ + struct tlv_n1 *tlv_n1 = tlv_n1_new(tmpctx); + u64 amount = 1; + u8 *p = tal_arr(tmpctx, u8, 0); + + tlv_n1->tlv1 = &amount; + tlvstream_set_raw(&tlv_n1->fields, 5, "hi", 2); + assert(tal_count(tlv_n1->fields) == 1); + assert(tlv_n1->fields[0].meta == NULL); + + towire_tlv_n1(&p, tlv_n1); + assert(tal_arr_eq(p, stream(tmpctx, "01 01 01 05 02 6869"))); + + tlv_update_fields(tlv_n1, tlv_n1, &tlv_n1->fields); + assert(tal_count(tlv_n1->fields) == 2); + assert(tlv_n1->fields[0].meta == &tlvs_tlv_n1[0]); + assert(tlv_n1->fields[1].numtype == 5); + assert(tlv_n1->fields[1].meta == NULL); + + /* Overwriting an existing field keeps its meta. */ + tlvstream_set_raw(&tlv_n1->fields, 1, "\x02", 1); + assert(tal_count(tlv_n1->fields) == 2); + assert(tlv_n1->fields[0].meta == &tlvs_tlv_n1[0]); + assert(tlv_n1->fields[0].length == 1); + assert(tlv_n1->fields[0].value[0] == 2); +} + int main(int argc, char *argv[]) { common_setup(argv[0]); @@ -643,5 +674,7 @@ int main(int argc, char *argv[]) assert(tal_arr_eq(orig_p, p2)); } } + + test_tlvstream_set_raw(); common_shutdown(); } diff --git a/wire/tlvstream.c b/wire/tlvstream.c index 8f276a7257d0..a9546147cc7d 100644 --- a/wire/tlvstream.c +++ b/wire/tlvstream.c @@ -58,7 +58,9 @@ void tlvstream_set_raw(struct tlv_field **stream, u64 type, const void *value TA e->length = valuelen; e->value = tal_dup_arr(*stream, u8, value, e->length, 0); } else { - /* If we haven't found it insert it insead. */ + /* If we haven't found it insert it insead. We have no + * tlv_record_type for it, so it's an unknown field. */ + f.meta = NULL; f.length = valuelen; f.numtype = type; f.value = tal_dup_arr(*stream, u8, value, f.length, 0);