Add payment_hash to PaymentSent #999 #1062

galderz · 2021-08-27T10:12:39Z

Implementation for the payment_hash part of #999.

galderz · 2021-08-27T12:49:00Z

Ups, seems like I was working on a very old main. I'll rebase asap.

galderz · 2021-08-27T14:30:54Z

Rebased 😅

codecov · 2021-08-27T14:57:11Z

Codecov Report

Merging #1062 (204bfd2) into main (801d6e5) will decrease coverage by 0.09%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
- Coverage   90.76%   90.66%   -0.10%     
==========================================
  Files          65       66       +1     
  Lines       33894    34631     +737     
==========================================
+ Hits        30764    31399     +635     
- Misses       3130     3232     +102

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.08% <ø> (+0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <ø> (ø)`
lightning/src/util/events.rs	`32.16% <50.00%> (+4.67%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.90% <75.00%> (-0.10%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.39% <92.30%> (-0.15%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/shutdown_tests.rs	`95.89% <100.00%> (+0.17%)`	⬆️
lightning/src/util/ser.rs	`89.14% <0.00%> (-2.79%)`	⬇️
lightning-block-sync/src/lib.rs	`93.49% <0.00%> (-1.69%)`	⬇️
lightning/src/ln/script.rs	`92.64% <0.00%> (-1.11%)`	⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 801d6e5...204bfd2. Read the comment docs.

lightning/src/chain/chainmonitor.rs

lightning/src/ln/functional_test_utils.rs

TheBlueMatt · 2021-08-27T16:52:06Z

lightning/src/util/events.rs

 				2u8.write(writer)?;
 				write_tlv_fields!(writer, {
 					(0, payment_preimage, required),
+					(2, payment_hash, required),


This should be an odd number (implying that old versions will ignore it and be fine with that), and probably also an option at least for the first version (implying that if we deserialize a paymentsent written by an old version we'll just treat it as None. In a future version, we could probably break compat and say "we cannot read old PaymentSent events that are pending".

Why not do something like this so the payment_hash field doesn't have to be an Option?

diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 03553651..8bcb3f2b 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -21,6 +21,7 @@ use routing::network_graph::NetworkUpdate; use util::ser::{Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper}; use bitcoin::blockdata::script::Script; +use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::secp256k1::key::PublicKey; @@ -128,7 +129,7 @@ pub enum Event { /// in which case the value of this field will be `None`. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment - payment_hash: Option<PaymentHash>, + payment_hash: PaymentHash, }, /// Indicates an outbound payment we made failed. Probably some intermediary node dropped /// something. You may wish to retry with a different route. @@ -233,7 +234,7 @@ impl Writeable for Event { 2u8.write(writer)?; write_tlv_fields!(writer, { (0, payment_preimage, required), - (1, payment_hash, option), + (1, payment_hash, required), }); }, &Event::PaymentFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref all_paths_failed, @@ -322,9 +323,12 @@ impl MaybeReadable for Event { (0, payment_preimage, required), (1, payment_hash, option), }); + if payment_hash.is_none() { + payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner())); + } Ok(Some(Event::PaymentSent { payment_preimage, - payment_hash, + payment_hash: payment_hash.unwrap(), })) }; f()

Ah, yea, actually that's a good idea, would prefer that.

galderz · 2021-08-31T07:57:02Z

Pushed a new commit. It should address all the comments raised so far.

TheBlueMatt · 2021-08-31T17:56:53Z

lightning/src/util/events.rs

@@ -118,6 +118,8 @@ pub enum Event {
 		/// Note that this serves as a payment receipt, if you wish to have such a thing, you must
 		/// store it somehow!
 		payment_preimage: PaymentPreimage,
+		/// The hash which was given to ChannelManager::send_payment.


This should be a link (ie wrapped in [` and ]). Also, should mention when its None` and when its not (reading old events)

I've extended the comment. See the latest update.

Links are resolved locally, so you need to provide a full path at the end which tells cargo doc exactly where to look, eg:

--- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -120,5 +120,7 @@ pub enum Event { payment_preimage: PaymentPreimage, - /// The hash which was given to [ChannelManager::send_payment]. + /// The hash which was given to [`ChannelManager::send_payment`]. /// When the payment sent message comes from an older library version, /// the payment hash will be missing, in which case the value of this field will be `None`. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_hash: Option<PaymentHash>,

lightning/src/ln/channelmanager.rs

valentinewallace

ACK mod fixing CI

valentinewallace · 2021-09-15T17:03:34Z

lightning/src/util/events.rs

+		/// When the payment sent message comes from an older library version,
+		/// the payment hash will be missing, in which case the value of this field will be `None`.


nit: s/payment sent/PaymentSent

nit: 100char lines plz

lightning/src/ln/functional_tests.rs

galderz · 2021-09-21T07:59:07Z

@TheBlueMatt @valentinewallace rebased and pushed a new commit addressing all your latests comments.

valentinewallace

One last actual question here: #1062 (comment)
Otherwise LGTM

valentinewallace · 2021-09-21T17:28:03Z

lightning/src/util/events.rs

+		/// The hash which was given to [`ChannelManager::send_payment`].
+		/// When the `PaymentSent` message comes from an older library version,
+		/// the payment hash will be missing,
+		/// in which case the value of this field will be `None`.


nit: 100char lines

Hmmmm? What line are you exactly complaining about?

The only one that exceeds 100 chars is the last of these:

/// The hash which was given to [`ChannelManager::send_payment`]. /// When the `PaymentSent` message comes from an older library version, /// the payment hash will be missing, /// in which case the value of this field will be `None`. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment

But, there are plenty of those earlier in the source file

This is the diff I'm looking for:

diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 03553651..cc54f9b1 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -122,10 +122,9 @@ pub enum Event { /// Note that this serves as a payment receipt, if you wish to have such a thing, you must /// store it somehow! payment_preimage: PaymentPreimage, - /// The hash which was given to [`ChannelManager::send_payment`]. - /// When the `PaymentSent` message comes from an older library version, - /// the payment hash will be missing, - /// in which case the value of this field will be `None`. + /// The hash which was given to [`ChannelManager::send_payment`]. When the `PaymentSent` message + /// comes from an older library version, the payment hash will be missing, in which case the + /// value of this field will be `None`. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_hash: Option<PaymentHash>,

So we want lines to not be too short or too long.

I'm sorry but that's not really very helpful feedback. You might as well the exact format that you want :\

Pushed a change based on your proposal.

Well, the idea is that we want to fit as much text as is reasonable onto the page (hence avoiding too-short lines) in while also allowing multiple terminals side-by-side (hence the line length limit). Vim makes this super easy. Not sure how to be more helpful.

That said, it was a nit so you can ignore it if you really want.

galderz · 2021-09-28T12:06:17Z

Update the commit to address #1062 (comment)

TheBlueMatt · 2021-09-29T23:55:27Z

Update the commit to address #1062 (comment)

Hmm, I don't see any change and github seems to think the last push was 6 days ago.

galderz · 2021-10-01T12:07:37Z

Hmmm, I think I forgot to push. Should be done now.

TheBlueMatt · 2021-10-01T17:12:48Z

lightning/src/util/events.rs

@@ -122,6 +123,12 @@ pub enum Event {
 		/// Note that this serves as a payment receipt, if you wish to have such a thing, you must
 		/// store it somehow!
 		payment_preimage: PaymentPreimage,
+		/// The hash which was given to [`ChannelManager::send_payment`]. When the `PaymentSent` message
+		/// comes from an older library version, the payment hash will be missing, in which case the


The latter part of the documentation here should be dropped.

TheBlueMatt

One note on docs, but otherwise looks good.

lightning/src/util/events.rs

valentinewallace · 2021-10-08T20:46:23Z

lightning/src/ln/channelmanager.rs

-						events::Event::PaymentSent { payment_preimage }
+						events::Event::PaymentSent {
+							payment_preimage,
+							payment_hash: payment_hash


nit: s/payment_hash: payment_hash/payment_hash

galderz force-pushed the t_payment_hash_999 branch from 88e4998 to d2589bc Compare August 27, 2021 14:30

TheBlueMatt reviewed Aug 27, 2021

View reviewed changes

galderz force-pushed the t_payment_hash_999 branch from d2589bc to 46d4568 Compare August 31, 2021 07:56

TheBlueMatt reviewed Aug 31, 2021

View reviewed changes

galderz force-pushed the t_payment_hash_999 branch from 46d4568 to 38cce30 Compare September 15, 2021 06:21

valentinewallace reviewed Sep 15, 2021

View reviewed changes

galderz force-pushed the t_payment_hash_999 branch 2 times, most recently from 5ccd140 to 1f9788a Compare September 21, 2021 07:57

TheBlueMatt approved these changes Sep 21, 2021

View reviewed changes

valentinewallace reviewed Sep 21, 2021

View reviewed changes

galderz force-pushed the t_payment_hash_999 branch from 1f9788a to df04817 Compare September 23, 2021 12:41

galderz force-pushed the t_payment_hash_999 branch from df04817 to af1ce40 Compare October 1, 2021 12:06

TheBlueMatt reviewed Oct 1, 2021

View reviewed changes

galderz force-pushed the t_payment_hash_999 branch from af1ce40 to d9f55be Compare October 4, 2021 20:37

TheBlueMatt reviewed Oct 5, 2021

View reviewed changes

lightning/src/util/events.rs Show resolved Hide resolved

Add payment_hash to PaymentSent lightningdevkit#999

204bfd2

galderz force-pushed the t_payment_hash_999 branch from d9f55be to 204bfd2 Compare October 8, 2021 04:59

TheBlueMatt approved these changes Oct 8, 2021

View reviewed changes

valentinewallace approved these changes Oct 8, 2021

View reviewed changes

TheBlueMatt merged commit e635db0 into lightningdevkit:main Oct 8, 2021

		/// When the payment sent message comes from an older library version,
		/// the payment hash will be missing, in which case the value of this field will be `None`.

Add payment_hash to PaymentSent #999 #1062

Add payment_hash to PaymentSent #999 #1062

Uh oh!

Conversation

galderz commented Aug 27, 2021

Uh oh!

galderz commented Aug 27, 2021

Uh oh!

galderz commented Aug 27, 2021

Uh oh!

codecov bot commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Aug 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

galderz commented Sep 21, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galderz commented Sep 28, 2021

Uh oh!

TheBlueMatt commented Sep 29, 2021

Uh oh!

galderz commented Oct 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 27, 2021 •

edited

Loading