Skip to content

Commit a7f103c

Browse files
wvanlintwvanlint
committed
Add WaitingOn* spans to outbound HTLCs
1 parent 4222522 commit a7f103c

File tree

2 files changed

+219
-22
lines changed

2 files changed

+219
-22
lines changed

‎lightning/src/ln/channel.rs‎

Lines changed: 171 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -613,15 +613,97 @@ impl OutboundHTLCOutput {
613613
where
614614
L::Target: Logger,
615615
{
616+
mem::drop(self.state_wrapper.waiting_on_peer_span.take());
617+
mem::drop(self.state_wrapper.waiting_on_monitor_persist_span.take());
616618
mem::drop(self.state_wrapper.span.take());
617619
self.state_wrapper =
618620
OutboundHTLCStateWrapper::new(state, self.span.as_user_span_ref::<L>(), logger);
619621
}
622+
623+
fn is_waiting_on_peer(&self, reason: Option<WaitingOnPeerReason>) -> bool {
624+
match (&self.state_wrapper.waiting_on_peer_span, reason) {
625+
(Some((_, _)), None) => true,
626+
(Some((_, span_reason)), Some(given_reason)) => *span_reason == given_reason,
627+
_ => false,
628+
}
629+
}
630+
631+
fn waiting_on_peer<L: Deref>(&mut self, logger: &L, reason: WaitingOnPeerReason)
632+
where
633+
L::Target: Logger,
634+
{
635+
self.state_wrapper.waiting_on_peer_span = Some((
636+
BoxedSpan::new(logger.start(
637+
Span::WaitingOnPeer,
638+
self.state_wrapper.span.as_ref().map(|s| s.as_user_span_ref::<L>()).flatten(),
639+
)),
640+
reason,
641+
));
642+
}
643+
644+
fn peer_responded(&mut self) {
645+
if self.state_wrapper.waiting_on_peer_span.is_some() {
646+
mem::drop(self.state_wrapper.waiting_on_peer_span.take());
647+
}
648+
}
649+
650+
fn is_waiting_on_monitor_persist(&self) -> bool {
651+
self.state_wrapper.waiting_on_monitor_persist_span.is_some()
652+
}
653+
654+
fn waiting_on_monitor_persist<L: Deref>(&mut self, logger: &L)
655+
where
656+
L::Target: Logger,
657+
{
658+
self.state_wrapper.waiting_on_monitor_persist_span = Some(BoxedSpan::new(logger.start(
659+
Span::WaitingOnMonitorPersist,
660+
self.state_wrapper.span.as_ref().map(|s| s.as_user_span_ref::<L>()).flatten(),
661+
)));
662+
}
663+
664+
fn monitor_persisted(&mut self) {
665+
if self.state_wrapper.waiting_on_monitor_persist_span.is_some() {
666+
mem::drop(self.state_wrapper.waiting_on_monitor_persist_span.take());
667+
}
668+
}
669+
670+
fn waiting_on_async_signing<L: Deref>(&mut self, logger: &L)
671+
where
672+
L::Target: Logger,
673+
{
674+
self.state_wrapper.waiting_on_async_signing_span = Some(BoxedSpan::new(logger.start(
675+
Span::WaitingOnAsyncSigning,
676+
self.state_wrapper.span.as_ref().map(|s| s.as_user_span_ref::<L>()).flatten(),
677+
)));
678+
}
679+
680+
fn async_signing_completed(&mut self) {
681+
if self.state_wrapper.waiting_on_async_signing_span.is_some() {
682+
mem::drop(self.state_wrapper.waiting_on_async_signing_span.take());
683+
}
684+
}
685+
686+
fn is_waiting_on_async_signing(&self) -> bool {
687+
self.state_wrapper.waiting_on_async_signing_span.is_some()
688+
}
689+
}
690+
691+
// This additional reason allows us to recognize the different stages in the
692+
// OutboundHTLCState::Committed state. For other states, this can easily be derived.
693+
#[derive(Debug, Clone, PartialEq, Eq)]
694+
enum WaitingOnPeerReason {
695+
Commitment,
696+
Revocation,
697+
HTLCResolution,
620698
}
621699

