anchorspend: fix weight estimation.

Now we've fixed various underlying weight mis-estimation, we can do a few final tweaks and
test that anchors work as intended.

1. We assumed a p2wpkh output, but modern change is p2tr.
2. We handed `2` instead of `1` to bitcoin_tx_core_weight (which doesn't matter, but was weird).
3. Make change calculation clearer.  I'm not sure the previous one was wrong, but it was harder
   to understand.
4. Fix the test and make it clearly test that we are aiming for (and achieving) the right feerate.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Protocol: anchors' fees are now much closer to the feerate targets.

Author: rustyrussell

lightningd/anchorspend.c

@@ -232,13 +232,14 @@ struct anchor_details *create_anchor_details(const tal_t *ctx,
 	return adet;
 }
 
-/* total_weight includes the commitment tx we're trying to push! */
+/* total_weight includes the commitment tx we're trying to push, and the anchor fee output */
 static struct wally_psbt *anchor_psbt(const tal_t *ctx,
 				      struct channel *channel,
 				      const struct one_anchor *anch,
 				      struct utxo **utxos,
 				      u32 feerate_target,
-				      size_t total_weight)
+				      size_t total_weight,
+				      bool insufficient_funds)
 {
 	struct lightningd *ld = channel->peer->ld;
 	struct wally_psbt *psbt;
@@ -263,12 +264,23 @@ static struct wally_psbt *anchor_psbt(const tal_t *ctx,
 				AMOUNT_SAT(330));
 	psbt_input_add_pubkey(psbt, psbt->num_inputs - 1, &channel->local_funding_pubkey, false);
 
-	/* A zero-output tx is invalid: we must have change, even if not really economic */
+	/* Calculate fee we need, given rate and weight */
+	fee = amount_tx_fee(feerate_target, total_weight);
+	/* Some fee already paid by commitment tx */
+	if (!amount_sat_sub(&fee, fee, anch->info.commitment_fee))
+		fee = AMOUNT_SAT(0);
+
+	/* How much do we have? */
 	change = psbt_compute_fee(psbt);
-	/* Assume we add a change output, what would the total fee be? */
-	fee = amount_tx_fee(feerate_target, total_weight + change_weight());
+
+	/* We have to pay dust, at least! */
 	if (!amount_sat_sub(&change, change, fee)
 	    || amount_sat_less(change, chainparams->dust_limit)) {
+		/* If we didn't run out of UTXOs, this implies our estimation was wrong! */
+		if (!insufficient_funds)
+			log_broken(channel->log, "anchor: could not afford fee %s from change %s, reducing fee",
+				   fmt_amount_sat(tmpctx, fee),
+				   fmt_amount_sat(tmpctx, psbt_compute_fee(psbt)));
 		change = chainparams->dust_limit;
 	}
 
@@ -293,6 +305,7 @@ static struct wally_psbt *try_anchor_psbt(const tal_t *ctx,
 	struct lightningd *ld = channel->peer->ld;
 	struct wally_psbt *psbt;
 	struct amount_sat fee;
+	bool insufficient_funds;
 
 	/* Ask for some UTXOs which could meet this feerate, and since
 	 * we need one output, meet the minumum output requirement */
@@ -303,11 +316,11 @@ static struct wally_psbt *try_anchor_psbt(const tal_t *ctx,
 				   anch->info.commitment_fee,
 				   chainparams->dust_limit,
 				   feerate_target,
-				   total_weight, NULL);
+				   total_weight, &insufficient_funds);
 
 	/* Create a new candidate PSBT */
 	psbt = anchor_psbt(ctx, channel, anch, *utxos, feerate_target,
-			   *total_weight);
+			   *total_weight, insufficient_funds);
 	*fee_spent = psbt_compute_fee(psbt);
 
 	/* Add in base commitment fee to calculate *overall* package feerate */
@@ -336,11 +349,11 @@ static struct bitcoin_tx *spend_anchor(const tal_t *ctx,
 	const u8 *msg;
 
 	/* Estimate weight of anchorspend tx plus commitment_tx (not including any UTXO we add) */
-	base_weight = bitcoin_tx_core_weight(2, 1)
+	base_weight = bitcoin_tx_core_weight(1, 1)
 		+ bitcoin_tx_input_weight(false,
 					  bitcoin_tx_input_sig_weight()
 					  + 1 + tal_bytelen(anch->adet->anchor_wscript))
-		+ bitcoin_tx_output_weight(BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN)
+		+ change_weight()
 		+ anch->info.commitment_weight;
 
 	total_value = AMOUNT_MSAT(0);
@@ -387,15 +400,17 @@ static struct bitcoin_tx *spend_anchor(const tal_t *ctx,
 						 &fee,
 						 &feerate,
 						 &utxos);
+		log_debug(channel->log, "candidate_psbt total weight = %zu (commitment weight %u, anchor %zu)",
+			  weight, anch->info.commitment_weight, weight - anch->info.commitment_weight);
 
 		/* Is it even worth spending this fee to meet the deadline? */
 		if (!amount_msat_greater_sat(total_value, fee)) {
 			log_debug(channel->log,
-				  "Not worth fee %s for %s commit tx to get %s in %u blocks at feerate %uperkw",
+				  "Not worth fee %s for %s commit tx to get %s at block %u (%+i) at feerate %uperkw",
 				  fmt_amount_sat(tmpctx, fee),
 				  anch->commit_side == LOCAL ? "local" : "remote",
 				  fmt_amount_msat(tmpctx, val->msat),
-				  val->block, feerate_target);
+				  val->block, val->block - get_block_height(ld->topology), feerate_target);
 			break;
 		}
 
