test: Temporarily work around Windows bug in walletpassphrase

MarcoFalke commented at 2:47 pm on July 30, 2022: member

The test passes on all platforms, such as freebsd, linux, or macos. However, it does not pass on Windows. Temporarily relax it on Windows to avoid wasting resources on manually triggered CI re-runs. The workaround can be reverted when the issue is understood and fixed.

fanquake added the label Tests on Jul 30, 2022

test: Temporarily work around Windows bug in walletpassphrase faa2df9cd3

MarcoFalke force-pushed on Jul 30, 2022

DrahtBot commented at 7:55 pm on July 31, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25878 (tests: Use mocktime for wallet encryption timeout by achow101)
#25021 (test: fix intermittent wallet_encryption failures on win64 task by jonatack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke force-pushed on Aug 4, 2022

in test/functional/wallet_encryption.py:85 in faa2df9cd3

78@@ -78,8 +79,13 @@ def run_test(self):
79         MAX_VALUE = 100000000
80         expected_time = int(time.time()) + MAX_VALUE - 600
81         self.nodes[0].walletpassphrase(passphrase2, MAX_VALUE - 600)
82-        # give buffer for walletpassphrase, since it iterates over all encrypted keys
83+        # Calculate the expected timeout after the walletpassphrase call, since
84+        # it iterates over all encrypted keys
85         expected_time_with_buffer = time.time() + MAX_VALUE - 600
86+        if sys.platform == "win32":

vasild commented at 3:29 pm on August 11, 2022:

From the discussion in #25482 (comment) it seems that time() and std::chrono can deviate a bit on both Windows and Linux and that is the root of the problem. We only observe a problem on Windows because it happens that Python’s time.time() uses one on Windows and the other on Linux. But we are not guaranteed that it will be like that forever. E.g. future Python on Linux may start to behave as the Python on Windows.

Is the above correct? If yes, then it would be better to fix this test in platform agnostic way.

MarcoFalke commented at 3:42 pm on August 11, 2022:

No, I still fail to see how this bug exits in the first place.

We know:

It has only been observed on Windows MSVC, after commit 0000a63689036dc4368d04c0648a55fdf507932f
It has never been observed on any other OS or a different compiler. Though, that doesn’t mean the bug doesn’t exist on Windows with clang or gcc, or on Linux with a different setting.
After commit 0000a63689036dc4368d04c0648a55fdf507932f, both python time.time() and MSVC std::chrono use the same source for time (precise file time). So commit 0000a63689036dc4368d04c0648a55fdf507932f should have fixed the issue (if it existed before), not introduce it
The issue shouldn’t be reproducible on systems where time() is behind std::chrono::system_clock. I’ve observed the time() to be behind by usually 1/2/6 ms on Linux. While this lag wouldn’t lead to any test failues, commit 0000a63689036dc4368d04c0648a55fdf507932f removed the lag on Linux.

ryanofsky approved

ryanofsky commented at 4:05 pm on August 18, 2022: contributor

Code review ACK faa2df9cd3e19e9a5104614485fa19d4383a407f. Agree with Marco it is probably good to limit the check to win32 if we don’t know cause of the problem yet.

One speculation about the cause: maybe this is a rounding bug? int(time.time()) will round the expected_time_with_buffer value down in python, while it’s not clear from https://en.cppreference.com/w/cpp/chrono/time_point/time_point_cast if the c++ code necessarily will truncate

MarcoFalke commented at 8:48 am on August 19, 2022: member

One speculation about the cause: maybe this is a rounding bug?

I don’t think so. floor and cast do the same thing, see #25021 (comment)

MarcoFalke closed this on Aug 20, 2022

MarcoFalke deleted the branch on Aug 20, 2022

bitcoin locked this on Aug 20, 2023

test: Temporarily work around Windows bug in walletpassphrase #25750

Conflicts