test: Replace hard-coded hex tx with class in test framework #15282

stevenroose commented at 10:43 AM on January 29, 2019: contributor

Came across these breaking Elements.

fanquake added the label Tests on Jan 29, 2019

DrahtBot commented at 1:04 PM on January 29, 2019: member

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

Conflicts

No conflicts as of last run.

MarcoFalke commented at 4:01 PM on January 29, 2019: member

This is used in some "hot loops", so making a benchmark to prove it is not significantly slowing down the function call would help.

stevenroose commented at 10:20 PM on January 29, 2019: contributor

I don't know if this helps:

$ cat exec.sh 
#!/bin/sh
./test/functional/mempool_limit.py && ./test/functional/mining_prioritisetransaction.py 
$ time ./exec.sh 
2019-01-29T22:22:07.529000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_vn7gv6oi
2019-01-29T22:22:07.814000Z TestFramework (INFO): Check that mempoolminfee is minrelytxfee
2019-01-29T22:22:08.241000Z TestFramework (INFO): Create a mempool tx that will be evicted
2019-01-29T22:22:09.816000Z TestFramework (INFO): The tx should be evicted by now
2019-01-29T22:22:09.829000Z TestFramework (INFO): Check that mempoolminfee is larger than minrelytxfee
2019-01-29T22:22:09.834000Z TestFramework (INFO): Create a mempool tx that will not pass mempoolminfee
2019-01-29T22:22:09.906000Z TestFramework (INFO): Stopping nodes
2019-01-29T22:22:10.110000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_vn7gv6oi on exit
2019-01-29T22:22:10.110000Z TestFramework (INFO): Tests successful
2019-01-29T22:22:10.215000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_5zgic6c6
2019-01-29T22:22:12.664000Z TestFramework (INFO): Assert that prioritised transaction was mined
2019-01-29T22:22:12.797000Z TestFramework (INFO): Assert that de-prioritised transaction is still in mempool
2019-01-29T22:22:12.905000Z TestFramework (INFO): Assert that prioritised free transaction is accepted to mempool
2019-01-29T22:22:13.070000Z TestFramework (INFO): Stopping nodes
2019-01-29T22:22:13.372000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_5zgic6c6 on exit
2019-01-29T22:22:13.372000Z TestFramework (INFO): Tests successful
./exec.sh  3.88s user 0.49s system 72% cpu 5.997 total
$ gco master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
$ time ./exec.sh
2019-01-29T22:22:22.500000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_c_ph_h1g
2019-01-29T22:22:22.772000Z TestFramework (INFO): Check that mempoolminfee is minrelytxfee
2019-01-29T22:22:23.222000Z TestFramework (INFO): Create a mempool tx that will be evicted
2019-01-29T22:22:24.519000Z TestFramework (INFO): The tx should be evicted by now
2019-01-29T22:22:24.521000Z TestFramework (INFO): Check that mempoolminfee is larger than minrelytxfee
2019-01-29T22:22:24.522000Z TestFramework (INFO): Create a mempool tx that will not pass mempoolminfee
2019-01-29T22:22:24.578000Z TestFramework (INFO): Stopping nodes
2019-01-29T22:22:24.831000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_c_ph_h1g on exit
2019-01-29T22:22:24.831000Z TestFramework (INFO): Tests successful
2019-01-29T22:22:24.947000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nf8kqb1j
2019-01-29T22:22:27.376000Z TestFramework (INFO): Assert that prioritised transaction was mined
2019-01-29T22:22:27.520000Z TestFramework (INFO): Assert that de-prioritised transaction is still in mempool
2019-01-29T22:22:27.600000Z TestFramework (INFO): Assert that prioritised free transaction is accepted to mempool
2019-01-29T22:22:27.752000Z TestFramework (INFO): Stopping nodes
2019-01-29T22:22:27.905000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_nf8kqb1j on exit
2019-01-29T22:22:27.905000Z TestFramework (INFO): Tests successful
./exec.sh  3.70s user 0.51s system 75% cpu 5.546 total

0.18 seconds increase over the 3.7s original runtime of the two tests using the method. That's 4.86% increase (over those few seconds it's used).