622700
#[cfg_attr(test, derive(Clone, Debug, PartialEq))]
623701
struct OutboundHTLCStateWrapper {
624702
state: OutboundHTLCState,
703+
waiting_on_peer_span: Option<(BoxedSpan, WaitingOnPeerReason)>,
704+
waiting_on_monitor_persist_span: Option<BoxedSpan>,
705+
waiting_on_async_signing_span: Option<BoxedSpan>,
706+
// Drop full span last.
625707
span: Option<BoxedSpan>,
626708
}
627709

@@ -635,7 +717,13 @@ impl OutboundHTLCStateWrapper {
635717
{
636718
let state_span =
637719
logger.start(Span::OutboundHTLCState { state: (&state).into() }, parent_span);
638-
OutboundHTLCStateWrapper { state, span: Some(BoxedSpan::new(state_span)) }
720+
OutboundHTLCStateWrapper {
721+
state,
722+
span: Some(BoxedSpan::new(state_span)),
723+
waiting_on_peer_span: None,
724+
waiting_on_monitor_persist_span: None,
725+
waiting_on_async_signing_span: None,
726+
}
639727
}
640728
}
641729

@@ -6886,6 +6974,7 @@ where
68866974
return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))),
68876975
OutboundHTLCState::Committed => {
68886976
htlc.set_state(OutboundHTLCState::RemoteRemoved(outcome), logger);
6977+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::Commitment);
68896978
},
68906979
OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) | OutboundHTLCState::RemoteRemoved(_) =>
68916980
return Err(ChannelError::close(format!("Remote tried to fulfill/fail HTLC ({}) that they'd already fulfilled/failed", htlc_id))),
@@ -7286,25 +7375,35 @@ where
72867375
}
72877376
let mut claimed_htlcs = Vec::new();
72887377
for htlc in self.context.pending_outbound_htlcs.iter_mut() {
7289-
if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) =
7290-
&mut htlc.state_wrapper.state
7291-
{
7292-
log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
7293-
&htlc.payment_hash, &self.context.channel_id);
7294-
// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
7295-
let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
7296-
mem::swap(outcome, &mut reason);
7297-
if let OutboundHTLCOutcome::Success(preimage, _) = reason {
7298-
// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
7299-
// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
7300-
// have a `Success(None)` reason. In this case we could forget some HTLC
7301-
// claims, but such an upgrade is unlikely and including claimed HTLCs here
7302-
// fixes a bug which the user was exposed to on 0.0.104 when they started the
7303-
// claim anyway.
7304-
claimed_htlcs.push((SentHTLCId::from_source(&htlc.source), preimage));
7305-
}
7306-
htlc.set_state(OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason), logger);
7307-
need_commitment = true;
7378+
match &mut htlc.state_wrapper.state {
7379+
&mut OutboundHTLCState::RemoteRemoved(ref mut outcome) => {
7380+
log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
7381+
&htlc.payment_hash, &self.context.channel_id);
7382+
// Swap against a dummy variant to avoid a potentially expensive clone of `OutboundHTLCOutcome::Failure(HTLCFailReason)`
7383+
let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
7384+
mem::swap(outcome, &mut reason);
7385+
if let OutboundHTLCOutcome::Success(preimage, _) = reason {
7386+
// If a user (a) receives an HTLC claim using LDK 0.0.104 or before, then (b)
7387+
// upgrades to LDK 0.0.114 or later before the HTLC is fully resolved, we could
7388+
// have a `Success(None)` reason. In this case we could forget some HTLC
7389+
// claims, but such an upgrade is unlikely and including claimed HTLCs here
7390+
// fixes a bug which the user was exposed to on 0.0.104 when they started the
7391+
// claim anyway.
7392+
claimed_htlcs.push((SentHTLCId::from_source(&htlc.source), preimage));
7393+
}
7394+
htlc.set_state(OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason), logger);
7395+
if self.context.channel_state.is_awaiting_remote_revoke() {
7396+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::Revocation);
7397+
}
7398+
need_commitment = true;
7399+
},
7400+
OutboundHTLCState::Committed => {
7401+
if htlc.is_waiting_on_peer(Some(WaitingOnPeerReason::Commitment)) {
7402+
htlc.peer_responded();
7403+
htlc.waiting_on_monitor_persist(logger);
7404+
}
7405+
},
7406+
_ => {},
73087407
}
73097408
}
73107409

