From 454715cffdc7f986bab2389685404b5a2c78b6be Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 2 Jul 2024 16:50:26 +0200 Subject: [PATCH 1/3] close: Do not publish unilateral when witnessing a close onchain Changelog-Changed: close: We no longer attempt to publish a unilateral close that'd fail anyway when we witness a close onchain. --- lightningd/channel.c | 21 +++++++++++++++++---- lightningd/closing_control.c | 2 +- lightningd/peer_control.c | 10 +++++++--- lightningd/peer_control.h | 11 ++++++++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index a835db6554d9..2ef8209c315b 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -902,13 +902,19 @@ void channel_fail_permanent(struct channel *channel, struct lightningd *ld = channel->peer->ld; va_list ap; char *why; + /* Do we want to rebroadcast close transactions? If we're + * witnessing the close on-chain there is no point in doing + * this. */ + bool rebroadcast; va_start(ap, fmt); why = tal_vfmt(tmpctx, fmt, ap); va_end(ap); - log_unusual(channel->log, "Peer permanent failure in %s: %s", - channel_state_name(channel), why); + log_unusual(channel->log, + "Peer permanent failure in %s: %s (reason=%s)", + channel_state_name(channel), why, + channel_change_state_reason_str(reason)); /* We can have multiple errors, eg. onchaind failures. */ if (!channel->error) @@ -916,8 +922,15 @@ void channel_fail_permanent(struct channel *channel, &channel->cid, "%s", why); channel_set_owner(channel, NULL); - /* Drop non-cooperatively (unilateral) to chain. */ - drop_to_chain(ld, channel, false); + + /* Drop non-cooperatively (unilateral) to chain. If we detect + * the close from the blockchain (i.e., reason is + * REASON_ONCHAIN, or FUNDING_SPEND_SEEN) then we can observe + * passively, and not broadcast our own unilateral close, as + * it doesn't stand a chance anyway. */ + rebroadcast = !(channel->state == ONCHAIN || + channel->state == FUNDING_SPEND_SEEN); + drop_to_chain(ld, channel, false, rebroadcast); if (channel_state_wants_onchain_fail(channel->state)) channel_set_state(channel, diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index b6d9a44d2698..df20a97dc719 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -303,7 +303,7 @@ static void peer_closing_complete(struct channel *channel, const u8 *msg) return; /* Channel gets dropped to chain cooperatively. */ - drop_to_chain(channel->peer->ld, channel, true); + drop_to_chain(channel->peer->ld, channel, true, true /* rebroadcast */); channel_set_state(channel, CLOSINGD_SIGEXCHANGE, CLOSINGD_COMPLETE, diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index bb7a2fb67172..7e0945270578 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -348,7 +348,7 @@ static enum watch_result closed_inflight_depth_cb(struct lightningd *ld, } void drop_to_chain(struct lightningd *ld, struct channel *channel, - bool cooperative) + bool cooperative, bool rebroadcast) { struct channel_inflight *inflight; const char *cmd_id; @@ -387,6 +387,10 @@ void drop_to_chain(struct lightningd *ld, struct channel *channel, log_broken(channel->log, "Cannot broadcast our commitment tx:" " it's invalid! (ancient channel?)"); + } else if (!rebroadcast && !cooperative) { + log_unusual(channel->log, + "Not dropping our unilateral close onchain since " + "we already saw theirs confirm."); } else { struct bitcoin_tx *tx COMPILER_WANTS_INIT("gcc 12.3.0"); @@ -448,10 +452,10 @@ void resend_closing_transactions(struct lightningd *ld) case CLOSED: continue; case CLOSINGD_COMPLETE: - drop_to_chain(ld, channel, true); + drop_to_chain(ld, channel, true, true); continue; case AWAITING_UNILATERAL: - drop_to_chain(ld, channel, false); + drop_to_chain(ld, channel, false, true); continue; } abort(); diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index a693e0d87e08..face71198575 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -109,7 +109,16 @@ void peer_set_dbid(struct peer *peer, u64 dbid); /* At startup, re-send any transactions we want bitcoind to have */ void resend_closing_transactions(struct lightningd *ld); -void drop_to_chain(struct lightningd *ld, struct channel *channel, bool cooperative); +/** + * Initiate the close of a channel. + * + * @param rebroadcast: Whether we should be broadcasting our + * commitment transaction in order to close the channel, or not. + */ +void drop_to_chain(struct lightningd *ld, + struct channel *channel, + bool cooperative, + bool rebroadcast); void update_channel_from_inflight(struct lightningd *ld, struct channel *channel, From 88b8abb47cac39f02d48bc99c8c2896e3ebdf0fb Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 2 Jul 2024 18:24:20 +0200 Subject: [PATCH 2/3] tests: Adjust tests for the lack of unilateral attempt now --- tests/test_plugin.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 00b1ce454397..8a69a763d20c 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2059,11 +2059,8 @@ def mock_sendrawtransaction(tx): # Restart l2, and it should continue where the watchtower left off: l2.start() - # l2 will still try to broadcast its latest commitment tx, but it'll fail - # since l1 has cheated. All commitments share the same prefix, so look for - # that. - penalty_prefix = tx[:(4 + 1 + 36) * 2] # version, txin_count, first txin in hex - l2.daemon.wait_for_log(r'Expected error broadcasting tx {}'.format(penalty_prefix)) + # l2 notices that there has been a unilateral close from l1. + l2.daemon.wait_for_log(r'Peer permanent failure in') # Now make sure the penalty output ends up in our wallet fund_txids = [o['txid'] for o in l2.rpc.listfunds()['outputs']] From 47fea4fcc29bd372d265ac74053d849f17afecac Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 10 Jul 2024 17:43:54 +0200 Subject: [PATCH 3/3] onchaind: Remove key derivation involving the signer The signer may not be present at this time. If we want to keep the check to protect against bit flips we should move it into `onchaind` where it doesn't matter as much that the signer may be slow to respond. --- lightningd/onchain_control.c | 5 ----- onchaind/onchaind.c | 4 ---- onchaind/onchaind_wire.csv | 2 -- onchaind/test/run-grind_feerate-bug.c | 6 ------ onchaind/test/run-grind_feerate.c | 2 +- 5 files changed, 1 insertion(+), 18 deletions(-) diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 662b3bc55491..7317ab7bef5a 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -1504,7 +1504,6 @@ enum watch_result onchaind_funding_spent(struct channel *channel, u8 *msg; struct bitcoin_txid our_last_txid; struct lightningd *ld = channel->peer->ld; - struct pubkey final_key; int hsmfd; enum state_change reason; @@ -1564,8 +1563,6 @@ enum watch_result onchaind_funding_spent(struct channel *channel, return KEEP_WATCHING; } - bip32_pubkey(ld, &final_key, channel->final_key_idx); - struct ext_key final_wallet_ext_key; if (bip32_key_from_parent( ld->bip32_base, @@ -1611,8 +1608,6 @@ enum watch_result onchaind_funding_spent(struct channel *channel, channel->shutdown_scriptpubkey[LOCAL], channel->shutdown_scriptpubkey[REMOTE], channel->final_key_idx, - &final_wallet_ext_key, - &final_key, channel->opener, &channel->local_basepoints, &channel->channel_info.theirbase, diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index 537ff976b068..e9d61f37e094 100644 --- a/onchaind/onchaind.c +++ b/onchaind/onchaind.c @@ -48,8 +48,6 @@ static u32 to_self_delay[NUM_SIDES]; /* Where we send money to (our wallet) */ static u32 our_wallet_index; -static struct ext_key our_wallet_ext_key; -static struct pubkey our_wallet_pubkey; /* Their revocation secret (only if they cheated). */ static const struct secret *remote_per_commitment_secret; @@ -3432,8 +3430,6 @@ int main(int argc, char *argv[]) &scriptpubkey[LOCAL], &scriptpubkey[REMOTE], &our_wallet_index, - &our_wallet_ext_key, - &our_wallet_pubkey, &opener, &basepoints[LOCAL], &basepoints[REMOTE], diff --git a/onchaind/onchaind_wire.csv b/onchaind/onchaind_wire.csv index 32d5d37d2c96..d75b3ab65e86 100644 --- a/onchaind/onchaind_wire.csv +++ b/onchaind/onchaind_wire.csv @@ -28,8 +28,6 @@ msgdata,onchaind_init,local_scriptpubkey,u8,local_scriptpubkey_len msgdata,onchaind_init,remote_scriptpubkey_len,u16, msgdata,onchaind_init,remote_scriptpubkey,u8,remote_scriptpubkey_len msgdata,onchaind_init,ourwallet_index,u32, -msgdata,onchaind_init,ourwallet_ext_key,ext_key, -msgdata,onchaind_init,ourwallet_pubkey,pubkey, # We need these two for commit number obscurer msgdata,onchaind_init,opener,enum side, msgdata,onchaind_init,local_basepoints,basepoints, diff --git a/onchaind/test/run-grind_feerate-bug.c b/onchaind/test/run-grind_feerate-bug.c index 227210b0128b..8be39f6c9635 100644 --- a/onchaind/test/run-grind_feerate-bug.c +++ b/onchaind/test/run-grind_feerate-bug.c @@ -42,9 +42,6 @@ void fromwire_basepoints(const u8 **ptr UNNEEDED, size_t *max UNNEEDED, /* Generated stub for fromwire_chain_coin_mvt */ void fromwire_chain_coin_mvt(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct chain_coin_mvt *mvt UNNEEDED) { fprintf(stderr, "fromwire_chain_coin_mvt called!\n"); abort(); } -/* Generated stub for fromwire_ext_key */ -void fromwire_ext_key(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct ext_key *bip32 UNNEEDED) -{ fprintf(stderr, "fromwire_ext_key called!\n"); abort(); } /* Generated stub for fromwire_hsmd_get_per_commitment_point_reply */ bool fromwire_hsmd_get_per_commitment_point_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct pubkey *per_commitment_point UNNEEDED, struct secret **old_commitment_secret UNNEEDED) { fprintf(stderr, "fromwire_hsmd_get_per_commitment_point_reply called!\n"); abort(); } @@ -225,9 +222,6 @@ void towire_basepoints(u8 **pptr UNNEEDED, const struct basepoints *b UNNEEDED) /* Generated stub for towire_chain_coin_mvt */ void towire_chain_coin_mvt(u8 **pptr UNNEEDED, const struct chain_coin_mvt *mvt UNNEEDED) { fprintf(stderr, "towire_chain_coin_mvt called!\n"); abort(); } -/* Generated stub for towire_ext_key */ -void towire_ext_key(u8 **pptr UNNEEDED, const struct ext_key *bip32 UNNEEDED) -{ fprintf(stderr, "towire_ext_key called!\n"); abort(); } /* Generated stub for towire_hsmd_get_per_commitment_point */ u8 *towire_hsmd_get_per_commitment_point(const tal_t *ctx UNNEEDED, u64 n UNNEEDED) { fprintf(stderr, "towire_hsmd_get_per_commitment_point called!\n"); abort(); } diff --git a/onchaind/test/run-grind_feerate.c b/onchaind/test/run-grind_feerate.c index 06485657dc12..f390399264f5 100644 --- a/onchaind/test/run-grind_feerate.c +++ b/onchaind/test/run-grind_feerate.c @@ -56,7 +56,7 @@ bool fromwire_onchaind_dev_memleak(const void *p 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) { 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, u32 *ourwallet_index UNNEEDED, struct ext_key *ourwallet_ext_key UNNEEDED, struct pubkey *ourwallet_pubkey 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) +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, u32 *ourwallet_index 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) { fprintf(stderr, "fromwire_onchaind_init called!\n"); abort(); } /* Generated stub for fromwire_onchaind_known_preimage */ bool fromwire_onchaind_known_preimage(const void *p UNNEEDED, struct preimage *preimage UNNEEDED)