Mind commit tx fee while assembling a route #2080
Conversation
Otherwise the resulting path might overshoot and thus fail at payment sending time.
|
I'm actually not sure how the #1126 OP issue is possible since The only way #1126 OP could happen is when the reserve gets changed after we construct a route, but before we send an htlc? |
There was a problem hiding this comment.
Thanks for working on this! Indeed, I don't believe we have an issue directly with the reserve anymore, but if we overrun the reserve by paying more in commitment tx fees the error we'll see is still the "we can't send more than the counterparty's reserve" error, even though the accounting issue was the fees rather than reserve.
lightning/src/ln/channel.rs
Outdated
| let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); | ||
| let htlc_dust = HTLCCandidate::new(real_dust_limit_success_sat - 1, HTLCInitiator::RemoteOffered); | ||
| let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); | ||
| let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; |
There was a problem hiding this comment.
Hmm, if I add an assert!(one_htlc_difference_msat == 0); it seems to always be true.
lightning/src/ln/channel.rs
Outdated
| let min_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_dust, None); | ||
| let one_htlc_difference_msat = max_reserved_commit_tx_fee_msat - min_reserved_commit_tx_fee_msat; | ||
|
|
||
| outbound_capacity_msat -= max_reserved_commit_tx_fee_msat; |
There was a problem hiding this comment.
I don't think we want to as aggressively change outbound_capacity_msat - the way I read the docs its really "what's the maximum amount you could send, possibly across multiple HTLCs, and in the future, over this channel". Thus, we don't need to consider the full current commitment tx fee, but rather maybe only one htlc's worth of commitment tx fee.
next_outbound_htlc_limit_msat is differentl, there we should be as accurate about the next htlc as possible.
| @@ -1609,6 +1609,19 @@ macro_rules! get_route_and_payment_hash { | |||
| }} | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| #[macro_export] | |||
| macro_rules! get_route_or_error { | |||
There was a problem hiding this comment.
nit: no reason this shouldn't be a function rather than a macro.
lightning/src/ln/channel.rs
Outdated
| real_dust_limit_success_sat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false) / 1000; | ||
| } | ||
|
|
||
| let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_sat, HTLCInitiator::RemoteOffered); |
There was a problem hiding this comment.
Can we add some comments or maybe DRY this up with some of the existing tx fee construction logic? Its kinda hard to read what's going on here.
There was a problem hiding this comment.
Yeah it would be nice to re-use some of the send_htlc logic. Ultimately, I think it'd be useful to refactor Channel such that we have one method that can provide any type of balance we may need across LDK.
It's very unclear to me how it worked in the first place
Instead, only change next_outbound_htlc_limit_msat
| // This complicates the computation around dust-values, up to the one-htlc-value. | ||
| let mut real_dust_limit_success_msat = self.holder_dust_limit_satoshis * 1000; | ||
| if !self.opt_anchors() { | ||
| real_dust_limit_success_msat += self.feerate_per_kw as u64 * htlc_success_tx_weight(false); |
There was a problem hiding this comment.
If it's an outbound HTLC, why are we relying on the success dust limit? The timeout dust limit should apply to us, and the success dust limit to the counterparty.
| } | ||
|
|
||
| let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_msat, HTLCInitiator::RemoteOffered); | ||
| let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); |
There was a problem hiding this comment.
We may also want to set fee_spike_buffer_htlc to Some since we can't actually send out an HTLC that would prevent us from sending another one later on.
| AvailableBalances { | ||
| inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000 | ||
| - self.value_to_self_msat as i64 | ||
| - self.get_inbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64 | ||
| - self.holder_selected_channel_reserve_satoshis as i64 * 1000, | ||
| 0) as u64, | ||
| outbound_capacity_msat, | ||
| next_outbound_htlc_limit_msat: cmp::max(cmp::min(outbound_capacity_msat as i64, | ||
| next_outbound_htlc_limit_msat: cmp::max(cmp::min(available_capacity_msat as i64, |
There was a problem hiding this comment.
Pre-existing, but this doesn't take into account the max number of HTLCs we can actually send out. In that case this should be 0.
|
|
||
| let htlc_above_dust = HTLCCandidate::new(real_dust_limit_success_msat, HTLCInitiator::RemoteOffered); | ||
| let max_reserved_commit_tx_fee_msat = FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE * self.next_local_commit_tx_fee_msat(htlc_above_dust, None); | ||
| let htlc_dust = HTLCCandidate::new(real_dust_limit_success_msat - 1000, HTLCInitiator::RemoteOffered); |
There was a problem hiding this comment.
Can be - 1 since we always round down?
|
Any update on this @naumenkogs? Are you still working on it? |
|
I won't be able to work on this for some time (at least another month), sorry for the distraction. Feel free to pick it. |
|
That's alright, not sure I'm gonna get around to it either. Thanks |
|
Superseded by #2312 with the main commit copied there (with some bugfixes). |
Otherwise, the resulting path might overshoot and thus fail at payment sending time.
Partially closes #1126. It doesn't address the OP issue, but it addresses the issues in the comments.