@@ -420,11 +435,11 @@ static struct bitcoin_tx *spend_anchor(const tal_t *ctx,
 			break;
 		}
 
-		log_debug(channel->log, "Worth fee %s for %s commit tx to get %s in %u blocks at feerate %uperkw",
+		log_debug(channel->log, "Worth fee %s for %s commit tx to get %s at block %u (%+i) at feerate %uperkw",
 			  fmt_amount_sat(tmpctx, fee),
 			  anch->commit_side == LOCAL ? "local" : "remote",
 			  fmt_amount_msat(tmpctx, val->msat),
-			  val->block, feerate);
+			  val->block, val->block - get_block_height(ld->topology), feerate);
 		psbt = candidate_psbt;
 		psbt_fee = fee;
 		psbt_weight = weight;
@@ -539,6 +554,7 @@ static struct bitcoin_tx *spend_anchor(const tal_t *ctx,
 	tx->wtx = psbt_final_tx(tx, signed_psbt);
 	assert(tx->wtx);
 	tx->psbt = tal_steal(tx, signed_psbt);
+	log_debug(channel->log, "anchor actual weight: %zu", bitcoin_tx_weight(tx));
 
 	return tx;
 }

tests/test_closing.py

@@ -4000,26 +4000,36 @@ def test_peer_anchor_push(node_factory, bitcoind, executor, chainparams):
     # We put l3's tx in the mempool, but won't mine it.
     bitcoind.rpc.sendrawtransaction(closetx)
 
-    # We aim for feerate ~3750, so this won't mine it.
-    # HTLC's going to time out at block 119
-    for block in range(108, 119):
+    # We aim for feerate ~3750, so this won't mine l3's unilateral close.
+    # HTLC's going to time out at block 120 (we give one block grace)
+    for block in range(110, 120):
         bitcoind.generate_block(1, needfeerate=5000)
+        assert bitcoind.rpc.getblockcount() == block
         sync_blockheight(bitcoind, [l2])
+    assert only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL'
 
     # Drops to chain
+    bitcoind.generate_block(1, needfeerate=5000)
     wait_for(lambda: only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['state'] == 'AWAITING_UNILATERAL')
 
-    # But, l3's tx already there, and identical feerate will not RBF
+    # But, l3's tx already there, and identical feerate will not RBF.
     l2.daemon.wait_for_log("rejecting replacement")
-    assert bitcoind.rpc.getrawmempool() != []
+    wait_for(lambda: len(bitcoind.rpc.getrawmempool()) == 2)
 
     # As blocks pass, we will use anchor to boost l3's tx.
-    for block in range(119, 124):
-        bitcoind.generate_block(1, needfeerate=5000)
+    for block, feerate in zip(range(120, 124), (12000, 13000, 14000, 15000)):
+        l2.daemon.wait_for_log(fr"Worth fee [0-9]*sat for remote commit tx to get 100000000msat at block 125 \(\+{125 - block}\) at feerate {feerate}perkw")
+        bitcoind.generate_block(1, needfeerate=16000)
         sync_blockheight(bitcoind, [l2])
+        assert len(bitcoind.rpc.getrawmempool()) == 2
 
-    # mempool should be empty, but our 'needfeerate' logic is bogus and leaves
-    # the anchor spend tx!  So just check that l2 did see the commitment tx
+    # Feerate tops out at +1, so this is the same.  This time we mine it!
+    l2.daemon.wait_for_log(fr"Worth fee [0-9]*sat for remote commit tx to get 100000000msat at block 125 \(\+1\) at feerate 15000perkw")
+    l2.daemon.wait_for_log("sendrawtx exit 0")
+
+    bitcoind.generate_block(1, needfeerate=15000)
+
+    assert bitcoind.rpc.getrawmempool() == []
     wait_for(lambda: only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['state'] == 'ONCHAIN')
 
 
@@ -4274,7 +4284,7 @@ def censoring_sendrawtx(r):
     height = bitcoind.rpc.getblockchaininfo()['blocks']
     l1.daemon.wait_for_log(r"Low-priority anchorspend aiming for block {} \(feerate 7458\)".format(height + 13))
     # Can be out-by-one (short sig)!
-    l1.daemon.wait_for_log(r"Anchorspend for local commit tx fee 12037 \(w=674\), commit_tx fee 1735sat \(w=768\): package feerate 9550 perkw")
+    l1.daemon.wait_for_log(r"Anchorspend for local commit tx fee 9377sat \(w=722\), commit_tx fee 1735sat \(w=768\): package feerate 7457 perkw")
     assert not l1.daemon.is_in_log("Low-priority anchorspend aiming for block {}".format(height + 12))
 
     bitcoind.generate_block(1)