Skip to content

fuzz-tests: Add a test for calculate_our_funding() #8312

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fuzz-tests: Add a test for calculate_our_funding() #8312

wants to merge 2 commits into from

Conversation

Chand-ra
Copy link

@Chand-ra Chand-ra commented Jun 2, 2025 •
edited
Loading

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@Chand-ra
Copy link
Author

Chand-ra commented Jun 2, 2025

Hey @morehouse ,

this test fails with the error:

our_funds 0 < per_channel_min 18446744073709551615

but the function under test, plugins/funder_policy.c::calculate_our_funding(), has a check that's supposed to guard against this error:

if (amount_sat_less(*our_funding, policy->per_channel_min)) {
	*our_funding = AMOUNT_SAT(0);
	return error;
...
...
...
}

What could be the cause? I tried inspecting the breakage using gdb but the values upon inspection seem to vary from the error output. I've added the breaking input as the corpus, I'd appreciate your thoughts on this.

@morehouse
Copy link
Contributor

When it fails, is our_funds always 0?

The check is skipped when the returned amount is 0.

lightning/plugins/funder_policy.c

Lines 297 to 299 in ad2dc97

/* Don't return an 'error' if we're already at 0 */
if (amount_sat_is_zero(*our_funding))
return NULL;

Chandra Pratap added 2 commits June 11, 2025 05:27
fuzz-tests: Add a test for calculate_our_funding()
ee1076a 
Changelog-None: `calculate_our_funding()` in `plugins/funder_policy.c`
is responsible for calculating our funding policy. Add a test for it.
fuzz-tests: Add a seed corpus for the new test
776eea0 
Add a minimal input set as a seed corpus for the newly introduced
test. This leads to discovery of interesting code paths faster.
@Chand-ra Chand-ra marked this pull request as ready for review June 11, 2025 05:31
@Chand-ra
Copy link
Author

This test is now ready to review.

morehouse
morehouse reviewed Jun 12, 2025
View reviewed changes
tests/fuzz/fuzz-funder-policy.c
{ fprintf(stderr, "json_add_string called!\n"); abort(); }
/* AUTOGENERATED MOCKS END */

#define MAX_BTC ((u32)WALLY_SATOSHI_PER_BTC * WALLY_BTC_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this multiplication overflows u32.

Copy link
Author

@Chand-ra Chand-ra Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I copied this over from tests/fuzz/fuzz-close_tx.c:

57     max = AMOUNT_SAT((u32)WALLY_SATOSHI_PER_BTC * WALLY_BTC_MAX);

Should I fix it there as well? Possibly by making this small change:

(u64)WALLY_SATOSHI_PER_BTC

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please create a separate PR for that fix and then run the fuzz target to make sure it still works.

tests/fuzz/fuzz-funder-policy.c
static struct amount_sat fromwire_amount_sat_bounded(const u8 **cursor, size_t *max)
{
struct amount_sat amt = fromwire_amount_sat(cursor, max);
amt.satoshis %= (MAX_BTC + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: whitespace

Suggested change
amt.satoshis %= (MAX_BTC + 1);
amt.satoshis %= (MAX_BTC + 1);

tests/fuzz/fuzz-funder-policy.c

void run(const u8 *data, size_t size)
{
if (size < 85)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since data is only ever read using fromwire* functions, we probably don't need to check size at all. If we run out of data, fromwire should automatically return 0 values.

tests/fuzz/fuzz-funder-policy.c
struct amount_sat channel_size;
struct amount_sat lease_request;
struct funder_policy policy;
struct amount_sat exp_our_funds;
Copy link
Contributor

Choose a reason for hiding this comment

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

exp_our_funds is unused.

tests/fuzz/fuzz-funder-policy.c
struct amount_sat their_funds;
struct amount_sat available_funds;
struct amount_sat *our_last_funds;
struct amount_sat channel_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the max channel size allowed.

Suggested change
struct amount_sat channel_size;
struct amount_sat max_channel_size;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morehouse morehouse morehouse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Chand-ra @morehouse