Skip to content

Commit 187a2cb

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #3285 from TheBlueMatt/2024-08-tx-too-small
Correct handling of added `OP_RETURN` outputs
2 parents 660f5c2 + 5f5c275 commit 187a2cb

File tree

2 files changed

+254
-83
lines changed

2 files changed

+254
-83
lines changed

‎lightning/src/events/bump_transaction.rs

+224-82
Original file line numberDiff line numberDiff line change
@@ -582,11 +582,25 @@ where
582582
// match, but we still need to have at least one output in the transaction for it to be
583583
// considered standard. We choose to go with an empty OP_RETURN as it is the cheapest
584584
// way to include a dummy output.
585-
log_debug!(self.logger, "Including dummy OP_RETURN output since an output is needed and a change output was not provided");
586-
tx.output.push(TxOut {
587-
value: Amount::ZERO,
588-
script_pubkey: ScriptBuf::new_op_return(&[]),
589-
});
585+
if tx.input.len() <= 1 {
586+
// Transactions have to be at least 65 bytes in non-witness data, which we can run
587+
// under if we have too few witness inputs.
588+
log_debug!(self.logger, "Including large OP_RETURN output since an output is needed and a change output was not provided and the transaction is small");
589+
debug_assert!(!tx.input.is_empty());
590+
tx.output.push(TxOut {
591+
value: Amount::ZERO,
592+
// Minimum transaction size is 60 bytes, so we need a 5-byte script to get a
593+
// 65 byte transaction. We do that as OP_RETURN <3 0 bytes, plus 1 byte len>.
594+
script_pubkey: ScriptBuf::new_op_return(&[0, 0, 0]),
595+
});
596+
debug_assert_eq!(tx.base_size(), 65);
597+
} else {
598+
log_debug!(self.logger, "Including dummy OP_RETURN output since an output is needed and a change output was not provided");
599+
tx.output.push(TxOut {
600+
value: Amount::ZERO,
601+
script_pubkey: ScriptBuf::new_op_return(&[]),
602+
});
603+
}
590604
}
591605
}
592606

