test: fix constructor of msg_tx #30552

mzumsande · 2024-07-30T21:58:31Z

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

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.

DrahtBot · 2024-07-30T21:58:34Z

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.

sipa · 2024-07-30T22:06:48Z

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

maflcko · 2024-07-31T05:22:41Z

Nice fix

ACK ec5e294

vasild

ACK ec5e294 ❤️

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:

$ git grep 'def .*=.*\(' test/functional/
test/functional/feature_block.py:1329:    def create_tx(self, spend_tx, n, value, script=CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])):
test/functional/feature_block.py:1341:    def create_and_sign_transaction(self, spend_tx, value, script=CScript([OP_TRUE])):
test/functional/feature_block.py:1347:    def next_block(self, number, spend=None, additional_coinbase_value=0, script=CScript([OP_TRUE]), *, version=4):
test/functional/test_framework/blocktools.py:157:def create_tx_with_script(prevtx, n, script_sig=b"", *, amount, script_pub_key=CScript()):
test/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):
test/functional/test_framework/util.py:268:def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0):
test/functional/wallet_backup.py:98:    def start_three(self, args=()):
test/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!

vasild · 2024-07-31T08:01:03Z

test/functional/test_framework/messages.py

+        if tx is None:
+            self.tx = CTransaction()
+        else:
+            self.tx = tx


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.

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.

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

theStack

ACK ec5e294

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. Github-Pull: bitcoin#30552 Rebased-From: ec5e294

fanquake · 2024-07-31T11:06:29Z

Backported to 27.x in #30558.

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. Github-Pull: bitcoin#30552 Rebased-From: ec5e294

b06c4c6 [WIP] doc: update release notes for 27.x (fanquake) 57de0f5 policy/feerate.h: avoid constraint self-dependency (Matt Whitlock) ccff378 add missing #include <cstdint> for GCC 15 (Matt Whitlock) 500bba0 test: fix constructor of msg_tx (Martin Zumsande) Pull request description: Backports: * #30552 * #30633 ACKs for top commit: stickies-v: ACK b06c4c6 Tree-SHA512: 1b669d1c7e0c6c2c2a1b123970c2b5b59a417a423ee1133296ebad2ecb50e5c3889a6ae8dc640f8ae464a969b1b0287a8005a3317ee7d7252b61d96e59c131a4

ec5e294 test: fix constructor of msg_tx (Martin Zumsande) Pull request description: 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 bitcoin#30543, and possibly various other intermittent bugs in the functional tests, see bitcoin#29621 (comment). Fixes bitcoin#30543 Fixes bitcoin#29621 Fixes bitcoin#25128 ACKs for top commit: sipa: utACK ec5e294. I believe some linters even warn about doing this. maflcko: ACK ec5e294 vasild: ACK ec5e294 ❤️ theStack: ACK ec5e294 Tree-SHA512: a6204fb1a326de3f9aa965f345fd658f6a4dcf78731db25cc905ff6eb8d4eeb65d14cc316305eebd89387aec8748c57c3a4f4ca62408f8e5ee53f535b88b1411

b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script) 745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow) 01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow) 432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script) 1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script) 8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script) f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script) ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script) e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script) df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script) 57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script) e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script) 62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script) 745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow) 4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov) 69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script) ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script) 9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow) 479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script) ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script) 63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script) 3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script) 3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b654479 kwvg: utACK b654479 Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043

…ment to follow-up bitcoin#30552

