Relock channel_state in for each HTLC in claim_funds and lay the groundwork for async event generation
#1886
Conversation
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
b5f4bfa to
0938c50
Compare
|
Rebased. |
lightning/src/ln/channelmanager.rs
Outdated
| /// as an [`events::Event::PaymentClaimed`]. | ||
| /// | ||
| /// See `ChannelManager` struct-level documentation for lock order requirements. | ||
| pending_claimed_payments: Mutex<HashMap<PaymentHash, PendingClaimingPayment>>, |
There was a problem hiding this comment.
This needs to be added to the lock_order tree, after pending_events. As this is also held when claimable_htlcs is held, claimable_htlcs should be changed to be a separate level in "the main branch" of the tree. Given that we don't want to take the opportunity to change how we illustrate the lock orders :).
Also: Would we like to add docs to pending_claimed_payments that the claimable_htlcs lock must also be held when inserting elements into pending_claimed_payments, as well as docs to claimable_htlcs that states that when inserting elements it's required to check that pending_claimed_payments doesn't include the payment hash, while also holding the claimable_htlcs lock? As that's what's protecting us from the possibility to get duplicate pending claim events IIUC.
There was a problem hiding this comment.
I wonder if we should just drop the lock order comment - the lock order tests should catch basically any lock inversions as long as the code is reached in tests.
There was a problem hiding this comment.
Hmmm I see your point, though IMO it still makes sense to provide some type of documentation of the lock orders, if anything as it's clearer and easier for new contributors. Though I'm increasingly more convinced that it's better to just have an ordered list as illustration of the lock orders, with an arbitrary order for the locks that do not currently have a connected lock order, instead of having different lock order branches in the illustration.
There was a problem hiding this comment.
Well, we no longer have to figure it out here - this PR now has no new lock, just a rename.
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
0938c50 to
88d931a
Compare
lightning/src/ln/channelmanager.rs
Outdated
| /// [`events::Event::PaymentClaimed`] to the user iff no such event has already been surfaced. | ||
| PaymentClaimed { payment_hash: PaymentHash }, | ||
| /// Indicates an [`events::Event`] should be surfaced to the user. | ||
| SurfaceEvent { event: events::Event }, |
There was a problem hiding this comment.
Two comments here:
- I'm not quite sure why we have a specific
PaymentClaimedvariant and a generic variant that could just as well be anevent::Event::PaymentClaimed? As the variant does a bit more than just issue the event, maybe it could at least be renamedClaimPayment? - We use at least two other expressions for the same action (i.e., give the user an event): the channel close events are issued and the channel ready events emitted (my bad). I think we should not add a third variant ("surface"), but clean the terminology up since all of them mean the same thing. +1 for using 'issue' everywhere I think.
There was a problem hiding this comment.
- It is neither just surfacing an event, nor claiming the payment? It is basically just surfacing an event but also de-duplicating it. The doc comment explains this a big, but I'm happy to clarify it if you have a suggested clearer text? It currently says "iff no such event has already been surfaced".
- Hmm, let's stick with "emit" for now and we can rename the "issued" things to emit later - emit is much more standard terminology I think.
There was a problem hiding this comment.
So regarding:
- No, I just wasn't sure whether this is kind of redundant, even though they do different things. Still think that it would make it a bit clearer to not name the action after the event but follow the
VerbNounstructure ofSurfaceEvent. - I'm also fine with emit, as long as we end up with the same terminology for the same thing eventually.
There was a problem hiding this comment.
- Sure, do you have a suggestion for a clearer name?
ClaimPaymentisn't accurate as we aren't claiming the payment as our action.ClaimedPaymentdoesn't really capture it either because the thing that happened is we claimed the payment, but that's not the thing that we're doing, which is whatEmitEventis.
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
2 times, most recently
from
016b83f to
d1c60f8
Compare
lightning/src/ln/channelmanager.rs
Outdated
| @@ -424,6 +424,18 @@ pub(super) enum RAACommitmentOrder { | |||
| RevokeAndACKFirst, | |||
| } | |||
|
|
|||
| /// Information about a payment which is currently being claimed. | |||
| struct PendingClaimingPayment { | |||
There was a problem hiding this comment.
Eek, what a name. What about PendingClaimedPayment or MidClaimPayment? ClaimAwaitingUpdate?
There was a problem hiding this comment.
Went with just ClaimingPayment. It is a payment that is in a claiming state, so that makes sense, I think? MidClaimPayment doesn't seem any cleaner, ClaimAwaitingUpdate seems to imply we're waiting on an update to claim, and PendingClaimedPayment implies we've claimed.
There was a problem hiding this comment.
By that logic, I think we should rename the pending_claimed_htlcs map as well. Although I'm a bit confused because it seems like we have sent the update_fulfill, which to me means we've "claimed"
I guess the definition of "claim" is actually "sent the fulfill + commitment signed dance and persisted the preimage"? It might be helpful to spell that out in the map docs
There was a problem hiding this comment.
No, once we send the update_fulfill we immediately remove it from the pending-claiming set. The point of the set is that if we're doing async monitor updates we may be waiting on the monitor update to complete before we're allowed to send the update_fulfill and thus remove it from the pending-claiming set
| amount_msat: u64, | ||
| payment_purpose: events::PaymentPurpose, | ||
| receiver_node_id: PublicKey, | ||
| } |
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
d1c60f8 to
afb6011
Compare
|
Addressed a handful of the comments and had to rebase, mostly didnt squash but some intermediate commits had to be changed. |
lightning/src/ln/channelmanager.rs
Outdated
| /// as an [`events::Event::PaymentClaimed`]. | ||
| /// | ||
| /// See `ChannelManager` struct-level documentation for lock order requirements. | ||
| pending_claimed_payments: Mutex<HashMap<PaymentHash, PendingClaimingPayment>>, |
There was a problem hiding this comment.
Hmmm I see your point, though IMO it still makes sense to provide some type of documentation of the lock orders, if anything as it's clearer and easier for new contributors. Though I'm increasingly more convinced that it's better to just have an ordered list as illustration of the lock orders, with an arbitrary order for the locks that do not currently have a connected lock order, instead of having different lock order branches in the illustration.
|
I'd be fine with a squash |
| @@ -4389,10 +4427,36 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | |||
| if drop { | |||
| chan.remove_entry(); | |||
| } | |||
| return ClaimFundsFromHop::MonitorUpdateFail(counterparty_node_id, res, None); | |||
| self.handle_monitor_update_completion_actions(completion_action(None)); | |||
There was a problem hiding this comment.
Tests pass with this line commented out
There was a problem hiding this comment.
Ugh, yea, we sadly don't have many tests that hit the perm failure cases. I'll add a second TODO here cause its really the same case as the one further down and we'll want to address them at the same time.
There was a problem hiding this comment.
Looks like we can also hit this if the channel's signer failed to sign the new commitment, but I guess we'll just claim onchain and it's fine to generate the claimed event?
There was a problem hiding this comment.
Won't be an error after #1879 :)
| // We got a temporary failure updating monitor, but will claim the | ||
| // HTLC when the monitor updating is restored (or on chain). | ||
| log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err); | ||
| } else { errs.push((pk, err)); } |
There was a problem hiding this comment.
Pre-existing and I know PermFails are kind of a no man's land, but FWIW
- all tests pass with this line commented out
- as of this PR we'll generate
PaymentClaimedeven if all channel updates permfail whereas previously we wouldn't.
There was a problem hiding this comment.
Let's leave it for now, neither answer is really correct and we can figure out whether we want it or not as a part of #1897, or, really, followups there - if we get a preimage and need to claim but get a perm failure we really need to just store the update and keep retrying it until morale improves.
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
afb6011 to
b408d90
Compare
|
Squashed some and added some further fixups. Had to reorder the commits to get lockorder checks to pass at each commit. |
Codecov ReportBase: 90.56% // Head: 91.05% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1886 +/- ##
==========================================
+ Coverage 90.56% 91.05% +0.48%
==========================================
Files 91 91
Lines 48556 54505 +5949
Branches 48556 54505 +5949
==========================================
+ Hits 43974 49628 +5654
- Misses 4582 4877 +295
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -4389,10 +4427,36 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | |||
| if drop { | |||
| chan.remove_entry(); | |||
| } | |||
| return ClaimFundsFromHop::MonitorUpdateFail(counterparty_node_id, res, None); | |||
| self.handle_monitor_update_completion_actions(completion_action(None)); | |||
There was a problem hiding this comment.
Looks like we can also hit this if the channel's signer failed to sign the new commitment, but I guess we'll just claim onchain and it's fine to generate the claimed event?
| // with a preimage we *must* somehow manage to propagate it to the upstream | ||
| // channel, or we must have an ability to receive the same event and try | ||
| // again on restart. | ||
| log_error!(self.logger, "Critical error: failed to update channel monitor with preimage {:?}: {:?}", |
There was a problem hiding this comment.
I think we should only say "critical error" on permfail since this'll just be a normal one for always-async monitor persisters
There was a problem hiding this comment.
Uhh, yea, true. I mean its a critical error in the code of LDK too :). But, let's just fix the issue, this isn't new verbiage, just moved code.
There was a problem hiding this comment.
I mean its a critical error in the code of LDK
I'm good with keeping it as-is, but for my understanding, this isn't a critical error in LDK if it tempfails with an always-async monitor persister, right?
There was a problem hiding this comment.
Well, its a critical error if we crash before the async persist completes :)
There was a problem hiding this comment.
Ahh, thanks that makes sense
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
b408d90 to
07fcc29
Compare
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
2 times, most recently
from
4db25af to
ac815b1
Compare
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
ac815b1 to
5d9cd8b
Compare
|
Squashed with a few minor changes - $ git diff-tree -U2 07fcc29 5d9cd8bda
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index d5f497139..7b99a83fe 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -823,7 +823,4 @@ pub struct ChannelManager<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
/// [`ClaimablePayments`]' individual field docs for more info.
///
- /// When adding to the map, [`Self::pending_claimed_payments`] must also be checked (under the
- /// same lock) to ensure we don't get a duplicate payment.
- ///
/// See `ChannelManager` struct-level documentation for lock order requirements.
claimable_payments: Mutex<ClaimablePayments>,
@@ -4331,4 +4328,5 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
}
if sources.is_empty() || expected_amt_msat.is_none() {
+ mem::drop(channel_state);
self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash);
log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!");
@@ -4336,4 +4334,5 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
}
if claimable_amt_msat != expected_amt_msat.unwrap() {
+ mem::drop(channel_state);
self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash);
log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",
|
In the next commits we'll move to generating `PaymentClaimed` events while handling `ChannelMonitorUpdate`s rather than directly in line. Thus, as a prerequisite, here we move to storing the info required to generate the `PaymentClaimed` event in a separate map. Note that while this does introduce a new map which is written as an even value which users cannot opt out of, the map is only filled in when users use the asynchronous `ChannelMonitor` updates and after a future PR. As these are still considered beta, breaking downgrades for such users is considered acceptable in the future PR (which will likely be one LDK version later).
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
5d9cd8b to
c3fc669
Compare
|
Oops, missed the comment from #1886 (comment), renamed the set as well. $ git diff-tree -U0 5d9cd8bda c3fc66992
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 7b99a83fe..498179f38 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -439 +439 @@ struct ClaimablePayments {
- /// When adding to the map, [`Self::pending_claimed_payments`] must also be checked to ensure
+ /// When adding to the map, [`Self::pending_claiming_payments`] must also be checked to ensure
@@ -446 +446 @@ struct ClaimablePayments {
- pending_claimed_payments: HashMap<PaymentHash, ClaimingPayment>,
+ pending_claiming_payments: HashMap<PaymentHash, ClaimingPayment>,
@@ -1632 +1632 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs: HashMap::new(), pending_claimed_payments: HashMap::new() }),
+ claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs: HashMap::new(), pending_claiming_payments: HashMap::new() }),
@@ -3516 +3516 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- if claimable_payments.pending_claimed_payments.contains_key(&payment_hash) {
+ if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) {
@@ -3593 +3593 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- if claimable_payments.pending_claimed_payments.contains_key(&payment_hash) {
+ if claimable_payments.pending_claiming_payments.contains_key(&payment_hash) {
@@ -4261 +4261 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- let dup_purpose = claimable_payments.pending_claimed_payments.insert(payment_hash,
+ let dup_purpose = claimable_payments.pending_claiming_payments.insert(payment_hash,
@@ -4331 +4331 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash);
+ self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
@@ -4337 +4337 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash);
+ self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
@@ -4367 +4367 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash);
+ self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
@@ -4581 +4581 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
- self.claimable_payments.lock().unwrap().pending_claimed_payments.remove(&payment_hash)
+ self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash)
@@ -7303,2 +7303,2 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelMana
- let mut pending_claimed_payments = Some(&claimable_payments.pending_claimed_payments);
- if pending_claimed_payments.as_ref().unwrap().is_empty() {
+ let mut pending_claiming_payments = Some(&claimable_payments.pending_claiming_payments);
+ if pending_claiming_payments.as_ref().unwrap().is_empty() {
@@ -7307 +7307 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelMana
- pending_claimed_payments = None;
+ pending_claiming_payments = None;
@@ -7309 +7309 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelMana
- debug_assert!(false, "While we have code to serialize pending_claimed_payments, the map should always be empty until a later PR");
+ debug_assert!(false, "While we have code to serialize pending_claiming_payments, the map should always be empty until a later PR");
@@ -7316 +7316 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelMana
- (4, pending_claimed_payments, option),
+ (4, pending_claiming_payments, option),
@@ -7643 +7643 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
- let mut pending_claimed_payments = Some(HashMap::new());
+ let mut pending_claiming_payments = Some(HashMap::new());
@@ -7648 +7648 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
- (4, pending_claimed_payments, option),
+ (4, pending_claiming_payments, option),
@@ -7907 +7907 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
- claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs, pending_claimed_payments: pending_claimed_payments.unwrap() }),
+ claimable_payments: Mutex::new(ClaimablePayments { claimable_htlcs, pending_claiming_payments: pending_claiming_payments.unwrap() }),
|
This adds a new enum, `MonitorUpdateCompletionAction` and a method to execute the "actions". They are intended to be done once a (potentially-async) `ChannelMonitorUpdate` persistence completes, however this behavior will be implemented in a future PR. For now, this adds the relevant infrastructure which will allow us to prepare `claim_funds` for better monitor async handling.
Currently `claim_funds` does all HTLC claims in one `channel_state` lock, ensuring that we always make claims from channels which are open. It can thus avoid ever having to generate a `ChannelMonitorUpdate` containing a preimage for a closed channel, which we only do in `claim_funds_internal` (for forwarded payments). In the next commit we'll change the locking of `claim_funds_from_hop` so that `claim_funds` is no longer under a single lock but takes a lock for each claim. This allows us to be more flexible with locks going forward, and ultimately isn't a huge change - if our counterparty intends to force-close a channel, us choosing to ignore it by holding the `channel_state` lock for the duration of the claim isn't going to result in a commitment update, it will just result in the preimage already being in the `ChannelMonitor`.
When `claim_funds` has to claim multiple HTLCs as a part of a single MPP payment, it currently does so holding the `channel_state` lock for the entire duration of the claim loop. Here we swap that for taking the lock once for each HTLC. This allows us to be more flexible with locks going forward, and ultimately isn't a huge change - if our counterparty intends to force-close a channel, us choosing to ignore it by holding the `channel_state` lock for the duration of the claim isn't going to result in a commitment update, it will just result in the preimage already being in the `ChannelMonitor`.
Currently `claim_funds` and `claim_funds_internal` call `claim_funds_from_hop` and then surface and `Event` to the user informing them of the forwarded/claimed payment based on it's result. In both places we assume that a claim "completed" even if a monitor update is being done async. Instead, here we push that event generation through a `MonitorUpdateCompletionAction` and a call to `handle_monitor_update_completion_action`. This will allow us to hold the event(s) until async monitor updates complete in the future.
TheBlueMatt
force-pushed
the
2022-11-claim-relock
branch
from
c3fc669 to
616d3ac
Compare
|
Removed one more lockorder: $ git diff-tree -U1 c3fc6699 616d3ac7
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 498179f38..0d09ead3a 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4579,5 +4579,4 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
MonitorUpdateCompletionAction::PaymentClaimed { payment_hash } => {
- if let Some(ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id }) =
- self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash)
- {
+ let payment = self.claimable_payments.lock().unwrap().pending_claiming_payments.remove(&payment_hash);
+ if let Some(ClaimingPayment { amount_msat, payment_purpose: purpose, receiver_node_id }) = payment {
self.pending_events.lock().unwrap().push(events::Event::PaymentClaimed {
|
|
Looks good to me. Still not completely comfortable about my understanding of the full effects of this though, so prefer to not hit the approve button and instead wait for @tnull unless you really need to merge this. |
In order to lay the groundwork for adding event barrier support and more robust async monitor updates, this adds some structs that allow us to track events to generate after a monitor update completes. Its separated out to keep the next PR(s) smaller, but also because it changes the locking of
claim_fundswhich may be useful in #1507, see individual commits for more details.