Skip to content

Limit full_stack fuzz runtime by limiting block connection ops #3742

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
Apr 17, 2025

Conversation

TheBlueMatt
Copy link
Collaborator

The full_stack_target fuzzer is generally pretty slow, but on some inputs the fuzzer is able to connect hundreds of thousands to millions of blocks, which is by far the slowest operation. In some cases, individual inputs can take tens of minutes to complete, which is not a realistic issue in LDK (we don't expect there to be million-block bursts in Bitcoin) so we simply disable it.

The `full_stack_target` fuzzer is generally pretty slow, but on
some inputs the fuzzer is able to connect hundreds of thousands to
millions of blocks, which is by far the slowest operation. In some
cases, individual inputs can take tens of minutes to complete,
which is not a realistic issue in LDK (we don't expect there to be
million-block bursts in Bitcoin) so we simply disable it.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 16, 2025

I've assigned @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz April 16, 2025 19:38
@@ -299,6 +299,14 @@ impl<'a> MoneyLossDetector<'a> {
}

fn connect_block(&mut self, all_txn: &[Transaction]) {
if self.blocks_connected > 50_000 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Connecting blocks looks like it's interspersed with other fuzz input commands. So now we'll just disallow connecting any blocks after 50k total have been connected? It seems like what we want is more like "allow connecting up to X blocks in a row, then skip connecting any more until we get a fuzz input that does something else"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but if the fuzzer is doing something interesting it should be able to do it with fewer blocks, so just blindly skipping block connection calls when they get to an absolutely absurd level doesn't seem likely to materially reduce fuzz coverage and is also much simpler :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it just seems like a lot of operations will be limited if no blocks can be connected, particularly channel opens/closes. How can you tell fuzz coverage wasn't reduced?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean if something has connected 50k blocks then I'm kinda okay with the rest of that fuzz input getting ignored/not fully processed. The vast majority of inputs shouldn't be connecting that many blocks.

@TheBlueMatt TheBlueMatt merged commit 6241f2a into lightningdevkit:main Apr 17, 2025
27 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.

4 participants