test: fix constructor of msg

mzumsande commented at 9:58 pm on July 30, 2024: contributor

In python, if the default value is a mutable object (here: a class) it is shared over all instances, so that one instance being changed would affect others to be changed as well. This was the source of #30543, and possibly various other intermittent bugs in the functional tests, see #29621 (comment).

Fixes #30543 Fixes #29621 Fixes #25128

test: fix constructor of msg_tx

In python, if the default value is a mutable object (here: a class)
its shared over all instances, so that one instance being changed
would affect others to be changed as well.
This was likely the source of various intermittent bugs in the
functional tests.

ec5e294e4b

DrahtBot commented at 9:58 pm on July 30, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, maflcko, vasild, theStack

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

DrahtBot added the label Tests on Jul 30, 2024

sipa commented at 10:06 pm on July 30, 2024: member

utACK ec5e294e4b830766dcc4a80add0613d3705c1794. I believe some linters even warn about doing this.

maflcko added the label Needs backport (27.x) on Jul 31, 2024

maflcko added this to the milestone 28.0 on Jul 31, 2024

maflcko commented at 5:22 am on July 31, 2024: member

Nice fix

ACK ec5e294e4b830766dcc4a80add0613d3705c1794

in test/functional/test_framework/messages.py:1301 in ec5e294e4b

1298-        self.tx = tx
1299+    def __init__(self, tx=None):
1300+        if tx is None:
1301+            self.tx = CTransaction()
1302+        else:
1303+            self.tx = tx

vasild commented at 8:01 am on July 31, 2024:

Maybe a comment is warranted so that in the future somebody does not “simplify” it by reverting this patch. Maybe not necessary if #30553 enforces it.

maflcko commented at 8:16 am on July 31, 2024:

Maybe a comment is warranted so that in the future somebody does not “simplify” it by reverting this patch. Maybe not necessary if #30553 enforces it.

There are many other places where this approach is used, and I don’t think it makes sense to document all of them. It could make sense to document it in the python style guide of this project, but I’d be surprised if someone reads it and consistently enforces the rule during review. Also, it seems not a good use of reviewers time to enforce this.

My preference would be to just wait for 30553.

vasild commented at 8:44 am on July 31, 2024:

Right, #30553 is much better by a comment :)

vasild approved

vasild commented at 8:02 am on July 31, 2024: contributor

ACK ec5e294e4b830766dcc4a80add0613d3705c1794 :heart:

Amazing turnaround! I was stuck for quite some time at this before reporting it at #30543 and this quick fix is a blessing!

I confirm the test p2p_ttt.py from that issue passes with this fix even if I change the loop to iterate 100 times instead of 10.

Some other similar calls may remain, a quick grep:

0$ git grep 'def .*=.*\(' test/functional/
1test/functional/feature_block.py:1329:    def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])):
2test/functional/feature_block.py:1341:    def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
3test/functional/feature_block.py:1347:    def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4):
4test/functional/test_framework/blocktools.py:157:def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
5test/functional/test_framework/script.py:813:def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT):
6test/functional/test_framework/util.py:268:def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
7test/functional/wallet_backup.py:98:    def start_three(self, args=()):
8test/functional/wallet_bumpfee.py:780:def spend_one_input(node, dest_address, change_size=Decimal("0.00049000"), data=None):

Hopefully #30553 will catch all and prevent further additions.

Thank you!

theStack approved

theStack commented at 8:48 am on July 31, 2024: contributor

ACK ec5e294e4b830766dcc4a80add0613d3705c1794

fanquake merged this on Jul 31, 2024

fanquake closed this on Jul 31, 2024

fanquake referenced this in commit 500bba0561 on Jul 31, 2024

fanquake removed the label Needs backport (27.x) on Jul 31, 2024

fanquake commented at 11:06 am on July 31, 2024: member

Backported to 27.x in #30558.

mzumsande deleted the branch on Jul 31, 2024

test: fix constructor of msg_tx #30552

Code Coverage

Reviews