fix(test): Avoid a race condition in testing modified JoinSplits

[upbqdn]

Motivation

We recently saw the failure described in #5823, so we reopened the issue. This PR closes #5823.

Solution

I increased the number of test cases from 10 to 100. If the issue ever shows up again, we shouldn't increase the number of test cases, but accept both test outcomes as correct since increasing the number of test cases would significantly increase the upper bound for the running time of the test.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

[daira]

This is still quite fragile, and there's no reason why correct modifications (e.g. to make the signature verification faster) wouldn't start making it fail all the time.

It's up to yous of course, but I would suggest creating transactions for which you know the keys so that you can recreate the signature. An advantage of this is that you can recreate the signature only for v4, and that would effectively test that v5 signatures don't depend on proofs.

Accepting either a proof or signature failure is not the right thing to do: suppose that you do that, then improve signature verification time, and then introduce a regression in the proof checking. The test could silently succeed in that situation. (Just a proof checking regression might be enough to do the same thing if it increased proof checking time.)

[arya2]

This is still quite fragile, and there's no reason why correct modifications (e.g. to make the signature verification faster) wouldn't start making it fail all the time.

I think and hope this PR could make the failure rate negligible.

Accepting either a proof or signature failure is not the right thing to do

I agree with this, if it fails again after this PR or when we're addressing tech-debt (#6922).

[codecov]

Codecov Report

Merging #6921 (eff5d0a) into main (b10df28) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6921      +/-   ##
==========================================
- Coverage   77.60%   77.54%   -0.07%     
==========================================
  Files         310      310              
  Lines       41475    41475              
==========================================
- Hits        32188    32162      -26     
- Misses       9287     9313      +26     

[teor2345]

increasing the number of test cases would significantly increase the upper bound for the running time of the test

What is the typical running time for this test after these changes?

[upbqdn]

100 iterations, which is the upper bound, takes 12.96s on AMD Ryzen Threadripper 3970X 32-Core Processor. 10 iterations, which is the previous upper bound, takes 1.17s. These times are without the --release tag. Adding the tag changes the results to 11.96s and 1.99s, respectively. It's interesting to see there's an increase for 10 iterations. I repeated the measurement three times for each setup. The deviation was ~0.01s.

I think the new max time is still reasonable?

[upbqdn]

increasing the number of test cases would significantly increase the upper bound for the running time of the test

What is the typical running time for this test after these changes?

I didn't phrase that very well. I meant increasing the number by another order of magnitude would increase the upper bound beyond a reasonable limit.

[teor2345]

We discussed this in the Engineering sync and decided it was better to merge it before the stable release, to prevent further CI failures.

(I think 13 seconds is a reasonable test time, because it happens in parallel with other tests.)