Fixups for #2419

[dunxen]

Fixes#2955.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 96.82540% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 90.22%. Comparing base (ac9a2c8) to head (937f8bf).
Report is 39 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/interactivetxs.rs	97.34%	8 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	0.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2981 +/- ##
==========================================
+ Coverage 89.42% 90.22% +0.79% 
==========================================
Files 117 118 +1 Lines 96290 104849 +8559 Branches 96290 104849 +8559 ==========================================
+ Hits 86109 94600 +8491 - Misses 7962 8103 +141 + Partials 2219 2146 -73 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dunxen]

The most recent commit is a rough exploration of how we might handle splice in and out. Definitely needs polish.

[optout21]

LGTM

[TheBlueMatt]

Just some questions about the last commit, sorry for the delay here.

[optout21]

LGTM

[TheBlueMatt]

LGTM.

[dunxen]

Updated last commit. Sorry, there was rebase on main that I did earlier that made it through but these are the changes for this last push:

diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index e514ae63..5ba67ff2 100644
--- a/lightning/src/ln/interactivetxs.rs+++ b/lightning/src/ln/interactivetxs.rs@@ -182,3 +182,23 @@ impl ConstructedTransaction {
-	pub fn build_unsigned_tx(&self) -> Result<Transaction, AbortReason> {+	pub fn weight(&self) -> u64 {+ let inputs_weight = self.remote_inputs.iter().chain(self.local_inputs.iter()).fold(+ 0u64,+ |weight, InteractiveTxInput { prev_output, .. }| {+ weight.saturating_add(estimate_input_weight(prev_output))+ },+ );+ let outputs_weight = self.remote_outputs.iter().chain(self.local_outputs.iter()).fold(+ 0u64,+ |weight, InteractiveTxOutput { tx_out, .. }| {+ weight.saturating_add(get_output_weight(&tx_out.script_pubkey))+ },+ );++ let tx =+ Transaction { version: 2, lock_time: self.lock_time, input: vec![], output: vec![] };++ tx.weight().to_wu().saturating_add(inputs_weight).saturating_add(outputs_weight)+	}++	pub fn into_unsigned_tx(self) -> Result<Transaction, AbortReason> {
// Inputs and outputs must be sorted by serial_id
@@ -194,5 +214,5 @@ impl ConstructedTransaction {
let input: Vec<TxIn> =
- inputs.clone().into_iter().map(|InteractiveTxInput { input, .. }| input).collect();+ inputs.into_iter().map(|InteractiveTxInput { input, .. }| input).collect();
let output: Vec<TxOut> =
- outputs.clone().into_iter().map(|InteractiveTxOutput { tx_out, .. }| tx_out).collect();+ outputs.into_iter().map(|InteractiveTxOutput { tx_out, .. }| tx_out).collect();@@ -219,2 +239,14 @@ struct NegotiationContext {
+pub(crate) fn estimate_input_weight(prev_output: &TxOut) -> u64 {+	if prev_output.script_pubkey.is_v0_p2wpkh() {+ P2WPKH_INPUT_WEIGHT_LOWER_BOUND+	} else if prev_output.script_pubkey.is_v0_p2wsh() {+ P2WSH_INPUT_WEIGHT_LOWER_BOUND+	} else if prev_output.script_pubkey.is_v1_p2tr() {+ P2TR_INPUT_WEIGHT_LOWER_BOUND+	} else {+ UNKNOWN_SEGWIT_VERSION_INPUT_WEIGHT_LOWER_BOUND+	}+}+
pub(crate) fn get_output_weight(script_pubkey: &ScriptBuf) -> u64 {
@@ -482,11 +514,3 @@ impl NegotiationContext {
total_inputs_weight =
- total_inputs_weight.saturating_add(if prev_output.script_pubkey.is_v0_p2wpkh() {- P2WPKH_INPUT_WEIGHT_LOWER_BOUND- } else if prev_output.script_pubkey.is_v0_p2wsh() {- P2WSH_INPUT_WEIGHT_LOWER_BOUND- } else if prev_output.script_pubkey.is_v1_p2tr() {- P2TR_INPUT_WEIGHT_LOWER_BOUND- } else {- UNKNOWN_SEGWIT_VERSION_INPUT_WEIGHT_LOWER_BOUND- });+ total_inputs_weight.saturating_add(estimate_input_weight(prev_output));
}
@@ -552,4 +576,3 @@ impl NegotiationContext {
- let unsigned_tx = constructed_tx.build_unsigned_tx()?;- if unsigned_tx.weight().to_wu() > MAX_STANDARD_TX_WEIGHT as u64 {+ if constructed_tx.weight() > MAX_STANDARD_TX_WEIGHT as u64 {
return Err(AbortReason::TransactionTooLarge);
@@ -1233,4 +1256,4 @@ mod tests {
assert_eq!(
- final_tx_a.unwrap().build_unsigned_tx().unwrap(),- final_tx_b.unwrap().build_unsigned_tx().unwrap()+ final_tx_a.unwrap().into_unsigned_tx().unwrap(),+ final_tx_b.unwrap().into_unsigned_tx().unwrap()
);

[dunxen]

Changes: git diff-tree -U1 00edceff6 3ced4e329

git diff-tree -U1 00edceff6 3ced4e329
diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index 5ba67ff2..9f8ca4d1 100644
--- a/lightning/src/ln/interactivetxs.rs+++ b/lightning/src/ln/interactivetxs.rs@@ -126,6 +126,2 @@ pub(crate) struct ConstructedTransaction {
-	remote_fees: u64,-	local_fees: u64,-	total_fees: u64,-
lock_time: AbsoluteLockTime,
@@ -157,6 +153,2 @@ impl ConstructedTransaction {
- let local_fees = local_inputs_value_satoshis.saturating_sub(local_outputs_value_satoshis);- let remote_fees =- remote_inputs_value_satoshis.saturating_sub(remote_outputs_value_satoshis);-
Self {
@@ -174,6 +166,2 @@ impl ConstructedTransaction {
- local_fees,- remote_fees,- total_fees: local_fees.saturating_add(remote_fees),-
lock_time,
@@ -202,3 +190,3 @@ impl ConstructedTransaction {
-	pub fn into_unsigned_tx(self) -> Result<Transaction, AbortReason> {+	pub fn into_unsigned_tx(self) -> Transaction {
// Inputs and outputs must be sorted by serial_id
@@ -207,5 +195,5 @@ impl ConstructedTransaction {
let mut inputs: Vec<InteractiveTxInput> =
- inputs.iter().chain(self.remote_inputs.iter()).map(|input| input.clone()).collect();+ inputs.into_iter().chain(self.remote_inputs.into_iter()).collect();
let mut outputs: Vec<InteractiveTxOutput> =
- outputs.iter().chain(self.remote_outputs.iter()).map(|input| input.clone()).collect();+ outputs.into_iter().chain(self.remote_outputs.into_iter()).collect();
inputs.sort_unstable_by_key(|InteractiveTxInput { serial_id, .. }| *serial_id);
@@ -218,5 +206,3 @@ impl ConstructedTransaction {
- let unsigned_tx = Transaction { version: 2, lock_time: self.lock_time, input, output };-- Ok(unsigned_tx)+ Transaction { version: 2, lock_time: self.lock_time, input, output }
}
@@ -229,7 +215,5 @@ struct NegotiationContext {
received_tx_add_output_count: u16,
-	local_inputs: HashMap<SerialId, InteractiveTxInput>,-	remote_inputs: HashMap<SerialId, InteractiveTxInput>,+	inputs: HashMap<SerialId, InteractiveTxInput>,
prevtx_outpoints: HashSet<OutPoint>,
-	local_outputs: HashMap<SerialId, InteractiveTxOutput>,-	remote_outputs: HashMap<SerialId, InteractiveTxOutput>,+	outputs: HashMap<SerialId, InteractiveTxOutput>,
tx_locktime: AbsoluteLockTime,
@@ -256,18 +240,50 @@ pub(crate) fn get_output_weight(script_pubkey: &ScriptBuf) -> u64 {
+fn is_serial_id_valid_for_counterparty(holder_is_initiator: bool, serial_id: &SerialId) -> bool {+	// A received `SerialId`'s parity must match the role of the counterparty.+	holder_is_initiator == serial_id.is_for_non_initiator()+}+
impl NegotiationContext {
fn is_serial_id_valid_for_counterparty(&self, serial_id: &SerialId) -> bool {
- // A received `SerialId`'s parity must match the role of the counterparty.- self.holder_is_initiator == serial_id.is_for_non_initiator()+ is_serial_id_valid_for_counterparty(self.holder_is_initiator, serial_id)
}
-	fn total_input_count(&self) -> usize {- self.local_inputs.len().saturating_add(self.remote_inputs.len())+	fn remote_inputs_value(&self) -> u64 {+ self.inputs+ .iter()+ .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))+ .fold(0u64, |acc, (_, InteractiveTxInput { prev_output, .. })| {+ acc.saturating_add(prev_output.value)+ })
}
-	fn total_output_count(&self) -> usize {- self.local_outputs.len().saturating_add(self.remote_outputs.len())+	fn remote_outputs_value(&self) -> u64 {+ self.outputs+ .iter()+ .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))+ .fold(0u64, |acc, (_, InteractiveTxOutput { tx_out, .. })| {+ acc.saturating_add(tx_out.value)+ })+	}++	fn remote_inputs_weight(&self) -> u64 {+ self.inputs+ .iter()+ .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))+ .fold(0u64, |weight, (_, InteractiveTxInput { prev_output, .. })| {+ weight.saturating_add(estimate_input_weight(prev_output))+ })
}
-	fn total_input_and_output_count(&self) -> usize {- self.total_input_count().saturating_add(self.total_output_count())+	fn remote_outputs_weight(&self) -> u64 {+ self.outputs+ .iter()+ .filter_map(|(serial_id, output)| {+ if self.is_serial_id_valid_for_counterparty(serial_id) {+ Some(get_output_weight(&output.tx_out.script_pubkey))+ } else {+ None+ }+ })+ .sum()
}
@@ -332,4 +348,3 @@ impl NegotiationContext {
};
- let prev_outpoint = OutPoint { txid, vout: msg.prevtx_out };- match self.remote_inputs.entry(msg.serial_id) {+ match self.inputs.entry(msg.serial_id) {
hash_map::Entry::Occupied(_) => {
@@ -338,5 +353,6 @@ impl NegotiationContext {
// - the `serial_id` is already included in the transaction
- return Err(AbortReason::DuplicateSerialId);+ Err(AbortReason::DuplicateSerialId)
},
hash_map::Entry::Vacant(entry) => {
+ let prev_outpoint = OutPoint { txid, vout: msg.prevtx_out };
entry.insert(InteractiveTxInput {
@@ -350,6 +366,6 @@ impl NegotiationContext {
});
+ self.prevtx_outpoints.insert(prev_outpoint);+ Ok(())
},
}
- self.prevtx_outpoints.insert(prev_outpoint);- Ok(())
}
@@ -361,3 +377,3 @@ impl NegotiationContext {
- self.remote_inputs+ self.inputs
.remove(&msg.serial_id)
@@ -397,3 +413,3 @@ impl NegotiationContext {
let mut outputs_value: u64 = 0;
- for output in self.remote_outputs.iter() {+ for output in self.outputs.iter() {
outputs_value = outputs_value.saturating_add(output.1.tx_out.value);
@@ -427,7 +443,3 @@ impl NegotiationContext {
- let output = InteractiveTxOutput {- serial_id: msg.serial_id,- tx_out: TxOut { value: msg.sats, script_pubkey: msg.script.clone() },- };- match self.remote_outputs.entry(msg.serial_id) {+ match self.outputs.entry(msg.serial_id) {
hash_map::Entry::Occupied(_) => {
@@ -436,9 +448,12 @@ impl NegotiationContext {
// - the `serial_id` is already included in the transaction
- return Err(AbortReason::DuplicateSerialId);+ Err(AbortReason::DuplicateSerialId)
},
hash_map::Entry::Vacant(entry) => {
- entry.insert(output);+ entry.insert(InteractiveTxOutput {+ serial_id: msg.serial_id,+ tx_out: TxOut { value: msg.sats, script_pubkey: msg.script.clone() },+ });+ Ok(())
},
- };- Ok(())+ }
}
@@ -449,3 +464,3 @@ impl NegotiationContext {
}
- if let Some(_) = self.remote_outputs.remove(&msg.serial_id) {+ if let Some(_) = self.outputs.remove(&msg.serial_id) {
Ok(())
@@ -473,3 +488,3 @@ impl NegotiationContext {
}
- self.local_inputs.insert(+ self.inputs.insert(
msg.serial_id,
@@ -481,3 +496,3 @@ impl NegotiationContext {
fn sent_tx_add_output(&mut self, msg: &msgs::TxAddOutput) -> Result<(), AbortReason> {
- self.local_outputs.insert(+ self.outputs.insert(
msg.serial_id,
@@ -492,3 +507,3 @@ impl NegotiationContext {
fn sent_tx_remove_input(&mut self, msg: &msgs::TxRemoveInput) -> Result<(), AbortReason> {
- self.local_inputs.remove(&msg.serial_id);+ self.inputs.remove(&msg.serial_id);
Ok(())
@@ -497,3 +512,3 @@ impl NegotiationContext {
fn sent_tx_remove_output(&mut self, msg: &msgs::TxRemoveOutput) -> Result<(), AbortReason> {
- self.local_outputs.remove(&msg.serial_id);+ self.outputs.remove(&msg.serial_id);
Ok(())
@@ -504,16 +519,4 @@ impl NegotiationContext {
) -> Result<(), AbortReason> {
- let mut counterparty_weight_contributed: u64 = self- .remote_outputs- .values()- .map(|output| get_output_weight(&output.tx_out.script_pubkey))- .sum();- // We don't know the counterparty's witnesses ahead of time obviously, so we use the lower bounds- // specified in BOLT 3.- let mut total_inputs_weight: u64 = 0;- for InteractiveTxInput { prev_output, .. } in self.remote_inputs.values() {- total_inputs_weight =- total_inputs_weight.saturating_add(estimate_input_weight(prev_output));- }- counterparty_weight_contributed =- counterparty_weight_contributed.saturating_add(total_inputs_weight);+ let counterparty_weight_contributed =+ self.remote_inputs_weight().saturating_add(self.remote_outputs_weight());
let mut required_counterparty_contribution_fee =
@@ -539,13 +542,4 @@ impl NegotiationContext {
// - the peer's total input satoshis is less than their outputs
- let remote_inputs_value = self- .remote_inputs- .values()- .fold(0u64, |acc, InteractiveTxInput { prev_output, .. }| {- acc.saturating_add(prev_output.value)- });- let remote_outputs_value = self- .remote_outputs- .values()- .fold(0u64, |acc, InteractiveTxOutput { tx_out, .. }| acc.saturating_add(tx_out.value));-+ let remote_inputs_value = self.remote_inputs_value();+ let remote_outputs_value = self.remote_outputs_value();
// ...actually the counterparty might be splicing out, so that their balance also contributes
@@ -559,4 +553,4 @@ impl NegotiationContext {
// - there are more than 252 outputs
- if self.total_input_count() > MAX_INPUTS_OUTPUTS_COUNT- || self.total_output_count() > MAX_INPUTS_OUTPUTS_COUNT+ if self.inputs.len() > MAX_INPUTS_OUTPUTS_COUNT+ || self.outputs.len() > MAX_INPUTS_OUTPUTS_COUNT
{
@@ -568,7 +562,17 @@ impl NegotiationContext {
+ let holder_is_initiator = self.holder_is_initiator;+ let (remote_inputs, local_inputs): (Vec<_>, Vec<_>) =+ self.inputs.into_values().partition(|InteractiveTxInput { serial_id, .. }| {+ is_serial_id_valid_for_counterparty(holder_is_initiator, serial_id)+ });+ let (remote_outputs, local_outputs): (Vec<_>, Vec<_>) =+ self.outputs.into_values().partition(|InteractiveTxOutput { serial_id, .. }| {+ is_serial_id_valid_for_counterparty(holder_is_initiator, serial_id)+ });+
let constructed_tx = ConstructedTransaction::new(
- self.local_inputs.into_values().collect(),- self.local_outputs.into_values().collect(),- self.remote_inputs.into_values().collect(),- self.remote_outputs.into_values().collect(),+ local_inputs,+ local_outputs,+ remote_inputs,+ remote_outputs,
self.tx_locktime,
@@ -788,7 +792,5 @@ impl StateMachine {
received_tx_add_output_count: 0,
- local_inputs: new_hash_map(),- remote_inputs: new_hash_map(),+ inputs: new_hash_map(),
prevtx_outpoints: new_hash_set(),
- local_outputs: new_hash_map(),- remote_outputs: new_hash_map(),+ outputs: new_hash_map(),
feerate_sat_per_kw,
@@ -1125,3 +1127,3 @@ mod tests {
struct TestSession {
- description: String,+ description: &'static str,
inputs_a: Vec<(TxIn, TransactionU16LenLimited)>,
@@ -1255,6 +1257,3 @@ mod tests {
assert!(message_send_b.is_none());
- assert_eq!(- final_tx_a.unwrap().into_unsigned_tx().unwrap(),- final_tx_b.unwrap().into_unsigned_tx().unwrap()- );+ assert_eq!(final_tx_a.unwrap().into_unsigned_tx(), final_tx_b.unwrap().into_unsigned_tx());
assert!(session.expect_error.is_none(), "Test: {}", session.description);
@@ -1396,5 +1395,4 @@ mod tests {
fn test_interactive_tx_constructor() {
- // No contributions.
do_test_interactive_tx_constructor(TestSession {
- description: "No contributions".to_string(),+ description: "No contributions",
inputs_a: vec![],
@@ -1405,5 +1403,4 @@ mod tests {
});
- // Single contribution, no initiator inputs.
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, no initiator inputs".to_string(),+ description: "Single contribution, no initiator inputs",
inputs_a: vec![],
@@ -1414,5 +1411,4 @@ mod tests {
});
- // Single contribution, no initiator outputs.
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, no initiator outputs".to_string(),+ description: "Single contribution, no initiator outputs",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000)]),
@@ -1423,5 +1419,4 @@ mod tests {
});
- // Single contribution, no fees.
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, no fees".to_string(),+ description: "Single contribution, no fees",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000)]),
@@ -1432,3 +1427,2 @@ mod tests {
});
- // Single contribution with P2WPKH input, insufficient fees.
let p2wpkh_fee = fee_for_weight(TEST_FEERATE_SATS_PER_KW, P2WPKH_INPUT_WEIGHT_LOWER_BOUND);
@@ -1441,3 +1435,3 @@ mod tests {
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, with P2WPKH input, insufficient fees".to_string(),+ description: "Single contribution, with P2WPKH input, insufficient fees",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000)]),
@@ -1450,5 +1444,4 @@ mod tests {
});
- // Single contribution with P2WPKH, sufficient fees
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution with P2WPKH input, sufficient fees".to_string(),+ description: "Single contribution with P2WPKH input, sufficient fees",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000)]),
@@ -1461,6 +1454,5 @@ mod tests {
});
- // Single contribution with P2WSH input, insufficient fees.
let p2wsh_fee = fee_for_weight(TEST_FEERATE_SATS_PER_KW, P2WSH_INPUT_WEIGHT_LOWER_BOUND);
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, with P2WSH input, insufficient fees".to_string(),+ description: "Single contribution, with P2WSH input, insufficient fees",
inputs_a: generate_inputs(&[TestOutput::P2WSH(1_000_000)]),
@@ -1473,5 +1465,4 @@ mod tests {
});
- // Single contribution with P2WSH, sufficient fees
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution with P2WSH input, sufficient fees".to_string(),+ description: "Single contribution with P2WSH input, sufficient fees",
inputs_a: generate_inputs(&[TestOutput::P2WSH(1_000_000)]),
@@ -1484,7 +1475,6 @@ mod tests {
});
- // Single contribution with P2TR input, insufficient fees.
let p2tr_fee = fee_for_weight(TEST_FEERATE_SATS_PER_KW, P2TR_INPUT_WEIGHT_LOWER_BOUND);
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution, with P2TR input, insufficient fees".to_string(),- inputs_a: generate_inputs(&[TestOutput::P2WSH(1_000_000)]),+ description: "Single contribution, with P2TR input, insufficient fees",+ inputs_a: generate_inputs(&[TestOutput::P2TR(1_000_000)]),
outputs_a: generate_outputs(&[TestOutput::P2WPKH(
@@ -1496,5 +1486,4 @@ mod tests {
});
- // Single contribution with P2TR input, sufficient fees
do_test_interactive_tx_constructor(TestSession {
- description: "Single contribution with P2TR input, sufficient fees".to_string(),+ description: "Single contribution with P2TR input, sufficient fees",
inputs_a: generate_inputs(&[TestOutput::P2TR(1_000_000)]),
@@ -1507,6 +1496,4 @@ mod tests {
});
- // Initiator contributes sufficient fees, but non-initiator does not.
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator contributes sufficient fees, but non-initiator does not"- .to_string(),+ description: "Initiator contributes sufficient fees, but non-initiator does not",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000)]),
@@ -1517,5 +1504,4 @@ mod tests {
});
- // Multi-input-output contributions from both sides.
do_test_interactive_tx_constructor(TestSession {
- description: "Multi-input-output contributions from both sides".to_string(),+ description: "Multi-input-output contributions from both sides",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(1_000_000); 2]),
@@ -1536,5 +1522,4 @@ mod tests {
- // Prevout from initiator is not a witness program
do_test_interactive_tx_constructor(TestSession {
- description: "Prevout from initiator is not a witness program".to_string(),+ description: "Prevout from initiator is not a witness program",
inputs_a: generate_inputs(&[TestOutput::P2PKH(1_000_000)]),
@@ -1546,3 +1531,2 @@ mod tests {
- // Invalid input sequence from initiator.
let tx =
@@ -1554,3 +1538,3 @@ mod tests {
do_test_interactive_tx_constructor(TestSession {
- description: "Invalid input sequence from initiator".to_string(),+ description: "Invalid input sequence from initiator",
inputs_a: vec![(invalid_sequence_input, tx.clone())],
@@ -1561,3 +1545,2 @@ mod tests {
});
- // Duplicate prevout from initiator.
let duplicate_input = TxIn {
@@ -1568,3 +1551,3 @@ mod tests {
do_test_interactive_tx_constructor(TestSession {
- description: "Duplicate prevout from initiator".to_string(),+ description: "Duplicate prevout from initiator",
inputs_a: vec![(duplicate_input.clone(), tx.clone()), (duplicate_input, tx.clone())],
@@ -1575,3 +1558,2 @@ mod tests {
});
- // Non-initiator uses same prevout as initiator.
let duplicate_input = TxIn {
@@ -1582,3 +1564,3 @@ mod tests {
do_test_interactive_tx_constructor(TestSession {
- description: "Non-initiator uses same prevout as initiator".to_string(),+ description: "Non-initiator uses same prevout as initiator",
inputs_a: vec![(duplicate_input.clone(), tx.clone())],
@@ -1589,5 +1571,4 @@ mod tests {
});
- // Initiator sends too many TxAddInputs
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator sends too many TxAddInputs".to_string(),+ description: "Initiator sends too many TxAddInputs",
inputs_a: generate_fixed_number_of_inputs(MAX_RECEIVED_TX_ADD_INPUT_COUNT + 1),
@@ -1598,7 +1579,6 @@ mod tests {
});
- // Attempt to queue up two inputs with duplicate serial ids. We use a deliberately bad- // entropy source, `DuplicateEntropySource` to simulate this.
do_test_interactive_tx_constructor_with_entropy_source(
TestSession {
- description: "Attempt to queue up two inputs with duplicate serial ids".to_string(),+ // We use a deliberately bad entropy source, `DuplicateEntropySource` to simulate this.+ description: "Attempt to queue up two inputs with duplicate serial ids",
inputs_a: generate_fixed_number_of_inputs(2),
@@ -1611,5 +1591,4 @@ mod tests {
);
- // Initiator sends too many TxAddOutputs.
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator sends too many TxAddOutputs".to_string(),+ description: "Initiator sends too many TxAddOutputs",
inputs_a: vec![],
@@ -1620,5 +1599,4 @@ mod tests {
});
- // Initiator sends an output below dust value.
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator sends an output below dust value".to_string(),+ description: "Initiator sends an output below dust value",
inputs_a: vec![],
@@ -1631,5 +1609,4 @@ mod tests {
});
- // Initiator sends an output above maximum sats allowed.
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator sends an output above maximum sats allowed".to_string(),+ description: "Initiator sends an output above maximum sats allowed",
inputs_a: vec![],
@@ -1640,5 +1617,4 @@ mod tests {
});
- // Initiator sends an output without a witness program.
do_test_interactive_tx_constructor(TestSession {
- description: "Initiator sends an output without a witness program".to_string(),+ description: "Initiator sends an output without a witness program",
inputs_a: vec![],
@@ -1649,8 +1625,6 @@ mod tests {
});
- // Attempt to queue up two outputs with duplicate serial ids. We use a deliberately bad- // entropy source, `DuplicateEntropySource` to simulate this.
do_test_interactive_tx_constructor_with_entropy_source(
TestSession {
- description: "Attempt to queue up two outputs with duplicate serial ids"- .to_string(),+ // We use a deliberately bad entropy source, `DuplicateEntropySource` to simulate this.+ description: "Attempt to queue up two outputs with duplicate serial ids",
inputs_a: vec![],
@@ -1664,5 +1638,4 @@ mod tests {
- // Peer contributed more output value than inputs
do_test_interactive_tx_constructor(TestSession {
- description: "Peer contributed more output value than inputs".to_string(),+ description: "Peer contributed more output value than inputs",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(100_000)]),
@@ -1674,5 +1647,4 @@ mod tests {
- // Peer contributed more than allowed number of inputs.
do_test_interactive_tx_constructor(TestSession {
- description: "Peer contributed more than allowed number of inputs".to_string(),+ description: "Peer contributed more than allowed number of inputs",
inputs_a: generate_fixed_number_of_inputs(MAX_INPUTS_OUTPUTS_COUNT as u16 + 1),
@@ -1686,5 +1658,4 @@ mod tests {
});
- // Peer contributed more than allowed number of outputs.
do_test_interactive_tx_constructor(TestSession {
- description: "Peer contributed more than allowed number of outputs".to_string(),+ description: "Peer contributed more than allowed number of outputs",
inputs_a: generate_inputs(&[TestOutput::P2WPKH(TOTAL_BITCOIN_SUPPLY_SATOSHIS)]),

[dunxen]

Changes: git diff-tree -U1 3ced4e329 570ebfa07

diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index 9f8ca4d1..88f1aa19 100644
--- a/lightning/src/ln/interactivetxs.rs+++ b/lightning/src/ln/interactivetxs.rs@@ -114,7 +114,6 @@ pub(crate) struct InteractiveTxOutput {
pub(crate) struct ConstructedTransaction {
-	local_inputs: Vec<InteractiveTxInput>,-	local_outputs: Vec<InteractiveTxOutput>,+	holder_is_initiator: bool,-	remote_inputs: Vec<InteractiveTxInput>,-	remote_outputs: Vec<InteractiveTxOutput>,+	inputs: Vec<InteractiveTxInput>,+	outputs: Vec<InteractiveTxOutput>,@@ -131,30 +130,34 @@ impl ConstructedTransaction {
pub fn new(
- local_inputs: Vec<InteractiveTxInput>, local_outputs: Vec<InteractiveTxOutput>,- remote_inputs: Vec<InteractiveTxInput>, remote_outputs: Vec<InteractiveTxOutput>,- lock_time: AbsoluteLockTime,+ holder_is_initiator: bool, inputs: Vec<InteractiveTxInput>,+ outputs: Vec<InteractiveTxOutput>, lock_time: AbsoluteLockTime,
) -> Self {
- let local_inputs_value_satoshis =- local_inputs.iter().fold(0u64, |acc, InteractiveTxInput { ref prev_output, .. }| {- acc.saturating_add(prev_output.value)- });- let local_outputs_value_satoshis =- local_outputs.iter().fold(0u64, |acc, InteractiveTxOutput { ref tx_out, .. }| {- acc.saturating_add(tx_out.value)- });+ let mut local_inputs_value_satoshis = 0u64;+ let mut remote_inputs_value_satoshis = 0u64;+ for input in &inputs {+ if is_serial_id_valid_for_counterparty(holder_is_initiator, &input.serial_id) {+ remote_inputs_value_satoshis =+ remote_inputs_value_satoshis.saturating_add(input.prev_output.value);+ } else {+ local_inputs_value_satoshis =+ local_inputs_value_satoshis.saturating_add(input.prev_output.value);+ }+ }- let remote_inputs_value_satoshis =- remote_inputs.iter().fold(0u64, |acc, InteractiveTxInput { ref prev_output, .. }| {- acc.saturating_add(prev_output.value)- });- let remote_outputs_value_satoshis =- remote_outputs.iter().fold(0u64, |acc, InteractiveTxOutput { ref tx_out, .. }| {- acc.saturating_add(tx_out.value)- });+ let mut local_outputs_value_satoshis = 0u64;+ let mut remote_outputs_value_satoshis = 0u64;+ for output in &outputs {+ if is_serial_id_valid_for_counterparty(holder_is_initiator, &output.serial_id) {+ remote_outputs_value_satoshis =+ remote_outputs_value_satoshis.saturating_add(output.tx_out.value);+ } else {+ local_outputs_value_satoshis =+ local_outputs_value_satoshis.saturating_add(output.tx_out.value);+ }+ }
Self {
- local_inputs,- local_outputs,+ holder_is_initiator,- remote_inputs,- remote_outputs,+ inputs,+ outputs,@@ -171,14 +174,10 @@ impl ConstructedTransaction {
pub fn weight(&self) -> u64 {
- let inputs_weight = self.remote_inputs.iter().chain(self.local_inputs.iter()).fold(- 0u64,- |weight, InteractiveTxInput { prev_output, .. }| {+ let inputs_weight =+ self.inputs.iter().fold(0u64, |weight, InteractiveTxInput { prev_output, .. }| {
weight.saturating_add(estimate_input_weight(prev_output))
- },- );- let outputs_weight = self.remote_outputs.iter().chain(self.local_outputs.iter()).fold(- 0u64,- |weight, InteractiveTxOutput { tx_out, .. }| {+ });+ let outputs_weight =+ self.outputs.iter().fold(0u64, |weight, InteractiveTxOutput { tx_out, .. }| {
weight.saturating_add(get_output_weight(&tx_out.script_pubkey))
- },- );+ });@@ -192,8 +191,4 @@ impl ConstructedTransaction {
// Inputs and outputs must be sorted by serial_id
- let ConstructedTransaction { local_inputs: inputs, local_outputs: outputs, .. } = self;+ let ConstructedTransaction { mut inputs, mut outputs, .. } = self;- let mut inputs: Vec<InteractiveTxInput> =- inputs.into_iter().chain(self.remote_inputs.into_iter()).collect();- let mut outputs: Vec<InteractiveTxOutput> =- outputs.into_iter().chain(self.remote_outputs.into_iter()).collect();
inputs.sort_unstable_by_key(|InteractiveTxInput { serial_id, .. }| *serial_id);
@@ -562,17 +557,6 @@ impl NegotiationContext {
- let holder_is_initiator = self.holder_is_initiator;- let (remote_inputs, local_inputs): (Vec<_>, Vec<_>) =- self.inputs.into_values().partition(|InteractiveTxInput { serial_id, .. }| {- is_serial_id_valid_for_counterparty(holder_is_initiator, serial_id)- });- let (remote_outputs, local_outputs): (Vec<_>, Vec<_>) =- self.outputs.into_values().partition(|InteractiveTxOutput { serial_id, .. }| {- is_serial_id_valid_for_counterparty(holder_is_initiator, serial_id)- });-
let constructed_tx = ConstructedTransaction::new(
- local_inputs,- local_outputs,- remote_inputs,- remote_outputs,+ self.holder_is_initiator,+ self.inputs.into_values().collect(),+ self.outputs.into_values().collect(),
self.tx_locktime,

[dunxen]

Update the last commit message as we no longer split inputs and outputs.

[dunxen]

Changes: git diff-tree -U1 570ebfa07 937f8bf39

diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index 88f1aa19..a667df19 100644
--- a/lightning/src/ln/interactivetxs.rs+++ b/lightning/src/ln/interactivetxs.rs@@ -17,3 +17,4 @@ use bitcoin::policy::MAX_STANDARD_TX_WEIGHT;
use bitcoin::{
-	absolute::LockTime as AbsoluteLockTime, OutPoint, ScriptBuf, Sequence, Transaction, TxIn, TxOut,+	absolute::LockTime as AbsoluteLockTime, OutPoint, ScriptBuf, Sequence, Transaction, TxIn,+	TxOut, Weight,
};
@@ -129,35 +130,21 @@ pub(crate) struct ConstructedTransaction {
impl ConstructedTransaction {
-	pub fn new(- holder_is_initiator: bool, inputs: Vec<InteractiveTxInput>,- outputs: Vec<InteractiveTxOutput>, lock_time: AbsoluteLockTime,-	) -> Self {- let mut local_inputs_value_satoshis = 0u64;- let mut remote_inputs_value_satoshis = 0u64;- for input in &inputs {- if is_serial_id_valid_for_counterparty(holder_is_initiator, &input.serial_id) {- remote_inputs_value_satoshis =- remote_inputs_value_satoshis.saturating_add(input.prev_output.value);- } else {- local_inputs_value_satoshis =- local_inputs_value_satoshis.saturating_add(input.prev_output.value);- }- }+	fn new(context: NegotiationContext) -> Self {+ let local_inputs_value_satoshis = context+ .inputs+ .iter()+ .filter(|(serial_id, _)| {+ !is_serial_id_valid_for_counterparty(context.holder_is_initiator, serial_id)+ })+ .fold(0u64, |value, (_, input)| value.saturating_add(input.prev_output.value));- let mut local_outputs_value_satoshis = 0u64;- let mut remote_outputs_value_satoshis = 0u64;- for output in &outputs {- if is_serial_id_valid_for_counterparty(holder_is_initiator, &output.serial_id) {- remote_outputs_value_satoshis =- remote_outputs_value_satoshis.saturating_add(output.tx_out.value);- } else {- local_outputs_value_satoshis =- local_outputs_value_satoshis.saturating_add(output.tx_out.value);- }- }+ let local_outputs_value_satoshis = context+ .outputs+ .iter()+ .filter(|(serial_id, _)| {+ !is_serial_id_valid_for_counterparty(context.holder_is_initiator, serial_id)+ })+ .fold(0u64, |value, (_, output)| value.saturating_add(output.tx_out.value));
Self {
- holder_is_initiator,-- inputs,- outputs,+ holder_is_initiator: context.holder_is_initiator,@@ -166,6 +153,9 @@ impl ConstructedTransaction {
- remote_inputs_value_satoshis,- remote_outputs_value_satoshis,+ remote_inputs_value_satoshis: context.remote_inputs_value(),+ remote_outputs_value_satoshis: context.remote_outputs_value(),++ inputs: context.inputs.into_values().collect(),+ outputs: context.outputs.into_values().collect(),- lock_time,+ lock_time: context.tx_locktime,
}
@@ -173,6 +163,6 @@ impl ConstructedTransaction {
-	pub fn weight(&self) -> u64 {+	pub fn weight(&self) -> Weight {
let inputs_weight =
self.inputs.iter().fold(0u64, |weight, InteractiveTxInput { prev_output, .. }| {
- weight.saturating_add(estimate_input_weight(prev_output))+ weight.saturating_add(estimate_input_weight(prev_output).to_wu())
});
@@ -180,3 +170,3 @@ impl ConstructedTransaction {
self.outputs.iter().fold(0u64, |weight, InteractiveTxOutput { tx_out, .. }| {
- weight.saturating_add(get_output_weight(&tx_out.script_pubkey))+ weight.saturating_add(get_output_weight(&tx_out.script_pubkey).to_wu())
});
@@ -186,3 +176,5 @@ impl ConstructedTransaction {
- tx.weight().to_wu().saturating_add(inputs_weight).saturating_add(outputs_weight)+ Weight::from_wu(+ tx.weight().to_wu().saturating_add(inputs_weight).saturating_add(outputs_weight),+ )
}
@@ -218,4 +210,4 @@ struct NegotiationContext {
-pub(crate) fn estimate_input_weight(prev_output: &TxOut) -> u64 {-	if prev_output.script_pubkey.is_v0_p2wpkh() {+pub(crate) fn estimate_input_weight(prev_output: &TxOut) -> Weight {+	Weight::from_wu(if prev_output.script_pubkey.is_v0_p2wpkh() {
P2WPKH_INPUT_WEIGHT_LOWER_BOUND
@@ -227,8 +219,10 @@ pub(crate) fn estimate_input_weight(prev_output: &TxOut) -> u64 {
UNKNOWN_SEGWIT_VERSION_INPUT_WEIGHT_LOWER_BOUND
-	}+	})
}
-pub(crate) fn get_output_weight(script_pubkey: &ScriptBuf) -> u64 {-	(8 /* value */ + script_pubkey.consensus_encode(&mut sink()).unwrap() as u64)- * WITNESS_SCALE_FACTOR as u64+pub(crate) fn get_output_weight(script_pubkey: &ScriptBuf) -> Weight {+	Weight::from_wu(+ (8 /* value */ + script_pubkey.consensus_encode(&mut sink()).unwrap() as u64)+ * WITNESS_SCALE_FACTOR as u64,+	)
}
@@ -263,22 +257,22 @@ impl NegotiationContext {
-	fn remote_inputs_weight(&self) -> u64 {- self.inputs- .iter()- .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))- .fold(0u64, |weight, (_, InteractiveTxInput { prev_output, .. })| {- weight.saturating_add(estimate_input_weight(prev_output))- })+	fn remote_inputs_weight(&self) -> Weight {+ Weight::from_wu(+ self.inputs+ .iter()+ .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))+ .fold(0u64, |weight, (_, InteractiveTxInput { prev_output, .. })| {+ weight.saturating_add(estimate_input_weight(prev_output).to_wu())+ }),+ )
}
-	fn remote_outputs_weight(&self) -> u64 {- self.outputs- .iter()- .filter_map(|(serial_id, output)| {- if self.is_serial_id_valid_for_counterparty(serial_id) {- Some(get_output_weight(&output.tx_out.script_pubkey))- } else {- None- }- })- .sum()+	fn remote_outputs_weight(&self) -> Weight {+ Weight::from_wu(+ self.outputs+ .iter()+ .filter(|(serial_id, _)| self.is_serial_id_valid_for_counterparty(serial_id))+ .fold(0u64, |weight, (_, InteractiveTxOutput { tx_out, .. })| {+ weight.saturating_add(get_output_weight(&tx_out.script_pubkey).to_wu())+ }),+ )
}
@@ -514,4 +508,6 @@ impl NegotiationContext {
) -> Result<(), AbortReason> {
- let counterparty_weight_contributed =- self.remote_inputs_weight().saturating_add(self.remote_outputs_weight());+ let counterparty_weight_contributed = self+ .remote_inputs_weight()+ .to_wu()+ .saturating_add(self.remote_outputs_weight().to_wu());
let mut required_counterparty_contribution_fee =
@@ -557,10 +553,5 @@ impl NegotiationContext {
- let constructed_tx = ConstructedTransaction::new(- self.holder_is_initiator,- self.inputs.into_values().collect(),- self.outputs.into_values().collect(),- self.tx_locktime,- );+ let constructed_tx = ConstructedTransaction::new(self);- if constructed_tx.weight() > MAX_STANDARD_TX_WEIGHT as u64 {+ if constructed_tx.weight().to_wu() > MAX_STANDARD_TX_WEIGHT as u64 {
return Err(AbortReason::TransactionTooLarge);
@@ -1414,3 +1405,3 @@ mod tests {
TEST_FEERATE_SATS_PER_KW,
- get_output_weight(&generate_p2wpkh_script_pubkey()),+ get_output_weight(&generate_p2wpkh_script_pubkey()).to_wu(),
);

[TheBlueMatt]

Otherwise lgtm.

[TheBlueMatt]

Wont this always fail for a splice-out cause the param will be 0 (we ignore self.to_remote_value_satoshis) and check_counterparty_fees will always fail cause the required fee is non-0?

[dunxen]

We decided to fix the splicing-specific issues in #2989 which is WIP as we wanted to unblock dual-funding first.

I'm going to go ahead and remove any mention of to_remote_value_satoshis in this PR and have that all handled in #2989, though :)

[dunxen]

Changes: git diff-tree -U1 937f8bf39 59a8bd5d6

diff --git a/lightning/src/ln/interactivetxs.rs b/lightning/src/ln/interactivetxs.rs
index a667df19..5f01bcd1 100644
--- a/lightning/src/ln/interactivetxs.rs+++ b/lightning/src/ln/interactivetxs.rs@@ -164,17 +164,18 @@ impl ConstructedTransaction {
pub fn weight(&self) -> Weight {
- let inputs_weight =- self.inputs.iter().fold(0u64, |weight, InteractiveTxInput { prev_output, .. }| {- weight.saturating_add(estimate_input_weight(prev_output).to_wu())- });- let outputs_weight =- self.outputs.iter().fold(0u64, |weight, InteractiveTxOutput { tx_out, .. }| {- weight.saturating_add(get_output_weight(&tx_out.script_pubkey).to_wu())- });-- let tx =- Transaction { version: 2, lock_time: self.lock_time, input: vec![], output: vec![] };-- Weight::from_wu(- tx.weight().to_wu().saturating_add(inputs_weight).saturating_add(outputs_weight),- )+ let inputs_weight = self.inputs.iter().fold(+ Weight::from_wu(0),+ |weight, InteractiveTxInput { prev_output, .. }| {+ weight.checked_add(estimate_input_weight(prev_output)).unwrap_or(Weight::MAX)+ },+ );+ let outputs_weight = self.outputs.iter().fold(+ Weight::from_wu(0),+ |weight, InteractiveTxOutput { tx_out, .. }| {+ weight.checked_add(get_output_weight(&tx_out.script_pubkey)).unwrap_or(Weight::MAX)+ },+ );+ Weight::from_wu(TX_COMMON_FIELDS_WEIGHT)+ .checked_add(inputs_weight)+ .and_then(|weight| weight.checked_add(outputs_weight))+ .unwrap_or(Weight::MAX)
}
@@ -207,3 +208,2 @@ struct NegotiationContext {
feerate_sat_per_kw: u32,
-	to_remote_value_satoshis: u64,
}
@@ -349,3 +349,3 @@ impl NegotiationContext {
input: TxIn {
- previous_output: prev_outpoint.clone(),+ previous_output: prev_outpoint,
sequence: Sequence(msg.sequence),
@@ -453,3 +453,3 @@ impl NegotiationContext {
}
- if let Some(_) = self.outputs.remove(&msg.serial_id) {+ if self.outputs.remove(&msg.serial_id).is_some() {
Ok(())
@@ -473,3 +473,3 @@ impl NegotiationContext {
tx.output.get(msg.prevtx_out as usize).ok_or(AbortReason::PrevTxOutInvalid)?.clone();
- if !self.prevtx_outpoints.insert(input.previous_output.clone()) {+ if !self.prevtx_outpoints.insert(input.previous_output) {
// We have added an input that already exists
@@ -535,6 +535,3 @@ impl NegotiationContext {
let remote_outputs_value = self.remote_outputs_value();
- // ...actually the counterparty might be splicing out, so that their balance also contributes- // to the total input value.- if remote_inputs_value.saturating_add(self.to_remote_value_satoshis) < remote_outputs_value- {+ if remote_inputs_value < remote_outputs_value {
return Err(AbortReason::OutputsValueExceedsInputsValue);
@@ -758,6 +755,3 @@ macro_rules! define_state_machine_transitions {
impl StateMachine {
-	fn new(- feerate_sat_per_kw: u32, is_initiator: bool, tx_locktime: AbsoluteLockTime,- to_remote_value_satoshis: u64,-	) -> Self {+	fn new(feerate_sat_per_kw: u32, is_initiator: bool, tx_locktime: AbsoluteLockTime) -> Self {
let context = NegotiationContext {
@@ -771,3 +765,2 @@ impl StateMachine {
feerate_sat_per_kw,
- to_remote_value_satoshis,
};
@@ -879,5 +872,2 @@ impl InteractiveTxConstructor {
///
-	/// If this is for a dual_funded channel then the `to_remote_value_satoshis` parameter should be set-	/// to zero.-	///
/// A tuple is returned containing the newly instantiate `InteractiveTxConstructor` and optionally
@@ -888,3 +878,3 @@ impl InteractiveTxConstructor {
inputs_to_contribute: Vec<(TxIn, TransactionU16LenLimited)>,
- outputs_to_contribute: Vec<TxOut>, to_remote_value_satoshis: u64,+ outputs_to_contribute: Vec<TxOut>,
) -> (Self, Option<InteractiveTxMessageSend>)
@@ -893,8 +883,4 @@ impl InteractiveTxConstructor {
{
- let state_machine = StateMachine::new(- feerate_sat_per_kw,- is_initiator,- funding_tx_locktime,- to_remote_value_satoshis,- );+ let state_machine =+ StateMachine::new(feerate_sat_per_kw, is_initiator, funding_tx_locktime);
let mut inputs_to_contribute: Vec<(SerialId, TxIn, TransactionU16LenLimited)> =
@@ -1003,3 +989,3 @@ impl InteractiveTxConstructor {
let msg_send = self.maybe_send_message()?;
- return match &self.state_machine {+ match &self.state_machine {
StateMachine::NegotiationComplete(s) => {
@@ -1012,5 +998,5 @@ impl InteractiveTxConstructor {
debug_assert!(false, "We cannot transition to any other states after receiving `tx_complete` and responding");
- return Err(AbortReason::InvalidStateTransition);+ Err(AbortReason::InvalidStateTransition)
},
- };+ }
},
@@ -1139,3 +1125,2 @@ mod tests {
session.outputs_a,
- 0,
);
@@ -1149,3 +1134,2 @@ mod tests {
session.outputs_b,
- 0,
);

[TheBlueMatt]

Your turn @jkczyz

[TheBlueMatt]

Gonna go ahead and merge since this only really touches interactivetxs.rs which is unused in non-flags LDK.