fuzz-tests: Add a test for full_channel operations
#8437
Conversation
There was a problem hiding this comment.
This is a nice target at a high level.
I think we're missing a lot of opportunities to fuzz invalid/unexpected/rare states, where bugs could be hiding. Some ideas for improvement:
- Unshackle the fuzzer a bit, so it can pick random values sometimes.
- Consider allowing the fuzzer to simulate the various commitment dance messages arriving at unexpected times (e.g., sending
revoke_and_ackwhen the peer hasn't updated the commitment yet). - Consider allowing the fuzzer to generate multiple HTLC operations before doing the commitment signed dance. This is valid behavior per the spec.
| memset(&p_htlc.preimage, (int)p_htlc.id, sizeof(p_htlc.preimage)); | ||
|
|
||
| if (add_htlc(lchannel, sender, p_htlc.id, &p_htlc.preimage, msat, cltv)) { | ||
| add_htlc(rchannel, sender, p_htlc.id, &p_htlc.preimage, msat, cltv); |
There was a problem hiding this comment.
Probably should assert that the HTLC is successfully added to rchannel here.
There was a problem hiding this comment.
This immediately causes an assertion failure here because owed = view->lowest_splice_amnt[side] = 0.
tests/fuzz/fuzz-full_channel.c
Outdated
| struct pending_htlc p_htlc = pending_htlcs[idx]; | ||
|
|
||
| if (fulfill_htlc(lchannel, p_htlc.sender, p_htlc.id, &p_htlc.preimage)) { | ||
| fulfill_htlc(rchannel, p_htlc.sender, p_htlc.id, &p_htlc.preimage); |
There was a problem hiding this comment.
Probably should assert that the HTLC is successfully fulfilled on rchannel.
There was a problem hiding this comment.
This causes an assertion failure here because htlc_get() returns NULL.
|
|
||
| enum side sender = (fromwire_u8(&data, &size) % 2) ? REMOTE : LOCAL; | ||
| struct amount_msat msat = fromwire_amount_msat_bounded(&data, &size); | ||
| u32 cltv = current_blockheight + fromwire_u16(&data, &size); |
There was a problem hiding this comment.
This might be too restrictive. The peer could set any CLTV in update_add_htlc.
| break; | ||
|
|
||
| enum side sender = (fromwire_u8(&data, &size) % 2) ? REMOTE : LOCAL; | ||
| struct amount_msat msat = fromwire_amount_msat_bounded(&data, &size); |
There was a problem hiding this comment.
The peer could set any amount in their update_add_htlc message.
There was a problem hiding this comment.
Hmm, changing this to:
struct amount_msat msat = fromwire_amount_msat(&data, &size);
causes an assertion failure here in channel_txs().
| * recv_revoke_and_ack -> recv_commit -> sending_revoke_and_ack) | ||
| * in the correct opener vs. accepter order. | ||
| */ | ||
| static void exchange_commits(struct channel *channel, enum side side, const struct htlc ***changed_htlcs) |
There was a problem hiding this comment.
It would be very interesting to fuzz the commitment dance as well. For each of these messages, the channel peer can send all kinds of unexpected fields.
There was a problem hiding this comment.
Not sure how to go about implementing this. The only fields passed to the functions in this routine are channel and changed_htlcs out of which changed_htlcs is set by the said functions, and fuzzing channel would be probably not be worth the trouble since we already create our own channel which is passed here.
Is it fine to fuzz the order in which we call the handlers? Maybe we should create a separate fuzz_exchange_commits() which calls the handlers in random order, and add both of these as new branches in the switch case? I think that would nicely incorporate these feedbacks as well:
- Consider allowing the fuzzer to generate multiple HTLC operations before doing the commitment signed dance. This is valid behavior per the spec.
- Consider allowing the fuzzer to simulate the various commitment dance messages arriving at unexpected times (e.g., sending revoke_and_ack when the peer hasn't updated the commitment yet).
tests/fuzz/fuzz-full_channel.c
Outdated
| if (!channel_get_htlc(channel, original_sender, id)) | ||
| return false; |
There was a problem hiding this comment.
The peer could send an invalid HTLC ID.
There was a problem hiding this comment.
The parameters are passed to this caller from run() so it's probably better to make changes there. Maybe something like:
if (fromwire_u8(&data, &size) % 2) {
if (fail_htlc(lchannel, p_htlc.sender, p_htlc.id)) {
fail_htlc(rchannel, p_htlc.sender, p_htlc.id);
tal_arr_remove(&pending_htlcs, idx);
}
} else {
fail_htlc(lchannel, fromwire_u8(&data, &size) % 2 ? LOCAL : REMOTE,
fromwire_u64(&data, &size));
fail_htlc(rchannel, fromwire_u8(&data, &size) % 2 ? LOCAL : REMOTE,
fromwire_u64(&data, &size));
}
to make sure both valid and non-valid paths are exercised.
| } | ||
| case 3: /* UPDATE_FEE */ | ||
| { | ||
| u32 feerate = fromwire_bounded_feerate(&data, &size); |
There was a problem hiding this comment.
The peer could set any feerate.
There was a problem hiding this comment.
There seems to be checks that prevent fees too low or too high from reaching the target function, channel_update_feerate(), in the channel daemon. Changing this to fromwire_u32() causes this assertion failure:
fuzz-full_channel: common/fee_states.c:175: u32 marginal_feerate(u32): Assertion `current_feerate >= minfeerate' failed.
where minfeerate is 253.
tests/fuzz/fuzz-full_channel.c
Outdated
| { | ||
| u32 feerate = fromwire_bounded_feerate(&data, &size); | ||
| if (update_feerate(lchannel, feerate)) | ||
| update_feerate(rchannel, feerate); |
There was a problem hiding this comment.
Probably should assert that the feerate is successfully updated for rchannel.
tests/fuzz/fuzz-full_channel.c
Outdated
| u32 height_increase = 1 + fromwire_u32(&data, &size); | ||
| current_blockheight += height_increase; |
There was a problem hiding this comment.
The peer could set any blockheight, including one that is less than the previous one.
tests/fuzz/fuzz-full_channel.c
Outdated
| channel_txs(tmpctx, &funding, funding_amount, &htlc_map, NULL, &funding_wscript_alt, | ||
| lchannel, &local_per_commitment_point, 42, LOCAL, 0, 0, &anchor, NULL); | ||
| channel_txs(tmpctx, &funding, funding_amount, &htlc_map, NULL, &funding_wscript_alt, | ||
| rchannel, &local_per_commitment_point, 42, REMOTE, 0, 0, &anchor, NULL); |
There was a problem hiding this comment.
Is it expensive to generate these transactions? Maybe we should generate them after each loop iteration.
Changelog-None: Functions defined in `channeld/full_channel.h` contain channel operations like `ADD_HTLC`, `FULFILL_HTLC`, `UPDATE_FEERATE`, etc. Since they are a critical part of the HTLC state machine and may be influenced by external agents, add a stateful test for them.
Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.
Functions defined in
channeld/full_channel.hcontain channel operations likeADD_HTLC,FULFILL_HTLC,UPDATE_FEERATE, etc. Since they are a critical part of the HTLC state machine and may be influenced by external agents, add a stateful test for them.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:
CC: @morehouse