test: refactor: support sending funds with outpoint result #28264

theStack commented at 2:18 PM on August 13, 2023: contributor

In wallet-related functional tests we often want to send funds to an address and use the resulting (non-change) UTXO directly after as input for another transaction. Doing that is currently tedious, as it involves finding the index part of the outpoint manually by calling helpers like find_vout_for_address or find_output first. This results in two different txid/vout variables which then again have to be combined to a single dictionary {"txid": ..., "vout": ...} in order to be specified as input for RPCs like createrawtransaction or createpsbt. For example:

txid1 = node1.sendtoaddress(addr1, value1)
vout1 = find_vout_for_address(node1, txid1, addr1)
txid2 = node2.sendtoaddress(addr2, value2)
vout2 = find_vout_for_address(node2, txid2, addr2)
node.createrawtransaction([{'txid': txid1, 'vout': vout1}, {'txid': txid2, 'vout': vout2}], .....)

This PR introduces a helper create_outpoints to immediately return the outpoint as UTXO dictionary in the common format, making the tests more readable and avoiding unnecessary duplication:

utxo1 = self.create_outpoints(node1, outputs=[{addr1: value1}])[0]
utxo2 = self.create_outpoints(node2, outputs=[{addr2: value2}])[0]
node.createrawtransaction([utxo1, utxo2], .....)

Tests are switched to work with UTXO-objects rather than two individual txid/vout variables accordingly.

The find_output helper is removed, as it seems generally a bad idea to search for an outpoint only based on the output value. If that's really ever needed in the future, it makes probably more sense to add it as an additional parameter to find_vout_of_address. Note that find_output supported specifying a block-hash for where to look for the transaction (being passed on to the getrawtransaction RPC). This seems to be unneeded, as txids are always unique and for the only test that used that parameter (rpc_psbt.py) there was no observed difference in run-time, so it was not reintroduced in the new helper.

There are still some find_vout_of_address calls remaining, used for detecting change outputs or for whenever the sending happens via sendrawtransaction instead, so this PR tackles not all, but the most common case.

DrahtBot commented at 2:18 PM on August 13, 2023: 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	BrandonOdiwuor, maflcko, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28528 (test: Use test framework utils in functional tests by osagie98)
#24142 (Deprecate SubtractFeeFromOutputs by achow101)

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.

DrahtBot added the label Tests on Aug 13, 2023

in test/functional/test_framework/util.py:541 in 1c21e51a8a outdated

 535 | @@ -548,3 +536,13 @@ def find_vout_for_address(node, txid, addr):
 536 |          if addr == tx["vout"][i]["scriptPubKey"]["address"]:
 537 |              return i
 538 |      raise RuntimeError("Vout not found for address: txid=%s, addr=%s" % (txid, addr))
 539 | +
 540 | +
 541 | +def sendtoaddress_and_get_outpoint(node, address, amount):

maflcko commented at 9:28 AM on August 14, 2023:

def create_outpoints(*, node, outputs):

Maybe take and return an array? Also, could force named args? Finally, could make this a member of TestFramework or TestNode? (It is not an independent util, but requires a node/wallet, ...)

theStack commented at 11:44 AM on August 14, 2023:

SGTM to make it a more generic helper that supports multiple outputs / returns multiple UTXOs (need to use send instead of sendtoaddress, but that shouldn't matter). With that, even more instances of find_vout_address can be replaced. Moving the helper to TestNode wouldn't be a good fit, as the outpoints creations not only operate on TestNode objects, but in some instances also on RPCOverloadWrappers (result from get_wallet_rpc). Moving it to TestFramework should work though.

theStack force-pushed on Aug 14, 2023

theStack commented at 12:26 PM on August 14, 2023: contributor

Force-pushed with the feedback #28264 (review) tackled and adapted the PR description accordingly.

in test/functional/rpc_psbt.py:416 in 63eacf62ac outdated

 416 | -        txid2 = self.nodes[0].sendtoaddress(node2_addr, 13)
 417 | -        blockhash = self.generate(self.nodes[0], 6)[0]
 418 | -        vout1 = find_output(self.nodes[1], txid1, 13, blockhash=blockhash)
 419 | -        vout2 = find_output(self.nodes[2], txid2, 13, blockhash=blockhash)
 420 | +        utxo1 = self.create_outpoints(node=self.nodes[0], outputs=[{node1_addr: 13}])[0]
 421 | +        utxo2 = self.create_outpoints(node=self.nodes[0], outputs=[{node2_addr: 13}])[0]

maflcko commented at 3:41 PM on August 14, 2023:

        utxos = self.create_outpoints(self.nodes[0], outputs=[{node1_addr: 13},{node2_addr: 13},
        ])

theStack commented at 6:19 PM on August 14, 2023:

Creating two different txs is intentional here, as otherwise the "Update psbts, should only have data for one input and not the other" test below would fail (if both inputs are part of the same tx, all data is available to both nodes). Added a comment to clarify.

in test/functional/test_framework/test_framework.py:676 in 63eacf62ac outdated

 672 | @@ -672,6 +673,22 @@ def generatetodescriptor(self, generator, *args, sync_fun=None, **kwargs):
 673 |          sync_fun() if sync_fun else self.sync_all()
 674 |          return blocks
 675 |  
 676 | +    def create_outpoints(self, *, node, outputs):

maflcko commented at 3:43 PM on August 14, 2023:

    def create_outpoints(self, node, *, outputs):

nit: I guess it is also fine to omit the named arg for node.

theStack commented at 6:18 PM on August 14, 2023:

Thanks, done.

maflcko approved

maflcko commented at 3:46 PM on August 14, 2023: member

lgtm, two nits only

DrahtBot added the label Needs rebase on Aug 14, 2023

theStack force-pushed on Aug 14, 2023

theStack commented at 6:20 PM on August 14, 2023: contributor

Rebased on master (tiny merge conflict due to #28232) and tackled the comments #28264 (review) and #28264 (review).

DrahtBot removed the label Needs rebase on Aug 14, 2023

in test/functional/test_framework/test_framework.py:701 in f1d685b5e5 outdated

 672 | @@ -672,6 +673,22 @@ def generatetodescriptor(self, generator, *args, sync_fun=None, **kwargs):
 673 |          sync_fun() if sync_fun else self.sync_all()
 674 |          return blocks
 675 |  
 676 | +    def create_outpoints(self, node, *, outputs):

BrandonOdiwuor commented at 10:38 AM on August 18, 2023:

Review ACK 7154542

The proposed refactor is a commendable improvement, streamlining test cases by leveraging the create_outpoints(...) function to consolidate calls to nodes.sendtoaddress() and find_vout_for_address() into a single invocation.

Suggestion: It might be beneficial to add documentation clarifying that the outputs argument should be an array of dictionaries following the format {address: amount_to_send}. This would enhance usability for future users, eliminating the need to inspect example invocations.

theStack commented at 4:17 PM on October 20, 2023:

Suggestion: It might be beneficial to add documentation clarifying that the outputs argument should be an array of dictionaries following the format {address: amount_to_send}. This would enhance usability for future users, eliminating the need to inspect example invocations.

Good idea. Made the following (simple) change, I hope that's clear enough for users:

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 045fd2f5cb..70b3943478 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -699,7 +699,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         return blocks

     def create_outpoints(self, node, *, outputs):
-        """Send funds to a given list of address/amount targets using the bitcoind
+        """Send funds to a given list of `{address: amount}` targets using the bitcoind
         wallet and return the corresponding outpoints as a list of dictionaries
         `[{"txid": txid, "vout": vout1}, {"txid": txid, "vout": vout2}, ...]`.
         The result can be used to specify inputs for RPCs like `createrawtransaction`,

DrahtBot added the label CI failed on Sep 4, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

maflcko commented at 9:22 AM on October 20, 2023: member

Are you still working on this?

theStack commented at 11:54 AM on October 20, 2023: contributor

Are you still working on this?

I considered the PR ready since my latest force-push weeks ago (https://github.com/bitcoin/bitcoin/pull/28264#issuecomment-1677849699). However, will tackle the suggestion in #28264 (review) though, I must have missed that last paragraph back then.

theStack force-pushed on Oct 20, 2023

theStack commented at 4:17 PM on October 20, 2023: contributor

Rebased on master and tackled #28264 (review).

DrahtBot added the label CI failed on Oct 20, 2023

DrahtBot added the label Needs rebase on Oct 23, 2023

test: refactor: support sending funds with outpoint result

This commit introduces a helper `create_outpoints` to execute the
`send` RPC and immediately return the target address outpoints as UTXO
dictionary in the common format, making the tests more readable and
avoiding unnecessary duplication.

73a339abc3

test: remove unused `find_output` helper 50d1ac1207

theStack force-pushed on Oct 24, 2023

DrahtBot removed the label Needs rebase on Oct 24, 2023

theStack commented at 11:18 AM on October 24, 2023: contributor

Rebased on master again (due to conflict after #28609 has been merged).

BrandonOdiwuor approved

BrandonOdiwuor commented at 3:51 PM on October 24, 2023: contributor

ACK 50d1ac120716ab17f32b28513c0ac9940001a783

maflcko commented at 8:14 AM on October 25, 2023: member

ACK 50d1ac120716ab17f32b28513c0ac9940001a783 🖨

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 50d1ac120716ab17f32b28513c0ac9940001a783 🖨
f/xrAIdGqM2CyeNbY9XT0oGt51+NcrDKS00Nej69S1xkQelpkubslZL0jaHX+RUdivnyh2xEo3bJJ/00I7AsDQ==

</details>

maflcko assigned achow101 on Oct 25, 2023

achow101 commented at 2:56 PM on October 25, 2023: member

ACK 50d1ac120716ab17f32b28513c0ac9940001a783

achow101 merged this on Oct 25, 2023

achow101 closed this on Oct 25, 2023

theStack deleted the branch on Oct 25, 2023

luke-jr referenced this in commit 9fa5635d0b on May 3, 2024

luke-jr referenced this in commit 539b0cb3db on May 6, 2024

bitcoin locked this on Oct 24, 2024