expose from/to_channel_id in PaymentForwarded Event#1231

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

Closed
johncantrell97 wants to merge 1 commit into from
Closed

expose from/to_channel_id in PaymentForwarded Event #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
pub(crate) payment_preimage: Option<PaymentPreimage>,
pub(crate) source: HTLCSource,
pub(crate) onchain_value_satoshis: Option<u64>,
pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
(0, payment_hash, required),
(1, onchain_value_satoshis, option),
(2, source, required),
(4, payment_preimage, option),
(6, forward_channel_id, required)

@johncantrell97johncantrell97Jan 9, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base

@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).

@TheBlueMattTheBlueMattJan 10, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).

});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: None,
source: source.clone(),
onchain_value_satoshis,
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
if let Some(idx) = input_idx {
self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: Some(payment_preimage),
payment_hash,
onchain_value_satoshis: Some(amount_msat / 1000),
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
}
} else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
payment_preimage: Some(payment_preimage),
payment_hash,
onchain_value_satoshis: Some(amount_msat / 1000),
forward_channel_id: self.funding_info.0.to_channel_id(),
}));
}
} else {
Expand Down
12 changes: 9 additions & 3 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
}

fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
match source {
HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// update to. Instead, we simply document in `PaymentForwarded` that this
// can happen.
}

// Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());

@johncantrell97johncantrell97Jan 9, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed

@TheBlueMattTheBlueMattJan 10, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?


mem::drop(channel_state_lock);
if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push(events::Event::PaymentForwarded {
from_channel_id,
to_channel_id,
fee_earned_msat,
claim_from_onchain_tx: from_onchain,
});
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
}
};
self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
Ok(())
}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
MonitorEvent::HTLCEvent(htlc_update) => {
if let Some(preimage) = htlc_update.payment_preimage {
log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
} else {
log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down
27 changes: 24 additions & 3 deletions lightning/src/util/events.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
/// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
/// transaction.
claim_from_onchain_tx: bool,
/// The channel_id of the channel which sent us the payment. If the channel has been
/// force-closed this will be None
from_channel_id: Option<[u8; 32]>,
/// The channel_id of the channel which we forwarded the payment along
to_channel_id: [u8; 32],
Comment on lines +363 to +367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.

},
/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
(0, VecWriteWrapper(outputs), required),
});
},
&Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
&Event::PaymentForwarded {
fee_earned_msat,
claim_from_onchain_tx,
from_channel_id,
to_channel_id,
} => {
7u8.write(writer)?;
write_tlv_fields!(writer, {
(0, fee_earned_msat, option),
(2, claim_from_onchain_tx, required),
(4, from_channel_id, option),
(6, to_channel_id, required)

@johncantrell97johncantrell97Jan 9, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help

@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.

});
},
}
&Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
9u8.write(writer)?;
write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
let f = || {
let mut fee_earned_msat = None;
let mut claim_from_onchain_tx = false;
let mut from_channel_id = None;
let mut to_channel_id = [0; 32];
read_tlv_fields!(reader, {
(0, fee_earned_msat, option),
(2, claim_from_onchain_tx, required),
(4, from_channel_id, option),
(6, to_channel_id, required)
});
Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
Ok(Some(Event::PaymentForwarded {
fee_earned_msat,
claim_from_onchain_tx,
from_channel_id,
to_channel_id,
}))
};
f()
},
Expand Down
, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down

, '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
expose from/to_channel_id in PaymentForwarded Event#1231

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

 Closed

johncantrell97
wants to merge
1
commit into

from

 Closed


expose from/to_channel_id in PaymentForwarded Event
#1231
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


1 commit


Select commit


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


