test: migrate functional test equality asserts to `assert

l0rinc commented at 10:22 PM on March 8, 2026: contributor

Problem

Plain assert x == y is a poor fit for this test framework, assert_equal() gives more useful failure output and keeps equality checks consistent with the rest of the functional tests.

Design

A simple scripted diff cannot safely rewrite all of them because many files use == inside larger expressions, such as chained conditions, comprehensions, and other compound assertions. That makes a one-shot mechanical conversion either incorrect or harder to review.

Fix

This series first rewrites the non-mechanical cases into standalone assertions so patterns are easier to identify, then applies a scripted diff to the remaining cases that are safe to convert mechanically.

Partially fixes #23119 by adjusting the == case only (which is similar to the BOOST alternatives so it's expected to be uncontroversial).

DrahtBot added the label Tests on Mar 8, 2026

DrahtBot commented at 10:23 PM on March 8, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, rkrux, theStack
Concept ACK	kevkevinpal

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35049 (test: remove circular dependency between authproxy and util by rkrux)
#34287 ([RFC] test: integrate secp256k1lab as subtree and use it for low-level EC ops by theStack)

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.

l0rinc force-pushed on Mar 8, 2026

DrahtBot added the label CI failed on Mar 8, 2026

DrahtBot removed the label CI failed on Mar 8, 2026

maflcko commented at 5:11 PM on March 10, 2026: member

Fix

Nerd-sniped by #34761,

Instead of referring to a random closed draft-pull request, it would be better to refer to the tracking issue, wihch is https://github.com/bitcoin/bitcoin/issues/23119

kevkevinpal commented at 8:42 PM on March 13, 2026: contributor

Concept ACK

The use of scripted diff is good, glad someone is picking up this cleanup change!

in test/functional/mempool_cluster.py:84 in 068333c5c3 outdated

  80 | @@ -81,7 +81,7 @@ def check_feerate_diagram(self, node):
  81 |          last_val = {"weight": 0, "fee": 0}
  82 |          for x in feeratediagram:
  83 |              # The weight is always positive, except for the first iteration
  84 | -            assert x['weight'] > 0 or x['fee'] == 0
  85 | +            assert x['weight'] > 0 or not x['fee']

maflcko commented at 4:17 PM on March 16, 2026:

068333c5c3387cf0809fa779f9fb7ac5fc7d4118: I don't think this is a good change, nor correct.

fee==0 reads easier than not fee. Also, the replacement is wrong:

>>> fee=None; fee==0
False
>>> fee=None; not fee
True

l0rinc commented at 4:31 PM on March 16, 2026:

It seems to me fee isn't optional here: https://github.com/bitcoin/bitcoin/blob/e31ab8040fc85311dae5390f7ad5d9320828f30b/src/rpc/mempool.cpp#L644

But I can improve it regardless if you think that should also be covered, thanks!

maflcko commented at 9:59 AM on March 17, 2026:

The goal of the tests is to also catch type errors. I know there are internal type checks, but they are incomplete: E.g. #34799 (et al)

In any case, this reads harder, so I just don't see the benefit of making something harder to read and more brittle at the same time.

l0rinc commented at 10:23 AM on March 18, 2026:

Valid point, thanks, fixed in latest push!

maflcko commented at 10:18 AM on April 10, 2026:

(can be resolved)

in test/functional/p2p_segwit.py:1361 in 068333c5c3 outdated

1357 | @@ -1358,7 +1358,7 @@ def test_segwit_versions(self):
1358 |              temp_utxo.append(UTXO(tx.txid_int, 0, tx.vout[0].nValue))
1359 |  
1360 |          self.generate(self.nodes[0], 1)  # Mine all the transactions
1361 | -        assert len(self.nodes[0].getrawmempool()) == 0
1362 | +        assert not self.nodes[0].getrawmempool()

maflcko commented at 4:17 PM on March 16, 2026:

Same here

maflcko commented at 4:17 PM on March 16, 2026:

and all other places ...

l0rinc commented at 4:33 PM on March 16, 2026:

Thanks, if you think these can be None, I'll adjust them differently to avoid the comparison that hinders the scripted diff

l0rinc force-pushed on Mar 18, 2026

in test/functional/wallet_miniscript.py:247 in b318d4b09f

 242 | @@ -243,7 +243,8 @@ def watchonly_test(self, desc):
 243 |              lambda: len(self.ms_wo_wallet.listunspent(minconf=0, addresses=[addr])) == 1
 244 |          )
 245 |          utxo = self.ms_wo_wallet.listunspent(minconf=0, addresses=[addr])[0]
 246 | -        assert utxo["txid"] == txid and utxo["solvable"]
 247 | +        assert_equal(utxo["txid"], txid)
 248 | +        assert utxo["solvable"]

maflcko commented at 1:22 PM on March 30, 2026:

nit in b318d4b09f9de1fc4f18a479a2078b818e1bc778: Should use assert_equal (probably with `True)

l0rinc commented at 1:13 PM on April 6, 2026:

