Improve Robustness of Inbound MPP Claims Across Restart by TheBlueMatt · Pull Request #1434 · lightningdevkit/rust-lightning

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt · 2022-04-19T22:16:32Z

This rewrites our claim pipeline to be substantially more robust against particularly poorly timed crashes, including the potentially partial-MPP-claim-causing bug described in Ensure all HTLCs for a claimed payment are claimed on startup!

It also adds some belt-and-suspenders checks when claiming and removes some API bits that I find somewhat confusing and likely hard to use (without replacement, so hopefully users can live without it, otherwise we'll have to revert the last commit and do some really careful documentation).

valentinewallace · 2022-04-20T02:52:12Z

Wanted to throw out the idea of breaking this PR into the first 3 commits and the last 4 (or so)? I think people have mentioned preferring smaller PRs when feasible

TheBlueMatt · 2022-04-20T14:47:04Z

Pulled the first two commits into #1435, the rest are all really just "update claim_funds/fail_htlc and then some tests".

TheBlueMatt · 2022-04-20T21:25:30Z

Remaining issue: (a) claim payment, (b) crash, (c) restart see PaymentReceived + PaymentClaimed events still in processing queue but payment has been claimed (on-chain), (d) sender sends again (e) call claim_funds in response to the dup PaymentRecived claiming the payment duplicatively. Not a big deal but should fix it by dropping the PaymentReceived event from the event queue on startup if we do a re-claim.

valentinewallace · 2022-04-21T19:26:31Z

+ }
 match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
 ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
 if let msgs::ErrorAction::IgnoreError = err.err.action {


If we get a permanent failure here (but claimed_any_htlcs is true), will the permfailed monitor be able to claim the htlc on-chain? Just concerned it won't have learned the preimage in this case

Yea, I mean there's no good answer here (and this PR doesn't change that). TBH I think we need to rethink the whole perm-failure logic a bit. That's kinda the intent behind #1106, but ultimately the "what does perm-failure mean" topic isn't super well-defined currently. Lets leave this for a followup because there's nothing changed here, but, does perm-failure mean "we updated the in-memory copy and nothing else" or "we failed to update anything" needs addressing.

valentinewallace · 2022-04-22T15:34:45Z

