From ec1f1167d709404dd99a00d043a1eb7fce58afab Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 24 Mar 2023 10:35:39 +1030 Subject: [PATCH] lightningd: do RBF again for all the txs. Now we've set everything up, the replacement code is quite simple. Some tests now have to deal with RBF though, and our rbf tests need work since they look for the old onchaind messages. In particular, when we can't afford the fee we want, we back off to the next blockcount estimate, rather than spending all on fees (necessarily). So test_penalty_rbf_burn no longer applies. Changelog-Changed: Protocol: spending unilateral close transactions now use dynamic fees based on deadlines (and RBF), instead of fixed fees. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 14 ++ lightningd/onchain_control.c | 67 +++++++- tests/test_closing.py | 177 +++------------------ tests/test_pay.py | 2 + tests/test_plugin.py | 2 + 5 files changed, 109 insertions(+), 153 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 6aed38a54b86..8cdd4d535442 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -1227,6 +1227,20 @@ def wait_for_onchaind_txs(self, *args): def wait_for_onchaind_tx(self, name, resolve): return self.wait_for_onchaind_txs((name, resolve))[0] + def wait_for_txid_or_rbf(self, txid): + """Wait for a txid to be broadcast, or an rbf. Returns the actual txid""" + # Hack so we can mutate the txid: pass it in a list + def rbf_or_txid_broadcast(maybe_rbf): + # RBF onchain txid d4b597505b543a4b8b42ab4d481fd7a533febb7e7df150ca70689e6d046612f7 (fee 6564sat) with txid 979878b8f855d3895d1cd29bd75a60b21492c4842e38099186a8e649bee02c7c (fee 8205sat) + line = self.daemon.is_in_log("RBF onchain txid {}".format(maybe_rbf[0])) + if line is not None: + maybe_rbf[0] = re.search(r'with txid ([0-9a-fA-F]*)', line).group(1) + return maybe_rbf[0] in self.bitcoin.rpc.getrawmempool() + + maybe_rbf = [txid] + wait_for(lambda: rbf_or_txid_broadcast(maybe_rbf)) + return maybe_rbf[0] + def wait_for_onchaind_broadcast(self, name, resolve=None): """Wait for onchaind to drop tx name to resolve (if any)""" if resolve: diff --git a/lightningd/onchain_control.c b/lightningd/onchain_control.c index 4821d814aa56..993cd42869e7 100644 --- a/lightningd/onchain_control.c +++ b/lightningd/onchain_control.c @@ -833,7 +833,68 @@ static bool consider_onchain_rebroadcast(struct channel *channel, const struct bitcoin_tx **tx, struct onchain_signing_info *info) { - /* FIXME: Implement rbf! */ + struct bitcoin_tx *newtx; + struct amount_sat newfee, rbf_thresh; + struct bitcoin_txid oldtxid, newtxid; + u8 **witness; + + newtx = onchaind_tx_unsigned(tmpctx, channel, info, &newfee, NULL); + if (!newtx) + return true; + + /* BIP-125: + * + * One or more transactions currently in the mempool (original + * transactions) will be replaced by a new transaction (replacement + * transaction) that spends one or more of the same inputs if, + *... + * + * 4. The replacement transaction must also pay for its own bandwidth + * at or above the rate set by the node's minimum relay fee + * setting. For example, if the minimum relay fee is 1 satoshi/byte + * and the replacement transaction is 500 bytes total, then the + * replacement must pay a fee at least 500 satoshis higher than the + * sum of the originals. + */ + if (!amount_sat_add(&rbf_thresh, + amount_sat(bitcoin_tx_weight(*tx)), info->fee)) { + log_broken(channel->log, "Can't add weight %zu to fee %s!", + bitcoin_tx_weight(*tx), + fmt_amount_sat(tmpctx, info->fee)); + return true; + } + + bitcoin_txid(*tx, &oldtxid); + if (amount_sat_less(newfee, rbf_thresh)) { + log_debug(channel->log, + "Not going to RBF onchain %s (fee %s vs %s)", + type_to_string(tmpctx, struct bitcoin_txid, &oldtxid), + fmt_amount_sat(tmpctx, info->fee), + fmt_amount_sat(tmpctx, newfee)); + return true; + } + + /* OK! RBF time! */ + witness = sign_and_get_witness(NULL, channel, newtx, info); + bitcoin_tx_input_set_witness(newtx, 0, take(witness)); + + bitcoin_txid(newtx, &newtxid); + log_info(channel->log, + "RBF onchain txid %s (fee %s) with txid %s (fee %s)", + type_to_string(tmpctx, struct bitcoin_txid, &oldtxid), + fmt_amount_sat(tmpctx, info->fee), + type_to_string(tmpctx, struct bitcoin_txid, &newtxid), + fmt_amount_sat(tmpctx, newfee)); + log_debug(channel->log, + "RBF %s->%s", + type_to_string(tmpctx, struct bitcoin_tx, *tx), + type_to_string(tmpctx, struct bitcoin_tx, newtx)); + + /* FIXME: This is ugly, but we want the same parent as old tx. */ + tal_steal(tal_parent(*tx), newtx); + tal_free(*tx); + *tx = newtx; + info->fee = newfee; return true; } @@ -917,8 +978,10 @@ static void create_onchain_tx(struct channel *channel, type_to_string(tmpctx, struct bitcoin_tx, tx), worthwhile ? "" : "(NOT WORTHWHILE, LOWBALL FEE!)"); + /* We allow "excessive" fees, as we may be fighting with censors and + * we'd rather spend fees than have our adversary win. */ broadcast_tx(ld->topology, - channel, take(tx), NULL, false, info->minblock, + channel, take(tx), NULL, true, info->minblock, NULL, consider_onchain_rebroadcast, take(info)); subd_send_msg(channel->owner, diff --git a/tests/test_closing.py b/tests/test_closing.py index 7b60ac33d88a..f22d6d11b6a1 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -1562,7 +1562,6 @@ def test_penalty_htlc_tx_timeout(node_factory, bitcoind, chainparams): assert acc['resolved_at_block'] > 0 -@pytest.mark.xfail(strict=True) @pytest.mark.developer("uses dev_sign_last_tx") def test_penalty_rbf_normal(node_factory, bitcoind, executor, chainparams): ''' @@ -1628,24 +1627,30 @@ def censoring_sendrawtx(r): # l2 notices. l2.daemon.wait_for_log(' to ONCHAIN') - def get_rbf_tx(self, depth, name, resolve): - r = self.daemon.wait_for_log('Broadcasting RBF {} .* to resolve {} depth={}' - .format(name, resolve, depth)) - return re.search(r'.* \(([0-9a-fA-F]*)\)', r).group(1) + ((_, txid1, blocks1), (_, txid2, blocks2)) = \ + l2.wait_for_onchaind_txs(('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC'), + ('OUR_PENALTY_TX', + 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) + assert blocks1 == 0 + assert blocks2 == 0 + + def get_rbf_txid(node, txid): + line = node.daemon.wait_for_log("RBF onchain .*{}".format(txid)) + if "Not going to RBF" in line: + return txid + return re.search(r'with txid ([0-9a-fA-F]*)', line).group(1) - rbf_txes = [] # Now the censoring miners generate some blocks. - for depth in range(2, 8): + for depth in range(2, 10): bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) # l2 should RBF, twice even, one for the l1 main output, # one for the l1 HTLC output. - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC')) - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) + # Don't assume a specific order! + start = l2.daemon.logsearch_start + txid1 = get_rbf_txid(l2, txid1) + l2.daemon.logsearch_start = start + txid2 = get_rbf_txid(l2, txid2) # Now that the transactions have high fees, independent miners # realize they can earn potentially more money by grabbing the @@ -1653,15 +1658,8 @@ def get_rbf_tx(self, depth, name, resolve): # hashpower arises, evicting the censor. l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', None) - # Check that the order in which l2 generated RBF transactions - # would be acceptable to Bitcoin. - for tx in rbf_txes: - # Use the bcli interface as well, so that we also check the - # bcli interface. - l2.rpc.call('sendrawtransaction', [tx, True]) - # Now the non-censoring miners overpower the censoring miners. - bitcoind.generate_block(1) + bitcoind.generate_block(1, wait_for_mempool=[txid1, txid2]) sync_blockheight(bitcoind, [l2]) # And l2 should consider it resolved now. @@ -1687,134 +1685,6 @@ def get_rbf_tx(self, depth, name, resolve): check_utxos_channel(l2, [channel_id], expected_2) -@pytest.mark.xfail(strict=True) -@pytest.mark.developer("uses dev_sign_last_tx") -def test_penalty_rbf_burn(node_factory, bitcoind, executor, chainparams): - ''' - Test that penalty transactions are RBFed and we are willing to burn - it all up to spite the thief. - ''' - # We track channel balances, to verify that accounting is ok. - coin_mvt_plugin = os.path.join(os.getcwd(), 'tests/plugins/coin_movements.py') - to_self_delay = 10 - # l1 is the thief, which causes our honest upstanding lightningd - # code to break, so l1 can fail. - # Initially, disconnect before the HTLC can be resolved. - l1 = node_factory.get_node(options={'dev-disable-commit-after': 1}, - may_fail=True, allow_broken_log=True) - l2 = node_factory.get_node(options={'dev-disable-commit-after': 1, - 'watchtime-blocks': to_self_delay, - 'plugin': coin_mvt_plugin}, - # Exporbitant feerates mean we don't have cap on RBF! - feerates=(15000000, 11000, 7500, 3750)) - - l1.rpc.connect(l2.info['id'], 'localhost', l2.port) - l1.fundchannel(l2, 10**7) - channel_id = first_channel_id(l1, l2) - - # Trigger an HTLC being added. - t = executor.submit(l1.pay, l2, 1000000 * 1000) - - # Make sure the channel is still alive. - assert len(l1.getactivechannels()) == 2 - assert len(l2.getactivechannels()) == 2 - - # Wait for the disconnection. - l1.daemon.wait_for_log('dev-disable-commit-after: disabling') - l2.daemon.wait_for_log('dev-disable-commit-after: disabling') - # Make sure l1 gets the new HTLC. - l1.daemon.wait_for_log('got commitsig') - - # l1 prepares a theft commitment transaction - theft_tx = l1.rpc.dev_sign_last_tx(l2.info['id'])['tx'] - - # Now continue processing until fulfilment. - l1.rpc.dev_reenable_commit(l2.info['id']) - l2.rpc.dev_reenable_commit(l1.info['id']) - - # Wait for the fulfilment. - l1.daemon.wait_for_log('peer_in WIRE_UPDATE_FULFILL_HTLC') - l1.daemon.wait_for_log('peer_out WIRE_REVOKE_AND_ACK') - l2.daemon.wait_for_log('peer_out WIRE_UPDATE_FULFILL_HTLC') - l1.daemon.wait_for_log('peer_in WIRE_REVOKE_AND_ACK') - - # Now payment should complete. - t.result(timeout=10) - - # l1 goes offline and bribes the miners to censor transactions from l2. - l1.rpc.stop() - - def censoring_sendrawtx(r): - return {'id': r['id'], 'result': {}} - - l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', censoring_sendrawtx) - - # l1 now performs the theft attack! - bitcoind.rpc.sendrawtransaction(theft_tx) - bitcoind.generate_block(1) - - # l2 notices. - l2.daemon.wait_for_log(' to ONCHAIN') - - def get_rbf_tx(self, depth, name, resolve): - r = self.daemon.wait_for_log('Broadcasting RBF {} .* to resolve {} depth={}' - .format(name, resolve, depth)) - return re.search(r'.* \(([0-9a-fA-F]*)\)', r).group(1) - - rbf_txes = [] - # Now the censoring miners generate some blocks. - for depth in range(2, 10): - bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) - # l2 should RBF, twice even, one for the l1 main output, - # one for the l1 HTLC output. - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/THEIR_HTLC')) - rbf_txes.append(get_rbf_tx(l2, depth, - 'OUR_PENALTY_TX', - 'THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM')) - - # Now that the transactions have high fees, independent miners - # realize they can earn potentially more money by grabbing the - # high-fee censored transactions, and fresh, non-censoring - # hashpower arises, evicting the censor. - l2.daemon.rpcproxy.mock_rpc('sendrawtransaction', None) - - # Check that the last two txes can be broadcast. - # These should donate the total amount to miners. - rbf_txes = rbf_txes[-2:] - for tx in rbf_txes: - l2.rpc.call('sendrawtransaction', [tx, True]) - - # Now the non-censoring miners overpower the censoring miners. - bitcoind.generate_block(1) - sync_blockheight(bitcoind, [l2]) - - # And l2 should consider it resolved now. - l2.daemon.wait_for_log('Resolved THEIR_REVOKED_UNILATERAL/DELAYED_CHEAT_OUTPUT_TO_THEM by our proposal OUR_PENALTY_TX') - l2.daemon.wait_for_log('Resolved THEIR_REVOKED_UNILATERAL/THEIR_HTLC by our proposal OUR_PENALTY_TX') - - # l2 donated it to the miners, so it owns nothing - assert(len(l2.rpc.listfunds()['outputs']) == 0) - assert account_balance(l2, channel_id) == 0 - - expected_2 = { - 'A': [('cid1', ['channel_open'], ['channel_close'], 'B')], - 'B': [('cid1', ['penalty'], ['to_miner'], 'C'), ('cid1', ['penalty'], ['to_miner'], 'D')], - } - - if anchor_expected(): - expected_2['B'].append(('external', ['anchor'], None, None)) - expected_2['B'].append(('wallet', ['anchor', 'ignored'], None, None)) - - check_utxos_channel(l2, [channel_id], expected_2) - - # Make sure that l2's account is considered closed (has a fee output) - fees = [e for e in l2.rpc.bkpr_listincome()['income_events'] if e['tag'] == 'onchain_fee'] - assert len(fees) == 1 - - @pytest.mark.developer("needs DEVELOPER=1") def test_onchain_first_commit(node_factory, bitcoind): """Onchain handling where opener immediately drops to chain""" @@ -1964,6 +1834,8 @@ def test_onchaind_replay(node_factory, bitcoind): 'OUR_UNILATERAL/DELAYED_OUTPUT_TO_US') assert blocks == 200 bitcoind.generate_block(200) + # Could be RBF! + txid = l1.wait_for_txid_or_rbf(txid) bitcoind.generate_block(1, wait_for_mempool=txid) @@ -2457,6 +2329,8 @@ def try_pay(): assert not t.is_alive() # 100 blocks after last spend, l1+l2 should be done. + # Could be RBF! + txid = l1.wait_for_txid_or_rbf(txid) l2.bitcoin.generate_block(100, wait_for_mempool=txid) l1.daemon.wait_for_log('onchaind complete, forgetting peer') l2.daemon.wait_for_log('onchaind complete, forgetting peer') @@ -2577,7 +2451,8 @@ def test_onchain_feechange(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) - # Make sure that gets included. + # Could be RBF! + txid = l1.wait_for_txid_or_rbf(txid) bitcoind.generate_block(1, wait_for_mempool=txid) # Now we restart with different feerates. l1.stop() diff --git a/tests/test_pay.py b/tests/test_pay.py index 8a23264875ec..1861e0aee331 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1626,6 +1626,8 @@ def test_forward_local_failed_stats(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) + # Could be RBF! + txid = l2.wait_for_txid_or_rbf(txid) bitcoind.generate_block(1, wait_for_mempool=txid) l2.daemon.wait_for_log('Resolved THEIR_UNILATERAL/OUR_HTLC by our proposal OUR_HTLC_TIMEOUT_TO_US') l4.daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC') diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 58148b434b1e..a792c1990af8 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1337,6 +1337,8 @@ def test_forward_event_notification(node_factory, bitcoind, executor): assert blocks == 5 bitcoind.generate_block(5) + # Could be RBF! + txid = l2.wait_for_txid_or_rbf(txid) bitcoind.generate_block(1, wait_for_mempool=txid) l2.daemon.wait_for_log('Resolved THEIR_UNILATERAL/OUR_HTLC by our proposal OUR_HTLC_TIMEOUT_TO_US') l5.daemon.wait_for_log('Ignoring output.*: OUR_UNILATERAL/THEIR_HTLC')