2026c59 perf: re-order functional tests to make slowest one to run faster (Konstantin Akimov) d35ce1e fix: uncomment sethdseed to follow-up for #6017 and bitcoin#12560 (Konstantin Akimov) 71cd68d feat: run rpc_quorum.py with only one type of wallet (Konstantin Akimov) af1923f fix: missing governance.dat, spork.dat do not trigger a loud error message in logs (Konstantin Akimov) 678db6f fix: error in log if no masternode's registered yet "ERROR! Failed to get payees for block at height" (Konstantin Akimov) 80c5481 fix: use unique port for interface_zmq_dash.py - follow-up for bitcoin#26805 (Konstantin Akimov) 87caafb fix: dashify doc/JSON-RPC-interface.md to follow-up bitcoin#27225 (Konstantin Akimov) 237460c fix: initialization of CMerkleBlock() due to misusage of default argument to follow-up bitcoin#30552 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented It fixes several minor issues: - error messages in logs on regtest: [httpworker.0] [masternode/payments.cpp:116] [IsTransactionValid] CMNPaymentsProcessor::IsTransactionValid -- ERROR! Failed to get payees for block at height 110 [httpworker.0] [masternode/payments.cpp:278] [IsBlockPayeeValid] CMNPaymentsProcessor::IsBlockPayeeValid -- Valid masternode payment at height 110: CTransaction(hash=837e35fab5, ver=3, type=5, vin.size=1, vout.size=1, nLockTime=0, vExtraPayload.size=70) CTxIn(COutPoint(0000000000000000000000000000000000000000000000000000000000000000, 4294967295), coinbase 016e0101) CTxOut(nValue=500.00000000, scriptPubKey=76a9148708dff2bf8b31363cb4201d) - error messages for non-existing files for first run: [ init] [util/system.h:57] [error] ERROR: CoreRead: Failed to open file DASH/node0/regtest/governance.dat - non-dashified `bitcoin-cli` in `doc/JSON-RPC-interface.md` - incorrect initialization of `CMerkleBlock` in functional tests which can cause an un-explainable failure - use random ports for `interface_zmq_dash.py` to let tests run simultaneously - fix missing todo about using `sethdseed` after #6017 - optimization of local run of functional tests (slowest go first) ## What was done? See each commit ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes N/A ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2026c59 PastaPastaPasta: utACK 2026c59 Tree-SHA512: 81f7ac0160203bd0a636869402108ca9b94c81aa966263b64ce74cbc69ac6a875ab111cb078a02ddbfd9408caa418c612cb9c7fb9a666786e14095dc378065c4

DrahtBot added the Tests label Jul 30, 2024

mzumsande mentioned this pull request Jul 30, 2024

tests: the python test framework mixes/confuses messages to different peers #30543

Closed

1 task

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

maflcko added this to the 28.0 milestone Jul 31, 2024

vasild approved these changes Jul 31, 2024

View reviewed changes

vasild mentioned this pull request Jul 31, 2024

lint: Find function calls in default arguments #30553

Merged

theStack approved these changes Jul 31, 2024

View reviewed changes

fanquake merged commit be96929 into bitcoin:master Jul 31, 2024
16 checks passed

fanquake mentioned this pull request Jul 31, 2024

[27.x] Even more backports #30558

Merged

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

mzumsande deleted the 202407_test_defaultarg branch July 31, 2024 14:28

knst added a commit to knst/dash that referenced this pull request Oct 29, 2024

fix: initialization of CMerkleBlock() due to misusage of default argu…

f09b915

…ment to follow-up bitcoin#30552

knst added a commit to knst/dash that referenced this pull request Oct 29, 2024

fix: initialization of CMerkleBlock() due to misusage of default argu…

237460c

…ment to follow-up bitcoin#30552

bitcoin locked and limited conversation to collaborators Jul 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: fix constructor of msg_tx #30552

test: fix constructor of msg_tx #30552

Uh oh!

mzumsande commented Jul 30, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Jul 30, 2024 •

edited

Loading

Uh oh!

sipa commented Jul 30, 2024

Uh oh!

maflcko commented Jul 31, 2024

Uh oh!

vasild left a comment

Uh oh!

vasild Jul 31, 2024

Uh oh!

maflcko Jul 31, 2024

Uh oh!

vasild Jul 31, 2024

Uh oh!

theStack left a comment

Uh oh!

Uh oh!

fanquake commented Jul 31, 2024

Uh oh!

Uh oh!

test: fix constructor of msg_tx #30552

test: fix constructor of msg_tx #30552

Uh oh!

Conversation

mzumsande commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

sipa commented Jul 30, 2024

Uh oh!

maflcko commented Jul 31, 2024

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

vasild Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Jul 31, 2024

Uh oh!

Uh oh!

mzumsande commented Jul 30, 2024 •

edited

Loading

DrahtBot commented Jul 30, 2024 •

edited

Loading