-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable decoding HTLC onions when fully committed #2933
base: main
Are you sure you want to change the base?
Enable decoding HTLC onions when fully committed #2933
Conversation
b66e094
to
4210c01
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2933 +/- ##
========================================
Coverage 89.66% 89.66%
========================================
Files 126 126
Lines 102411 102739 +328
Branches 102411 102739 +328
========================================
+ Hits 91826 92126 +300
- Misses 7857 7879 +22
- Partials 2728 2734 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4210c01
to
7c6cff3
Compare
Tagging 0.0.122 not because we need to land this for 0.0.122, but because we have to at least be roughly confident in it working and the tests in it being sufficient. |
7c6cff3
to
11c18ec
Compare
As discussed offline, we'll want to land a |
Okay, sounds like @valentinewallace took a glance at this, and I did too. Nothing too wild here, tests pass, we can do this in the next release (or whenever), but should cfg-flag the changes and land it sooner so that we don't have this branch get toooo stale. |
11c18ec
to
574554b
Compare
Before I go down the cfg flag path, does it make sense to merge this now as is if the project is doing branched releases going forward? |
Yea, I don't see why not? Just cause we're branching off for future releases doesn't mean we can't/shouldn't have future code behind cfg flags on the working tip. |
Chatted with @wpaulino today. His quiescence branch is based on this PR and IIUC requires it. He agreed to rebase it soon. @TheBlueMatt @wpaulino Do we still want a |
574554b
to
bcdf60e
Compare
I think we're probably okay without now. @valentinewallace checked today and said support was added in 0.0.123, which means if we ship this we'll support downgrading two versions of LDK but not more, which is pretty normal for us, I think. |
@wpaulino Needs another rebase. |
This argument will be asserted on in a future commit to ensure we obtain the intended `HTLCHandlingFailed::failed_next_destination` per HTLC failure.
bcdf60e
to
0562830
Compare
MIN_SERIALIZATION_VERSION | ||
}; | ||
write_ver_prefix!(writer, version_to_write, MIN_SERIALIZATION_VERSION); | ||
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be bumping the MIN_SERIALIZATION_VERSION
here (and should have done so in the old code, but it didn't matter then)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't recall some of the context here. It does seem safe to bump it now given that we're not supporting downgrades past 0.0.122. I think it just wasn't bumped in the parent PR because we were still using the named constant to write the current version.
@@ -6409,7 +6402,7 @@ impl<SP: Deref> Channel<SP> where | |||
}; | |||
let exposure_dust_limit_timeout_sats = htlc_timeout_dust_limit + self.context.counterparty_dust_limit_satoshis; | |||
if msg.amount_msat / 1000 < exposure_dust_limit_timeout_sats { | |||
let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat + msg.amount_msat; | |||
let on_counterparty_tx_dust_htlc_exposure_msat = htlc_stats.on_counterparty_tx_dust_exposure_msat; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you justify all the changes to dust calculation here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The incoming HTLC is now included in the pending stats. This wasn't the case before because we'd previously evaluate the incoming add upon receiving it, while now we wait until it's fully committed by both sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update comments/git commit message so that its clearer?
if intro_fails { | ||
let mut updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); | ||
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]); | ||
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false); | ||
let failed_destination = match check { | ||
ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion, | ||
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be ::InvalidOnion
...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so too, but the error we get from processing doesn't come out as malformed from construct_pending_htlc_status
.
We get a PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC))
with the reason:
Got blinded non final data with an HMAC of 0
I believe this has always been the case and is just being surfaced by this PR now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of the intro node returning update_fail
rather than malformed
there aligns with the route blinding spec; intro nodes always error in the same way regardless of the "real" error. So it seems the issue is that real error isn't being bubbled up to be accurately included in the event, only the blinded one. That said, this case seems quite rare so doesn't seem worth addressing.
This commit ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain `HTLCHandlingFailed` events for _any_ failed HTLC that comes across a channel. We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported. Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.
0562830
to
183fd66
Compare
// side, only on the sender's. Note that with anchor outputs we are no longer as | ||
// sensitive to fee spikes, so we need to account for them. | ||
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); | ||
let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(())); | ||
let mut remote_fee_cost_incl_stuck_buffer_msat = self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment 2 lines up needs updating. Also, it looks like we'll always pass in None
to this method for the fee spike buffer now, may be able to remove that parameter (I'm still trying to figure out why this patch affects whether we include a fee spike buffer, though...)
Edit: no test coverage when I revert this diff btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still trying to figure out why this patch affects whether we include a fee spike buffer
This is related to https://github.com/lightningdevkit/rust-lightning/pull/2933/files#r1769645521.
Before this PR, we'd evaluate can_accept_incoming_htlc
prior to the incoming HTLC being committed, so all stats computations would not include it. Now, it's evaluated after it has been accepted.
We still check within update_add_htlc
that we can accept it (to return an actual error/force close), but the check here is concerned with whether we can accept another HTLC (to prevent stuck channels). The HTLCCandidate
argument to next_remote_commit_tx_fee_msat
now replaces the use of the fee_spike_buffer_htlc
being Some
in this context.
Thanks for pointing this out though, because I think the amount of this HTLCCandidate
should now be updated to be the smallest non-dust HTLC allowed for the channel? cc @TheBlueMatt
no test coverage when I revert this diff btw
With some of the context above, I assume that's because none of our tests care for whether we can accept more than one HTLC after accepting one, they only care for exactly one more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out though, because I think the amount of this HTLCCandidate should now be updated to be the smallest non-dust HTLC allowed for the channel? cc @TheBlueMatt
Right, the API for next_remote_commit_tx_fee_msat
probably needs to change - the second argument is now always None
but here we really want to pass no HTLC for the first argument but Some
for the second. Maybe we just make the HTLCCandidate
amount an Option
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But indeed we should update the comment here too.
This PR ensures all new incoming HTLCs going forward will have their onion decoded when they become fully committed to decide how we should proceed with each one. As a result, we'll obtain
HTLCHandlingFailed
events for any failed HTLC that comes across a channel.We will now start writing channels with the new serialization version (4), and we will still be able to downgrade back to the commit that introduced it since reading version 4 is supported.
Note that existing pending inbound HTLCs may already have their resolution if they were received in a previous version of LDK. We must support those until we no longer allow downgrading beyond this commit.