From f2f02f9de6184010af30c16268ad18ebd53df15b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 23 Mar 2023 16:17:52 +1030 Subject: [PATCH] chaintopology: allow minblock for broadcast_tx. Fun story. We're changing onchaind to hand txs to us, and we will construct them and do the broadcast for it. lightningd tells onchaind the witness it used (with flags to indicate which fields were signatures so should be ignored) so onchaind can recognize the tx when/if it is mined. And when onchaind was waiting for a CLTV delay, it wouldn't tell lightningd yet, but wait until the parent was sufficiently deep But this caused bugs! In particular, on replay, onchaind would see transactions which it hasn't sent yet. This was not a problem before, as onchaind had created the tx, even if it hadn't told lightningd to broadcast it, so recognized the variant when it came in. When we're relying on lightningd to tell us what the tx will look like, this doesn't work any more. The cause of this is that we fire off txowatches ("this output was spent!") while we process blocks, and only fire off txwatches ("this tx increased depth") once all the current blocks are processed. Often this didn't matter, since we replay messages to onchaind from the database, *but* we trim the last few blocks on restart (or, if there's a small reorg while we're stopped), and we can hit this misordering. Changing our topology code to only ever process one block at a time would be a solution, but slows down catchup (and tests, where we often mine a run of blocks). So, this seems like a premature optimization, but it's really required! And in future, lightningd can use this knowledge of pending transactions to combine them in more clever ways. Note that if a tx is valid at block N, we broadcast it once we see block N-1, to get it in the mempool for block N. Signed-off-by: Rusty Russell --- lightningd/chaintopology.c | 26 ++++++++++++++++++--- lightningd/chaintopology.h | 8 ++++--- lightningd/onchain_control.c | 2 +- lightningd/peer_control.c | 2 +- lightningd/test/run-invoice-select-inchan.c | 2 +- wallet/test/run-wallet.c | 2 +- 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 12f44dd3c..9cc00d9a7 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -167,6 +167,11 @@ static void rebroadcast_txs(struct chain_topology *topo) if (wallet_transaction_height(topo->ld->wallet, &otx->txid)) continue; + /* Don't send ones which aren't ready yet. Note that if the + * minimum block is N, we broadcast it when we have block N-1! */ + if (get_block_height(topo) + 1 < otx->minblock) + continue; + /* Don't free from txmap inside loop! */ if (otx->refresh && !otx->refresh(otx->channel, &otx->tx, otx->refresh_arg)) { @@ -230,7 +235,7 @@ static void broadcast_done(struct bitcoind *bitcoind, void broadcast_tx_(struct chain_topology *topo, struct channel *channel, const struct bitcoin_tx *tx, - const char *cmd_id, bool allowhighfees, + const char *cmd_id, bool allowhighfees, u32 minblock, void (*finished)(struct channel *channel, bool success, const char *err), @@ -245,6 +250,7 @@ void broadcast_tx_(struct chain_topology *topo, otx->channel = channel; bitcoin_txid(tx, &otx->txid); otx->tx = clone_bitcoin_tx(otx, tx); + otx->minblock = minblock; otx->finished = finished; otx->refresh = refresh; otx->refresh_arg = refresh_arg; @@ -254,8 +260,22 @@ void broadcast_tx_(struct chain_topology *topo, otx->cmd_id = tal_strdup(otx, cmd_id); else otx->cmd_id = NULL; - tal_add_destructor2(channel, clear_otx_channel, otx); + /* Note that if the minimum block is N, we broadcast it when + * we have block N-1! */ + if (get_block_height(topo) + 1 < otx->minblock) { + log_debug(topo->log, "Deferring broadcast of txid %s until block %u", + type_to_string(tmpctx, struct bitcoin_txid, &otx->txid), + otx->minblock - 1); + + /* For continual rebroadcasting, until channel freed. */ + tal_steal(otx->channel, otx); + outgoing_tx_map_add(topo->outgoing_txs, otx); + tal_add_destructor2(otx, destroy_outgoing_tx, topo); + return; + } + + tal_add_destructor2(channel, clear_otx_channel, otx); log_debug(topo->log, "Broadcasting txid %s%s%s", type_to_string(tmpctx, struct bitcoin_txid, &otx->txid), cmd_id ? " for " : "", cmd_id ? cmd_id : ""); @@ -372,7 +392,7 @@ static void update_feerates(struct bitcoind *bitcoind, /* Initial smoothed feerate is the polled feerate */ if (!old_feerates[i]) { - notify_feerate_changed = true; + notify_feerate_changed = true; old_feerates[i] = feerate; init_feerate_history(topo, i, feerate); diff --git a/lightningd/chaintopology.h b/lightningd/chaintopology.h index fa3b1f56a..486fab0f3 100644 --- a/lightningd/chaintopology.h +++ b/lightningd/chaintopology.h @@ -21,6 +21,7 @@ struct outgoing_tx { struct channel *channel; const struct bitcoin_tx *tx; struct bitcoin_txid txid; + u32 minblock; const char *cmd_id; void (*finished)(struct channel *channel, bool success, const char *err); bool (*refresh)(struct channel *, const struct bitcoin_tx **, void *arg); @@ -180,15 +181,16 @@ u32 penalty_feerate(struct chain_topology *topo); * @tx: the transaction * @cmd_id: the JSON command id which triggered this (or NULL). * @allowhighfees: set to true to override the high-fee checks in the backend. + * @minblock: minimum block we can send it at (or 0). * @finished: if non-NULL, call that and don't rebroadcast. * @refresh: if non-NULL, callback before re-broadcasting (can replace tx): * if returns false, delete. * @refresh_arg: argument for @refresh */ #define broadcast_tx(topo, channel, tx, cmd_id, allowhighfees, \ - finished, refresh, refresh_arg) \ + minblock, finished, refresh, refresh_arg) \ broadcast_tx_((topo), (channel), (tx), (cmd_id), (allowhighfees), \ - (finished), \ + (minblock), (finished), \ typesafe_cb_preargs(bool, void *, \ (refresh), (refresh_arg), \ struct channel *, \ @@ -198,7 +200,7 @@ u32 penalty_feerate(struct chain_topology *topo); void broadcast_tx_(struct chain_topology *topo, struct channel *channel, const struct bitcoin_tx *tx TAKES, - const char *cmd_id, bool allowhighfees, + const char *cmd_id, bool allowhighfees, u32 minblock, void (*finished)(struct channel *, bool success, const char *err), diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index a472e7355..c1372e5ad 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -345,7 +345,7 @@ static void handle_onchain_broadcast_tx(struct channel *channel, * set allowhighfees, as the transaction may be RBFed into * high feerates as protection against the MAD-HTLC attack. */ broadcast_tx(channel->peer->ld->topology, channel, - tx, NULL, is_rbf, + tx, NULL, is_rbf, 0, is_rbf ? &handle_onchain_broadcast_rbf_tx_cb : NULL, NULL, NULL); } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index d281a6059..0a6b25d38 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -284,7 +284,7 @@ static void sign_and_send_last(struct lightningd *ld, /* Keep broadcasting until we say stop (can fail due to dup, * if they beat us to the broadcast). */ - broadcast_tx(ld->topology, channel, last_tx, cmd_id, false, NULL, + broadcast_tx(ld->topology, channel, last_tx, cmd_id, false, 0, NULL, NULL, NULL); remove_sig(last_tx); diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 09ca2b3dd..7f16360c2 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -54,7 +54,7 @@ char *bolt11_encode_(const tal_t *ctx UNNEEDED, void broadcast_tx_(struct chain_topology *topo UNNEEDED, struct channel *channel UNNEEDED, const struct bitcoin_tx *tx TAKES UNNEEDED, - const char *cmd_id UNNEEDED, bool allowhighfees UNNEEDED, + const char *cmd_id UNNEEDED, bool allowhighfees UNNEEDED, u32 minblock UNNEEDED, void (*finished)(struct channel * UNNEEDED, bool success UNNEEDED, const char *err) UNNEEDED, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index d01088aa1..c66eb4f1e 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -72,7 +72,7 @@ bool blinding_next_pubkey(const struct pubkey *pk UNNEEDED, void broadcast_tx_(struct chain_topology *topo UNNEEDED, struct channel *channel UNNEEDED, const struct bitcoin_tx *tx TAKES UNNEEDED, - const char *cmd_id UNNEEDED, bool allowhighfees UNNEEDED, + const char *cmd_id UNNEEDED, bool allowhighfees UNNEEDED, u32 minblock UNNEEDED, void (*finished)(struct channel * UNNEEDED, bool success UNNEEDED, const char *err) UNNEEDED,