I can do these on next push, thanks.

maflcko commented at 8:12 AM on April 7, 2026:

I am the only reviewer here so far, and I am happy to re-review this, so I am not sure why it needs to wait, possibly for another reviewer, which would then only waste review resourced, because more than one reviewer has to re-review.

l0rinc commented at 1:26 PM on April 7, 2026:

I am happy to re-review this

Appreciate that, pushed in a separate commit!

theStack commented at 10:01 PM on April 10, 2026:

nit in b318d4b: Should use assert_equal (probably with `True)

follow-up curiosity nit: is the intention to eventually get rid all of those truthy asserts? There are still hundreds of them around, see e.g. $ git grep "assert res" or $ git grep "assert not", and I think I wouldn't currently see it as a good use of our time to replace those as well, even though it would be (negligibly?) more type-safe.

l0rinc commented at 1:23 PM on April 12, 2026:

I think we agree with that - we only suggested/applied the ones that seem related to the current changes. Thanks for taking the time to review this.

in test/functional/rpc_psbt.py:966 in b318d4b09f

 965 |          # After signing and finalizing, needs extracting
 966 |          signed = self.nodes[1].walletprocesspsbt(updated)['psbt']
 967 |          analyzed = self.nodes[0].analyzepsbt(signed)
 968 | -        assert analyzed['inputs'][0]['has_utxo'] and analyzed['inputs'][0]['is_final'] and analyzed['next'] == 'extractor'
 969 | +        assert analyzed['inputs'][0]['has_utxo']
 970 | +        assert analyzed['inputs'][0]['is_final']

maflcko commented at 1:22 PM on March 30, 2026:

same

rkrux commented at 8:55 AM on April 10, 2026:

Can you resolve these comments? I thought these changes are not done when I reached reviewing the second commit 4f4516e3f6c8f25a1d903e0aa43d55ef53a1533b.

rkrux commented at 9:44 AM on April 10, 2026:

Ah I see these are resolved now, I might have been looking at an older state.

in test/functional/rpc_psbt.py:944 in b318d4b09f

 939 | @@ -937,20 +940,31 @@ def test_psbt_input_keys(psbt_input, keys):
 940 |          self.generate(self.nodes[0], 6)[0]
 941 |          psbt = self.nodes[1].createpsbt([utxo], {self.nodes[0].getnewaddress("", "p2sh-segwit"):Decimal('6.999')})
 942 |          analyzed = self.nodes[0].analyzepsbt(psbt)
 943 | -        assert not analyzed['inputs'][0]['has_utxo'] and not analyzed['inputs'][0]['is_final'] and analyzed['inputs'][0]['next'] == 'updater' and analyzed['next'] == 'updater'
 944 | +        assert not analyzed['inputs'][0]['has_utxo']
 945 | +        assert not analyzed['inputs'][0]['is_final']

maflcko commented at 1:23 PM on March 30, 2026:

same (False)

in test/functional/test_framework/script.py:819 in 0bc3496f82 outdated

 814 | @@ -814,8 +815,8 @@ def BIP341_sha_outputs(txTo):
 815 |      return sha256(b"".join(o.serialize() for o in txTo.vout))
 816 |  
 817 |  def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index=0, *, scriptpath=False, leaf_script=None, codeseparator_pos=-1, annex=None, leaf_ver=LEAF_VERSION_TAPSCRIPT):
 818 | -    assert (len(txTo.vin) == len(spent_utxos))
 819 | -    assert (input_index < len(txTo.vin))
 820 | +    assert_equal(len(txTo.vin), len(spent_utxos))
 821 | +    assert input_index < len(txTo.vin)

maflcko commented at 1:38 PM on March 30, 2026:

nit in 0bc3496f82267ce1fc0c591088921a640f5ae1c9:

assert input_index < len(txTo.vin) → assert_greater_than(len(txTo.vin), input_index)

l0rinc commented at 1:13 PM on April 6, 2026:

assert_greater_than

I strongly dislike assert_greater_than, I find it very noisy and inconsistent with BOOST, it's why I only migrated assert_equal.

maflcko approved

maflcko commented at 1:51 PM on March 30, 2026: member

review ACK 2680ef65035f3da58097f3b34c68bdf7a6d4346b 🍹

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 2680ef65035f3da58097f3b34c68bdf7a6d4346b 🍹
ubvetapgdre5kedJ9KqlFlce1nJ+V8M/ll8CJLl3m/PAxKPcYQTfWpBBPk2sFSCD3+yp7om7zCW2709FWZ1SCw==

</details>

test: use `in` for two-value equality asserts

Some tests spell a two-value membership check as `assert x == a or x == b`.
Rewrite those sites as `assert x in (a, b)`.
This keeps the check the same and removes `==` forms that the later cleanup should not touch.

76a5570b36

test: split equality asserts joined by `and`

Some tests combine multiple equality checks in one `assert`.
Split those checks into separate assertions so failures point at the exact mismatch.
This also removes mixed `==` expressions that the later cleanup should not touch.

4f4516e3f6

test: prep manual equality assert conversions

The later scripted diff only handles plain `assert x == y` lines.
Some remaining tests still use equality inside comprehensions, parenthesized asserts, and other shapes that the line-based rewrite would misread.
Rewrite those sites by hand first so the later mechanical conversion stays safe.
The commit also simplifies the dead `len(["errors"]) == 0` branch in `blocktools.py`, which can never be true.

dcd90fbe54

test: convert equality asserts with comments or special chars

Some remaining `assert x == y` lines include inline comments, list literals, or descriptor strings with `#`.
Convert those sites by hand so the later mechanical rewrite can stay simple.

23c06d4e6d

test: convert simple equality asserts in excluded files

A few files still need to stay out of the later scripted diff because they contain more complicated assert shapes.
Convert the straightforward equality checks in those files by hand first.
Use a local import in `authproxy.py` so the change does not create an import cycle.

d9a3cf20a4

test: convert truthy asserts in `wallet_miniscript` and `rpc_psbt`

Replace the last remaining truthy asserts (and a remaining `is False`) in `wallet_miniscript.py` and `rpc_psbt.py` with `assert_equal`.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

06a4176c42

test: add missing `assert_equal` imports

The later scripted diff only rewrites call sites.
Add `assert_equal` imports in the remaining files that still need them so the mechanical replacement can apply cleanly.

301b1d7b1f

scripted-diff: replace remaining Python test equality asserts

Some Python functional tests still use plain `assert x == y`.
The earlier commits convert the ambiguous assert patterns by hand, so this commit can rewrite the remaining safe cases mechanically.
The verify script excludes `wallet_bumpfee.py`, `test_framework/netutil.py`, and `test_framework/authproxy.py`, which still contain assert forms that the plain line-based substitution would misidentify.

-BEGIN VERIFY SCRIPT-
perl -pi -e 's/^(\s*)assert (.+?) == ([^,#]+?)$/\1assert_equal(\2, \3)/' $(git ls-files -- 'test/functional' \
':(exclude)test/functional/wallet_bumpfee.py' ':(exclude)test/functional/test_framework/netutil.py' ':(exclude)test/functional/test_framework/authproxy.py')
-END VERIFY SCRIPT-

3fd68a95e6

l0rinc force-pushed on Apr 7, 2026

maflcko commented at 8:04 AM on April 10, 2026: member

review ACK 3fd68a95e68b4c6f3bb6c59d41dd196001110f3a 🚆

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 3fd68a95e68b4c6f3bb6c59d41dd196001110f3a 🚆
HHRd1sEnE0E5t5t7PO2zbXnf4j5akuLDtwCCisLBUYCPuua7i+w9aak40W02z6p1ogEV7fr7/CCfdruINX4cBg==

</details>

maflcko added this to the milestone 32.0 on Apr 10, 2026

maflcko commented at 8:06 AM on April 10, 2026: member

cc @theStack (you created the issue, so you may be interesting in taking a look here)

rkrux approved

rkrux commented at 9:43 AM on April 10, 2026: contributor

lgtm ACK 3fd68a95e68b4c6f3bb6c59d41dd196001110f3a

There's improved readability in the testing code that's quite visible, specially in the portions where the conditions are longer to read.

It's relatively not easy to raise such clean up PRs, thanks for picking it up & splitting it into multiple commits so that the scripted diff can be applied at the end.

in test/functional/test_framework/authproxy.py:147 in d9a3cf20a4

 143 | @@ -144,7 +144,8 @@ def __call__(self, *args, **argsn):
 144 |              else:
 145 |                  return response['result']
 146 |          else:
 147 | -            assert response['jsonrpc'] == '2.0'
 148 | +            from .util import assert_equal

rkrux commented at 11:29 AM on April 10, 2026:

I don't like the import here but I realise it had to be done because of the circular dependency.

So I raised #35049 to address it because removal of the circular dependency goes outside the scope of this PR.

maflcko commented at 1:25 PM on April 10, 2026:

If the import is a problem then it may be fine to just leave the file as-is for now. But anything is fine here

rkrux commented at 1:38 PM on April 10, 2026:

Yeah, this lazy/deferred import in this PR is fine I guess, but it did highlight the circular dependency that can be addressed separately.

theStack approved

theStack commented at 10:01 PM on April 10, 2026: contributor

ACK 3fd68a95e68b4c6f3bb6c59d41dd196001110f3a

nit: I guess I'm not 100% sold on 06a4176c420c1b469adebcae147694e2f46e6644, especially since it's unclear what this means for the rest of the functional test codebase (see comment), but no reason to delay this change any further

// EDIT: fixed commit reference

maflcko commented at 6:01 AM on April 13, 2026: member

rfm with three acks?

fanquake merged this on Apr 13, 2026

fanquake closed this on Apr 13, 2026

l0rinc deleted the branch on Apr 13, 2026

test: migrate functional test equality asserts to `assert_equal` #34773

Problem

Design

Fix

Reviews

Conflicts

Fix