Skip to content
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

Fix future unknown Event variant backwards compatibility #1087

Conversation

TheBlueMatt
Copy link
Collaborator

In 8ffc2d1, in 0.0.100, we added
a backwards compatibility feature to the reading of Events - if
the type was unknown and odd, we'd simply ignore the event and
treat it as no event. However, we failed to read the
length-prefixed TLV stream when doing so, resulting in us reading
some of the skipped-event data as the next event or other data in
the ChannelManager.

We fix this by reading the varint length prefix written, then
skipping that many bytes when we come across an unknown odd event
type.

We really need a good test framework for reading new or old data with old or new channelmanagers/etc, but for 0.0.101 it doesn't make sense to try to build one.

@TheBlueMatt TheBlueMatt added this to the 0.0.101 milestone Sep 21, 2021
@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #1087 (2cf42aa) into main (2cf42aa) will not change coverage.
The diff coverage is n/a.

❗ Current head 2cf42aa differs from pull request most recent head 71e0173. Consider uploading reports for the commit 71e0173 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1087   +/-   ##
=======================================
  Coverage   90.72%   90.72%           
=======================================
  Files          65       65           
  Lines       34212    34212           
=======================================
  Hits        31039    31039           
  Misses       3173     3173           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf42aa...71e0173. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator Author

Will squash + land after #997.

In 8ffc2d1, in 0.0.100, we added
a backwards compatibility feature to the reading of `Event`s - if
the type was unknown and odd, we'd simply ignore the event and
treat it as no event. However, we failed to read the
length-prefixed TLV stream when doing so, resulting in us reading
some of the skipped-event data as the next event or other data in
the ChannelManager.

We fix this by reading the varint length prefix written, then
skipping that many bytes when we come across an unknown odd event
type.
@TheBlueMatt TheBlueMatt force-pushed the 2021-09-event-backwards-compat-fix branch from 94875fa to 71e0173 Compare September 21, 2021 20:39
@TheBlueMatt
Copy link
Collaborator Author

Squashed and rebased with no other changes.

grammar fixup commit was:

94875fade6a1215ccda3bcc5633e30ab9405f1d2
--- a/lightning/src/util/events.rs
+++ b/lightning/src/util/events.rs
@@ -392,3 +392,3 @@ impl MaybeReadable for Event {
                                // odd-type unknown, we should treat it as `Ok(None)` even if it has some TLV
-                               // fields which are event. Thus, we avoid using `read_tlv_fields` and simply read
+                               // fields that are even. Thus, we avoid using `read_tlv_fields` and simply read
                                // exactly the number of bytes specified, ignoring them entirely.

Range-diff is git range-diff 801d6e5256d6ac91d5d5668da1fa5a2b55303246..2ec1991cef8d023d581b0aeca739fd11bbf276c2 2cf42aa3888e665541faf34a634f054b4580d25f..71e0173d8db17116a1a17517b40512be8c05af50

@TheBlueMatt TheBlueMatt merged commit 5262e77 into lightningdevkit:main Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants