-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
adds node_id to Event::Payment{Received, Claimed} #1766
Conversation
lightning/src/util/events.rs
Outdated
@@ -631,6 +635,7 @@ impl Writeable for Event { | |||
(4, amount_msat, required), | |||
(6, 0u64, required), // user_payment_id required for compatibility with 0.0.103 and earlier | |||
(8, payment_preimage, option), | |||
(10, node_id, required), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New fields should (generally) be odd and generally an , option
rather than , required
. required
implies we'll reject existing data (as we fail to deserialize if its missing) and even id implies old versions of LDK will refuse to read the data serialized by new versions, whereas odd allows them to ignore it.
lightning/src/util/events.rs
Outdated
@@ -249,6 +249,8 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The received node, ie whether the node was a phantom node or not. | |||
node_id: Option<PublicKey>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we could communicate more details with the name here - something like our_received_node_id or something like that that communicates that its our node_id rather than the node_id of the peer that forwarded the payment to us.
Codecov ReportBase: 90.70% // Head: 90.58% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1766 +/- ##
==========================================
- Coverage 90.70% 90.58% -0.12%
==========================================
Files 91 91
Lines 47967 48009 +42
Branches 47967 48009 +42
==========================================
- Hits 43508 43489 -19
- Misses 4459 4520 +61
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Both very valid points, updated code based on feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
Also, right now the node_id is wrapped in an Option just to make the default easier, but can look into making it so it is not wrapped.
Are you referring to a default value for serialization? If so, you should be able to make the field a PublicKey
and use the Option<PublicKey>
wrapping only in a temporary variable in the serialization logic.
I changed the logic to follow this, it seems I either need to make the 'our_node_Id' be an Option<> and mark it as 'option' in write_tlv_fields! & read_tlv_fields! or have it not be wrapped and set it as 'required'. Please let me know what the preferred approach is here, I can see a case for either. |
Ugh, I think you're right. We're probably just should to make it an |
Reverted the last commit to keep the 'our_node_id' as optional to not break existing clients of the library. |
lightning/src/util/events.rs
Outdated
@@ -249,6 +249,8 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The node that recieved the payment, ie our node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// The node that recieved the payment, ie our node | |
/// The node that received the payment, i.e., our node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this field always going to be populated with "our" node id?
I was wondering if there is merit in the "our" prefix here if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, also the variable name should therefore maybe be called receiver_node_id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I should drop the i.e., our node
part of the comment as well as change the variable name to receiver_node_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems reasonable, otherwise you could change it to something along the lines of e.g., our local node or a phantom node.
@@ -1380,9 +1380,10 @@ macro_rules! expect_payment_received { | |||
let events = $node.node.get_and_clear_pending_events(); | |||
assert_eq!(events.len(), 1); | |||
match events[0] { | |||
$crate::util::events::Event::PaymentReceived { ref payment_hash, ref purpose, amount_msat } => { | |||
$crate::util::events::Event::PaymentReceived {ref payment_hash, ref purpose, amount_msat, our_node_id } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
$crate::util::events::Event::PaymentReceived {ref payment_hash, ref purpose, amount_msat, our_node_id } => { | |
$crate::util::events::Event::PaymentReceived { ref payment_hash, ref purpose, amount_msat, our_node_id } => { |
lightning/src/util/events.rs
Outdated
@@ -273,6 +275,8 @@ pub enum Event { | |||
/// | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
PaymentClaimed { | |||
/// The node that recieved the payment, ie our node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// The node that recieved the payment, ie our node | |
/// The node that received the payment, i.e., our node. |
lightning/src/util/events.rs
Outdated
19u8.write(writer)?; | ||
write_tlv_fields!(writer, { | ||
(0, payment_hash, required), | ||
(2, purpose, required), | ||
(4, amount_msat, required), | ||
(6, our_node_id, option), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be 7
lightning/src/util/events.rs
Outdated
@@ -249,6 +249,8 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The node that recieved the payment, ie our node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this field always going to be populated with "our" node id?
I was wondering if there is merit in the "our" prefix here if that's the case.
36bf056
to
d474665
Compare
renamed |
d474665
to
674b0d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nit for me
lightning/src/util/events.rs
Outdated
@@ -612,7 +616,7 @@ impl Writeable for Event { | |||
// We never write out FundingGenerationReady events as, upon disconnection, peers | |||
// drop any channels which have not yet exchanged funding_signed. | |||
}, | |||
&Event::PaymentReceived { ref payment_hash, ref amount_msat, ref purpose } => { | |||
&Event::PaymentReceived { ref payment_hash, ref amount_msat, ref purpose, receiver_node_id } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: receiver_node_id doesn't need an explicit ref
here, but may be nice for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tiny nits, LGTM otherwise.
lightning/src/util/events.rs
Outdated
@@ -249,6 +249,8 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The node that received the payment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// The node that received the payment | |
/// The node that received the payment. |
lightning/src/util/events.rs
Outdated
@@ -273,6 +275,8 @@ pub enum Event { | |||
/// | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
PaymentClaimed { | |||
/// The node that received the payment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// The node that received the payment | |
/// The node that received the payment. |
lightning/src/ln/channelmanager.rs
Outdated
@@ -3360,6 +3360,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana | |||
} else if total_value == $payment_data.total_msat { | |||
htlcs.push(claimable_htlc); | |||
new_events.push(events::Event::PaymentReceived { | |||
receiver_node_id: Some(self.our_network_pubkey), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want our_node_pubkey
universally. If we are receiving a phantom payment (see further up in this function for the phantom_shared_secret) we actually want to use the pubkey equivalent to self.keys_manager.get_node_secret(Recipient::PhantomNode);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to do something here like you did below. I think it probably makes sense to move the code block you have below up above and then you can just use the receiver_node_id
in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above goes for, I think, basically all the sites where we generate the event. We'll need to figure out if the payment is phantom based on the ClaimableHTLC
object itself, I believe.
a40f5b3
to
9911e81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick pass, mostly some formatting nits.
lightning/src/util/events.rs
Outdated
}); | ||
if purpose.is_none() { return Ok(None); } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this line can be removed.
lightning/src/util/events.rs
Outdated
read_tlv_fields!(reader, { | ||
(0, payment_hash, required), | ||
(2, purpose, ignorable), | ||
(4, amount_msat, required), | ||
(7, receiver_node_id, option), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also probably should be the first available odd type, i.e., 1?
lightning/src/util/events.rs
Outdated
let mut _user_payment_id = None::<u64>; // For compatibility with 0.0.103 and earlier | ||
read_tlv_fields!(reader, { | ||
(0, payment_hash, required), | ||
(2, payment_secret, option), | ||
(4, amount_msat, required), | ||
(6, _user_payment_id, option), | ||
(8, payment_preimage, option), | ||
(11, receiver_node_id, option), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to eat my own words from above: This probably should be the first available odd type, i.e., 1? Sorry for any confusion here!
lightning/src/ln/channelmanager.rs
Outdated
htlcs.push(claimable_htlc); | ||
let mut receiver_node_id = self.our_network_pubkey; | ||
if phantom_shared_secret.is_some(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add space
if phantom_shared_secret.is_some(){ | |
if phantom_shared_secret.is_some() { |
lightning/src/ln/channelmanager.rs
Outdated
e.insert((purpose.clone(), vec![claimable_htlc])); | ||
let mut receiver_node_id = Some(self.our_network_pubkey); | ||
if phantom_shared_secret.is_some(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add space
if phantom_shared_secret.is_some(){ | |
if phantom_shared_secret.is_some() { |
lightning/src/ln/channelmanager.rs
Outdated
let phantom_shared_secret = claimable_htlcs[0].prev_hop.phantom_shared_secret; | ||
if phantom_shared_secret.is_some(){ | ||
let phantom_pubkey = args.keys_manager.get_node_id(Recipient::PhantomNode) | ||
.expect("Failed to get node_id for phantom node recipient"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove space.
.expect("Failed to get node_id for phantom node recipient"); | |
.expect("Failed to get node_id for phantom node recipient"); |
lightning/src/ln/channelmanager.rs
Outdated
@@ -7487,6 +7510,13 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> | |||
if let Some((payment_purpose, claimable_htlcs)) = claimable_htlcs.remove(&payment_hash) { | |||
log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0)); | |||
let mut claimable_amt_msat = 0; | |||
let mut receiver_node_id = Some(our_network_pubkey); | |||
let phantom_shared_secret = claimable_htlcs[0].prev_hop.phantom_shared_secret; | |||
if phantom_shared_secret.is_some(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add space.
if phantom_shared_secret.is_some(){ | |
if phantom_shared_secret.is_some() { |
@@ -1433,20 +1433,22 @@ macro_rules! expect_pending_htlcs_forwardable_from_events { | |||
} | |||
}} | |||
} | |||
|
|||
// expect_payment_received!(&nodes[fwd_idx], payment_hash, payment_secret, payment_amt, payment_preimage_opt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should probably be removed?
// expect_payment_received!(&nodes[fwd_idx], payment_hash, payment_secret, payment_amt, payment_preimage_opt); |
lightning/src/util/events.rs
Outdated
@@ -758,6 +770,7 @@ impl Writeable for Event { | |||
(4, amount_msat, required), | |||
(6, 0u64, required), // user_payment_id required for compatibility with 0.0.103 and earlier | |||
(8, payment_preimage, option), | |||
(11, receiver_node_id, option), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comments: should likely be 1.
lightning/src/util/events.rs
Outdated
19u8.write(writer)?; | ||
write_tlv_fields!(writer, { | ||
(0, payment_hash, required), | ||
(2, purpose, required), | ||
(4, amount_msat, required), | ||
(7, receiver_node_id, option), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comments: should likely be 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically looks good to me! Can you squash the commits down into one or two logical commits and ensure your commit descriptions contain a bit more detail and dont have lines longer than 70-ish characters. Then we can get a second reviewer on this and land it!
lightning/src/ln/channelmanager.rs
Outdated
htlcs.push(claimable_htlc); | ||
let mut receiver_node_id = self.our_network_pubkey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pull this and the next hunk out into code above the macro to DRY it up?
lightning/src/ln/channelmanager.rs
Outdated
let phantom_shared_secret = htlc.prev_hop.phantom_shared_secret; | ||
if phantom_shared_secret.is_some(){ | ||
let phantom_pubkey = self.keys_manager.get_node_id(Recipient::PhantomNode) | ||
.expect("Failed to get node_id for phantom node recipient"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to indent this far.
dd09b2d
to
56851a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good and pretty close to being good to go.
One suggestion and a some formatting nits from my side, feel free to squash any additional changes you make right away so we can land this soon!
if phantom_shared_secret.is_some() { | ||
receiver_node_id = self.keys_manager.get_node_id(Recipient::PhantomNode) | ||
.expect("Failed to get node_id for phantom node recipient"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could re-add the blank line before the macro.
lightning/src/ln/channelmanager.rs
Outdated
@@ -3526,8 +3532,16 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | |||
match channel_state.claimable_htlcs.entry(payment_hash) { | |||
hash_map::Entry::Vacant(e) => { | |||
let purpose = events::PaymentPurpose::SpontaneousPayment(preimage); | |||
let phantom_shared_secret = &claimable_htlc.prev_hop.phantom_shared_secret.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone()
and mut
can be avoided here like so:
let is_phantom = claimable_htlc.prev_hop.phantom_shared_secret.is_some();
e.insert((purpose.clone(), vec![claimable_htlc]));
let receiver_node_id = if is_phantom {
let phantom_pubkey = self.keys_manager.get_node_id(Recipient::PhantomNode)
.expect("Failed to get node_id for phantom node recipient");
Some(phantom_pubkey)
} else {
Some(self.our_network_pubkey)
};
assert_eq!($expected_payment_hash, *payment_hash); | ||
assert_eq!($expected_recv_value, amount_msat); | ||
assert_eq!($expected_receiver_node_id, receiver_node_id.unwrap()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove blank line.
lightning/src/util/events.rs
Outdated
/// The node that received the payment. | ||
/// This is useful to identify payments which were received via [phantom node payments]. | ||
/// This field will always be filled in when the event was generated by LDK versions 0.0.113 and above. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove trailing whitespace.
/// | |
/// |
lightning/src/util/events.rs
Outdated
@@ -342,6 +342,12 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The node that received the payment. | |||
/// This is useful to identify payments which were received via [phantom node payments]. | |||
/// This field will always be filled in when the event was generated by LDK versions 0.0.113 and above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a 100 character line width formatting guideline.It's not enforced strictly everywhere right now, but we try to improve the codebase where we touch it. So feel free to break at 100 chars. :)
0f956da
to
242adaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one tiny, non-blocking nit.
lightning/src/util/events.rs
Outdated
@@ -342,6 +342,13 @@ pub enum Event { | |||
/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds | |||
/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards | |||
PaymentReceived { | |||
/// The node that received the payment. | |||
/// This is useful to identify payments which were received via [phantom node payments]. | |||
/// This field will always be filled in when the event was generated by LDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super-tiny nit: pushing versions
to the next line here and below looks a bit off (and introduced trailing whitespaces again). Feel free to leave as is though, if no other changes are requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, so maybe I just keep it as one long line? not sure where else in that sentence to break on to keep it under the desired character limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? If you'd keep "versions" on the previous line, my editor shows a line width of 92?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow this guideline: #1766 (review) but will update to move versions
to the line below, no biggie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, there is a difference for commit messages (the ones you enter when creating new commits in git), which should be shorter than 80 characters and the code line width, which should be shorter than 100 characters. I believe Matt was referring to the former in his comment, while I meant the latter. All just minor details/nits however :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you squash, please shorten your commit title to no longer than ~70 chars, and any commit text should be wrapped around the same.
lightning/src/ln/channelmanager.rs
Outdated
receiver_node_id = self.keys_manager.get_node_id(Recipient::PhantomNode) | ||
.expect("Failed to get node_id for phantom node recipient"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: excess whitespace on this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my point was that this line isn't actually blank, its got lots of whitespace (spaces/tabs) in it, ideally keep the line but make it blank.
lightning/src/ln/channelmanager.rs
Outdated
e.insert((purpose.clone(), vec![claimable_htlc])); | ||
let mut receiver_node_id = Some(self.our_network_pubkey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to entirely remove this hunk of code - the receiver_node_id
calculated in the above hunk is still accessible here and can be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errr, I meant the entire if phantom_shared_secret.is_some()
and everything hunk - we don't need to have any of that we already did it.
69dbad1
to
dd1e69c
Compare
I think this just needs #1766 (comment) addressed and it should be good to go! |
dd1e69c
to
9b3aa62
Compare
9b3aa62
to
babde3a
Compare
Hopefully we were looking for 'our_node_id' and not the counterparty, but please correct me if that assumption is wrong. Also, right now the node_id is wrapped in an Option just to make the default easier, but can look into making it so it is not wrapped. Thank you for any review.