@@ -603,85 +617,102 @@ where
603617
let mut anchor_utxo = anchor_descriptor.previous_utxo();
604618
let commitment_tx_fee_sat = Amount::from_sat(commitment_tx_fee_sat);
605619
anchor_utxo.value += commitment_tx_fee_sat;
606-
let must_spend = vec![Input {
607-
outpoint: anchor_descriptor.outpoint,
608-
previous_utxo: anchor_utxo,
609-
satisfaction_weight: commitment_tx.weight().to_wu() + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT,
610-
}];
611-
#[cfg(debug_assertions)]
612-
let must_spend_amount = must_spend.iter().map(|input| input.previous_utxo.value).sum::<Amount>();
613-
614-
log_debug!(self.logger, "Performing coin selection for commitment package (commitment and anchor transaction) targeting {} sat/kW",
615-
package_target_feerate_sat_per_1000_weight);
616-
let coin_selection: CoinSelection = self.utxo_source.select_confirmed_utxos(
617-
claim_id, must_spend, &[], package_target_feerate_sat_per_1000_weight,
618-
)?;
619-
620-
let mut anchor_tx = Transaction {
621-
version: Version::TWO,
622-
lock_time: LockTime::ZERO, // TODO: Use next best height.
623-
input: vec![anchor_descriptor.unsigned_tx_input()],
624-
output: vec![],
625-
};
626-
627-
#[cfg(debug_assertions)]
628-
let total_satisfaction_weight = ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT +
629-
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>();
630-
#[cfg(debug_assertions)]
631-
let total_input_amount = must_spend_amount +
632-
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum();
633-
634-
self.process_coin_selection(&mut anchor_tx, &coin_selection);
635-
let anchor_txid = anchor_tx.compute_txid();
620+
let starting_package_and_fixed_input_satisfaction_weight =
621+
commitment_tx.weight().to_wu() + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT;
622+
let mut package_and_fixed_input_satisfaction_weight =
623+
starting_package_and_fixed_input_satisfaction_weight;
624+
625+
loop {
626+
let must_spend = vec![Input {
627+
outpoint: anchor_descriptor.outpoint,
628+
previous_utxo: anchor_utxo.clone(),
629+
satisfaction_weight: package_and_fixed_input_satisfaction_weight,
630+
}];
631+
let must_spend_amount = must_spend.iter().map(|input| input.previous_utxo.value).sum::<Amount>();
632+
633+
log_debug!(self.logger, "Performing coin selection for commitment package (commitment and anchor transaction) targeting {} sat/kW",
634+
package_target_feerate_sat_per_1000_weight);
635+
let coin_selection: CoinSelection = self.utxo_source.select_confirmed_utxos(
636+
claim_id, must_spend, &[], package_target_feerate_sat_per_1000_weight,
637+
)?;
638+
639+
let mut anchor_tx = Transaction {
640+
version: Version::TWO,
641+
lock_time: LockTime::ZERO, // TODO: Use next best height.
642+
input: vec![anchor_descriptor.unsigned_tx_input()],
643+
output: vec![],
644+
};
636645

637-
// construct psbt
638-
let mut anchor_psbt = Psbt::from_unsigned_tx(anchor_tx).unwrap();
639-
// add witness_utxo to anchor input
640-
anchor_psbt.inputs[0].witness_utxo = Some(anchor_descriptor.previous_utxo());
641-
// add witness_utxo to remaining inputs
642-
for (idx, utxo) in coin_selection.confirmed_utxos.into_iter().enumerate() {
643-
// add 1 to skip the anchor input
644-
let index = idx + 1;
645-
debug_assert_eq!(anchor_psbt.unsigned_tx.input[index].previous_output, utxo.outpoint);
646-
if utxo.output.script_pubkey.is_witness_program() {
647-
anchor_psbt.inputs[index].witness_utxo = Some(utxo.output);
646+
let total_satisfaction_weight = ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT +
647+
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.satisfaction_weight).sum::<u64>();
648+
let total_input_amount = must_spend_amount +
649+
coin_selection.confirmed_utxos.iter().map(|utxo| utxo.output.value).sum();
650+
651+
self.process_coin_selection(&mut anchor_tx, &coin_selection);
652+
let anchor_txid = anchor_tx.compute_txid();
653+
654+
// construct psbt
655+
let mut anchor_psbt = Psbt::from_unsigned_tx(anchor_tx).unwrap();
656+
// add witness_utxo to anchor input
657+
anchor_psbt.inputs[0].witness_utxo = Some(anchor_descriptor.previous_utxo());
658+
// add witness_utxo to remaining inputs
659+
for (idx, utxo) in coin_selection.confirmed_utxos.into_iter().enumerate() {
660+
// add 1 to skip the anchor input
661+
let index = idx + 1;
662+
debug_assert_eq!(anchor_psbt.unsigned_tx.input[index].previous_output, utxo.outpoint);
663+
if utxo.output.script_pubkey.is_witness_program() {
664+
anchor_psbt.inputs[index].witness_utxo = Some(utxo.output);
665+
}
648666
}
649-
}
650-
651-
debug_assert_eq!(anchor_psbt.unsigned_tx.output.len(), 1);
652-
#[cfg(debug_assertions)]
653-
let unsigned_tx_weight = anchor_psbt.unsigned_tx.weight().to_wu() - (anchor_psbt.unsigned_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT);
654667

655-
log_debug!(self.logger, "Signing anchor transaction {}", anchor_txid);
656-
anchor_tx = self.utxo_source.sign_psbt(anchor_psbt)?;
668+
debug_assert_eq!(anchor_psbt.unsigned_tx.output.len(), 1);
669+
let unsigned_tx_weight = anchor_psbt.unsigned_tx.weight().to_wu() - (anchor_psbt.unsigned_tx.input.len() as u64 * EMPTY_SCRIPT_SIG_WEIGHT);
657670

658-
let signer = anchor_descriptor.derive_channel_signer(&self.signer_provider);
659-
let anchor_sig = signer.sign_holder_anchor_input(&anchor_tx, 0, &self.secp)?;
660-
anchor_tx.input[0].witness = anchor_descriptor.tx_input_witness(&anchor_sig);
671+
let package_fee = total_input_amount -
672+
anchor_psbt.unsigned_tx.output.iter().map(|output| output.value).sum();
673+
let package_weight = unsigned_tx_weight + 2 /* wit marker */ + total_satisfaction_weight + commitment_tx.weight().to_wu();
674+
if package_fee.to_sat() * 1000 / package_weight < package_target_feerate_sat_per_1000_weight.into() {
675+
// On the first iteration of the loop, we may undershoot the target feerate because
676+
// we had to add an OP_RETURN output in `process_coin_selection` which we didn't
677+
// select sufficient coins for. Here we detect that case and go around again
678+
// seeking additional weight.
679+
if package_and_fixed_input_satisfaction_weight == starting_package_and_fixed_input_satisfaction_weight {
680+
debug_assert!(anchor_psbt.unsigned_tx.output[0].script_pubkey.is_op_return(),
681+
"Coin selection failed to select sufficient coins for its change output");
682+
package_and_fixed_input_satisfaction_weight += anchor_psbt.unsigned_tx.output[0].weight().to_wu();
683+
continue;
684+
} else {
685+
debug_assert!(false, "Coin selection failed to select sufficient coins");
686+
}
687+
}
661688

662-
#[cfg(debug_assertions)] {
663-
let signed_tx_weight = anchor_tx.weight().to_wu();
664-
let expected_signed_tx_weight = unsigned_tx_weight + total_satisfaction_weight;
665-
// Our estimate should be within a 1% error margin of the actual weight and we should
666-
// never underestimate.
667-
assert!(expected_signed_tx_weight >= signed_tx_weight &&
668-
expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight);
689+
log_debug!(self.logger, "Signing anchor transaction {}", anchor_txid);
690+
anchor_tx = self.utxo_source.sign_psbt(anchor_psbt)?;
691+
692+
let signer = anchor_descriptor.derive_channel_signer(&self.signer_provider);
693+
let anchor_sig = signer.sign_holder_anchor_input(&anchor_tx, 0, &self.secp)?;
694+
anchor_tx.input[0].witness = anchor_descriptor.tx_input_witness(&anchor_sig);
695+
696+
#[cfg(debug_assertions)] {
697+
let signed_tx_weight = anchor_tx.weight().to_wu();
698+
let expected_signed_tx_weight = unsigned_tx_weight + 2 /* wit marker */ + total_satisfaction_weight;
699+
// Our estimate should be within a 1% error margin of the actual weight and we should
700+
// never underestimate.
701+
assert!(expected_signed_tx_weight >= signed_tx_weight &&
702+
expected_signed_tx_weight - (expected_signed_tx_weight / 100) <= signed_tx_weight);
703+
704+
let expected_package_fee = Amount::from_sat(fee_for_weight(package_target_feerate_sat_per_1000_weight,
705+
signed_tx_weight + commitment_tx.weight().to_wu()));
706+
// Our feerate should always be at least what we were seeking. It may overshoot if
707+
// the coin selector burned funds to an OP_RETURN without a change output.
708+
assert!(package_fee >= expected_package_fee);
709+
}
669710

