Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheBlueMatt merged 7 commits into from
Sep 12, 2023
Merged

Block the mon update removing a preimage until upstream mon writes #2169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4835b16
Fix various unused warnings in test and regular builds
TheBlueMatt Aug 31, 2023
e37a400
Clean up test handling of resending responding commitment_signed
TheBlueMatt Aug 31, 2023
5ff51b7
Block the mon update removing a preimage until upstream mon writes
TheBlueMatt Sep 7, 2023
46453bf
Correct `expect_payment_forwarded` upstream channel checking
TheBlueMatt Aug 27, 2023
0d8b096
Update tests to test re-claiming of forwarded HTLCs on startup
TheBlueMatt Aug 21, 2023
6c3029d
Split `expect_payment_forwarded` into a function called by macro
TheBlueMatt Jul 11, 2023
9f3e127
Test monitor update completion actions on pre-startup completion
TheBlueMatt Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
LatestHolderCommitmentTXInfo {
commitment_tx: HolderCommitmentTransaction,
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/chan_utils.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
/// The broadcaster's per-commitment public key which was used to derive the other keys.
pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
inner: CommitmentTransaction,
/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
/// The commitment transaction
pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
commitment_number: u64,
to_broadcaster_value_sat: u64,
Expand Down
450 changes: 444 additions & 6 deletions lightning/src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop

Large diffs are not rendered by default.

67 changes: 56 additions & 11 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
// Note that this may be an outbound SCID alias for the associated channel.
short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
PreviousHopData(HTLCPreviousHopData),
OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
#[allow(unused)]
fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
Self::ForwardedPaymentInboundClaim {
channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
}

fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
) {
match source {
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
"We don't support claim_htlc claims during startup - monitors may not be available yet");
if let Some(pubkey) = next_channel_counterparty_node_id {
debug_assert_eq!(pubkey, path.hops[0].pubkey);
}
let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
channel_funding_outpoint: next_channel_outpoint,
counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
},
HTLCSource::PreviousHopData(hop_data) => {
let prev_outpoint = hop_data.outpoint;
let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
let res = self.claim_funds_from_hop(hop_data, payment_preimage,
|htlc_claim_value_msat| {
if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
next_channel_id: Some(next_channel_outpoint.to_channel_id()),
outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
},
downstream_counterparty_and_funding_outpoint: None,
downstream_counterparty_and_funding_outpoint:
if let Some(node_id) = next_channel_counterparty_node_id {
Some((node_id, next_channel_outpoint, completed_blocker))
} else {
// We can only get `None` here if we are processing a
// `ChannelMonitor`-originated event, in which case we
// don't care about ensuring we wake the downstream
// channel's monitor updating - the channel is already
// closed.
None
},
})
} else { None }
});
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
hash_map::Entry::Occupied(mut chan_phase_entry) => {
if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
.or_insert_with(Vec::new)
.push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
}
// Note that we do not need to push an `actions_blocking_raa_monitor_updates`
// entry here, even though we *do* need to block the next RAA monitor update.
// We do this instead in the `claim_funds_internal` by attaching a
// `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
// outbound HTLC is claimed. This is guaranteed to all complete before we
// process the RAA as messages are processed from single peers serially.
funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
res
} else {
Expand All@@ -6054,7 +6081,7 @@ where
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
}
};
self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
Ok(())
}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
})
}

#[cfg(any(test, feature = "_test_utils"))]
pub(crate) fn test_raa_monitor_updates_held(&self,
counterparty_node_id: PublicKey, channel_id: ChannelId
) -> bool {
let per_peer_state = self.per_peer_state.read().unwrap();
if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
let mut peer_state_lck = peer_state_mtx.lock().unwrap();
let peer_state = &mut *peer_state_lck;

if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
chan.context().get_funding_txo().unwrap(), counterparty_node_id);
}
}
false
}

fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
let (htlcs_to_fail, res) = {
let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
match monitor_event {
MonitorEvent::HTLCEvent(htlc_update) => {
if let Some(preimage) = htlc_update.payment_preimage {
log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
} else {
log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
// downstream chan is closed (because we don't have a
// channel_id -> peer map entry).
counterparty_opt.is_none(),
counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),

@valentinewallacevalentinewallaceAug 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around

@TheBlueMattTheBlueMattAug 31, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.

monitor.get_funding_txo().0))
} else { None }
} else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
}

for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
// We use `downstream_closed` in place of `from_onchain` here just as a guess - we
// don't remember in the `ChannelMonitor` where we got a preimage from, but if the
// channel is closed we just assume that it probably came from an on-chain claim.
channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
downstream_closed, downstream_funding);
downstream_closed, downstream_node_id, downstream_funding);
}

//TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Block the mon update removing a preimage until upstream mon writes#2169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 7 commits into

from

Sep 12, 2023
 Merged


Block the mon update removing a preimage until upstream mon writes
#2169
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
4835b16

Fix various unused warnings in test and regular builds


TheBlueMatt Aug 31, 2023
e37a400

Clean up test handling of resending responding commitment_signed


TheBlueMatt Aug 31, 2023
5ff51b7

Block the mon update removing a preimage until upstream mon writes


TheBlueMatt Sep 7, 2023
46453bf

Correct `expect_payment_forwarded` upstream channel checking


TheBlueMatt Aug 27, 2023
0d8b096

Update tests to test re-claiming of forwarded HTLCs on startup


TheBlueMatt Aug 21, 2023
6c3029d

Split `expect_payment_forwarded` into a function called by macro


TheBlueMatt Jul 11, 2023
9f3e127

Test monitor update completion actions on pre-startup completion


TheBlueMatt Jul 5, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -67,7 +67,7 @@ use crate::sync::{Mutex, LockTestExt};
/// much smaller than a full [`ChannelMonitor`]. However, for large single commitment transaction
/// updates (e.g. ones during which there are hundreds of HTLCs pending on the commitment
/// transaction), a single update may reach upwards of 1 MiB in serialized size.
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
#[must_use]
pub struct ChannelMonitorUpdate {
	pub(crate) updates: Vec<ChannelMonitorUpdateStep>,
Expand DownExpand Up@@ -487,7 +487,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent,

);

