test: use MiniWallet for mempool_package_onemore.py by theStack · Pull Request #24637 · bitcoin/bitcoin

theStack · 2022-03-22T11:20:08Z

This PR enables one more of the non-wallet functional tests (mempool_package_onemore.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078. For this purpose helper methods MiniWallet.{create,send}_self_transfer_multi are introduced which serve as a replacement for chain_transaction. With this, it should be also quite straight-forward to change the larger related test mempool_packages.py to use MiniWallet.

maflcko

Concept ACK. Some questions/thoughts.

test/functional/test_framework/wallet.py

maflcko

Left some more nits 😅

maflcko · 2022-03-22T13:57:54Z

test/functional/test_framework/wallet.py

nit: Is this needed? The next line will assert even without this, but I can see the reason for a slightly better error message.

maflcko · 2022-03-22T13:58:53Z

test/functional/test_framework/wallet.py

nit: Maybe wrap all of this into a create_self_transfer_multi helper for symmetry with the existing helpers?

maflcko · 2022-03-22T14:00:18Z

test/functional/test_framework/wallet.py

nit: (Some renames for symmetry)

Suggested change

def chain_tx(self, node, utxos, num_outputs=1, fee_per_output=1000):

def send_self_transfer_multi(self, node_from, utxos, *, num_outputs=1, fee_per_output=1000):

theStack · 2022-03-22T14:40:14Z

Thanks again for the valuable feedback @MarcoFalke! Force-pushed with the following suggested changes:

removed the assertion for non-empty UTXO list (test: use MiniWallet for mempool_package_onemore.py #24637 (comment))
renamed helper method and split it up into create_self_transfer_multi/send_self_transfer_multi for the symmetry with existing helpers (test: use MiniWallet for mempool_package_onemore.py #24637 (comment) and test: use MiniWallet for mempool_package_onemore.py #24637 (comment))

The PR description was also adapted accordingly.

maflcko

(feel free to ignore)

maflcko · 2022-03-22T16:00:13Z

test/functional/test_framework/wallet.py

nit

Suggested change

def create_self_transfer_multi(self, from_node, utxos, *, num_outputs=1, fee_per_output=1000):

def create_self_transfer_multi(self, *, from_node, utxos, num_outputs=1, fee_per_output=1000):

maflcko · 2022-03-22T16:03:17Z

test/functional/test_framework/wallet.py

nit:

Suggested change

def send_self_transfer_multi(self, from_node, utxos, **kwargs):

def send_self_transfer_multi(self, *, from_node, utxos, **kwargs):

Also, "utxos" can be renamed to "inputs" or "utxos_to_spend".

maflcko · 2022-03-22T16:04:45Z

test/functional/test_framework/wallet.py

nit: Maybe return a dict similar to send_self_transfer?

Suggested change

return [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))]

return {"new_utxo":[self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))], "tx": tx}

maflcko · 2022-03-22T16:24:27Z

test/functional/test_framework/wallet.py

Suggested change

def send_self_transfer_multi(self, from_node, utxos, **kwargs):

def send_self_transfer_multi(self, **kwargs):

or maybe even just

This test can now be run even with the Bitcoin Core wallet disabled.

theStack · 2022-03-22T17:54:08Z

Force-pushed again with the following changes:

all parameters for the newly introduced methods {create,send}_self_transfer_multi methods must be passed as keyword arguments now (to be consistent with {create,self}_transfer methods)
send_self_transfer_multi returns a dictionary with several informations (txid, tx serialized as hex, tx as CTransaction instance, list of new UTXOs)
introduced small helper chain_tx in the test in order to avoid repeatedly passing from_node arguments and being able to pass utxos ~~and num_outputs~~ as positional arguments; returns new UTXOs
getrawtransaction RPC at the end of the test is not needed anymore, since we can directly access the tx field from the result dictionary

maflcko

ACK 2b6dd4e 💾

Show signature

Signature:

maflcko · 2022-03-24T11:06:15Z

test/functional/test_framework/wallet.py

+        for i in range(num_outputs):
+            tx.vout[i].nValue = outputs_value_total // num_outputs


nit: Haven't tried, but this might be possible to write shorter

Suggested change

for i in range(num_outputs):

tx.vout[i].nValue = outputs_value_total // num_outputs

for o in tx.vout:

o.nValue = outputs_value_total // num_outputs

Done in #24623

