Handle network updates from failed payments in BackgroundProcessor #1043
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1043 +/- ##
==========================================
+ Coverage 90.83% 91.04% +0.21%
==========================================
Files 65 65
Lines 33281 33762 +481
==========================================
+ Hits 30232 30740 +508
+ Misses 3049 3022 -27
Continue to review full report at Codecov.
|
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
2 times, most recently
from
f5e9985 to
bec75d7
Compare
| } | ||
| } | ||
|
|
||
| self.inner.handle_event(event) |
There was a problem hiding this comment.
Hmmmmmm, have we considered maybe just allowing events to be handled by multiple handlers? Passing them off to an inner handler works, I suppose, but its kinda awkward. I guess I'm not a huge fan of passing the events by reference instead of value, but most of them have no heap allocation, and eg SpendableOutputs should be pretty rare (requires on-chain txn).
There was a problem hiding this comment.
The decorator approach here is nice because it allows us to short-circuit handling. This is needed for payment retries where the inner handler is only invoked after the final failure.
There was a problem hiding this comment.
Hmm, yea, good point. This isn't a trait change, though, so we could do different things for different handlers, right?
There was a problem hiding this comment.
Not quite sure I follow what you mean by do "different things for different handlers". Do you have a concrete example that may clarify?
There was a problem hiding this comment.
As in, there's nothing here that requires that we handle ignoring/passing down events the same for NetworkGraph as we do for PaymentRetryer, or whatever, is there?
There was a problem hiding this comment.
Do you also want to update to just implement
EventHandleron theNetworkGraph? Given the background processor already explicitly takes aNetworkGraph, it wouldn't complicate the background processor much at all (and maybe simplify it if we make the network graph there optional).
(see also other comment about removing NetworkGraph from BackgroundProcessor)
We'd have to parameterize NetworkGraph by a Logger if we did that. 😕 How about implementing EventHandler for NetGraphMsgHandler? I've been toying around with renaming that NetworkGossip, making the naming less specific about how and that it updates NetworkGraph.
If we're gonna go down the path of decorators eating and passing events onto inner handlers, I dunno if we need to bother passing events by reference.
Using references allows a bit more flexibility like post-handling event as mentioned above.
There was a problem hiding this comment.
We'd have to parameterize NetworkGraph by a Logger if we did that.
Eh, we basically already do that for everything, not a big deal IMO.
How about implementing EventHandler for NetGraphMsgHandler? I've been toying around with renaming that NetworkGossip, making the naming less specific about how and that it updates NetworkGraph.
No strong feelings, do what you want for both.
There was a problem hiding this comment.
Haven't pushed it yet, but I went with implementing it on NetGraphMsgHandler. It's also nice to not have to know about a decorated EventHandler when deserializing a NetworkGraph.
That said, just want to point out a drawback with either is that the guide for setting up a node will need to change. Now event handling (step 16) can no longer be discussed independently of initializing NetGraphMsgHandler (optional step 12).
cc: @valentinewallace Any opinions on this?
There was a problem hiding this comment.
Thanks for pointing that out. That is a slight drawback but not a deal breaker IMO
There was a problem hiding this comment.
I think that's ok. Indeed, ideally we'd have everything be nice and separate, but if we're gonna have something related to the network graph handle events, that's the outcome we're gonna have. It is nice that all the bindings have moved towards using the lightning-background-processor crate, though, so it'll all be common infrastructure in any case.
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
2 times, most recently
from
99c2337 to
de7cb1d
Compare
|
Implemented in the sample: valentinewallace/ldk-sample@ddc9414 |
Note that you don't actually need the clone - https://git.bitcoin.ninja/index.cgi?p=ldk-sample;a=commit;h=bc119d8fb23d8e55c91df002ca490062f9cc72a6 |
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
de7cb1d to
4e2f2a4
Compare
|
Updated the docs! There's a couple open issues with testing:
For the latter, I commented out the assertion in |
lightning/src/ln/functional_tests.rs
Outdated
| @@ -1007,23 +1007,17 @@ fn htlc_fail_async_shutdown() { | |||
| nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates_2.update_fail_htlcs[0]); | |||
| commitment_signed_dance!(nodes[0], nodes[1], updates_2.commitment_signed, false, true); | |||
|
|
|||
| expect_payment_failed!(nodes[0], our_payment_hash, false); | |||
| expect_payment_failed_with_update!(nodes[0], our_payment_hash, false, chan_1.0.contents.short_channel_id, false); | |||
There was a problem hiding this comment.
I'd argue the issue here is on the channel end - we fail with chan_disabled instead of permanent_channel_failure. I think we should swap the shutdown checks in Channel::update_add_htlc to instead use 0x4000|8 and then fix channelmanager to handle it (I haven't reviewed this, just hacked it up, so may need more thought):
@@ -3461,33 +3464,32 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
let create_pending_htlc_status = |chan: &Channel<Signer>, pending_forward_info: PendingHTLCStatus, error_code: u16| {
- // Ensure error_code has the UPDATE flag set, since by default we send a
- // channel update along as part of failing the HTLC.
- assert!((error_code & 0x1000) != 0);
// If the update_add is completely bogus, the call will Err and we will close,
// but if we've sent a shutdown and they haven't acknowledged it yet, we just
// want to reject the new HTLC and fail it backwards instead of forwarding.
match pending_forward_info {
PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
- let reason = if let Ok(upd) = self.get_channel_update_for_unicast(chan) {
- onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
- let mut res = Vec::with_capacity(8 + 128);
- // TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
- res.extend_from_slice(&byte_utils::be16_to_array(0));
- res.extend_from_slice(&upd.encode_with_len()[..]);
- res
- }[..])
+ let reason = if (error_code & 0x1000) != 0 {
+ if let Ok(upd) = self.get_channel_update_for_unicast(chan) {
+ onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
+ let mut res = Vec::with_capacity(8 + 128);
+ // TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
+ res.extend_from_slice(&byte_utils::be16_to_array(0));
+ res.extend_from_slice(&upd.encode_with_len()[..]);
+ res
+ }[..])
+ } else {
+ // The only case where we'd be unable to successfully get a
+ // channel update is if the channel isn't in the fully-funded
+ // state yet, implying our counterparty is trying to route
+ // payments over the channel back to themselves (cause no one
+ // else should know the short_id is a lightning channel yet).
+ // We should have no problem just calling this
+ // unknown_next_peer (0x4000|10).
+ onion_utils::build_first_hop_failure_packet(incoming_shared_secret, 0x4000|10, &[])
+ }
} else {
- // The only case where we'd be unable to
- // successfully get a channel update is if the
- // channel isn't in the fully-funded state yet,
- // implying our counterparty is trying to route
- // payments over the channel back to themselves
- // (cause no one else should know the short_id
- // is a lightning channel yet). We should have
- // no problem just calling this
- // unknown_next_peer (0x4000|10).
- onion_utils::build_first_hop_failure_packet(incoming_shared_secret, 0x4000|10, &[])
+ onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &[])
};
let msg = msgs::UpdateFailHTLC {
channel_id: msg.channel_id,
There was a problem hiding this comment.
Seems to work though I had to change which channel is used in the failing test otherwise the short channel id wouldn't match. Take a look at d5ea8e8. I'll properly backport the fixup if all looks well.
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
4e2f2a4 to
d5ea8e8
Compare
|
Needs rebase. |
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
d5ea8e8 to
9828dcc
Compare
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// # extern crate bitcoin; |
There was a problem hiding this comment.
Hides it from showing up in the documentation. Useful if you have an elaborate setup that may distract from the example. Could leave some of these lines in if you'd like. Need to fix the indentation anyhow.
| } | ||
| } | ||
|
|
||
| self.inner.handle_event(event) |
There was a problem hiding this comment.
Do you also want to update to just implement EventHandler on the NetworkGraph? Given the background processor already explicitly takes a NetworkGraph, it wouldn't complicate the background processor much at all (and maybe simplify it if we make the network graph there optional).
If we're gonna go down the path of decorators eating and passing events onto inner handlers, I dunno if we need to bother passing events by reference.
| > | ||
| (persister: CMP, event_handler: EH, chain_monitor: M, channel_manager: CM, peer_manager: PM, logger: L) -> Self | ||
| (persister: CMP, event_handler: EH, chain_monitor: M, channel_manager: CM, peer_manager: PM, network_graph: G, logger: L) -> Self |
There was a problem hiding this comment.
network_graph needs to be Optional.
There was a problem hiding this comment.
I think we may actually want to remove this entirely from BackgroundProcessor. With #1059, users will want to decorate InvoicePayer with NetworkUpdateHandler so that updates are applied prior to retrying. Given they'll want to use the InvoicePayer to make payments, we can't construct that here without returning it back to them somehow.
So instead we may want to leave it to the user to set up the event handler to pass to BackgroundProcessor. But we could perhaps provide a utility for decorating their event handler with standard decorators.
There was a problem hiding this comment.
Yea, I guess that makes sense - you only get those errors if you send a payment and it fails, so requiring a payment retryer in order to handle those errors isn't crazy.
There was a problem hiding this comment.
I removed the changes to BackgroundProcessor.
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
2 times, most recently
from
e0aff9c to
f9bee36
Compare
|
I removed the commit that made |
| let is_enabled = msg.contents.flags & (1 << 1) != (1 << 1); | ||
| let status = if is_enabled { "enabled" } else { "disabled" }; | ||
| log_debug!(self.logger, "Updating channel with channel_update from a payment failure. Channel {} is {}.", short_channel_id, status); | ||
| let _ = self.network_graph.update_channel(msg, &self.secp_ctx); |
There was a problem hiding this comment.
should there be a TODO to handle the error?
There was a problem hiding this comment.
Not sure. The code that produces them is reused in other places but likely isn't actionable here. Might be able to log one marked IgnoreAndLog.
|
Needs rebase. |
Added a short note about decorators in |
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
f9bee36 to
1997e75
Compare
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
1997e75 to
69fc1f9
Compare
Passing an Event by reference rather and by move gives more flexibility for composing event handlers without needing to clone events.
|
Moving this (and related stuff) to 0.0.102 because it looks like we may want to ship an 0.0.101 to get some fixes out. |
In preparation for giving NetworkGraph shared ownership, wrap individual fields in RwLock. This allows removing the outer RwLock used in NetGraphMsgHandler.
Now that NetworkGraph uses interior mutability, the RwLock used around it in NetGraphMsgHandler is no longer needed. This allows for shared ownership without a lock.
Hide the internal locking of NetworkGraph by providing a read-only view. This way the locking order is handled internally.
It doesn't seem to be testing anything useful that isn't covered elsewhere.
This affects the htlc_fail_async_shutdown test.
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
69fc1f9 to
4455d5f
Compare
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
2 times, most recently
from
1bda8c4 to
8362ead
Compare
|
Based on an offline conversation with @TheBlueMatt, I removed the decorated This approach is a bit cleaner since it simplifies |
|
ACK aside from the two comments above. |
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
8362ead to
0b452b7
Compare
| /// It will also call [`PeerManager::process_events`] periodically though this shouldn't be relied | ||
| /// upon as doing so may result in high latency. |
There was a problem hiding this comment.
Hmm, so I checked lightningdevkit.org guides, the sample, process_events docs and I don't think we indicate anywhere that the user should be calling this function perioically (i.e., users do currently rely on BackgroundProcessor to call process_events atm).
Should we update some other docs/the sample, or open an issue to do so?
There was a problem hiding this comment.
Its primarily called by lightning-net-tokio or by the NioPeerHandler in Java, so I don't think users need to care, at least not normal users.
There was a problem hiding this comment.
I did open an issue at #1075.
MessageSendEvent::PaymentFailureNetworkUpdate served as a hack to pass an HTLCFailChannelUpdate from ChannelManager to NetGraphMsgHandler via PeerManager. Instead, remove the event entirely and move the contained data (renamed NetworkUpdate) to Event::PaymentFailed to be processed by an event handler.
PaymentFailed events contain an optional NetworkUpdate describing changes to the NetworkGraph as conveyed by a node along a failed payment path according to BOLT 4. An EventHandler should apply the update to the graph so that future routing decisions can account for it. Implement EventHandler for NetGraphMsgHandler to update NetworkGraph. Previously, NetGraphMsgHandler::handle_htlc_fail_channel_update implemented this behavior.
Decorate the user-supplied EventHandler with NetGraphMsgHandler in the BackgroundProcessor. The resulting handler will intercept PaymentFailed events in order to update the NetworkGraph in the background before delegating to the user's event handler.
jkczyz
force-pushed
the
2021-07-network-update-handler
branch
from
0b452b7 to
992df51
Compare
MessageSendEvent::PaymentFailureNetworkUpdateserved as a hack to pass anHTLCFailChannelUpdatefromChannelManagertoNetGraphMsgHandlerviaPeerManager. Instead, remove the event entirely and move the containeddata (renamed
NetworkUpdate) intoEvent::PaymentFailedto be processed by an event handler.This requires making
NetGraphMsgHandleranEventHandler, which is used inBackgroundProcessorto decorate the user-providedEventHandlerwith the ability to update theNetworkGraphaccordingly.The general concept of intercepting events via a decorator will be used in a follow-up PR for scoring channels and retrying failed payments.