Skip to content

Drop broken debug assertion on peer buffer lengths #3399

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

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

TheBlueMatt
Copy link
Collaborator

1b711ed changed it so that we can enqueue broadcast gossip messages to peers even though their buffer is full as long as its our ChannelMessageHandler doing it. This broke a debug assertion that the buffer isn't too large when appending the broadcast message which is simply dropped here.

Found by the full_stack_target fuzzer.

1b711ed changed it so that we can
enqueue broadcast gossip messages to peers even though their buffer
is full as long as its our `ChannelMessageHandler` doing it. This
broke a debug assertion that the buffer isn't too large when
appending the broadcast message which is simply dropped here.

Found by the `full_stack_target` fuzzer.
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Makes sense.

Somehow orthogonal, I wonder if we could/should start checking-in failed test cases to have the fuzzer check for regressions?

@TheBlueMatt
Copy link
Collaborator Author

Yea, I should probably do that...I keep not getting around to it because every time I get it done there's some new bug in the fuzz debug assertions cause the fuzzer finds yet another part of the code that gets sad if the blockchain has an invalid set of transactions (in which case we should panic) but where it panics cause the fuzzer can do invalid chains.

@TheBlueMatt TheBlueMatt merged commit 6bf2181 into lightningdevkit:main Nov 7, 2024
13 of 18 checks passed
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.

2 participants