@@ -7868,6 +7967,7 @@ where
78687967
&htlc.payment_hash
78697968
);
78707969
htlc.set_state(OutboundHTLCState::Committed, logger);
7970+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::Commitment);
78717971
*expecting_peer_commitment_signed = true;
78727972
}
78737973
if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) =
@@ -7878,6 +7978,7 @@ where
78787978
let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
78797979
mem::swap(outcome, &mut reason);
78807980
htlc.set_state(OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason), logger);
7981+
htlc.waiting_on_monitor_persist(logger);
78817982
require_commitment = true;
78827983
}
78837984
}
@@ -8246,6 +8347,7 @@ where
82468347
// commitment_signed, we need to move it back to Committed and they can re-send
82478348
// the update upon reconnection.
82488349
htlc.set_state(OutboundHTLCState::Committed, logger);
8350+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::HTLCResolution);
82498351
}
82508352
}
82518353

@@ -8405,6 +8507,32 @@ where
84058507
_ => {},
84068508
}
84078509
}
8510+
for htlc in self.context.pending_outbound_htlcs.iter_mut() {
8511+
match htlc.state() {
8512+
OutboundHTLCState::LocalAnnounced(_) |
8513+
OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => {
8514+
if htlc.is_waiting_on_monitor_persist() {
8515+
htlc.monitor_persisted();
8516+
if commitment_update.is_some() {
8517+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::Revocation);
8518+
} else if self.context.signer_pending_commitment_update {
8519+
htlc.waiting_on_async_signing(logger);
8520+
}
8521+
}
8522+
},
8523+
OutboundHTLCState::Committed => {
8524+
if htlc.is_waiting_on_monitor_persist() {
8525+
htlc.monitor_persisted();
8526+
if raa.is_some() {
8527+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::HTLCResolution);
8528+
} else if self.context.signer_pending_revoke_and_ack {
8529+
htlc.waiting_on_async_signing(logger);
8530+
}
8531+
}
8532+
},
8533+
_ => {},
8534+
}
8535+
}
84088536

84098537
self.context.monitor_pending_revoke_and_ack = false;
84108538
self.context.monitor_pending_commitment_signed = false;
@@ -8561,6 +8689,24 @@ where
85618689
_ => {},
85628690
}
85638691
}
8692+
for htlc in self.context.pending_outbound_htlcs.iter_mut() {
8693+
match htlc.state() {
8694+
OutboundHTLCState::LocalAnnounced(_) |
8695+
OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => {
8696+
if htlc.is_waiting_on_async_signing() && commitment_update.is_some() {
8697+
htlc.async_signing_completed();
8698+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::Revocation);
8699+
}
8700+
},
8701+
OutboundHTLCState::Committed => {
8702+
if htlc.is_waiting_on_async_signing() && revoke_and_ack.is_some() {
8703+
htlc.async_signing_completed();
8704+
htlc.waiting_on_peer(logger, WaitingOnPeerReason::HTLCResolution);
8705+
}
8706+
},
8707+
_ => {},
8708+
}
8709+
}
85648710

