qa wallet: Activate dormant checks in wallet_multisig_descriptor

hodlinator commented at 10:30 pm on February 14, 2025: contributor

Conditions had virtually no effect, lets activate them.

DrahtBot commented at 10:30 pm on February 14, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31874.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, ryanofsky, l0rinc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in test/functional/wallet_multisig_descriptor_psbt.py:102 in 84485368c4 outdated

 96@@ -96,9 +97,9 @@ def run_test(self):
 97         self.log.info("Check that every participant's multisig generates the same addresses...")
 98         for _ in range(10):  # we check that the first 10 generated addresses are the same for all participant's multisigs
 99             receive_addresses = [multisig.getnewaddress() for multisig in participants["multisigs"]]
100-            all(address == receive_addresses[0] for address in receive_addresses)
101+            assert_true(all(address == receive_addresses[0] for address in receive_addresses))
102             change_addresses = [multisig.getrawchangeaddress() for multisig in participants["multisigs"]]
103-            all(address == change_addresses[0] for address in change_addresses)
104+            assert_true(all(address == change_addresses[0] for address in change_addresses))

l0rinc commented at 11:06 pm on February 14, 2025:

Good find! In other cases we seem to simply compare with the expected boolean value, e.g. https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_addrman.py#L89 - is that an option here?

in test/functional/wallet_multisig_descriptor_psbt.py:73 in 84485368c4 outdated

69@@ -69,7 +70,7 @@ def participants_create_multisigs(self, external_xpubs, internal_xpubs):
70                     "timestamp": "now",
71                 },
72             ])
73-            assert all(r["success"] for r in result)
74+            assert_true(all(r["success"] for r in result))

l0rinc commented at 11:07 pm on February 14, 2025:

Can we unify the remaining assert all() constructs as well, e.g. https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_conflicts.py#L152 ?

l0rinc changes_requested

l0rinc commented at 11:07 pm on February 14, 2025: contributor

Good find, two observations/requests: *) Can we use assert_equal(all(), True) for consistency? *) Can we convert the remaining assert all() constructs to whatever we will end up using here?

Edit: a full example: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_encryption.py#L162

hodlinator commented at 4:04 pm on February 15, 2025: contributor

Thanks for having a look @l0rinc!

Can we use assert_equal(all(), True) for consistency?

I prefer assert_true(all()), since the condition may span the full line, so having what we are testing against at the beginning of the line is more readable. One could go Yoda-style and assert_equal(True, all()) to solve that issue, but why not bite the bullet and support assert_true(all())? Other frameworks like GoogleTest have ASSERT_TRUE().

*) Can we convert the remaining assert all() constructs to whatever we will end up using here?

My first preference is to just have assert_true() roll out organically. But I am open to authoring scripted diffs if there is enough support.

git grep "assert_equal.*, True)$" |wc -l => 228 This seems like a reasonable candidate for conversion.

git grep "^ *assert\b" *.py |wc -l => 1592 I’m a bit more reluctant to change all built-in assert usages in a scripted diff as some may be intentionally intended to be internal checks for non-optimized runs. See discussion in other PR: #30529 (review). Maybe I could do a scripted diff for git grep -E "^ *assert (all|any)" *.py |wc -l => 30.

Want to point out that I don’t want to spend too much of our time bikeshedding, I’m okay with switching back to mis-using assert for now and kick this can down the road. In practice it’s very rare that anyone attempts to run tests in optimized mode, evidenced by several tests failing if run that way.

l0rinc commented at 4:44 pm on February 15, 2025: contributor

why not bite the bullet and support

I don’t really see what it adds, but won’t block if others are in favor.