#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum ChannelMonitorUpdateStep {
	LatestHolderCommitmentTXInfo {
 commitment_tx: HolderCommitmentTransaction,
Expand Down

8 changes: 4 additions & 4 deletions
lightning/src/ln/chan_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -450,7 +450,7 @@ pub fn derive_public_revocation_key<T: secp256k1::Verification>(secp_ctx: &Secp2
/// channel basepoints via the new function, or they were obtained via
/// CommitmentTransaction.trust().keys() because we trusted the source of the
/// pre-calculated keys.
#[derive(PartialEq, Eq, Clone)]
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct TxCreationKeys {
	/// The broadcaster's per-commitment public key which was used to derive the other keys.
	pub per_commitment_point: PublicKey,
Expand DownExpand Up@@ -1028,7 +1028,7 @@ impl<'a> DirectedChannelTransactionParameters<'a> {
/// Information needed to build and sign a holder's commitment transaction.
///
/// The transaction is only signed once we are ready to broadcast.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct HolderCommitmentTransaction {
	inner: CommitmentTransaction,
	/// Our counterparty's signature for the transaction
Expand DownExpand Up@@ -1134,7 +1134,7 @@ impl HolderCommitmentTransaction {
}

/// A pre-built Bitcoin commitment transaction and its txid.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct BuiltCommitmentTransaction {
	/// The commitment transaction
	pub transaction: Transaction,
Expand DownExpand Up@@ -1305,7 +1305,7 @@ impl<'a> TrustedClosingTransaction<'a> {
///
/// This class can be used inside a signer implementation to generate a signature given the relevant
/// secret key.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
	commitment_number: u64,
	to_broadcaster_value_sat: u64,
Expand Down

450 changes: 444 additions & 6 deletions
lightning/src/ln/chanmon_update_fail_tests.rs

View file


Open in desktop


Large diffs are not rendered by default.


67 changes: 56 additions & 11 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -177,7 +177,7 @@ pub(super) enum HTLCForwardInfo {
}

/// Tracks the inbound corresponding to an outbound HTLC
#[derive(Clone, Hash, PartialEq, Eq)]
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub(crate) struct HTLCPreviousHopData {
	// Note that this may be an outbound SCID alias for the associated channel.
	short_channel_id: u64,
Expand DownExpand Up@@ -283,7 +283,7 @@ impl Readable for InterceptId {
	}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
/// Uniquely describes an HTLC by its source. Just the guaranteed-unique subset of [`HTLCSource`].
pub(crate) enum SentHTLCId {
	PreviousHopData { short_channel_id: u64, htlc_id: u64 },
Expand DownExpand Up@@ -314,7 +314,7 @@ impl_writeable_tlv_based_enum!(SentHTLCId,

/// Tracks the inbound corresponding to an outbound HTLC
#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash
#[derive(Clone, PartialEq, Eq)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum HTLCSource {
	PreviousHopData(HTLCPreviousHopData),
	OutboundRoute {
Expand DownExpand Up@@ -656,7 +656,6 @@ pub(crate) enum RAAMonitorUpdateBlockingAction {
}

impl RAAMonitorUpdateBlockingAction {
	#[allow(unused)]
	fn from_prev_hop_data(prev_hop: &HTLCPreviousHopData) -> Self {
 Self::ForwardedPaymentInboundClaim {
 channel_id: prev_hop.outpoint.to_channel_id(),
Expand DownExpand Up@@ -5175,11 +5174,17 @@ where
 self.pending_outbound_payments.finalize_claims(sources, &self.pending_events);
	}

	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, next_channel_outpoint: OutPoint) {
	fn claim_funds_internal(&self, source: HTLCSource, payment_preimage: PaymentPreimage,
 forwarded_htlc_value_msat: Option<u64>, from_onchain: bool,
 next_channel_counterparty_node_id: Option<PublicKey>, next_channel_outpoint: OutPoint
	) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 debug_assert!(self.background_events_processed_since_startup.load(Ordering::Acquire),
 "We don't support claim_htlc claims during startup - monitors may not be available yet");
 if let Some(pubkey) = next_channel_counterparty_node_id {
 debug_assert_eq!(pubkey, path.hops[0].pubkey);
 }
 let ev_completion_action = EventCompletionAction::ReleaseRAAChannelMonitorUpdate {
 channel_funding_outpoint: next_channel_outpoint,
 counterparty_node_id: path.hops[0].pubkey,
Expand All@@ -5190,6 +5195,7 @@ where
 },
 HTLCSource::PreviousHopData(hop_data) => {
 let prev_outpoint = hop_data.outpoint;
 let completed_blocker = RAAMonitorUpdateBlockingAction::from_prev_hop_data(&hop_data);
 let res = self.claim_funds_from_hop(hop_data, payment_preimage,
 |htlc_claim_value_msat| {
 if let Some(forwarded_htlc_value) = forwarded_htlc_value_msat {
Expand All@@ -5205,7 +5211,17 @@ where
 next_channel_id: Some(next_channel_outpoint.to_channel_id()),
 outbound_amount_forwarded_msat: forwarded_htlc_value_msat,
 },
 downstream_counterparty_and_funding_outpoint: None,
 downstream_counterparty_and_funding_outpoint:
 if let Some(node_id) = next_channel_counterparty_node_id {
 Some((node_id, next_channel_outpoint, completed_blocker))
 } else {
 // We can only get `None` here if we are processing a
 // `ChannelMonitor`-originated event, in which case we
 // don't care about ensuring we wake the downstream
 // channel's monitor updating - the channel is already
 // closed.
 None
 },
 })
 } else { None }
 });
Expand DownExpand Up@@ -6044,6 +6060,17 @@ where
 hash_map::Entry::Occupied(mut chan_phase_entry) => {
 if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 let res = try_chan_phase_entry!(self, chan.update_fulfill_htlc(&msg), chan_phase_entry);
 if let HTLCSource::PreviousHopData(prev_hop) = &res.0 {
 peer_state.actions_blocking_raa_monitor_updates.entry(msg.channel_id)
 .or_insert_with(Vec::new)
 .push(RAAMonitorUpdateBlockingAction::from_prev_hop_data(&prev_hop));
 }
 // Note that we do not need to push an `actions_blocking_raa_monitor_updates`
 // entry here, even though we *do* need to block the next RAA monitor update.
 // We do this instead in the `claim_funds_internal` by attaching a
 // `ReleaseRAAChannelMonitorUpdate` action to the event generated when the
 // outbound HTLC is claimed. This is guaranteed to all complete before we
 // process the RAA as messages are processed from single peers serially.
 funding_txo = chan.context.get_funding_txo().expect("We won't accept a fulfill until funded");
 res
 } else {
Expand All@@ -6054,7 +6081,7 @@ where
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.channel_id))
 }
 };
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, funding_txo);
 self.claim_funds_internal(htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, Some(*counterparty_node_id), funding_txo);
 Ok(())
	}

Expand DownExpand Up@@ -6256,6 +6283,23 @@ where
 })
	}

	#[cfg(any(test, feature = "_test_utils"))]
	pub(crate) fn test_raa_monitor_updates_held(&self,
 counterparty_node_id: PublicKey, channel_id: ChannelId
	) -> bool {
 let per_peer_state = self.per_peer_state.read().unwrap();
 if let Some(peer_state_mtx) = per_peer_state.get(&counterparty_node_id) {
 let mut peer_state_lck = peer_state_mtx.lock().unwrap();
 let peer_state = &mut *peer_state_lck;

 if let Some(chan) = peer_state.channel_by_id.get(&channel_id) {
 return self.raa_monitor_updates_held(&peer_state.actions_blocking_raa_monitor_updates,
 chan.context().get_funding_txo().unwrap(), counterparty_node_id);
 }
 }
 false
	}

	fn internal_revoke_and_ack(&self, counterparty_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
 let (htlcs_to_fail, res) = {
 let per_peer_state = self.per_peer_state.read().unwrap();
Expand DownExpand Up@@ -6477,8 +6521,8 @@ where
 match monitor_event {
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", &preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, funding_outpoint);
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", preimage);
 self.claim_funds_internal(htlc_update.source, preimage, htlc_update.htlc_value_satoshis.map(|v| v * 1000), true, counterparty_node_id, funding_outpoint);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", &htlc_update.payment_hash);
 let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
Expand DownExpand Up@@ -9298,6 +9342,7 @@ where
 // downstream chan is closed (because we don't have a
 // channel_id -> peer map entry).
 counterparty_opt.is_none(),
 counterparty_opt.cloned().or(monitor.get_counterparty_node_id()),
@valentinewallacevalentinewallaceAug 30, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be effectively the same to just pass in counterparty_opt? It seems like we won't do anything unless the channel is around
@TheBlueMattTheBlueMattAug 31, 2023

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, yea, we definitely could, might even be better cause we'd never bother trying to wake a channel that's already been closed. That said, it does feel like the kinda thing we'll screw up in the future trying to use that arg and then end up not calling some important code. At least with the fallback all remotely-recently-deployed nodes will still work? I dunno.
 monitor.get_funding_txo().0))
 } else { None }
 } else {
Expand DownExpand Up@@ -9576,12 +9621,12 @@ where
 channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 }

 for (source, preimage, downstream_value, downstream_closed, downstream_funding) in pending_claims_to_replay {
 for (source, preimage, downstream_value, downstream_closed, downstream_node_id, downstream_funding) in pending_claims_to_replay {
 // We use `downstream_closed` in place of `from_onchain` here just as a guess - we
 // don't remember in the `ChannelMonitor` where we got a preimage from, but if the
 // channel is closed we just assume that it probably came from an on-chain claim.
 channel_manager.claim_funds_internal(source, preimage, Some(downstream_value),
 downstream_closed, downstream_funding);
 downstream_closed, downstream_node_id, downstream_funding);
 }

 //TODO: Broadcast channel update for closed channels, but only after we've made a
Expand Down
Loading