Skip to content

Commit a17c1d8

Browse files
committed
Remove unnecessary funding tx clone & remote tx_sigs received check
We actually don't need to check if the counterparty had already sent their `tx_signatures` in `InteractiveTxSigningSession::received_tx_signatures`. Further, we can get rid of the clone of `funding_tx_opt` in `FundedChannel::tx_signatures` when setting the `ChannelContext::funding_transaction` as we don't actually need to propagate it through to `ChannelManager::internal_tx_complete` as we can use `ChannelContext::unbroadcasted_funding()` which clones the `ChannelContext::funding_transaction` anyway.
1 parent 3c1e603 commit a17c1d8

File tree

5 files changed

+98
-70
lines changed

5 files changed

+98
-70
lines changed

lightning/src/ln/channel.rs

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2159,6 +2159,7 @@ impl<SP: Deref> PendingV2Channel<SP> where SP::Target: SignerProvider {
21592159
context: self.context,
21602160
interactive_tx_signing_session: Some(signing_session),
21612161
holder_commitment_point,
2162+
is_v2_established: true,
21622163
};
21632164
Ok((funded_chan, commitment_signed, funding_ready_for_sig_event))
21642165
},
@@ -4500,6 +4501,9 @@ pub(super) struct FundedChannel<SP: Deref> where SP::Target: SignerProvider {
45004501
pub context: ChannelContext<SP>,
45014502
pub interactive_tx_signing_session: Option<InteractiveTxSigningSession>,
45024503
holder_commitment_point: HolderCommitmentPoint,
4504+
/// Indicates whether this funded channel had been established with V2 channel
4505+
/// establishment (i.e. is a dual-funded channel).
4506+
is_v2_established: bool,
45034507
}
45044508

45054509
#[cfg(any(test, fuzzing))]
@@ -5954,10 +5958,10 @@ impl<SP: Deref> FundedChannel<SP> where
59545958
}
59555959
}
59565960

5957-
pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError>
5961+
pub fn tx_signatures<L: Deref>(&mut self, msg: &msgs::TxSignatures, logger: &L) -> Result<Option<msgs::TxSignatures>, ChannelError>
59585962
where L::Target: Logger
59595963
{
5960-
if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) {
5964+
if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) {
59615965
return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned()));
59625966
}
59635967

@@ -5991,25 +5995,23 @@ impl<SP: Deref> FundedChannel<SP> where
59915995
// for spending. Doesn't seem to be anything in rust-bitcoin.
59925996
}
59935997

5994-
let (tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone())
5998+
let (holder_tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg.clone())
59955999
.map_err(|_| ChannelError::Warn("Witness count did not match contributed input count".to_string()))?;
6000+
if holder_tx_signatures_opt.is_some() && self.is_awaiting_initial_mon_persist() {
6001+
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
6002+
self.context.monitor_pending_tx_signatures = holder_tx_signatures_opt;
6003+
return Ok(None);
6004+
}
59966005
if funding_tx_opt.is_some() {
6006+
// We have a persisted channel monitor and and a finalized funding transaction, so we can move
6007+
// the channel state forward, set the funding transaction and reset the signing session fields.
6008+
self.context.funding_transaction = funding_tx_opt;
6009+
self.context.next_funding_txid = None;
6010+
self.interactive_tx_signing_session = None;
59976011
self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
59986012
}
5999-
self.context.funding_transaction = funding_tx_opt.clone();
6000-
6001-
self.context.next_funding_txid = None;
6002-
6003-
// Clear out the signing session
6004-
self.interactive_tx_signing_session = None;
6005-
6006-
if tx_signatures_opt.is_some() && self.context.channel_state.is_monitor_update_in_progress() {
6007-
log_debug!(logger, "Not sending tx_signatures: a monitor update is in progress. Setting monitor_pending_tx_signatures.");
6008-
self.context.monitor_pending_tx_signatures = tx_signatures_opt;
6009-
return Ok((None, None));
6010-
}
60116013

