Skip to content

Move channel_reestablish messages to the simple message fuzzer #2266

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

Conversation

TheBlueMatt
Copy link
Collaborator

In 16d0f2f the
ChannelReestablish messages was converted to the impl_writeable_msg serialization macro which handles a TLV stream suffix. In the case of our fuzzers, this implies we need to use the test_msg_simple checker rather than the test_msg one.

test_msg checks that any bytes which were read must be present when we write the message back out, which is useful for gossip processing, however if we don't store ignored TLVs we cannot possibly meet the requirements.

Instead, test_msg_simple only ensures that, if we serialized the message we can round-trip it exactly, i.e. at least the fields we understand round-trip.

In 16d0f2f the
`ChannelReestablish` messages was converted to the
`impl_writeable_msg` serialization macro which handles a TLV stream
suffix. In the case of our fuzzers, this implies we need to use the
`test_msg_simple` checker rather than the `test_msg` one.

`test_msg` checks that any bytes which were read must be present
when we write the message back out, which is useful for gossip
processing, however if we don't store ignored TLVs we cannot
possibly meet the requirements.

Instead, `test_msg_simple` only ensures that, if we serialized the
message we can round-trip it exactly, i.e. at least the fields we
understand round-trip.
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW @dunxen fixed this in #1794.

@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented May 4, 2023

Oh, right, I knew I'd seen this patch somewhere lol. I'm fine to leave it for that if we land that tomorrow, its just broken on main and CI is randomly failing.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (e94647c) 91.50% compared to head (bce9cf2) 91.48%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2266      +/-   ##
==========================================
- Coverage   91.50%   91.48%   -0.02%     
==========================================
  Files         104      104              
  Lines       52087    52087              
  Branches    52087    52087              
==========================================
- Hits        47660    47652       -8     
- Misses       4427     4435       +8     

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator Author

Eh, lets just land #1794 tomorrow.

@TheBlueMatt TheBlueMatt closed this May 5, 2023
@dunxen
Copy link
Contributor

dunxen commented May 5, 2023

Ugh, sorry about this. Should have copied it over. But I've addressed feedback in #1794 so hopefully good to go.

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.

4 participants