fuzz-tests: add test for amount-{sat, msat} arithmetic

[Chand-ra]

The fuzz-amount test doesn't fuzz the arithmetic operations for struct amount_sat and struct amount_msat. Add a test for the same.

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]

Hey @morehouse , running this test throws the following errors:

• common/amount.c:359:18: runtime error: -nan is outside the range of representable values of type 'unsigned long'
• common/amount.c:345:23: runtime error: -inf is outside the range of representable values of type 'unsigned long'
• common/amount.c:345:23: runtime error: -1.10313e+217 is outside the range of representable values of type 'unsigned long'

among a few others, which are probably due to faulty harness logic. The above failures happen in the following two functions:

WARN_UNUSED_RESULT bool amount_msat_scale(struct amount_msat *val,
					  struct amount_msat msat,
					  double scale)
{
	double scaled = msat.millisatoshis * scale;

	/* If mantissa is < 64 bits, a naive "if (scaled >
	 * UINT64_MAX)" doesn't work.  Stick to powers of 2. */
	if (scaled >= (double)((u64)1 << 63) * 2)
		return false;
345	val->millisatoshis = scaled;
	return true;
}

WARN_UNUSED_RESULT bool amount_sat_scale(struct amount_sat *val,
					 struct amount_sat sat,
					 double scale)
{
	double scaled = sat.satoshis * scale;

	/* If mantissa is < 64 bits, a naive "if (scaled >
	 * UINT64_MAX)" doesn't work.  Stick to powers of 2. */
	if (scaled >= (double)((u64)1 << 63) * 2)
		return false;
359	val->satoshis = scaled;
	return true;
}

Has the fuzzer found an actual bug or is it just another impossible edge case?

[morehouse]

Has the fuzzer found an actual bug or is it just another impossible edge case?

I think technically these are bugs -- amount_msat_scale and amount_sat_scale are supposed to return false when scaling fails and already do this for the positive-overflow case. Since there's no usage comment requiring scale to be positive, probably the functions should handle negative overflow as well.

In practice, all current usages of these functions in the codebase have positive values for scale, so the bug can never manifest in the existing codebase.

[Chand-ra]

In practice, all current usages of these functions in the codebase have positive values for scale, so the bug can never manifest in the existing codebase.

Makes me wonder why scale's type isn't unsigned in the first place...

[morehouse]

Makes me wonder why scale's type isn't unsigned in the first place...

Probably because there's no way to represent an unsigned float.

[Chand-ra]

Hey @morehouse,

As we discussed over email, I spent some time last week investigating the potential bug caused by an assertion failure in amount_msat_sub_fee() to determine whether it could be triggered externally. I grepped the codebase for all callers of amount_msat_sub_fee() and found that the only caller resides in plugins/askrene/refine.c. This, in turn, ultimately traces back to askrene’s getroutes command.

To test this path, I added a user-level Python test in tests/test_askrene.py to try passing the error-causing parameters to amount_msat_sub_fee(). I found that any parameter values that get too large are truncated to a set of ‘safe values’, making it impossible to reproduce the breakage through this command.

In summary, it doesn’t appear that any external message can trigger the failure seen in the fuzz test. I’ve pushed the test in case you’d like to take a look.

[Chand-ra]

The test works without any breakage now and is ready to be merged.

[morehouse]

I am hesitant to merge this fuzz target, since it basically has to reimplement all the operations in common/amount.c so we have something to compare against. It seems a bit silly when we could just directly review the implementations in common/amount.c.

That said, the target did find a couple cases of undefined behavior, which makes me think it would be worth merging this target at least as regression tests for those bugs.

[morehouse]

Nit: the whole file needs to be clang-format-ed. There's a mix of indentation styles going on.

[morehouse]

This will always return amts that are multiples of 1000. Ideally we would test the full precision.

[morehouse]

This seems odd. Why are we copying the double into a u64?

Since we have all these different params we might use, maybe we should just put them in a struct for simplicity:

struct fuzzing_params {
  enum op op;
  struct amount_msat a;
  struct amount_msat b;
  double f;
  u64 u64_param;
};

void run(const u8 *data, size_t size) {
  struct fuzzing_params f;
  if (size < sizeof(f)) return;

  memcpy(&f, data, sizeof(f));
  ...
}

[morehouse]

By limiting sats in this way, we will probably miss the overflow code paths during fuzzing.

[Chand-ra]

Right, but isn't it a valid assumption that in production we won't be doing arithmetic on anything greater than the greatest possible value for satoshis/millisatoshis?

[morehouse]

This is here to avoid the UBSan error that is fixed in: #8306. Same for OP_SAT_SCALE.

I think it would be better to merge that PR first and remove this guard here.

[morehouse]

It looks like this is testing that original + fee == original + fee, which will always be true. I think this whole case is probably pointless.

[Chand-ra]

I agree that this check is redundant but do think there is some merit to the test case, it at least makes sure amount_msat_fee doesn't crash with arbitrary input.

[morehouse]

Should they be exactly equal?

[Chand-ra]

In amount_msat_fee's defintion:

/* BOLT #7:
*
*   - SHOULD accept HTLCs that pay a fee equal to or greater than:
*    - fee_base_msat + ( amount_to_forward * fee_proportional_millionths / 1000000 )
*/

So it is valid for sum to be <= input.

[morehouse]

This check seems pointless, since the cast to size_t below will ensure that weight < SIZE_MAX.

[morehouse]

Masking is pointless if we're casting to u32 anyway.

[morehouse]

Maybe we should just mask the weight with UINT32_MAX and remove this check.

[Chand-ra]

The corpus for this test is now generated with the fix in #8306 applied, so that PR needs to be merged before this one can.