6012-
Ok((tx_signatures_opt, funding_tx_opt))
6014+
Ok(holder_tx_signatures_opt)
60136015
} else {
60146016
Err(ChannelError::Close((
60156017
"Unexpected tx_signatures. No funding transaction awaiting signatures".to_string(),
@@ -6212,12 +6214,12 @@ impl<SP: Deref> FundedChannel<SP> where
62126214
assert!(self.context.channel_state.is_monitor_update_in_progress());
62136215
self.context.channel_state.clear_monitor_update_in_progress();
62146216

6215-
// If we're past (or at) the AwaitingChannelReady stage on an outbound channel, try to
6216-
// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
6217+
// If we're past (or at) the AwaitingChannelReady stage on an outbound (or V2-established) channel,
6218+
// try to (re-)broadcast the funding transaction as we may have declined to broadcast it when we
62176219
// first received the funding_signed.
62186220
let mut funding_broadcastable = None;
62196221
if let Some(funding_transaction) = &self.context.funding_transaction {
6220-
if self.context.is_outbound() &&
6222+
if (self.context.is_outbound() || self.is_v2_established()) &&
62216223
(matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(flags) if !flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) ||
62226224
matches!(self.context.channel_state, ChannelState::ChannelReady(_)))
62236225
{
@@ -8509,6 +8511,10 @@ impl<SP: Deref> FundedChannel<SP> where
85098511
})
85108512
.chain(self.context.pending_outbound_htlcs.iter().map(|htlc| (&htlc.source, &htlc.payment_hash)))
85118513
}
8514+
8515+
pub fn is_v2_established(&self) -> bool {
8516+
self.is_v2_established
8517+
}
85128518
}
85138519

85148520
/// A not-yet-funded outbound (from holder) channel using V1 channel establishment.
@@ -8772,6 +8778,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
87728778
let mut channel = FundedChannel {
87738779
context: self.context,
87748780
interactive_tx_signing_session: None,
8781+
is_v2_established: false,
87758782
holder_commitment_point,
87768783
};
87778784

@@ -9037,6 +9044,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
90379044
let mut channel = FundedChannel {
90389045
context: self.context,
90399046
interactive_tx_signing_session: None,
9047+
is_v2_established: false,
90409048
holder_commitment_point,
90419049
};
90429050
let need_channel_ready = channel.check_get_channel_ready(0, logger).is_some()
@@ -9839,7 +9847,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
98399847
let mut _val: u64 = Readable::read(reader)?;
98409848
}
98419849

9842-
let channel_id = Readable::read(reader)?;
9850+
let channel_id: ChannelId = Readable::read(reader)?;
98439851
let channel_state = ChannelState::from_u32(Readable::read(reader)?).map_err(|_| DecodeError::InvalidValue)?;
98449852
let channel_value_satoshis = Readable::read(reader)?;
98459853

@@ -10275,6 +10283,10 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
1027510283
}
1027610284
},
1027710285
};
10286+
let is_v2_established = channel_id.is_v2_channel_id(
10287+
&channel_parameters.holder_pubkeys.revocation_basepoint,
10288+
&channel_parameters.counterparty_parameters.as_ref()
10289+
.expect("Persisted channel must have counterparty parameters").pubkeys.revocation_basepoint);
1027810290

1027910291
Ok(FundedChannel {
1028010292
context: ChannelContext {
@@ -10412,6 +10424,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
1041210424
next_funding_txid: None,
1041310425
},
1041410426
interactive_tx_signing_session: None,
10427+
is_v2_established,
1041510428
holder_commitment_point,
1041610429
})
1041710430
}