Can we separate the two problems? I mean adding back the assertions in one PR (though I’d vote for fixing all remaining assert (all|any in the same PR that fixes the unasserted ones), and proposing the new assert_true together with a scripted diff that applies it, to see if reviewers (or at least the author) thinks that assert_true is indeed better than assert_equals(True,.

hodlinator commented at 7:31 pm on February 15, 2025: contributor

I don’t really see what it adds, but won’t block if others are in favor.

Can we separate the two problems?

Lets see what others think. I think the PR content & size is okay as-is, both fixing a minor issue and demonstrating a slight use-case for assert_true(), but am open to expand it with scripted diffs if others agree. Not too keen on both expanding and splitting it at the same time though.

in test/functional/test_framework/util.py:77 in 84485368c4 outdated

68@@ -69,6 +69,14 @@ def summarise_dict_differences(thing1, thing2):
69             d2[k] = thing2[k]
70     return d1, d2
71 
72+
73+def assert_true(condition):
74+    """Separate from the `assert` keyword in that it should not be optimized out
75+    when environment var `PYTHONOPTIMIZE=1`, or Python is run with `-O`."""
76+    if not condition:
77+        raise AssertionError

maflcko commented at 7:38 pm on February 15, 2025:

Not sure about adding a new function, because assert may be optimized out. Is there any report of anyone ever setting this? I’d say it is more common to not run the tests at all.

If you think this should be addressed, it would be better to check it at startup and fail early, instead of (starting to ) apply this replacement to all code.

If there was a reason to apply this to all code, it should be enforced in one way or another.

hodlinator commented at 7:52 pm on February 15, 2025:

I would be open to explore adding an error at startup if someone attempts executing functional tests in optimized mode so we can continue using built-in assert to check test conditions (several tests fail already when attempting it). @ryanofsky expressed seeing value in reserving assert for internal sanity checks in the tests (see linked thread), which inspired me to author this PR in the way I did.

maflcko commented at 7:37 am on February 17, 2025:

expressed seeing value in reserving assert for internal sanity checks in the tests

Is there an example of an internal sanity check? How would that be different from the functional framework unit tests? Also, why would there be any risk in treating an internal test error as a “normal” test error?

I understand the distinction of internal errors in user-facing programs, but for dev-only test-only scripts, the meaning and the tradeoffs become less clear.

maflcko commented at 7:40 am on February 17, 2025:

Another review note, it seems like assert_true("abc") will pass. This seems confusing and I am also not sure if it is clearer than assert "abc". (Same for integral values like 3, …)

And on a general note, the attempt here seems similar or related to the discussion (and changes) stemming from #23119. However that issue is stale for more than 3 years, with no meaningful progress, so I wonder how important it is and whether it can be closed.

hodlinator commented at 1:45 pm on February 17, 2025:

Thanks, added type annotations to assert_true() in latest push. mypy --check-untyped-defs catches type mismatches for it, but not for built-in assert. It also triggers a deluge of other errors though.

The point of #23119 is for assertion failures to output the values that made them fail. While it also entails replacing a subset of instances of built-in assert with our own, the purpose is different.

l0rinc commented at 1:51 pm on February 17, 2025:

It’s also similar to #29500 from a year ago. Btw, type hints in Python are just hints, adding assert_true(1) will still pass the test. @hodlinator, could we separate fixing dead code from introducing another assertion helper?

hodlinator commented at 2:05 pm on February 17, 2025:

I’d say #29500 is more related to #23119 than this PR.

Btw, type hints in Python are just hints, adding assert_true(1) will still pass the test.

Yes, that’s why I commented about the linter situation above.

@hodlinator, could we separate fixing dead code from introducing another assertion helper?

Opinion noted (again). I won’t repeat myself.

hodlinator force-pushed on Feb 17, 2025

maflcko commented at 2:38 pm on February 17, 2025: member

Tend towards approach NACK for now, concerning PYTHONOPTIMIZE, due to the outstanding issues mentioned in #31874 (review)

laanwj added the label Tests on Mar 11, 2025

qa wallet: Actually make use of expressions

They were basically no-ops.

35d17cd5ee

hodlinator force-pushed on Mar 26, 2025

hodlinator commented at 8:57 am on March 26, 2025: contributor

Switched to stripped-down version of this PR without the former attempt to introduce assert_true(). It seemed fortuitous that I found these examples to illustrate it with, but will kick that can down the road for now.

maflcko commented at 9:04 am on March 26, 2025: member

lgtm ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

ryanofsky approved

ryanofsky commented at 6:03 pm on March 27, 2025: contributor

Code review ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

l0rinc approved

l0rinc commented at 6:07 pm on March 27, 2025: contributor

ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

ryanofsky merged this on Mar 27, 2025

ryanofsky closed this on Mar 27, 2025

hodlinator deleted the branch on Mar 27, 2025

qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py #31874

Code Coverage & Benchmarks

Reviews

Conflicts