Abstract `ChannelManager` outbound payment logic by valentinewallace · Pull Request #1923 · lightningdevkit/rust-lightning

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace · 2022-12-18T18:56:55Z

Moves ChannelManager outbound payment logic into its own module, in preparation for payment retries.

Just code moves mostly. I kept the serialization changes surgical but we could probably move more logic there too.

codecov-commenter · 2022-12-18T19:19:11Z

Codecov Report

Base: 90.73% // Head: 91.46% // Increases project coverage by +0.73% 🎉

Coverage data is based on head (77e2014) compared to base (56afbf5).
Patch coverage: 90.22% of modified lines in pull request are covered.

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

Additional details and impacted files

@@ Coverage Diff @@## main #1923 +/- ##
==========================================
+ Coverage 90.73% 91.46% +0.73% 
==========================================
Files 94 96 +2 Lines 49603 53540 +3937 Branches 49603 53540 +3937 ==========================================
+ Hits 45006 48969 +3963 + Misses 4597 4571 -26

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`95.00% <ø> (ø)`
lightning/src/ln/outbound_payment.rs	`89.18% <89.18%> (ø)`
lightning/src/ln/channelmanager.rs	`86.68% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/offers/refund.rs	`93.73% <0.00%> (ø)`
lightning/src/ln/monitor_tests.rs	`99.84% <0.00%> (+0.27%)`	⬆️
lightning/src/offers/offer.rs	`93.91% <0.00%> (+0.46%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.30% <0.00%> (+0.55%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.38% <0.00%> (+0.83%)`	⬆️
... and 5 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Nice!

dunxen

The moves and renames LGTM!

We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

And re-export it in channelmanager.rs so it can remain public

valentinewallace · 2022-12-19T20:34:20Z

Lmk when I can squash, @jkczyz

jkczyz

Feel free to squash

This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Separating out this commit to keep the main refactor move-only

Once ChannelManager supports payment retries, it will make more sense for its current send_payment method to be named send_payment_with_route because retrying should be the default. Here we get a head start on this by making the rename in outbound_payment, but not changing the public interface yet.

dunxen

The commits were pretty easy to follow! Cleans up ChannelManager a load.

TheBlueMatt · 2022-12-20T19:39:32Z

+	#[cfg(test)]
+	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
+ let best_block_height = self.best_block.read().unwrap().height();
+ self.pending_outbound_payments.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, &self.keys_manager, best_block_height)


nit: should the exposure of the real add_new_pending_payment be via a test-only wrapper so that non-test channelmanager code can't call it?

Yeah, it's definitely safer. Good for a follow-up.

TheBlueMatt · 2022-12-20T19:46:30Z

+ if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
+ if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+ log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+ return


Heh, is it really worth breaking the move-only-ness to drop a ; :). Not worth fixing, really.

Oops, that mistakenly introduced when parameterizing the method by pending_events. I'll fix if Jeff has feedback that needs to go in this PR.

TheBlueMatt · 2022-12-20T19:47:57Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


This indentation no longer makes any sense - it was left completely unindented since its a cfg (in the standard C ifdef style), but we could also put it indented like the line itself (is this standard?). It doesn't really make sense randomly indented once.

I've personally seen more code where it has the same indentation as the line itself (and not ifdef style). Don't think there's a standard though.

Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:31:35Z

+ log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
+
+ let path_failure = {
+	#[cfg(test)]


Likwise, I've mostly noticed them indented.

jkczyz · 2022-12-20T20:38:17Z


-	fn send_payment_internal<K: Deref, F>
-(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>,
+	fn send_payment_internal<K: Deref, F>(


nit: I'd also move the closing ) to the next line here and all the following methods as you did in the methods before this (ec55730).

valentinewallace · 2022-12-20T20:41:00Z

Will address the latest feedback in a new PR shortly!

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch from ada18d3 to a62a50aCompare December 18, 2022 19:06

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch from a62a50a to b32532cCompare December 19, 2022 15:15

This was referenced Dec 19, 2022
Parameterize ChannelManager by a Router#1812
Merged
ChannelManager Payment Retries #1916
Merged

TheBlueMatt reviewed Dec 19, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt added the Seeking Code Review label Dec 19, 2022

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch from b32532c to 99126eeCompare December 19, 2022 17:06

dunxen reviewed Dec 19, 2022
View reviewed changes

Comment threadlightning/src/ln/mod.rs Outdated

jkczyz self-requested a review December 19, 2022 19:04

valentinewallace added 2 commits December 19, 2022 14:09

Move PendingOutboundPayment to new outbound_payment module
0708326
We want to move all outbound payment-related things to this new module, to help break up ChannelManager so future payment retries work doesn't increase the size of ChannelManager.

Move PaymentSendFailure into outbound_payment module
278ebd2
And re-export it in channelmanager.rs so it can remain public

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch from 99126ee to 8e49b02Compare December 19, 2022 19:10

TheBlueMatt removed the Seeking Code Review label Dec 19, 2022

dunxen previously approved these changes Dec 19, 2022
View reviewed changes

jkczyz reviewed Dec 19, 2022
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/outbound_payment.rs Outdated

valentinewallace dismissed dunxen’s stale review via 77e2014December 19, 2022 20:19

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch 2 times, most recently from 77e2014 to d108befCompare December 19, 2022 20:30

jkczyz reviewed Dec 20, 2022
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated

valentinewallace added 5 commits December 19, 2022 21:04

Swap pending_outbound_payments for OutboundPayments struct
afd3150
This allows us to move a lot of outbound payment logic out of ChannelManager and into the new outbound_payment module, and helps avoid growing ChannelManager when we add retry logic to it in upcoming work.

Remove unnecessary mut in finalize_claims
e69a4c2

Start parameters on a newline if they don't fit
ec55730
Separating out this commit to keep the main refactor move-only

Reduce visibility of outbound payment methods
3e89cc7

valentinewallace force-pushed the 2022-12-outbound-payment-mod branch from d108bef to afdaa64Compare December 20, 2022 02:08

valentinewallace requested review from TheBlueMatt, dunxen and jkczyz December 20, 2022 18:20

dunxen approved these changes Dec 20, 2022
View reviewed changes

TheBlueMatt approved these changes Dec 20, 2022
View reviewed changes

jkczyz approved these changes Dec 20, 2022
View reviewed changes

valentinewallace merged commit e64ebe8 into lightningdevkit:mainDec 20, 2022

This was referenced Dec 20, 2022
Fix main build #1928
Merged
Outbound payment module follow-up #1929
Merged

jkczyz mentioned this pull request Dec 21, 2022
Expose pending payments through ChannelManager#1873
Merged