test: fix floating point conversion in `wallet_encryption.py` #25759

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:issue_25758 changing 1 files +18 −6

w0xlt commented at 10:04 AM on July 31, 2022: contributor

time.time() returns a floating point number and it is not being converted to int, causing the issue reported in #25758 in wallet_encryption.py test.

This PR proposes a fix, closes #25758 and #25482.
fanquake added the label Tests on Jul 31, 2022

fanquake commented at 10:30 AM on July 31, 2022: member

test/functional/wallet_encryption.py:10:1: F401 'test_framework.util.assert_greater_than' imported but unused
^---- failure generated from lint-python.py

jonatack commented at 10:42 AM on July 31, 2022: contributor

This was tracked for several months in #25021, which proposed a number of approaches.
fanquake commented at 10:50 AM on July 31, 2022: member

which proposed a number of approaches but was never merged.

The reason #25021 was never merged is outlined here: #25021 (comment). It had multiple unresolved review comments, and it was unclear if it was the correct fix, as it required modifying cpp code, unlike this change.
jonatack commented at 11:08 AM on July 31, 2022: contributor

unclear if it was the correct fix, as it required modifying cpp code, unlike this change.

It did not require modifying any cpp code. That was an optional tidy-up.
jonatack commented at 11:15 AM on July 31, 2022: contributor

The point of my comment was to bring up discussion and tracking of the issue to help connect the dots.
w0xlt force-pushed on Jul 31, 2022
DrahtBot commented at 7:46 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:
- #25750 (test: Temporarily work around Windows bug in walletpassphrase by MarcoFalke)
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.
w0xlt commented at 9:01 PM on July 31, 2022: contributor

#25759 (comment) addressed in https://github.com/bitcoin/bitcoin/pull/25759/commits/643675c4dfff6c0e174fd39e9a6ab10ed7681db5.

I didn't know about #25021. I'll take a look.
mzumsande commented at 3:39 AM on August 1, 2022: contributor

There is also #25750. None of the approaches fix the actual problem (which would require an explanation why it occurs / why it's windows-only/ideally steps to reproduce), but are workarounds relaxing the test assumptions in different ways so that the CI becomes green. I don't have an explanation either, but I think it is important to document that.

in test/functional/wallet_encryption.py:81 in 643675c4df outdated

  77 | @@ -79,18 +78,18 @@ def run_test(self):
  78 |          expected_time = int(time.time()) + MAX_VALUE - 600
  79 |          self.nodes[0].walletpassphrase(passphrase2, MAX_VALUE - 600)
  80 |          # give buffer for walletpassphrase, since it iterates over all encrypted keys
  81 | -        expected_time_with_buffer = time.time() + MAX_VALUE - 600
  82 | +        expected_time_with_buffer = int(time.time()) + MAX_VALUE - 600

maflcko commented at 6:07 AM on August 1, 2022:

This is wrong. From your linked bug report, expected_time_with_buffer is ...29.99 and actual_time is ...30.

With floor/int, this will give ...29, which will still fail the assertion.

w0xlt commented at 12:20 PM on August 1, 2022:

Done ind https://github.com/bitcoin/bitcoin/pull/25759/commits/a5401b8d4a1041130b6094c4a04832bca40750a7.

maflcko changes_requested
maflcko commented at 6:09 AM on August 1, 2022: member

This is wrong. You'll have to replace int/floor with ceil as in Jon's pull or with +1 as in my pull.
maflcko commented at 6:10 AM on August 1, 2022: member

Also, it would be good to limit the workaround to only the platform that fails, including a reference/link for the workaround in the comment.
w0xlt force-pushed on Aug 1, 2022
w0xlt commented at 12:21 PM on August 1, 2022: contributor

@MarcoFalke Thanks for review. a5401b8 addresses the requested changes.
test: fix floating point conversion cc6385ff9f

in test/functional/wallet_encryption.py:87 in a5401b8d4a outdated

  85 | +        # Calculate the expected timeout after the walletpassphrase call, since
  86 | +        # it iterates over all encrypted keys
  87 | +        if sys.platform == "win32":
  88 | +            # Temporarily work around Windows bug, see
  89 | +            # https://github.com/bitcoin/bitcoin/issues/25482
  90 | +            expected_time_with_buffer = math.ceil(time.time()) + MAX_VALUE - 600

maflcko commented at 12:22 PM on August 1, 2022:

This is still wrong ceil(29.99) gives 30, which fails the assertion below

w0xlt force-pushed on Aug 1, 2022
w0xlt commented at 1:54 PM on August 1, 2022: contributor

I think that implementing the requested changes will end up with code very similar to #25750. or #25021, so better continue the discussion in one of these PRs.
w0xlt closed this on Aug 1, 2022
bitcoin locked this on Feb 21, 2024

Contributors

Labels

Linked (view graph)

#25482 qa: Intermittent failure in `wallet_encryption.py --descriptors`#25758 ci: `AssertionError: 1759238429.9936483` <= 1759238430 in `wallet_encryption.py --descriptors`

test: fix floating point conversion in `wallet_encryption.py` #25759

Conflicts