Implement local channel tracking by Router at channel_update - #207
Conversation
|
A lot of shorts locks picking, maybe need some more care. |
|
I don't think calling into the Router from ChannelManager is the right approach here - better to keep this data in the Channel up-to-date and expose it via list_channels().
…On October 12, 2018 2:08:37 AM UTC, ariard ***@***.***> wrote:
A lot of shorts locks picking, maybe need some more care.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#207 (comment)
|
|
Heyy I rebased this stuff and changed some to avoid useless lock but sorry don't understand your last comment because how PeerHandler is supposed to filter our local channels to Router without accessing ChannelManager channels ? Need handle_local_channel on ChannelMessageHandler to get them no ? |
|
My point was that we shouldn't even need to modify the Router DB for this - we should keep the fee knowledge in the Channel object itself and then the user can use list_usable_channels()'s return value as a get_route() argument and we can use the info in get_route that way. Would need a little bit of augmentation of the information provided in ChannelDetails to include feerate information, but I think we'd have less layer violations that way (Router shouldn't need to know about what channels are ours). |
|
Ugh, oops, indeed, not sure if I misspoke or just thought different when I wrote that but it seems like a nontrivial layer violation (and one that would make it harder to stub in a custom router) to do it that way?
… On Nov 16, 2018, at 22:06, ariard ***@***.***> wrote:
Hmm so #149 is about using feerates from ChannelDetails to improve get_route ? Sorry last comment doesn't align with your first comment in #149 "I think the obvious solution is to track local channels in Router"..
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Rebased and updated with IRC comments (11-21-2018) |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Nice! This looks good modulo a few small things to fix.
|
|
||
| /// Set channel fields with remote peer forwarding parameters, used mainly by ChannelDetails to feed the Router | ||
| pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) { | ||
| self.their_to_self_delay = msg.contents.cltv_expiry_delta; |
There was a problem hiding this comment.
to_self_delay is the time before their commitment transaction loses the CSV delay, CLTV_EXPIRY_DELTA is the required difference between an incoming HTLC and the corresponding outgoing HTLC, they're very different values.
There was a problem hiding this comment.
urgh, ugly from my part, corrected, was thinking to_self_delay was CLTV_EXPIRY_DELTA while writing this by distraction, even if I'm pretty sure I used it correctly as CSV in others times
| /// Set channel fields with remote peer forwarding parameters, used mainly by ChannelDetails to feed the Router | ||
| pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) { | ||
| self.their_to_self_delay = msg.contents.cltv_expiry_delta; | ||
| self.their_htlc_minimum_msat = msg.contents.htlc_minimum_msat; |
There was a problem hiding this comment.
This should be sanity-checked against funding_satoshis (see new_from_req), and while ideally we'd sanity-check against the channel_limits we dont keep those around and I think its probably OK to let counterparties update such things if they need to temporarily disable a channel. This needs to be documented in Config, though, noting that a counterparty can increase this value once the channel is established beyond the minimum, but this is equivalent to them just rejecting all HTLCs so we can't enforce it anyway.
There was a problem hiding this comment.
Okay, added comments in Config and sanity-check against channel_value_satoshis (funding_satoshis) and if fail reject the whole update, but should we send back a ChannelError:Close here, or as you said interpret it as temporary disabling of channel?
There was a problem hiding this comment.
I think if they send back something completely bogus, nothing wrong with a ChannelError::Close.
There was a problem hiding this comment.
Updated, but don't do comparison for others fields because I think we don't have reference bases
| } | ||
|
|
||
| fn handle_channel_update(&self, msg: &msgs::ChannelUpdate) -> Result<(), HandleError> { | ||
| let mut channel_state_lock = self.channel_state.lock().unwrap(); |
There was a problem hiding this comment.
While technically I think its safe without it, probably best to keep consistency and add a total_consistency_lock.read().unwrap() at the top.
| assert_eq!(spend_txn.len(), 1); | ||
| check_spends!(spend_txn[0], closing_tx); | ||
| } | ||
|
|
| let channel_state = channel_state_lock.borrow_parts(); | ||
| if let Some(channel_id) = channel_state.short_to_id.get(&msg.contents.short_channel_id) { | ||
| if let Some(chan) = channel_state.by_id.get_mut(channel_id) { | ||
| chan.channel_update(&msg); |
There was a problem hiding this comment.
Need to check the source of the message ie their_node_id as in all the other handler functions.
There was a problem hiding this comment.
So refactored function with an internal part to catch easily MsgHandleErrInternal
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Oops, sorry, guess I didnt give this one a good shake, why are you tracking the HTLC minimum and fee info? See comment on get_route but the fees a node takes are always calculated on the basis of the announced fees for a given outbound channel. ie the channels from us dont matter for fee calculation, only the HTLC minimum needs to be respected.
|
Also this needs the corresponding updates in get_route so that we do enforce that requirement. |
|
Was thinking we needed to track our local channel to do cycle payment to ourselves, because BOLT 7 say "SHOULD accept channel_updates for its own channels (even if non-public), in order to learn the associated origin nodes' forwarding parameters". If we have topology A <--> B <--> C, what does forwarding parameters mean here ? Policy apply by B on link B -> C, or on link A <- B, or on link A -> B as relaying node ? Because if it means the last option, HTLC minimum_msat on A -> B is useless as it gonna be also enforced on B -> C (and seems to me that what really interested us is the lowest HTLC_minimum_msat of the whole payment route) |
|
Hmmmmmm, now I'm really confused. That requirement in the spec may be bogus. At a high level, if A is routing via B to C, the only fees taken are by B and they are the fees B announces on the B->C channel (the rationale being its up to A/the source of an HTLC to figure out what they want to charge to forward along a route, ie you care about the action of forwarding, not accepting). That said, the htlc_minimum_msat value is defined as "the minimum HTLC value (in millisatoshi) that the channel peer will accept" ie B should be announcing the value that we sent it in our channel_open/channel_accept, and I dont see a way to change this value at runtime, so maybe the "SHOULD accept channel_updates for its own channels (even if non-public), in order to learn the associated origin nodes' forwarding parameters." requirement is just bogus? Probably worth asking some other folks and investigating what others do and fixing the spec to make it much clearer in any case. In any case we definitely need include our counterparty's current htlc_minimum_msat in route calculations (and need some tests for route calculations with htlc_minimums). |
|
The spec isn't wrong here, but its kinda a layer violation implied - I believe its really about learning the inbound fees so that you can include them in route hints in invoices. There's no other reason we'd need that data, but, indeed, route hints are important. |
|
Superseded by #841 :) |
3 participants
Add test_channel_update_local_channel
Issue #149