laanwj renamed this:
~~TEST: Replace hard-coded hex tx with class in test framework~~
test: Replace hard-coded hex tx with class in test framework
on Jan 30, 2019

instagibbs commented at 3:15 PM on February 13, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15282/commits/03f7a8f392c0f5574ca2dbc19e7a16d32dc97312

Extremely minor slowdown imo for great increase in test clarity.

in test/functional/test_framework/util.py:523 in 03f7a8f392 outdated

 525 | -        txouts = txouts + "0000000000000000"
 526 | -        # add length of script_pubkey
 527 | -        txouts = txouts + "fd0402"
 528 | -        # add script_pubkey
 529 | -        txouts = txouts + script_pubkey
 530 | +        txout = CTxOut()

instagibbs commented at 3:17 PM on February 13, 2019:

You could take out everything from this hot loop except the txtouts.append line to see if we gain back that 0.18 secs?

stevenroose commented at 12:04 PM on February 14, 2019:

Strangely enough this made it 0.2 seconds slower on my machine :/ So I think it's better as-is.

stevenroose commented at 12:06 PM on February 14, 2019: contributor

It seems like #14691 is making this change obsolete. Not sure if that one will land, though.

laanwj commented at 9:52 AM on February 21, 2019: member

@MarcoFalke given the benchmarks, are you ok with this ?

MarcoFalke commented at 2:08 PM on February 21, 2019: member

Sure but there is also #14691 (tests: Speedup feature_pruning test and refactor big transaction logic by @conscott) which look promising

DrahtBot added the label Needs rebase on Mar 18, 2019

MarcoFalke commented at 10:57 AM on May 24, 2019: member

@stevenroose Is this still relevant?

fanquake commented at 1:25 AM on June 19, 2019: member

Ping @stevenroose.

stevenroose commented at 12:05 PM on July 1, 2019: contributor

Let me try rebase this.

TEST: Replace hard-coded hex tx with classes 8f250ab788

stevenroose force-pushed on Jul 1, 2019

DrahtBot removed the label Needs rebase on Jul 1, 2019

stevenroose commented at 8:59 PM on July 3, 2019: contributor

@fanquake @MarcoFalke rebased. Up to you guys whether it's relevant, of course. I think it's an improvement.

MarcoFalke commented at 9:35 PM on July 8, 2019: member

ACK 8f250ab7882a852f1b1947cef4837d2de5ca6913

practicalswift commented at 4:06 PM on July 10, 2019: contributor

Concept ACK

in test/functional/test_framework/util.py:542 in 8f250ab788

 547 |          rawtx = node.createrawtransaction(inputs, outputs)
 548 | -        newtx = rawtx[0:92]
 549 | -        newtx = newtx + txouts
 550 | -        newtx = newtx + rawtx[94:]
 551 | +        tx = CTransaction()
 552 | +        tx.deserialize(BytesIO(hex_str_to_bytes(rawtx)))

instagibbs commented at 4:11 PM on July 10, 2019:

please don't let this block merging: I think you can use FromHex(CTransaction(), rawtx) here for a short one-liner

instagibbs commented at 4:12 PM on July 10, 2019: member

utACK https://github.com/bitcoin/bitcoin/commit/8f250ab7882a852f1b1947cef4837d2de5ca6913

MarcoFalke merged this on Jul 15, 2019

MarcoFalke closed this on Jul 15, 2019

MarcoFalke referenced this in commit 0822b44d8a on Jul 15, 2019

sidhujag referenced this in commit 06fb3b1b9e on Jul 29, 2019

jasonbcox referenced this in commit f80a0bc3a9 on Nov 13, 2020

PastaPastaPasta referenced this in commit 2232846d05 on Jun 27, 2021

PastaPastaPasta referenced this in commit c4e51b7314 on Jun 28, 2021

PastaPastaPasta referenced this in commit 6ad46e6d64 on Jun 29, 2021

PastaPastaPasta referenced this in commit 5231450d89 on Jul 1, 2021

PastaPastaPasta referenced this in commit 3a82017a91 on Jul 1, 2021

PastaPastaPasta referenced this in commit 31f51427b5 on Jul 12, 2021

PastaPastaPasta referenced this in commit 6e626934af on Jul 13, 2021

DrahtBot locked this on Dec 16, 2021