5 changes: 5 additions & 0 deletions
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -167,12 +167,14 @@ pub struct HTLCUpdate {
	pub(crate) payment_preimage: Option<PaymentPreimage>,
	pub(crate) source: HTLCSource,
	pub(crate) onchain_value_satoshis: Option<u64>,
	pub(crate) forward_channel_id: [u8; 32],
}
impl_writeable_tlv_based!(HTLCUpdate, {
	(0, payment_hash, required),
	(1, onchain_value_satoshis, option),
	(2, source, required),
	(4, payment_preimage, option),
	(6, forward_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

little confused by the tlv encoding for internal messages? if a message isn't going over the wire then what's the deal with the identifier? i guess it functions the same? wasn't sure to mark this 5 or 6 and would love some generic thoughts on how these are used throughout the code base
@jkczyzjkczyzJan 10, 2022

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.

This is needed when serializing the data for persistence. Similar to TLVs over the wire, even fields are required and odd are optional. So a new field should typically be optional and use the next odd number (3 in this case). That way, if you serialize an object it can still be read by a newer version of the code that added new fields. In the code, the field would be an Option unless it could be generated somehow (e.g., a payment hash from a preimage).
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Right, we use it internally for most structs "because why not" - its a pretty decent way to do forward-compat and we already have the code for it, so we might as well use it. That said, we can't set this to even or required it - required means it must be there or the serialized object is invalid (ie it would imply all LDK objects from the current version(s) will be invalid to a new version with this patch), and even would imply that any old version would see this object as invalid (we try to ensure at least a version-or-two back can read objects written by the latest version of LDK, where possible).
});

/// If an HTLC expires within this many blocks, don't try to claim it in a shared transaction,
Expand DownExpand Up@@ -2512,6 +2514,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: None,
 source: source.clone(),
 onchain_value_satoshis,
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 if let Some(idx) = input_idx {
 self.htlcs_resolved_on_chain.push(IrrevocablyResolvedHTLC { input_idx: idx, payment_preimage: None });
Expand DownExpand Up@@ -2851,6 +2854,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else if offered_preimage_claim {
Expand All@@ -2872,6 +2876,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 payment_preimage: Some(payment_preimage),
 payment_hash,
 onchain_value_satoshis: Some(amount_msat / 1000),
 forward_channel_id: self.funding_info.0.to_channel_id(),
 }));
 }
 } else {
Expand Down

12 changes: 9 additions & 3 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3952,7 +3952,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 }
	}

	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
	fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool, to_channel_id: [u8; 32]) {
 match source {
 HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
 mem::drop(channel_state_lock);
Expand DownExpand Up@@ -4030,6 +4030,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // update to. Instead, we simply document in `PaymentForwarded` that this
 // can happen.
 }

 // Seems like if the prev hop force closed then we wont be able to get the channel_id from short_channel_id
 let from_channel_id = channel_state_lock.short_to_id.get(&hop_data.short_channel_id).map(|chan_id| chan_id.clone());
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

would be nice to have a way to always get the channel_id even if it has been closed
@TheBlueMattTheBlueMattJan 10, 2022

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Hmm, we don't keep that (or, really, any) information after a channel closure. In general ChannelManager never stores any historical data at all - it provides users with events and users can figure it out if they want to. I think its fine to have a None if the channel has been closed, but you could also provide the SCID and let users do the mapping themselves if they want it as a channel id?

 mem::drop(channel_state_lock);
 if let ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) = res {
 let result: Result<(), _> = Err(err);
Expand All@@ -4044,6 +4048,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

 let mut pending_events = self.pending_events.lock().unwrap();
 pending_events.push(events::Event::PaymentForwarded {
 from_channel_id,
 to_channel_id,
 fee_earned_msat,
 claim_from_onchain_tx: from_onchain,
 });
Expand DownExpand Up@@ -4458,7 +4464,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 }
 };
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false);
 self.claim_funds_internal(channel_lock, htlc_source, msg.payment_preimage.clone(), Some(forwarded_htlc_value), false, msg.channel_id);
 Ok(())
	}

Expand DownExpand Up@@ -4777,7 +4783,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 MonitorEvent::HTLCEvent(htlc_update) => {
 if let Some(preimage) = htlc_update.payment_preimage {
 log_trace!(self.logger, "Claiming HTLC with preimage {} from our monitor", log_bytes!(preimage.0));
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true);
 self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage, htlc_update.onchain_value_satoshis.map(|v| v * 1000), true, htlc_update.forward_channel_id);
 } else {
 log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
Expand Down

27 changes: 24 additions & 3 deletions
lightning/src/util/events.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -360,6 +360,11 @@ pub enum Event {
 /// If this is `true`, the forwarded HTLC was claimed by our counterparty via an on-chain
 /// transaction.
 claim_from_onchain_tx: bool,
 /// The channel_id of the channel which sent us the payment. If the channel has been
 /// force-closed this will be None
 from_channel_id: Option<[u8; 32]>,
 /// The channel_id of the channel which we forwarded the payment along
 to_channel_id: [u8; 32],

Comment on lines

+363
 to 
+367

@jkczyzjkczyzJan 10, 2022

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.

Note, given my earlier comment, these would both need to be an Option. You may need a different way to distinguish a force close in the case of from_channel_id, though.
	},
	/// Used to indicate that a channel with the given `channel_id` is in the process of closure.
	ChannelClosed {
Expand DownExpand Up@@ -478,13 +483,20 @@ impl Writeable for Event {
 (0, VecWriteWrapper(outputs), required),
 });
 },
 &Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx } => {
 &Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 } => {
 7u8.write(writer)?;
 write_tlv_fields!(writer, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
@johncantrell97johncantrell97Jan 9, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

similar to comment above about tlv fields for HTLCUpdate -- plz help
@jkczyzjkczyzJan 10, 2022

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.

These would be 1 and 3. IIRC, later you may have an odd field become required once we decide to no longer support an older serialization format. This would allow us to drop Option from the interface. For example, PaymentPathFailed has an odd Option<PaymentId> because the event preexisted PaymentId, but at some point we could make the field simply PaymentId and stop supporting old serialization formats.
 });
 },
 }
 &Event::ChannelClosed { ref channel_id, ref user_channel_id, ref reason } => {
 9u8.write(writer)?;
 write_tlv_fields!(writer, {
Expand DownExpand Up@@ -638,11 +650,20 @@ impl MaybeReadable for Event {
 let f = || {
 let mut fee_earned_msat = None;
 let mut claim_from_onchain_tx = false;
 let mut from_channel_id = None;
 let mut to_channel_id = [0; 32];
 read_tlv_fields!(reader, {
 (0, fee_earned_msat, option),
 (2, claim_from_onchain_tx, required),
 (4, from_channel_id, option),
 (6, to_channel_id, required)
 });
 Ok(Some(Event::PaymentForwarded { fee_earned_msat, claim_from_onchain_tx }))
 Ok(Some(Event::PaymentForwarded {
 fee_earned_msat,
 claim_from_onchain_tx,
 from_channel_id,
 to_channel_id,
 }))
 };
 f()
 },
Expand Down