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

theStack · 2023-08-13T14:18:55Z

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 · 2023-08-13T14:18:57Z

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.

test/functional/test_framework/util.py

theStack · 2023-08-14T12:26:32Z

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

maflcko

lgtm, two nits only

test/functional/rpc_psbt.py

test/functional/test_framework/test_framework.py

theStack · 2023-08-14T18:20:21Z

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

BrandonOdiwuor · 2023-08-18T10:38:13Z

test/functional/test_framework/test_framework.py

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.

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`,

maflcko · 2023-10-20T09:22:40Z

Are you still working on this?

theStack · 2023-10-20T11:54:03Z

Are you still working on this?

I considered the PR ready since my latest force-push weeks ago (#28264 (comment)). However, will tackle the suggestion in #28264 (comment) though, I must have missed that last paragraph back then.

theStack · 2023-10-20T16:17:21Z

Rebased on master and tackled #28264 (comment).

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.

theStack · 2023-10-24T11:18:12Z

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

BrandonOdiwuor

ACK 50d1ac1

maflcko

ACK 50d1ac1 🖨

Show signature

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==

achow101 · 2023-10-25T14:56:32Z

ACK 50d1ac1

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. Github-Pull: bitcoin#28264 Partial-Rebased-From: 73a339a

DrahtBot added the Tests label Aug 13, 2023

DrahtBot mentioned this pull request Aug 14, 2023

test: locked_wallet, skip default fee estimation #28232

Merged

maflcko reviewed Aug 14, 2023

View reviewed changes

test/functional/test_framework/util.py Outdated Show resolved Hide resolved

theStack force-pushed the test-add_sendtoaddress_helper_returning_utxo branch from 1c21e51 to 63eacf6 Compare August 14, 2023 12:21

DrahtBot mentioned this pull request Aug 14, 2023

Support JSON-RPC 2.0 when requested by client #27101

Merged

maflcko approved these changes Aug 14, 2023

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 14, 2023

theStack force-pushed the test-add_sendtoaddress_helper_returning_utxo branch from 63eacf6 to f1d685b Compare August 14, 2023 18:17

DrahtBot removed the Needs rebase label Aug 14, 2023

DrahtBot mentioned this pull request Aug 14, 2023

Implement BIP 370 PSBTv2 #21283

Open

BrandonOdiwuor reviewed Aug 18, 2023

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Sep 4, 2023

This was referenced Sep 25, 2023

test: Use test framework utils in functional tests #28528

Closed

Deprecate SubtractFeeFromOutputs #24142

Closed

DrahtBot mentioned this pull request Oct 7, 2023

wallet: Reload watchonly and solvables wallets after migration #28609

Merged

theStack force-pushed the test-add_sendtoaddress_helper_returning_utxo branch from f1d685b to 4bec09e Compare October 20, 2023 16:15

DrahtBot added CI failed Needs rebase labels Oct 20, 2023

theStack added 2 commits October 24, 2023 11:13

test: remove unused find_output helper

50d1ac1

theStack force-pushed the test-add_sendtoaddress_helper_returning_utxo branch from 4bec09e to 50d1ac1 Compare October 24, 2023 09:14

DrahtBot removed the Needs rebase label Oct 24, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

BrandonOdiwuor approved these changes Oct 24, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 25, 2023

test: replace random_bytes with random.randbytes #28727

Merged

maflcko reviewed Oct 25, 2023

View reviewed changes

maflcko assigned achow101 Oct 25, 2023

achow101 merged commit 2a349f9 into bitcoin:master Oct 25, 2023

theStack deleted the test-add_sendtoaddress_helper_returning_utxo branch October 25, 2023 15:14

bitcoin locked and limited conversation to collaborators Oct 24, 2024

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

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

Uh oh!

Conversation

theStack commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

theStack commented Aug 14, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented Aug 14, 2023

Uh oh!

BrandonOdiwuor Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

theStack Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 20, 2023

Uh oh!

theStack commented Oct 20, 2023

Uh oh!

theStack commented Oct 20, 2023

Uh oh!

theStack commented Oct 24, 2023

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

theStack commented Aug 13, 2023 •

edited

Loading

DrahtBot commented Aug 13, 2023 •

edited

Loading