Correct Channel outbound HTLC serialization and expand fuzzing coverage#892
Merged
TheBlueMatt merged 4 commits intolightningdevkit:mainfrom May 31, 2021
Merged
Conversation
caaba21 to
055f0b3
Compare
Codecov Report
@@ Coverage Diff @@
## main #892 +/- ##
==========================================
+ Coverage 90.42% 92.36% +1.93%
==========================================
Files 59 61 +2
Lines 30173 41340 +11167
==========================================
+ Hits 27285 38184 +10899
- Misses 2888 3156 +268
Continue to review full report at Codecov.
|
055f0b3 to
50e9525
Compare
Collaborator
Author
|
Rebased after #851 merge. |
50e9525 to
46697b9
Compare
jkczyz
reviewed
May 27, 2021
46697b9 to
1c818cd
Compare
Collaborator
Author
|
Added some additional documentation. |
jkczyz
approved these changes
May 27, 2021
valentinewallace
approved these changes
May 31, 2021
While trying to debug the issue ultimately tracked down to a `PeerHandler` locking bug in lightningdevkit#891, the ability to deliver only individual messages at a time in chanmon_consistency looked important. Specifically, it initially appeared there may be a race when an update_add_htlc was delivered, then a node sent a payment, and only after that, the corresponding commitment-signed was delivered. This commit adds such an ability, greatly expanding the potential for chanmon_consistency to identify channel state machine bugs.
Channel serialization should happen "as if remove_uncommitted_htlcs_and_mark_paused had just been called". This is true for the most part, but outbound RemoteRemoved HTLCs were being serialized as normal, even though `remote_uncommitted_htlcs_and_mark_paused` resets them to `Committed`. This led to a bug identified by the `chanmon_consistency_target` fuzzer wherein, if we receive a update_*_htlc message bug not the corresponding commitment_signed prior to a serialization roundtrip, we'd force-close the channel due to the peer "attempting to fail/claim an HTLC which was already failed/claimed".
1c818cd to
25dbd0d
Compare
Collaborator
Author
|
Squashed fixups with no changes. Will merge after CI: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is based on #851 as it is required to get chanmon_consistency_target to run clean anyway.
This fixes a few trivial bugs in full_stack_target, then expands the coverage of chanmon_consistency_target significantly, fixing one bug that that expansion found (so far).