rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
194 stars 55 forks source link

test_sym_encryption_s2k_msec fails #2024

Closed maxirmx closed 1 year ago

maxirmx commented 1 year ago

In Windows environment I observe the following failure

======================================================================
FAIL: test_sym_encryption_s2k_msec (__main__.Encryption.test_sym_encryption_s2k_msec)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\rnp\rnp\src\tests\cli_tests.py", line 4021, in test_sym_encryption_s2k_msec
    self.assertGreaterEqual(iters100msec, iters10msec)
AssertionError: 278528 not greater than or equal to 589824
----------------------------------------------------------------------

It is unstable and unpredictable. The values vary, of course. I believe it depends on GHA load. It looke like if GHA is busy it becomes more probable. I cannot reproducr it locally.

Originally posted by @maxirmx in https://github.com/rnpgp/rnp/issues/2021#issuecomment-1487365224

maxirmx commented 1 year ago

I guess uint64_t get_timestamp_usec() does not provide good precision Replacing it with an implementation based on <chrono> may help

maxirmx commented 1 year ago

This failure originates in _pgp_s2k_compute_iters(pgp_hash_alg_t alg, size_t desired_msec, size_t trialmsec) function. It is developed under assumtion that one can estimate the number that are required to achieve specific time complexity.

This assumption does not seem valid for me because in the evaluation <time> = <iterations>/<performance> It looks like on GHA runner performance fluctuates significantly and it tends to "heat up" on computing intensive tests.

Not sure if there are any issues in production environment.

If we want to fix it in test environment I would suggest to run the code the estimates "performance" not once but ~100 times in order to have an average estimate of "performance".

It won't give 100% relaiable result anyway but will make the issues really rare.

cc @ronaldtse @ni4

ni4 commented 1 year ago

@maxirmx Agree on this approach.