670-
let expected_package_fee = Amount::from_sat(fee_for_weight(package_target_feerate_sat_per_1000_weight,
671-
signed_tx_weight + commitment_tx.weight().to_wu()));
672-
let package_fee = total_input_amount -
673-
anchor_tx.output.iter().map(|output| output.value).sum();
674-
// Our fee should be within a 5% error margin of the expected fee based on the
675-
// feerate and transaction weight and we should never pay less than required.
676-
let fee_error_margin = expected_package_fee * 5 / 100;
677-
assert!(package_fee >= expected_package_fee &&
678-
package_fee - fee_error_margin <= expected_package_fee);
711+
log_info!(self.logger, "Broadcasting anchor transaction {} to bump channel close with txid {}",
712+
anchor_txid, commitment_tx.compute_txid());
713+
self.broadcaster.broadcast_transactions(&[&commitment_tx, &anchor_tx]);
714+
return Ok(());
679715
}
680-
681-
log_info!(self.logger, "Broadcasting anchor transaction {} to bump channel close with txid {}",
682-
anchor_txid, commitment_tx.compute_txid());
683-
self.broadcaster.broadcast_transactions(&[&commitment_tx, &anchor_tx]);
684-
Ok(())
685716
}
686717

687718
/// Handles a [`BumpTransactionEvent::HTLCResolution`] event variant by producing a
@@ -778,11 +809,9 @@ where
778809
let expected_signed_tx_fee = fee_for_weight(target_feerate_sat_per_1000_weight, signed_tx_weight);
779810
let signed_tx_fee = total_input_amount -
780811
htlc_tx.output.iter().map(|output| output.value.to_sat()).sum::<u64>();
781-
// Our fee should be within a 5% error margin of the expected fee based on the
782-
// feerate and transaction weight and we should never pay less than required.
783-
let fee_error_margin = expected_signed_tx_fee * 5 / 100;
784-
assert!(signed_tx_fee >= expected_signed_tx_fee &&
785-
signed_tx_fee - fee_error_margin <= expected_signed_tx_fee);
812+
// Our feerate should always be at least what we were seeking. It may overshoot if
813+
// the coin selector burned funds to an OP_RETURN without a change output.
814+
assert!(signed_tx_fee >= expected_signed_tx_fee);
786815
}
787816

