Add `PaymentHash` to `Record` #2930

jbesraa · 2024-03-12T14:28:15Z

resolves #2767

codecov-commenter · 2024-03-12T15:08:57Z

Codecov Report

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

Project coverage is 89.89%. Comparing base (38690bf) to head (f2d9eb5).
Report is 14 commits behind head on main.

❗ Current head f2d9eb5 differs from pull request most recent head fe7f548. Consider uploading reports for the commit fe7f548 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/peer_handler.rs	15.00%	31 Missing and 3 partials ⚠️
lightning/src/ln/channelmanager.rs	69.30%	27 Missing and 4 partials ⚠️
lightning/src/chain/chainmonitor.rs	83.33%	1 Missing ⚠️
lightning/src/chain/channelmonitor.rs	95.23%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2930      +/-   ##
==========================================
+ Coverage   89.85%   89.89%   +0.04%     
==========================================
  Files         116      116              
  Lines       96303    97456    +1153     
  Branches    96303    97456    +1153     
==========================================
+ Hits        86529    87608    +1079     
- Misses       7217     7294      +77     
+ Partials     2557     2554       -3

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

TheBlueMatt

Thanks for working on this! This looks good as a first step, but there's obviously a ton of logs that are missing payment hash info after this. Could you do a pass of all the logs in chain (eg channelmonitor) and channel and see what needs payment hash fields?

TheBlueMatt · 2024-03-12T19:27:46Z

lightning/src/ln/channelmanager.rs