85658711
log_trace!(logger, "Signer unblocked with {} commitment_update, {} revoke_and_ack, with resend order {:?}, {} funding_signed, {} channel_ready,
85668712
{} closing_signed, {} signed_closing_tx, and {} shutdown result",
@@ -11016,7 +11162,7 @@ where
1101611162
// that are simple to implement, and we do it on the outgoing side because then the failure message that encodes
1101711163
// the hold time still needs to be built in channel manager.
1101811164
let send_timestamp = duration_since_epoch();
11019-
self.context.pending_outbound_htlcs.push(OutboundHTLCOutput::new(
11165+
let mut htlc = OutboundHTLCOutput::new(
1102011166
self.context.channel_id(),
1102111167
OutboundHTLCOutputParams {
1102211168
htlc_id: self.context.next_holder_htlc_id,
@@ -11031,7 +11177,9 @@ where
1103111177
},
1103211178
forward_span,
1103311179
logger,
11034-
));
11180+
);
11181+
htlc.waiting_on_monitor_persist(logger);
11182+
self.context.pending_outbound_htlcs.push(htlc);
1103511183
self.context.next_holder_htlc_id += 1;
1103611184

1103711185
Ok(true)
@@ -11081,6 +11229,7 @@ where
1108111229
let mut reason = OutboundHTLCOutcome::Success(PaymentPreimage([0u8; 32]), None);
1108211230
mem::swap(outcome, &mut reason);
1108311231
htlc.set_state(OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason), logger);
11232+
htlc.waiting_on_monitor_persist(logger);
1108411233
}
1108511234
}
1108611235
if let Some((feerate, update_state)) = self.context.pending_update_fee {

‎lightning/src/ln/functional_tests.rs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11801,20 +11801,53 @@ pub fn test_payment_traces() {
1180111801
},
1180211802
Some(Span::OutboundHTLC { channel_id: channel_id_2, htlc_id: 0 }),
1180311803
),
11804+
TestSpanBoundary::Start(
11805+
Span::WaitingOnMonitorPersist,
11806+
Some(Span::OutboundHTLCState {
11807+
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd
11808+
}),
11809+
),
11810+
TestSpanBoundary::End(Span::WaitingOnMonitorPersist,),
11811+
TestSpanBoundary::Start(
11812+
Span::WaitingOnPeer,
11813+
Some(Span::OutboundHTLCState {
11814+
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd
11815+
}),
11816+
),
11817+
TestSpanBoundary::End(Span::WaitingOnPeer,),
1180411818
TestSpanBoundary::End(Span::OutboundHTLCState {
1180511819
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToAdd
1180611820
}),
1180711821
TestSpanBoundary::Start(
1180811822
Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed },
1180911823
Some(Span::OutboundHTLC { channel_id: channel_id_2, htlc_id: 0 }),
1181011824
),
11825+
TestSpanBoundary::Start(
11826+
Span::WaitingOnPeer,
11827+
Some(Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed }),
11828+
),
11829+
TestSpanBoundary::End(Span::WaitingOnPeer),
11830+
TestSpanBoundary::Start(
11831+
Span::WaitingOnMonitorPersist,
11832+
Some(Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed }),
11833+
),
11834+
TestSpanBoundary::End(Span::WaitingOnMonitorPersist),
11835+
TestSpanBoundary::Start(
11836+
Span::WaitingOnPeer,
11837+
Some(Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed }),
11838+
),
11839+
TestSpanBoundary::End(Span::WaitingOnPeer),
1181111840
TestSpanBoundary::End(Span::OutboundHTLCState {
1181211841
state: OutboundHTLCStateDetails::Committed
1181311842
}),
1181411843
TestSpanBoundary::Start(
1181511844
Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed },
1181611845
Some(Span::OutboundHTLC { channel_id: channel_id_2, htlc_id: 0 }),
1181711846
),
11847+
TestSpanBoundary::Start(
11848+
Span::WaitingOnPeer,
11849+
Some(Span::OutboundHTLCState { state: OutboundHTLCStateDetails::Committed }),
11850+
),
1181811851
TestSpanBoundary::End(Span::InboundHTLCState {
1181911852
state: Some(InboundHTLCStateDetails::Committed)
1182011853
}),
@@ -11837,6 +11870,7 @@ pub fn test_payment_traces() {
1183711870
state: Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFulfill)
1183811871
}),
1183911872
),
11873+
TestSpanBoundary::End(Span::WaitingOnPeer),
1184011874
TestSpanBoundary::End(Span::OutboundHTLCState {
1184111875
state: OutboundHTLCStateDetails::Committed
1184211876
}),
@@ -11855,6 +11889,20 @@ pub fn test_payment_traces() {
1185511889
},
1185611890
Some(Span::OutboundHTLC { channel_id: channel_id_2, htlc_id: 0 }),
1185711891
),
11892+
TestSpanBoundary::Start(
11893+
Span::WaitingOnMonitorPersist,
11894+
Some(Span::OutboundHTLCState {
11895+
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess
11896+
}),
11897+
),
11898+
TestSpanBoundary::End(Span::WaitingOnMonitorPersist),
11899+
TestSpanBoundary::Start(
11900+
Span::WaitingOnPeer,
11901+
Some(Span::OutboundHTLCState {
11902+
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess
11903+
}),
11904+
),
11905+
TestSpanBoundary::End(Span::WaitingOnPeer),
1185811906
TestSpanBoundary::End(Span::OutboundHTLCState {
1185911907
state: OutboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveSuccess
1186011908
}),

0 commit comments

Comments
 (0)