+ if let Some(channel) = by_id.get_mut(&previous_channel_id) {
+ channel.claim_htlc_while_disconnected_dropping_mon_update(claimable_htlc.prev_hop.htlc_id, payment_preimage, &args.logger);
+ }
+ if let Some(previous_hop_monitor) = args.channel_monitors.get(&claimable_htlc.prev_hop.outpoint) {


Not worth checking that this is the same monitor as from L6781, right?

My line numbers aren't matching up, but I dont think there's any harm in providing extra preimages for monitors in this super-rare case. They'll get pruned anyway, but even if not it wouldn't matter.

valentinewallace · 2022-04-22T15:58:13Z

+
+ nodes[0].node.handle_update_fulfill_htlc(&nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
+ commitment_signed_dance!(nodes[0], nodes[2], cs_updates.commitment_signed, false, true);
+ expect_payment_sent!(nodes[0], payment_preimage);


Kinda throwing me off that nodes[3] is not claiming the payment for the persisted monitor backwards, only the crashed monitor. To check my understanding, the only difference would be that there would be no PaymentSent generated here, it'd only be generated for the first successful backwards claim, right?

It is, just on-chain. I didn't bother writing the on-chain eforcement checks cause they're a pain and we have lots of coverage there already.

valentinewallace

Went through each commit, largely looks good.

valentinewallace · 2022-04-22T16:31:20Z

+	///
+	/// # Note
+	/// LDK will not stop an inbound payment from being paid multiple times, so multiple
+	/// `PaymentReceived` events may be generated for the same payment.


s/PaymentReceived/PaymentClaimed iiuc

Yea, that's confusing, I did mean to leave it as-is but added a second sentence.

valentinewallace · 2022-04-25T19:08:36Z

+ for (_, monitor) in args.channel_monitors.iter() {
+ for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
+ if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
+ log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));


nit: Re-claiming
It took me a second to figure out how we know this is a re-claim, and definitely not a new claim. Is it because the monitor knows the preimage yet the htlc is still in the claimable_htlcs map, so the monitor must be ahead of the ChannelManager?

Basically, yea. Technically its possible its a duplicate payment and the previous payment was claimed but not fully revoked before restart, but its not really worth checking for that case IMO. I updated the log line a bit.

valentinewallace · 2022-04-26T01:03:28Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


Would the API be more ergonomic if we returned a Result<(), Error> from this method, for cases like this?

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.

I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

Yea, basically an error should only occur in a strange restart case of if a user took several blocks to try to claim a payment.

I feel like that sends the wrong impression - an Ok(()) does not imply that the payment is fully claimed, which is ultimately why the bool was removed from the return value.
Wouldn't we want to return the Err variant in this case? User attempts to claim a payment that is no longer claimable.

I guess I feel like that's confusing - you call claim_funds and get an Err, a user would reasonably assume that the payment was not claimed/is still pending, but in fact it could have been claimed!

TheBlueMatt · 2022-04-28T02:46:57Z

Rebased after merge of #1435.

valentinewallace

ACK after CI and another reviewer

valentinewallace · 2022-04-28T16:14:16Z

+ return;
+ }
+ if claimable_amt_msat != expected_amt_msat.unwrap() {
+ log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",


I guess we expect these errors to be super rare? They do seem useful. Returning an Option<Error> is weird though

TheBlueMatt · 2022-05-04T18:31:18Z

Ended up having to rewrite this to some extent as we're moving towards removing the FinalOnionHopData from OnionPayload::Invoice which this was relying on. This now starts with two opening commits which extend that migration and adds another field tracking in the claimable HTLCs. See the commit messages on those for more info.

codecov-commenter · 2022-05-05T19:24:35Z

Codecov Report

Merging #1434 (531d6c8) into main (36817e0) will increase coverage by 1.19%.
The diff coverage is 88.71%.

@@ Coverage Diff @@## main #1434 +/- ##
==========================================
+ Coverage 90.90% 92.10% +1.19% 
==========================================
Files 77 80 +3 Lines 42201 53330 +11129 Branches 42201 53330 +11129 ==========================================
+ Hits 38364 49120 +10756 - Misses 3837 4210 +373

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`41.22% <26.47%> (+7.32%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.92% <75.16%> (+3.25%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`97.14% <96.66%> (+1.59%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.30% <98.91%> (+1.18%)`	⬆️
lightning/src/chain/chainmonitor.rs	`97.96% <100.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.02% <100.00%> (+0.96%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.77% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`91.70% <100.00%> (+3.12%)`	⬆️
lightning/src/ln/monitor_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
... and 29 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 36817e0...531d6c8. Read the comment docs.

While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.

TheBlueMatt · 2022-05-26T00:53:37Z

Squashed without changes:

$ git diff-tree -U1 fb7f2b61 319df3ac
$

valentinewallace · 2022-05-27T17:15:24Z

 for (payment_hash, payment_preimage) in monitor.get_stored_preimages() {
 if let Some(claimable_htlcs) = claimable_htlcs.remove(&payment_hash) {
- log_info!(args.logger, "Re-claimaing HTLCs with payment hash {} due to partial-claim.", log_bytes!(payment_hash.0));
+ log_info!(args.logger, "Re-claiming HTLCs with payment hash {} as we've released the preimage to a ChannelMonitor!", log_bytes!(payment_hash.0));


nit: this is in the wrong commit but wtv

This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

If we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.

As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.

`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.

TheBlueMatt · 2022-05-28T00:03:18Z

Squashed, only change since yesterday was taking @jkczyz's suggested wording from #1434 (comment)

TheBlueMatt · 2022-05-28T18:50:57Z

Oops lol, duh, force-pushed the last commit to actually change the references to amt not just the definitions.

TheBlueMatt added this to the 0.0.107 milestone Apr 19, 2022

TheBlueMatt added the Seeking Code Review label Apr 19, 2022

TheBlueMatt mentioned this pull request Apr 20, 2022
Separate ChannelDetails' outbound capacity from the next HTLC max #1435
Merged

TheBlueMatt added blocked on dependent pr and removed Seeking Code Review labels Apr 20, 2022

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch 2 times, most recently from 721c415 to 49fe7c9Compare April 20, 2022 19:33

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 49fe7c9 to 4e24f02Compare April 21, 2022 20:54

valentinewallace reviewed Apr 22, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 4e24f02 to 3c8c013Compare April 22, 2022 18:06

valentinewallace reviewed Apr 22, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 3c8c013 to 6c6d939Compare April 25, 2022 16:41

valentinewallace reviewed Apr 26, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch 2 times, most recently from b8c99f5 to bf9ec6dCompare April 28, 2022 02:46

TheBlueMatt added Seeking Code Review and removed blocked on dependent pr labels Apr 28, 2022

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from bf9ec6d to 763add0Compare April 28, 2022 03:53

valentinewallace reviewed Apr 28, 2022
View reviewed changes

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch 2 times, most recently from 43907a8 to 48e5842Compare May 4, 2022 18:28

valentinewallace reviewed May 5, 2022
View reviewed changes

Comment threadlightning/src/util/events.rs Outdated

valentinewallace reviewed May 5, 2022
View reviewed changes

Comment threadlightning/src/util/events.rs Outdated

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 48e5842 to b4cf258Compare May 5, 2022 19:24

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from f9515c4 to 7d4faf6Compare May 25, 2022 23:40

jkczyz reviewed May 25, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch 2 times, most recently from 0fdabf5 to fb7f2b6Compare May 26, 2022 00:27

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from fb7f2b6 to 319df3aCompare May 26, 2022 00:53

TheBlueMatt removed the Seeking Code Review label May 26, 2022

TheBlueMatt assigned jkczyz and valentinewallace May 26, 2022

valentinewallace previously approved these changes May 27, 2022
View reviewed changes

jkczyz reviewed May 27, 2022
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated

TheBlueMatt dismissed valentinewallace’s stale review via cc25213May 27, 2022 23:49

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 319df3a to cc25213Compare May 27, 2022 23:49

TheBlueMatt added 5 commits May 28, 2022 00:02

Add a PaymentClaimed event to indicate a payment was claimed
0a2a40c
This replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.

Add internal docs for ChannelMonitor::payment_preimages
8a56702

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from cc25213 to 8a56702Compare May 28, 2022 00:02

jkczyz reviewed May 28, 2022
View reviewed changes

Comment threadlightning/src/util/events.rs Outdated

valentinewallace previously approved these changes May 28, 2022
View reviewed changes

Change Eventamt fields to amount_msat for clarity
531d6c8

TheBlueMatt dismissed valentinewallace’s stale review via 531d6c8May 28, 2022 18:50

TheBlueMatt force-pushed the 2022-04-robust-payment-claims branch from 6bc4ad0 to 531d6c8Compare May 28, 2022 18:50

jkczyz approved these changes May 30, 2022
View reviewed changes

valentinewallace approved these changes May 30, 2022
View reviewed changes

valentinewallace merged commit a534a5e into lightningdevkit:mainMay 30, 2022