fuzz-tests: Add fuzz target for closing_complete by Chand-ra · Pull Request #8216 · ElementsProject/lightning

Chand-ra · 2025-04-06T15:13:15Z

closing_signed and closing_complete are channel closing negotiation messages defined in BOLT #2.

While closing_signed has a wire fuzz test, closing_complete does not. Add a test to perform a round-trip encoding check (towire -> fromwire) similar to the other wire fuzzers.

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.

vincenzopalazzo · 2025-04-09T09:13:19Z

Probably the best person to review this code is @morehouse

Chand-ra · 2025-04-09T12:35:05Z

Probably the best person to review this code is @morehouse

Yeah, I've had a conversation with him over mail. He said he'd get around to it soon.

morehouse · 2025-04-11T17:36:56Z

tests/fuzz/fuzz-wire-closing_complete.c

+	size_t upto_closer_scriptpubkey = (uintptr_t)&x->closer_scriptpubkey - (uintptr_t)x;
+	if (memcmp(x, y, upto_closer_scriptpubkey) != 0)
+		return false;


I'm concerned that some architectures may pad/align struct members to 8 bytes, which means there would be 4 bytes of uninitialized padding between locktime and fee_satoshis that could trigger false positives.

If we want to use the memcmp trick, we should probably move locktime to after fee_satoshis and then manually compare fields starting with locktime and thereafter.

The memcmp dance takes as much lines as individually comparing struct closing_complete's members, so I've replaced the former with the latter in the latest push.

morehouse · 2025-04-11T17:52:20Z

Also would be good to add a minimal input set as a seed corpus.

morehouse

Did you use the run.py script to minimize the corpus?

Also, I think this should probably be a Changelog-None.

morehouse · 2025-04-15T20:02:18Z

tests/fuzz/fuzz-wire-closing_complete.c

+	assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)));
+	assert(x->locktime == y->locktime);
+	assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)));


Nit: for better readability can we use assert(memcmp(...) == 0)? I've seen bugs arise because people thought !memcmp(...) meant the buffers differed.

Suggested change

assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)));

assert(x->locktime == y->locktime);

assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)));

assert(memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)) == 0);

assert(x->locktime == y->locktime);

assert(memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)) == 0);

morehouse · 2025-04-15T20:05:01Z

tests/fuzz/fuzz-wire-closing_complete.c

+	assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)));
+	assert(x->locktime == y->locktime);
+	assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)));


Nit: common trick to avoid the memcmp breaking if the struct type changes in the future:

Suggested change

assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(struct channel_id)));

assert(x->locktime == y->locktime);

assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(struct amount_sat)));

assert(!memcmp(&x->channel_id, &y->channel_id, sizeof(x->channel_id)));

assert(x->locktime == y->locktime);

assert(!memcmp(&x->fee_satoshis, &y->fee_satoshis, sizeof(x->fee_satoshis)));

Chand-ra · 2025-04-16T11:42:19Z

Did you use the run.py script to minimize the corpus?

I did. Doesn't seem to do a lot though. Am I using it right:

tests/fuzz/run.py tests/fuzz/corpora

Also, I think this should probably be a Changelog-None.

Right.

morehouse · 2025-04-21T18:27:26Z

I did. Doesn't seem to do a lot though. Am I using it right:

tests/fuzz/run.py tests/fuzz/corpora

No, you should be making sure that tests/fuzz/corpora/fuzz-wire-closing_complete is empty and then use --merge_dir:

./tests/fuzz/run.py tests/fuzz/corpora --merge_dir your_closing_complete_corpus

See documentation here.

And please also make sure any other PRs you have open with new corpus inputs are properly minimized this way (e.g., #8183).

Chand-ra · 2025-04-22T07:02:13Z

No, you should be making sure that tests/fuzz/corpora/fuzz-wire-closing_complete is empty and then use --merge_dir:
./tests/fuzz/run.py tests/fuzz/corpora --merge_dir your_closing_complete_corpus
See documentation here.

And please also make sure any other PRs you have open with new corpus inputs are properly minimized this way (e.g., #8183).

Done.

morehouse · 2025-04-22T12:35:47Z

tests/fuzz/fuzz-wire-closing_complete.c

+	assert(tal_arr_eq(x->closee_scriptpubkey, y->closee_scriptpubkey));
+
+	assert(x->tlvs && y->tlvs);
+	return tal_arr_eq(x->tlvs->closer_and_closee_outputs, y->tlvs->closer_and_closee_outputs);


Looks like we should also be comparing the other TLV fields:

closer_output_only closee_output_only

The closer_and_closee_outputs field is actually an amalgamation of the closer_output and closee_output fields. Is there any merit to comparing both these fields separately as well?

Pretty sure that's not true. They're all distinct signatures. https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#rationale-21

Oh okay, I seem to have misinterpreted a comment in wire/peer_wiregen.c:

struct tlv_closing_tlvs { struct tlv_field *fields; /* The following explicit fields could just point into the * tlv_field entries above to save memory. */ secp256k1_ecdsa_signature *closer_output_only; secp256k1_ecdsa_signature *closee_output_only; secp256k1_ecdsa_signature *closer_and_closee_outputs; };

Changelog-None: 'closing_signed' and 'closing_complete' are channel closing negotiation messages defined in BOLT ElementsProject#2. While 'closing_signed' has a wire fuzz test, 'closing_complete' does not. Add a test to perform a round-trip encoding check (towire -> fromwire) similar to the other wire fuzzers.

Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.

morehouse

ACK 2a2e9cd

morehouse suggested changes Apr 11, 2025

View reviewed changes

Chand-ra force-pushed the complete branch from a165dd4 to 3835530 Compare April 12, 2025 07:32

morehouse reviewed Apr 15, 2025

View reviewed changes

Chand-ra force-pushed the complete branch from 3835530 to 3bc7b7f Compare April 16, 2025 11:42

Chand-ra force-pushed the complete branch from 3bc7b7f to 1ba407a Compare April 22, 2025 07:01

Chand-ra mentioned this pull request Apr 22, 2025

fuzz-tests: Add fuzz target for closing_sig #8244

Merged

4 tasks

morehouse reviewed Apr 22, 2025

View reviewed changes

Chandra Pratap added 2 commits April 23, 2025 14:37

fuzz-tests: Add a seed corpus for the new test

2a2e9cd

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 force-pushed the complete branch from 1ba407a to 2a2e9cd Compare April 23, 2025 14:41

morehouse approved these changes Apr 23, 2025

View reviewed changes

rustyrussell merged commit 59ccced into ElementsProject:master Sep 15, 2025

Conversation

Chand-ra commented Apr 6, 2025

Checklist

Uh oh!

vincenzopalazzo commented Apr 9, 2025

Uh oh!

Chand-ra commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Apr 11, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chand-ra commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morehouse commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chand-ra commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Chand-ra commented Apr 16, 2025 •

edited

Loading

morehouse commented Apr 21, 2025 •

edited

Loading