-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
fix(test): Avoid a race condition in testing modified JoinSplits #6921
Conversation
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.) |
I think and hope this PR could make the failure rate negligible.
I agree with this, if it fails again after this PR or when we're addressing tech-debt (#6922). |
Codecov Report
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 |
What is the typical running time for this test after these changes? |
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 I think the new max time is still reasonable? |
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. |
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.) |
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