-
Notifications
You must be signed in to change notification settings - Fork 421
Release 0.1.6 #4155
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
Release 0.1.6 #4155
Conversation
We plan to rework this method in the following commit, so we start by formatting it first. Backport of 2fbaf96 Conflicts resolved in: * lightning/src/chain/channelmonitor.rs
Previously, we'd attempt to claim all HTLCs that have expired or that we have the preimage for on each preimage monitor update. This happened due to reusing the code path (`get_counterparty_output_claim_info`) used when producing all claims for a newly confirmed counterparty commitment. Unfortunately, this can result in invalid claim transactions and ultimately in loss of funds (if the HTLC expires and the counterparty claims it via the timeout), as it didn't consider that some of those HTLCs may have already been claimed by a separate transaction. This commit changes the behavior when handling preimage monitor updates only. We will now only attempt to claim HTLCs for the specific preimage that we learned via the monitor update. This is safe to do, as even if a preimage HTLC claim transaction is reorged out, the `OnchainTxHandler` is responsible for continuous claiming attempts until we see a reorg of the corresponding commitment transaction. Backport of 46f0d31 Conflicts resolved in: * lightning/src/chain/channelmonitor.rs Silent conflicts resolved in: * lightning/src/ln/monitor_tests.rs
👋 Thanks for assigning @valentinewallace as a reviewer! |
0b96831
to
6528aa7
Compare
This reverts commit 3968d5e. Not sure how this slipped in but it breaks semver (a new field in a public struct) for a field that isn't used, so should be dropped.
6528aa7
to
c5d06a6
Compare
Oops, missed a word: $ git diff-tree -U1 6528aa7c31 c5d06a622f
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 595927d6a9..66d88ad9dc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,3 +22,3 @@
in the same MPP claim complete, and the node restarts twice, the preimage may
- be lost and the MPP payment part may not claimed (#3928).
+ be lost and the MPP payment part may not be claimed (#3928).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.1 #4155 +/- ##
==========================================
+ Coverage 87.48% 87.50% +0.01%
==========================================
Files 149 149
Lines 101711 101834 +123
Branches 101711 101834 +123
==========================================
+ Hits 88985 89106 +121
Misses 10466 10466
- Partials 2260 2262 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/// Information about an HTLC that is part of a payment that can be claimed. | ||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
pub struct ClaimedHTLC { | ||
/// The counterparty of the channel. |
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 commit seems like it should go earlier in the PR, probably doesn't matter though
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 mean the commit shouldn't have been backported at all, but oh well :)
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.
Yeah, that was an oversight :( It's possible we should've broken up that backports PR, it was huge and there were some nontrivial backports.
I re-scanned and noticed the indexed_map
's public API changed as well in that PR, though I doubt anyone uses that directly, I think that would also qualify as a semver violation?
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.
Its mostly a CI failure. We have a semver check CI job now, and it did fail here (which is why I noticed it). We should have checked it in the previous PR (and maybe should mark it required on the 0.1/0.2 branches).
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.
Reviewed backport range-diff, reproduced git revert
, proofread release notes and checked PR numbers.
Backport of #4154 and release notes/version bump