@@ -2008,7 +2008,7 @@ macro_rules! handle_error {
 					let counterparty_node_id = shutdown_res.counterparty_node_id;
 					let channel_id = shutdown_res.channel_id;
 					let logger = WithContext::from(
-						&$self.logger, Some(counterparty_node_id), Some(channel_id),
+						&$self.logger, Some(counterparty_node_id), Some(channel_id), None


Hmm, we don't want to always have no payment_hash here, eg this can be called handling an update_add_htlc which we presumably want to log as associated with a payment hash.

How do I get a hold on the payment_hash here? or identify the update_add_htlc call ?

TheBlueMatt · 2024-03-12T20:14:23Z

lightning/src/ln/channelmanager.rs

@@ -6511,7 +6511,8 @@ where
 					let logger = WithContext::from(
 						&self.logger,
 						Some(chan.context.get_counterparty_node_id()),
-						Some(chan.context.channel_id())
+						Some(chan.context.channel_id()),
+                        None


nit: indentation is wrong.

TheBlueMatt · 2024-03-12T20:16:14Z

lightning/src/util/logger.rs

@@ -120,6 +120,8 @@ pub struct Record<$($args)?> {
 	pub file: &'static str,
 	/// The line containing the message.
 	pub line: u32,
+	/// The payment hash


Please add some real docs here (and maybe move it up to be next to channel/peer_ids).

jbesraa · 2024-03-22T14:34:05Z

Thanks for the reivew!

Regarding changes in lightning/src/chain/channelmonitor.rs, we would need to add the payment hash here so WithChannelMonitor should be initiated with an additional payment_hash arg as an Option<PaymentHash>. Is this the right approach here?

impl<'a, L: Deref> Logger for WithChannelMonitor<'a, L> where L::Target: Logger {
	fn log(&self, mut record: Record) {
		record.peer_id = self.peer_id;
		record.channel_id = self.channel_id;
		// record.payment_hash = self.payment_hash;
		self.logger.log(record)
	}
}

For the update_add_htlc I decided to add a new specific function from_update_add_htlc because its the only function that can forward htlc in that context and it didnt feel right to change send_err_msg_no_close just for that.

Regarding ln/channel.rs I think all the changes are in the initial comment, let me know If I missed something there.

TheBlueMatt · 2024-03-22T14:49:32Z

We could go one of two ways with CHannelMonitor, we could either use WithContext::from(WithChannelMonitor(...), None, None, payment_hash) or we could add a payment_hash field in WithChannelMonitor. I'm happy with either.

TheBlueMatt · 2024-03-22T14:52:21Z

Regarding ln/channel.rs I think all the changes are in the initial comment, let me know If I missed something there.

Hmm? There's no changes in this PR that pass payment hashes to logging in channel.rs, and the wrapping loggers passed into channel.rs methods don't always set a payment hash. We should actually set a payment hash if we're logging something relevant to a payment.

TheBlueMatt · 2024-05-08T14:43:03Z

Ugh, okay, sorry this kinda got put on the back burner during the release rush. Needs a rebase now then we can land it quickly. When you do rebase, please do a quick read-through of at least all the logging calls in channel.rs, channelmanager.rs, and channelmonitor.rs/onchaintx.rs and check that if they need a payment hash they have it.

Adding the `payment_hash` to `Record` so we are able to track it in logs.

jbesraa · 2024-05-09T13:41:06Z

rebased + went through the mentioned files and it looks good to me. I couldn't find anything relevant in channelmonitor.rs/onchaintx.rs though

TheBlueMatt

ACK if we drop the last two commits. I want to get these tags in messages in peer_handler but they're currently missing so its fine. There'a few places here where we really should have a payment_hash but we currently only have the payment_preimage in scope. In a followup we should consider if we can pass the payment_hash in.

TheBlueMatt · 2024-05-09T15:55:16Z

lightning/src/util/logger.rs

+	///
+	/// Note that this is only filled in for logs pertaining to a specific payment, and will be
+	/// `None` for logs which are not directly related to a payment.
+	pub payment_hash: Option<PaymentHash>,


In a followup, we should put this next to chanel_id/peer_id so its not hanging out here all on its own. Same for the Record constructor.

TheBlueMatt · 2024-05-09T16:36:19Z

lightning/src/ln/channelmanager.rs

 				let mut msg_event = None;

 				if let Some((shutdown_res, update_option)) = shutdown_finish {
 					let counterparty_node_id = shutdown_res.counterparty_node_id;
 					let channel_id = shutdown_res.channel_id;
 					let logger = WithContext::from(
-						&$self.logger, Some(counterparty_node_id), Some(channel_id), None
+						&$self.logger, Some(counterparty_node_id), Some(channel_id), payment_hash


Actually not sure if Force-closing channel should be logged with a payment_hash given its not specific to that payment but now broad.

valentinewallace · 2024-05-09T16:32:04Z

lightning/src/ln/channelmanager.rs

@@ -5278,6 +5299,7 @@ where
 										}, skimmed_fee_msat, ..
 									},
 								}) => {
+									let logger = WithChannelContext::from(&self.logger, &chan.context, Some(payment_hash));


In this method in the check_total_value macro and elsewhere, could use the contextual logger rather than self.logger. Those logs do currently contain the payment hash anyway, but I think they may print bytes instead of hex.

added 232d6af and 5776c43 to handle this

I needed the first commit so I can fallback to self.logger in case something goes wrong while initializing WithcChannelContext

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-05-09T16:37:51Z

lightning/src/ln/channelmanager.rs

@@ -6468,7 +6490,7 @@ where
 				if let hash_map::Entry::Occupied(mut chan_phase_entry) = peer_state.channel_by_id.entry(chan_id) {
 					if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
 						let counterparty_node_id = chan.context.get_counterparty_node_id();
-						let logger = WithChannelContext::from(&self.logger, &chan.context);
+						let logger = WithChannelContext::from(&self.logger, &chan.context, None);


Technically we do have the payment_preimage in this method, which means we could calculate the payment hash. Not sure if worth it but we could. Similar in internal_update_fulfill_htlc.

jbesraa · 2024-05-09T18:06:46Z

@TheBlueMatt @valentinewallace I just took out the last two commit but unfortunately wont have time today to look into the rest of the review. feel free to fixup changes if you want this merged, otherwise will handle it tomorrow

jbesraa · 2024-05-10T12:05:01Z

ACK if we drop the last two commits. I want to get these tags in messages in peer_handler but they're currently missing so its fine. There'a few places here where we really should have a payment_hash but we currently only have the payment_preimage in scope. In a followup we should consider if we can pass the payment_hash in.

can you please say more about the first part? in which messages in peer_handler we would expect to have payment preimage/hash?

there are a couple of places in channel_manager where payment_preimage is defined and I converted that to payment_hash(see last commit)

TheBlueMatt · 2024-05-10T13:15:01Z

can you please say more about the first part? in which messages in peer_handler we would expect to have payment preimage/hash?

Specifically the Enqueuing/Received message * messages, but lets do it in a followup.

there are a couple of places in channel_manager where payment_preimage is defined and I converted that to payment_hash(see last commit)

Let's not re-hash the payment preimage, I'd rather just leave it out for now. In at least some of the cases we have the payment hash a few functions up in the stack trace and we should look into passing it down, but we don't have to do it right now, it can come in a followup so this doesn't need rebased again.

jbesraa · 2024-05-10T15:08:17Z

Took out the preimage->hash commit. @valentinewallace would appreciate your input about the last two commits.

valentinewallace · 2024-05-10T15:42:32Z

Took out the preimage->hash commit. @valentinewallace would appreciate your input about the last two commits.

Thanks for adding that. Looks like a promising direction, but tend to agree with Matt that we should land the first two commits on their own to avoid delaying landing this and save them for follow-up :)

TheBlueMatt

Thanks! Glad we can land this but also looking forward to followup :)

TheBlueMatt reviewed Mar 12, 2024

View reviewed changes

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 7091aeb to 835f5aa Compare March 22, 2024 14:24

jbesraa force-pushed the Add-PaymentHash-to-Record branch 3 times, most recently from 5c67d47 to 6ceae20 Compare March 28, 2024 08:49

jbesraa force-pushed the Add-PaymentHash-to-Record branch 2 times, most recently from 64dc482 to 1e4b75f Compare April 8, 2024 15:32

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 1e4b75f to 7df3773 Compare May 9, 2024 08:56

jbesraa added 2 commits May 9, 2024 11:57

Add PaymentHash to Record

3fb329b

Adding the `payment_hash` to `Record` so we are able to track it in logs.

Add payment_hash to WithChannelMonitor

fe7f548

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 7df3773 to 99539d7 Compare May 9, 2024 08:59

TheBlueMatt reviewed May 9, 2024

View reviewed changes

valentinewallace reviewed May 9, 2024

View reviewed changes

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 99539d7 to fe7f548 Compare May 9, 2024 18:02

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 571e452 to f2d9eb5 Compare May 10, 2024 12:25

jbesraa force-pushed the Add-PaymentHash-to-Record branch from f2d9eb5 to 5776c43 Compare May 10, 2024 15:06

jbesraa force-pushed the Add-PaymentHash-to-Record branch from 5776c43 to fe7f548 Compare May 10, 2024 15:44

valentinewallace approved these changes May 10, 2024

View reviewed changes

TheBlueMatt approved these changes May 10, 2024

View reviewed changes

TheBlueMatt merged commit 0ffa4b3 into lightningdevkit:main May 10, 2024
31 of 32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `PaymentHash` to `Record` #2930

Add `PaymentHash` to `Record` #2930

jbesraa commented Mar 12, 2024

codecov-commenter commented Mar 12, 2024 •

edited

TheBlueMatt left a comment

TheBlueMatt Mar 12, 2024

jbesraa May 10, 2024

TheBlueMatt Mar 12, 2024

TheBlueMatt Mar 12, 2024

jbesraa commented Mar 22, 2024

TheBlueMatt commented Mar 22, 2024

TheBlueMatt commented Mar 22, 2024

TheBlueMatt commented May 8, 2024

jbesraa commented May 9, 2024

TheBlueMatt left a comment

TheBlueMatt May 9, 2024

TheBlueMatt May 9, 2024

valentinewallace May 9, 2024

jbesraa May 10, 2024

valentinewallace May 9, 2024

jbesraa commented May 9, 2024

jbesraa commented May 10, 2024 •

edited

TheBlueMatt commented May 10, 2024

jbesraa commented May 10, 2024

valentinewallace commented May 10, 2024

TheBlueMatt left a comment

Add PaymentHash to Record #2930

Add PaymentHash to Record #2930

Conversation

jbesraa commented Mar 12, 2024

codecov-commenter commented Mar 12, 2024 • edited

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Mar 22, 2024

TheBlueMatt commented Mar 22, 2024

TheBlueMatt commented Mar 22, 2024

TheBlueMatt commented May 8, 2024

jbesraa commented May 9, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented May 9, 2024

jbesraa commented May 10, 2024 • edited

TheBlueMatt commented May 10, 2024

jbesraa commented May 10, 2024

valentinewallace commented May 10, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

Add `PaymentHash` to `Record` #2930

Add `PaymentHash` to `Record` #2930

codecov-commenter commented Mar 12, 2024 •

edited

jbesraa commented May 10, 2024 •

edited