788817
log_info!(self.logger, "Broadcasting {}", log_tx!(htlc_tx));
@@ -822,3 +851,116 @@ where
822851
}
823852
}
824853
}
854+
855+
#[cfg(test)]
856+
mod tests {
857+
use super::*;
858+
859+
use crate::io::Cursor;
860+
use crate::ln::chan_utils::ChannelTransactionParameters;
861+
use crate::util::ser::Readable;
862+
use crate::util::test_utils::{TestBroadcaster, TestLogger};
863+
use crate::sign::KeysManager;
864+
865+
use bitcoin::hashes::Hash;
866+
use bitcoin::hex::FromHex;
867+
use bitcoin::{Network, ScriptBuf, Transaction, Txid};
868+
869+
struct TestCoinSelectionSource {
870+
// (commitment + anchor value, commitment + input weight, target feerate, result)
871+
expected_selects: Mutex<Vec<(u64, u64, u32, CoinSelection)>>,
872+
}
873+
impl CoinSelectionSource for TestCoinSelectionSource {
874+
fn select_confirmed_utxos(
875+
&self,
876+
_claim_id: ClaimId,
877+
must_spend: Vec<Input>,
878+
_must_pay_to: &[TxOut],
879+
target_feerate_sat_per_1000_weight: u32
880+
) -> Result<CoinSelection, ()> {
881+
let mut expected_selects = self.expected_selects.lock().unwrap();
882+
let (weight, value, feerate, res) = expected_selects.remove(0);
883+
assert_eq!(must_spend.len(), 1);
884+
assert_eq!(must_spend[0].satisfaction_weight, weight);
885+
assert_eq!(must_spend[0].previous_utxo.value.to_sat(), value);
886+
assert_eq!(target_feerate_sat_per_1000_weight, feerate);
887+
Ok(res)
888+
}
889+
fn sign_psbt(&self, psbt: Psbt) -> Result<Transaction, ()> {
890+
let mut tx = psbt.unsigned_tx;
891+
for input in tx.input.iter_mut() {
892+
if input.previous_output.txid != Txid::from_byte_array([44; 32]) {
893+
// Channel output, add a realistic size witness to make the assertions happy
894+
input.witness = Witness::from_slice(&[vec![42; 162]]);
895+
}
896+
}
897+
Ok(tx)
898+
}
899+
}
900+
901+
impl Drop for TestCoinSelectionSource {
902+
fn drop(&mut self) {
903+
assert!(self.expected_selects.lock().unwrap().is_empty());
904+
}
905+
}
906+
907+
#[test]
908+
fn test_op_return_under_funds() {
909+
// Test what happens if we have to select coins but the anchor output value itself suffices
910+
// to pay the required fee.
911+
//
912+
// This tests a case that occurred on mainnet (with the below transaction) where the target
913+
// feerate (of 868 sat/kW) was met by the anchor output's 330 sats alone. This caused the
914+
// use of an OP_RETURN which created a transaction which, at the time, was less than 64
915+
// bytes long (the current code generates a 65 byte transaction instead to meet
916+
// standardness rule). It also tests the handling of selection failure where we selected
917+
// coins which were insufficient once the OP_RETURN output was added, causing us to need to
918+
// select coins again with additional weight.
919+
920+
// Tx 18032ad172a5f28fa6e16392d6cc57ea47895781434ce15d03766cc47a955fb9
921+
let commitment_tx_bytes = Vec::<u8>::from_hex("02000000000101cc6b0a9dd84b52c07340fff6fab002fc37b4bdccfdce9f39c5ec8391a56b652907000000009b948b80044a01000000000000220020b4182433fdfdfbf894897c98f84d92cec815cee222755ffd000ae091c9dadc2d4a01000000000000220020f83f7dbf90e2de325b5bb6bab0ae370151278c6964739242b2e7ce0cb68a5d81cb4a02000000000022002024add256b3dccee772610caef82a601045ab6f98fd6d5df608cc756b891ccfe63ffa490000000000220020894bf32b37906a643625e87131897c3714c71b3ac9b161862c9aa6c8d468b4c70400473044022060abd347bff2cca0212b660e6addff792b3356bd4a1b5b26672dc2e694c3c5f002202b40b7e346b494a7b1d048b4ec33ba99c90a09ab48eb1df64ccdc768066c865c014730440220554d8361e04dc0ee178dcb23d2d23f53ec7a1ae4312a5be76bd9e83ab8981f3d0220501f23ffb18cb81ccea72d30252f88d5e69fd28ba4992803d03c00d06fa8899e0147522102817f6ce189ab7114f89e8d5df58cdbbaf272dc8e71b92982d47456a0b6a0ceee2102c9b4d2f24aca54f65e13f4c83e2a8d8e877e12d3c71a76e81f28a5cabc652aa352ae626c7620").unwrap();
922+
let commitment_tx: Transaction = Readable::read(&mut Cursor::new(&commitment_tx_bytes)).unwrap();
923+
let total_commitment_weight = commitment_tx.weight().to_wu() + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT;
924+
let commitment_and_anchor_fee = 930 + 330;
925+
let op_return_weight = TxOut {
926+
value: Amount::ZERO,
927+
script_pubkey: ScriptBuf::new_op_return(&[0; 3]),
928+
}.weight().to_wu();
929+
930+
let broadcaster = TestBroadcaster::new(Network::Testnet);
931+
let source = TestCoinSelectionSource {
932+
expected_selects: Mutex::new(vec![
933+
(total_commitment_weight, commitment_and_anchor_fee, 868, CoinSelection { confirmed_utxos: Vec::new(), change_output: None }),
934+
(total_commitment_weight + op_return_weight, commitment_and_anchor_fee, 868, CoinSelection {
935+
confirmed_utxos: vec![Utxo {
936+
outpoint: OutPoint { txid: Txid::from_byte_array([44; 32]), vout: 0 },
937+
output: TxOut { value: Amount::from_sat(200), script_pubkey: ScriptBuf::new() },
938+
satisfaction_weight: 5, // Just the script_sig and witness lengths
939+
}],
940+
change_output: None,
941+
})
942+
]),
943+
};
944+
let signer = KeysManager::new(&[42; 32], 42, 42);
945+
let logger = TestLogger::new();
946+
let handler = BumpTransactionEventHandler::new(&broadcaster, &source, &signer, &logger);
947+
948+
handler.handle_event(&BumpTransactionEvent::ChannelClose {
949+
channel_id: ChannelId([42; 32]),
950+
counterparty_node_id: PublicKey::from_slice(&[2; 33]).unwrap(),
951+
claim_id: ClaimId([42; 32]),
952+
package_target_feerate_sat_per_1000_weight: 868,
953+
commitment_tx_fee_satoshis: 930,
954+
commitment_tx,
955+
anchor_descriptor: AnchorDescriptor {
956+
channel_derivation_parameters: ChannelDerivationParameters {
957+
value_satoshis: 42_000_000,
958+
keys_id: [42; 32],
959+
transaction_parameters: ChannelTransactionParameters::test_dummy(),
960+
},
961+
outpoint: OutPoint { txid: Txid::from_byte_array([42; 32]), vout: 0 },
962+
},
963+
pending_htlcs: Vec::new(),
964+
});
965+
}
966+
}