494455f test: use MiniWallet for feature_fee_estimation.py (Sebastian Falbesoner) Pull request description: This PR enables one more of the non-wallet functional tests (feature_fee_estimation.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in bitcoin#20078. It takes use of the recently introduced methods `{create,send}_self_transfer_multi` (bitcoin#24637) which allows to specify multiple UTXOs to be spent rather than only one. Very likely the test can still be simplified (e.g. coin selection in `small_txpuzzle_randfee`), but this is a first step. ACKs for top commit: ayush933: tACK 494455f . The test runs successfully with the wallet disabled. vincenzopalazzo: tACK bitcoin@494455f Tree-SHA512: 89789fc34a4374c79c4b90acd926ac69153aad655dab50450ed796f03c770bd675ad872e906f516f90e8d4cb40b83b55f3c78a94b13bfb8fe8f5e27624937748

Github-Pull: bitcoin#24637 Rebased-From: eb3c5c4

0b78110 test: Move tx creation to create_self_transfer_multi (kouloumos) Pull request description: Two birds with one stone: replacement of #26278 with simplification of the MiniWallet's transaction creation logic. Currently the MiniWallet creates simple txns (1 input, 1 output) with `create_self_transfer`. #24637 introduced `create_self_transfer_multi` **which uses** `create_self_transfer` to create a "transaction template" which then adjusts (copy and mutate inputs and outputs) in order to create more complex multi-input multi-output transactions. This can more easily lead to issues such as #26278 and is more of a maintenance burden. This PR simplifies the logic by going the other way around. Now `create_self_transfer` **uses** `create_self_transfer_multi`. The transaction creation logic has been moved to `create_self_transfer_multi` which is being called by `create_self_transfer` to construct the simple case of 1 input 1 output transaction. ACKs for top commit: MarcoFalke: ACK 0b78110 👒 Tree-SHA512: 147e577ed5444bee57865bd375b37c9b49d6539e9875c30c2667e70fcba27fe80bcb4552a4e6efb42760d34b40d5dad826883b778eaeefe29425ec081787b4bd

…ulti 0b78110 test: Move tx creation to create_self_transfer_multi (kouloumos) Pull request description: Two birds with one stone: replacement of bitcoin#26278 with simplification of the MiniWallet's transaction creation logic. Currently the MiniWallet creates simple txns (1 input, 1 output) with `create_self_transfer`. bitcoin#24637 introduced `create_self_transfer_multi` **which uses** `create_self_transfer` to create a "transaction template" which then adjusts (copy and mutate inputs and outputs) in order to create more complex multi-input multi-output transactions. This can more easily lead to issues such as bitcoin#26278 and is more of a maintenance burden. This PR simplifies the logic by going the other way around. Now `create_self_transfer` **uses** `create_self_transfer_multi`. The transaction creation logic has been moved to `create_self_transfer_multi` which is being called by `create_self_transfer` to construct the simple case of 1 input 1 output transaction. ACKs for top commit: MarcoFalke: ACK 0b78110 👒 Tree-SHA512: 147e577ed5444bee57865bd375b37c9b49d6539e9875c30c2667e70fcba27fe80bcb4552a4e6efb42760d34b40d5dad826883b778eaeefe29425ec081787b4bd

fanquake added the Tests label Mar 22, 2022

maflcko approved these changes Mar 22, 2022

View reviewed changes

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

theStack force-pushed the 20220322-test-use_MiniWallet_for_mempool_packages_onemore branch from 87f28c1 to 913a09c Compare March 22, 2022 13:44

maflcko reviewed Mar 22, 2022

View reviewed changes

theStack force-pushed the 20220322-test-use_MiniWallet_for_mempool_packages_onemore branch from 913a09c to 5d0c26a Compare March 22, 2022 14:33

maflcko reviewed Mar 22, 2022

View reviewed changes

theStack added 2 commits March 22, 2022 18:43

test: MiniWallet: add helper methods {send,create}_self_transfer_multi

eb3c5c4

test: use MiniWallet for mempool_package_onemore.py

2b6dd4e

This test can now be run even with the Bitcoin Core wallet disabled.

theStack force-pushed the 20220322-test-use_MiniWallet_for_mempool_packages_onemore branch from 5d0c26a to 2b6dd4e Compare March 22, 2022 17:47

fanquake requested a review from glozow March 24, 2022 11:16

maflcko approved these changes Mar 24, 2022

View reviewed changes

maflcko merged commit 864fb89 into bitcoin:master Mar 24, 2022

theStack deleted the 20220322-test-use_MiniWallet_for_mempool_packages_onemore branch March 25, 2022 11:12

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 2, 2022

Merge bitcoin#24637: test: use MiniWallet for mempool_package_onemore.py

5a46099

theStack mentioned this pull request Apr 10, 2022

test: use MiniWallet for feature_fee_estimation.py #24817

Merged

shommel mentioned this pull request Apr 22, 2022

test: Convert non-wallet tests to use our python MiniWallet #20078

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

test: MiniWallet: add helper methods {send,create}_self_transfer_multi

2e596b6

Github-Pull: bitcoin#24637 Rebased-From: eb3c5c4

kouloumos mentioned this pull request Oct 28, 2022

test: Move tx creation to create_self_transfer_multi #26414

Merged

bitcoin locked and limited conversation to collaborators Mar 25, 2023

	def chain_tx(self, node, utxos, num_outputs=1, fee_per_output=1000):
	def send_self_transfer_multi(self, node_from, utxos, *, num_outputs=1, fee_per_output=1000):

	def create_self_transfer_multi(self, from_node, utxos, *, num_outputs=1, fee_per_output=1000):
	def create_self_transfer_multi(self, *, from_node, utxos, num_outputs=1, fee_per_output=1000):

	def send_self_transfer_multi(self, from_node, utxos, **kwargs):
	def send_self_transfer_multi(self, , from_node, utxos, *kwargs):

	return [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))]
	return {"new_utxo":[self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))], "tx": tx}

	def send_self_transfer_multi(self, from_node, utxos, **kwargs):
	def send_self_transfer_multi(self, **kwargs):

		for i in range(num_outputs):
		tx.vout[i].nValue = outputs_value_total // num_outputs

Conversation

theStack commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

theStack commented Mar 22, 2022 •

edited

Loading

theStack commented Mar 22, 2022 •

edited

Loading

theStack commented Mar 22, 2022 •

edited

Loading