lightning/src/ln/channelmanager.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8506,14 +8506,14 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
85068506
match chan_entry.get_mut().as_funded_mut() {
85078507
Some(chan) => {
85088508
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
8509-
let (tx_signatures_opt, funding_tx_opt) = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry);
8509+
let tx_signatures_opt = try_channel_entry!(self, peer_state, chan.tx_signatures(msg, &&logger), chan_entry);
85108510
if let Some(tx_signatures) = tx_signatures_opt {
85118511
peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures {
85128512
node_id: *counterparty_node_id,
85138513
msg: tx_signatures,
85148514
});
85158515
}
8516-
if let Some(ref funding_tx) = funding_tx_opt {
8516+
if let Some(ref funding_tx) = chan.context.unbroadcasted_funding() {
85178517
self.tx_broadcaster.broadcast_transactions(&[funding_tx]);
85188518
{
85198519
let mut pending_events = self.pending_events.lock().unwrap();

lightning/src/ln/dual_funding_tests.rs

Lines changed: 34 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,13 @@ use {
2020
crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint, RevocationBasepoint},
2121
crate::ln::functional_test_utils::*,
2222
crate::ln::msgs::ChannelMessageHandler,
23-
crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete},
23+
crate::ln::msgs::{CommitmentSigned, TxAddInput, TxAddOutput, TxComplete, TxSignatures},
2424
crate::ln::types::ChannelId,
2525
crate::prelude::*,
2626
crate::sign::ChannelSigner as _,
2727
crate::util::ser::TransactionU16LenLimited,
2828
crate::util::test_utils,
29+
bitcoin::{Weight, Witness},
2930
};
3031

3132
// Dual-funding: V2 Channel Establishment Tests
@@ -36,9 +37,7 @@ struct V2ChannelEstablishmentTestSession {
3637

3738
// TODO(dual_funding): Use real node and API for creating V2 channels as initiator when available,
3839
// instead of manually constructing messages.
39-
fn do_test_v2_channel_establishment(
40-
session: V2ChannelEstablishmentTestSession, test_async_persist: bool,
41-
) {
40+
fn do_test_v2_channel_establishment(session: V2ChannelEstablishmentTestSession) {
4241
let chanmon_cfgs = create_chanmon_cfgs(2);
4342
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
4443
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -195,37 +194,23 @@ fn do_test_v2_channel_establishment(
195194
partial_signature_with_nonce: None,
196195
};
197196

198-
if test_async_persist {
199-
chanmon_cfgs[1]
200-
.persister
201-
.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress);
202-
}
197+
chanmon_cfgs[1].persister.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::InProgress);
203198

204199
// Handle the initial commitment_signed exchange. Order is not important here.
205200
nodes[1]
206201
.node
207202
.handle_commitment_signed(nodes[0].node.get_our_node_id(), &msg_commitment_signed_from_0);
208203
check_added_monitors(&nodes[1], 1);
209204

210-
if test_async_persist {
211-
let events = nodes[1].node.get_and_clear_pending_events();
212-
assert!(events.is_empty());
205+
// The funding transaction should not have been broadcast before persisting initial monitor has
206+
// been completed.
207+
assert_eq!(nodes[1].tx_broadcaster.txn_broadcast().len(), 0);
208+
assert_eq!(nodes[1].node.get_and_clear_pending_events().len(), 0);
213209

214-
chanmon_cfgs[1]
215-
.persister
216-
.set_update_ret(crate::chain::ChannelMonitorUpdateStatus::Completed);
217-
let (latest_update, _) = *nodes[1]
218-
.chain_monitor
219-
.latest_monitor_update_id
220-
.lock()
221-
.unwrap()
222-
.get(&channel_id)
223-
.unwrap();
224-
nodes[1]
225-
.chain_monitor
226-
.chain_monitor
227-
.force_channel_monitor_updated(channel_id, latest_update);
228-
}
210+
// Complete the persistence of the monitor.
211+
let events = nodes[1].node.get_and_clear_pending_events();
212+
assert!(events.is_empty());
213+
nodes[1].chain_monitor.complete_sole_pending_chan_update(&channel_id);
229214

230215
let events = nodes[1].node.get_and_clear_pending_events();
231216
assert_eq!(events.len(), 1);
@@ -241,24 +226,30 @@ fn do_test_v2_channel_establishment(
241226
);
242227

243228
assert_eq!(tx_signatures_msg.channel_id, channel_id);
229+
230+
let mut witness = Witness::new();
231+
witness.push([0x0]);
232+
// Receive tx_signatures from channel initiator.
233+
nodes[1].node.handle_tx_signatures(
234+
nodes[0].node.get_our_node_id(),
235+
&TxSignatures {
236+
channel_id,
237+
tx_hash: funding_outpoint.unwrap().txid,
238+
witnesses: vec![witness],
239+
shared_input_signature: None,
240+
},
241+
);
242+
243+
// For an inbound channel V2 channel the transaction should be broadcast once receiving a
244+
// tx_signature and applying local tx_signatures:
245+
let broadcasted_txs = nodes[1].tx_broadcaster.txn_broadcast();
246+
assert_eq!(broadcasted_txs.len(), 1);
244247
}
245248

246249
#[test]
247250
fn test_v2_channel_establishment() {
248-
// Only initiator contributes, no persist pending
249-
do_test_v2_channel_establishment(
250-
V2ChannelEstablishmentTestSession {
251-
funding_input_sats: 100_000,
252-
initiator_input_value_satoshis: 150_000,
253-
},
254-
false,
255-
);
256-
// Only initiator contributes, persist pending
257-
do_test_v2_channel_establishment(
258-
V2ChannelEstablishmentTestSession {
259-
funding_input_sats: 100_000,
260-
initiator_input_value_satoshis: 150_000,
261-
},
262-
true,
263-
);
251+
do_test_v2_channel_establishment(V2ChannelEstablishmentTestSession {
252+
funding_input_sats: 100_00,
253+
initiator_input_value_satoshis: 150_000,
254+
});
264255
}

lightning/src/ln/interactivetxs.rs

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,14 +316,18 @@ impl InteractiveTxSigningSession {
316316

317317
/// Handles a `tx_signatures` message received from the counterparty.
318318
///
319+
/// If the holder is required to send their `tx_signatures` message and these signatures have
320+
/// already been provided to the signing session, then this return value will be `Some`, otherwise
321+
/// None.
322+
///
323+
/// If the holder has already provided their `tx_signatures` to the signing session, a funding
324+
/// transaction will be finalized and returned as Some, otherwise None.
325+
///
319326
/// Returns an error if the witness count does not equal the counterparty's input count in the
320327
/// unsigned transaction.
321328
pub fn received_tx_signatures(
322329
&mut self, tx_signatures: TxSignatures,
323330
) -> Result<(Option<TxSignatures>, Option<Transaction>), ()> {
324-
if self.counterparty_sent_tx_signatures {
325-
return Ok((None, None));
326-
};
327331
if self.remote_inputs_count() != tx_signatures.witnesses.len() {
328332
return Err(());
329333
}
@@ -336,13 +340,16 @@ impl InteractiveTxSigningSession {
336340
None
337341
};
338342

339-
let funding_tx = if self.holder_tx_signatures.is_some() {
343+
// Check if the holder has provided its signatures and if so,
344+
// return the finalized funding transaction.
345+
let funding_tx_opt = if self.holder_tx_signatures.is_some() {
340346
Some(self.finalize_funding_tx())
341347
} else {
348+
// This means we're still waiting for the holder to provide their signatures.
342349
None
343350
};
344351

345-
Ok((holder_tx_signatures, funding_tx))
352+
Ok((holder_tx_signatures, funding_tx_opt))
346353
}
347354

348355
/// Provides the holder witnesses for the unsigned transaction.

lightning/src/ln/types.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,11 @@ impl ChannelId {
101101
pub fn temporary_v2_from_revocation_basepoint(our_revocation_basepoint: &RevocationBasepoint) -> Self {
102102
Self(Sha256::hash(&[[0u8; 33], our_revocation_basepoint.0.serialize()].concat()).to_byte_array())
103103
}
104+
105+
/// Indicates whether this is a V2 channel ID for the given local and remote revocation basepoints.
106+
pub fn is_v2_channel_id(&self, ours: &RevocationBasepoint, theirs: &RevocationBasepoint) -> bool {
107+
*self == Self::v2_from_revocation_basepoints(ours, theirs)
108+
}
104109
}
105110

106111
impl Writeable for ChannelId {
@@ -211,4 +216,16 @@ mod tests {
211216

212217
assert_eq!(ChannelId::v2_from_revocation_basepoints(&ours, &theirs), expected_id);
213218
}
219+
220+
#[test]
221+
fn test_is_v2_channel_id() {
222+
let ours = RevocationBasepoint(PublicKey::from_slice(&<Vec<u8>>::from_hex("0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c").unwrap()[..]).unwrap());
223+
let theirs = RevocationBasepoint(PublicKey::from_slice(&<Vec<u8>>::from_hex("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap());
224+
225+
let channel_id = ChannelId::v2_from_revocation_basepoints(&ours, &theirs);
226+
assert!(channel_id.is_v2_channel_id(&ours, &theirs));
227+
228+
let channel_id = ChannelId::v1_from_funding_txid(&[2; 32], 1);
229+
assert!(!channel_id.is_v2_channel_id(&ours, &theirs))
230+
}
214231
}

0 commit comments

Comments
 (0)