‎lightning/src/ln/chan_utils.rs

+30-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,12 @@ pub(crate) const MIN_ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 136;
6868
pub const MAX_ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 143;
6969

7070
/// The upper bound weight of an anchor input.
71-
pub const ANCHOR_INPUT_WITNESS_WEIGHT: u64 = 116;
71+
#[cfg(feature = "grind_signatures")]
72+
pub const ANCHOR_INPUT_WITNESS_WEIGHT: u64 = 114;
73+
/// The upper bound weight of an anchor input.
74+
#[cfg(not(feature = "grind_signatures"))]
75+
pub const ANCHOR_INPUT_WITNESS_WEIGHT: u64 = 115;
76+
7277
/// The upper bound weight of an HTLC timeout input from a commitment transaction with anchor
7378
/// outputs.
7479
pub const HTLC_TIMEOUT_INPUT_ANCHOR_WITNESS_WEIGHT: u64 = 288;
@@ -922,6 +927,30 @@ impl ChannelTransactionParameters {
922927
holder_is_broadcaster: false
923928
}
924929
}
930+
931+
#[cfg(test)]
932+
pub fn test_dummy() -> Self {
933+
let dummy_keys = ChannelPublicKeys {
934+
funding_pubkey: PublicKey::from_slice(&[2; 33]).unwrap(),
935+
revocation_basepoint: PublicKey::from_slice(&[2; 33]).unwrap().into(),
936+
payment_point: PublicKey::from_slice(&[2; 33]).unwrap(),
937+
delayed_payment_basepoint: PublicKey::from_slice(&[2; 33]).unwrap().into(),
938+
htlc_basepoint: PublicKey::from_slice(&[2; 33]).unwrap().into(),
939+
};
940+
Self {
941+
holder_pubkeys: dummy_keys.clone(),
942+
holder_selected_contest_delay: 42,
943+
is_outbound_from_holder: true,
944+
counterparty_parameters: Some(CounterpartyChannelTransactionParameters {
945+
pubkeys: dummy_keys,
946+
selected_contest_delay: 42,
947+
}),
948+
funding_outpoint: Some(chain::transaction::OutPoint {
949+
txid: Txid::from_byte_array([42; 32]), index: 0
950+
}),
951+
channel_type_features: ChannelTypeFeatures::empty(),
952+
}
953+
}
925954
}
926955

927956
impl_writeable_tlv_based!(CounterpartyChannelTransactionParameters, {

0 commit comments

Comments
 (0)