Tests: Cleanup create_transaction implementations #13669

pull conscott wants to merge 3 commits into bitcoin:master from conscott:create_tx_cleanup changing 13 files +112 −137

conscott commented at 11:21 AM on July 15, 2018: contributor
There currently exist seven (1, 2 3, 4, 5, 6, 7) implementations of a function called something similar to create_transaction in the functional tests, some of which are exact copies of each other.

This PR aims to clean this up into three different cases implemented in blocktools.py
1. create_tx_with_script: Return transaction object spending generic tx output optionally specifying scriptSig and scriptPubKey
2. create_transaction: Return transaction object spending coinbase tx
3. create_raw_transaction: Return raw transaction (hex string) spending coinbase tx
I am not committed to any of these function names, so I'll gladly take suggestions on there.

Additionally there are some related cleanups to feature_block.py tests, specifically removing the PreviousSpendableOutput object, which seems like an unnecessary layer given that every instance spends the 0 output.
fanquake added the label Tests on Jul 15, 2018
conscott force-pushed on Jul 15, 2018
fanquake requested review from MarcoFalke on Jul 15, 2018
fanquake requested review from jnewbery on Jul 15, 2018
DrahtBot commented at 11:19 AM on July 16, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13054 (tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. by practicalswift)
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.
jnewbery commented at 9:57 PM on July 26, 2018: member

Concept ACK. I intend to review this.

My personal preference would be to split this into smaller commits (eg one just removing the PreviousSpendableOutput object, and then one each for the three new functions if possible). That would make it a bit easier to review.
conscott commented at 2:32 PM on July 28, 2018: contributor

Thanks @jnewbery - Agreed. I'll try to split this up better.
conscott force-pushed on Jul 31, 2018
conscott commented at 8:25 AM on July 31, 2018: contributor

I have split this up into 3 commits now.
in test/functional/test_framework/blocktools.py:121 in 83e35dae61 outdated
```
 117 | @@ -117,17 +118,36 @@ def create_coinbase(height, pubkey=None):
 118 |      coinbase.calc_sha256()
 119 |      return coinbase
 120 |  
 121 | -def create_transaction(prevtx, n, sig, value, script_pub_key=CScript()):
 122 | -    """Create a transaction.
 123 | +def create_tx_with_script(prevtx, n, amount, script_sig=b"", script_pub_key=CScript()):
```
jnewbery commented at 2:34 PM on July 31, 2018:

I actually prefer the argument ordering before (ie all TxIn fields (prevtx, n, script_sig) then all TxOut fields (amount, script_pub_key)). I like the renames (sig => script_sig and value => amount).

conscott commented at 10:36 AM on August 9, 2018:

:+1: - you are right, it makes more sense that way now that you mention it.

in test/functional/test_framework/blocktools.py:122 in 83e35dae61 outdated

 117 | @@ -117,17 +118,36 @@ def create_coinbase(height, pubkey=None):
 118 |      coinbase.calc_sha256()
 119 |      return coinbase
 120 |  
 121 | -def create_transaction(prevtx, n, sig, value, script_pub_key=CScript()):
 122 | -    """Create a transaction.
 123 | +def create_tx_with_script(prevtx, n, amount, script_sig=b"", script_pub_key=CScript()):
 124 | +    """Return a transaction object spending a previous output.

jnewbery commented at 2:36 PM on July 31, 2018:

Thanks for improving the comment. Can be improved further by stating that this is a one-input, one-output transaction.

in test/functional/test_framework/blocktools.py:142 in 83e35dae61 outdated
```
 142 | +    tx = CTransaction()
 143 | +    tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx)))
 144 | +    return tx
 145 | +
 146 | +def create_raw_transaction(node, coinbase_txid, to_address, amount):
 147 | +    """ Return raw signed transaction spending a coinbase tx
```
jnewbery commented at 2:52 PM on July 31, 2018:

nit: comment could be improved by:

saying this is spending the first output of transaction txid (I don't see why this function is specific to spending a coinbase tx).

stating that node must be able to sign for the txout provided

stating that the node must not be running multiple wallets
jnewbery commented at 3:03 PM on July 31, 2018: member

Tested ACK 83e35dae61f3c41d7adf8b00d6120a98fea5ca79. Nice tidyup!

A few nits inline. Up to you whether you want to take them.
[Tests] Rename create_tx and move to blocktools.py 157651855f
[Tests] Cleanup extra instances of create_transaction 736f941424
[Tests] Cleanup feature_block.py, remove unnecessary PreviousSpendableOutput object 44bbceeef1
conscott force-pushed on Aug 9, 2018
conscott force-pushed on Aug 9, 2018
conscott commented at 12:01 PM on August 9, 2018: contributor

Rebased on updated based on @jnewbery comments
jnewbery commented at 3:51 PM on August 9, 2018: member

utACK 44bbceeef1b76526b73cbdd3ff785c4d5b2c74f1

This is great. Cuts down on a lot of code duplication and improves commenting. :100:
MarcoFalke commented at 4:09 PM on August 9, 2018: member

utACK 44bbceeef1b76526b73cbdd3ff785c4d5b2c74f1
MarcoFalke merged this on Aug 9, 2018
MarcoFalke closed this on Aug 9, 2018
MarcoFalke referenced this in commit f66e1c793e on Aug 9, 2018
conscott deleted the branch on Aug 10, 2018
ken2812221 referenced this in commit b8eb0dfde4 on Aug 13, 2018
UdjinM6 referenced this in commit 9c066bcd37 on Jul 4, 2021
UdjinM6 referenced this in commit bf49749700 on Jul 6, 2021
UdjinM6 referenced this in commit 20e1f4a8ef on Jul 6, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Review Requested
MarcoFalke

Labels

Tests

Linked (view graph)

#13928 qa: blocktools enforce named args for amount