test: refactor: dedup utility function chain

theStack commented at 4:28 PM on June 2, 2021: member

Both tests mempool_packages.py and mempool_package_onemore.py define a utility function chain_transaction with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties:

pass a list of parent_txids/vouts instead of single values
always mark the BIP125-replaceable flag for txs, created via createrawtransaction (this is needed by the mempool_package_onemore.py test, but doesn't hurt the other one)

This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function chain_transaction in rpc_packages.py, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx.

theStack force-pushed on Jun 2, 2021

DrahtBot added the label Tests on Jun 2, 2021

fanquake commented at 3:40 AM on June 3, 2021: member

Concept ACK

brunoerg commented at 11:19 AM on June 3, 2021: member

Concept ACK

in test/functional/mempool_package_onemore.py:28 in d4d8c0bb0c outdated

  27 | @@ -24,23 +28,6 @@ def set_test_params(self):
  28 |      def skip_test_if_missing_module(self):
  29 |          self.skip_if_no_wallet()
  30 |  
  31 | -    # Build a transaction that spends parent_txid:vout
  32 | -    # Return amount sent

mjdietzx commented at 1:52 PM on June 3, 2021:

I'm a little confused bc it seems like this function is not returning the amount sent, but is returning the amount sent for a single output of the tx. So really the amount sent in this transaction is send_value * num_outputs right?

It doesn't seem like the existing tests rely on this either way (for the most part, and when it does num_outputs is usually 1 so it doesn't really matter), but maybe this function should return the full amount sent in the tx so it is more aligned with the docs and what the you'd expect?

Or if this behavior really is wanted maybe this is a good time to improve the function's comment

theStack commented at 5:18 PM on June 5, 2021:

Thanks for reviewing, good point! From what I see in the test mempool_packages.py, the returned amount per output is currently expected (e.g. at one place chain_transaction is called with 10 outputs and then the transactions package is constructed with the returned txid and amount per output). I updated the documentation accordingly, hopefully increasing the clarity.

theStack commented at 5:11 PM on June 5, 2021: member

Pushed another commit that improves the documentation of chain_transaction by changing to docstring format and describing its purpose and return value in more detail (thanks to @mjdietzx for reviewing: #22130 (review)).

theStack force-pushed on Jun 5, 2021

mjdietzx commented at 3:59 PM on June 7, 2021: contributor

crACK 489860e2997c9568bba1f214325b3abba3097f74

test: refactor: dedup utility function chain_transaction() 6e63e366d6

test: doc: improve doc for chain_transaction() helper

Change to docstring format and describe the functions
purpose, its parameters and return value in more detail.

01eedf3821

in test/functional/test_framework/util.py:484 in 489860e299 outdated

 479 | @@ -480,6 +480,28 @@ def create_confirmed_utxos(fee, node, count):
 480 |      return utxos
 481 |  
 482 |  
 483 | +def chain_transaction(node, parent_txids, vouts, value, fee, num_outputs):

mjdietzx commented at 4:12 PM on June 7, 2021:

Kind of a nit, but what are you thoughts on taking utxos as an input param instead of both parent_txids and vouts? Then you can get rid of:

for (txid, vout) in zip(parent_txids, vouts):
  inputs.append({'txid' : txid, 'vout' : vout})

as utxos would be of the form: [{ 'txid': '9d5b026e3dfe0d6c9ac95fc21d7f5802e046d85066c6df92fdb407471ace3b0c', 'vout': 0 }, ...]

To me it seems like this slightly simplifies this function, and is a bit more understandable. And it also probably simplifies the calling code a bit as it seems a lot of times you're parsing parent_txids and vouts from utxos before you call

I know it diverges a bit from existing code, but maybe it could be a refactor commit if this is a good time to do it. More of a nit to me

theStack commented at 3:27 PM on June 8, 2021:

Thanks for the suggestion, I think taking utxos as an input would be a good idea, leading to a cleaner interface and also improving readability. Right now though, at many places it would also need to introduce longer code in test, due to the need to repeatedly craft together the utxo dictionary objects.

Probably this is a good candidate for a follow-up PR that also takes care of the return value? I think the same utxo structure you suggested could then also be used as returned type. I.e. an utxo object created by chain_transaction could be directly fed as new input utxo in places where it is called in loops. Another idea would be to drop the amount parameter and simply always spend the sum of all utxo amounts (didn't check though yet if there is a place where less is spent).

mjdietzx commented at 3:35 PM on June 8, 2021:

Ok, agreed that this should be a follow up PR if anything. I'll circle back around to this some time after this PR is merged, and if no one has looked at it then I may take a whack at it

theStack force-pushed on Jun 8, 2021

theStack commented at 3:28 PM on June 8, 2021: member

Rebased on master.

mjdietzx commented at 3:35 PM on June 8, 2021: contributor

reACK 01eedf3821f2c3ee6bab8733f8549531c844add7

klementtan approved

klementtan commented at 2:24 PM on June 13, 2021: contributor

Code review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7

theStack commented at 4:03 PM on June 13, 2021: member

@klementtan: thanks for reviewing! It seems like the commit-hash you ACKed doesn't belong to this PR?

klementtan commented at 4:12 PM on June 13, 2021: contributor

@klementtan: thanks for reviewing! It seems like the commit-hash you ACKed doesn't belong to this PR?

My bad had a typo mistake 😅. Updated the commit

MarcoFalke approved

MarcoFalke commented at 4:27 PM on June 13, 2021: member

review ACK 01eedf3821f2c3ee6bab8733f8549531c844add7 🙅

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash bca0c7721ac2c95c57f0730aa4ba5b4f26eb4db6c5381094cff249c45101d85a -

</details>

MarcoFalke merged this on Jun 13, 2021

MarcoFalke closed this on Jun 13, 2021

sidhujag referenced this in commit d318d88b2c on Jun 13, 2021

theStack deleted the branch on Jul 31, 2021

gwillen referenced this in commit be04e946b9 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: refactor: dedup utility function chain_transaction() #22130