test: use MiniWallet for mempool_package

theStack commented at 11:20 am on March 22, 2022: member

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.

fanquake added the label Tests on Mar 22, 2022

in test/functional/test_framework/wallet.py:193 in 87f28c1d93 outdated

187@@ -188,6 +188,38 @@ def send_to(self, *, from_node, scriptPubKey, amount, fee=1000):
188         txid = self.sendrawtransaction(from_node=from_node, tx_hex=tx.serialize().hex())
189         return txid, 1
190 
191+    def chain_tx(self, node, parent_txids, num_outputs=1, fee_per_output=1000):
192+        """
193+        Build and send a transaction that spends the given inputs and creates a

MarcoFalke commented at 11:24 am on March 22, 2022:

0        Build and send a transaction that spends one output of the given txids and creates a

I think it only spends the first found (random) output

MarcoFalke commented at 11:37 am on March 22, 2022:

Maybe it would be better to get rid of this and put the burden of coin selection on the caller? This would also allow the caller to decide whether the inputs are marked as spent or not if the call fails. (Other methods here also mark them as spent by default, even on failure)

theStack commented at 1:47 pm on March 22, 2022:

Yes, I agree. Reworked the helper to accept and return UTXOs rather than txids now (i.e. caller has to do coin selection), which makes the test even shorter.

MarcoFalke approved

MarcoFalke commented at 11:39 am on March 22, 2022: member

Concept ACK. Some questions/thoughts.

theStack force-pushed on Mar 22, 2022

in test/functional/test_framework/wallet.py:199 in 913a09c913 outdated

194+        certain number of outputs with equal amounts.
195+
196+        Returns a list of newly created UTXOs, ordered by vout index.
197+        """
198+        # create simple tx template (1 input, 1 output)
199+        assert_greater_than_or_equal(len(utxos), 1)

MarcoFalke commented at 1:57 pm on March 22, 2022:

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

in test/functional/test_framework/wallet.py:228 in 913a09c913 outdated

210+
211+        # adapt output amounts (use fixed fee per output)
212+        inputs_value_total = sum([int(COIN * utxo['value']) for utxo in utxos])
213+        outputs_value_total = inputs_value_total - fee_per_output * num_outputs
214+        for i in range(num_outputs):
215+            tx.vout[i].nValue = outputs_value_total // num_outputs

MarcoFalke commented at 1:58 pm on March 22, 2022:

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

in test/functional/test_framework/wallet.py:191 in 913a09c913 outdated

187@@ -188,6 +188,35 @@ def send_to(self, *, from_node, scriptPubKey, amount, fee=1000):
188         txid = self.sendrawtransaction(from_node=from_node, tx_hex=tx.serialize().hex())
189         return txid, 1
190 
191+    def chain_tx(self, node, utxos, num_outputs=1, fee_per_output=1000):

MarcoFalke commented at 2:00 pm on March 22, 2022:

nit: (Some renames for symmetry)

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

MarcoFalke commented at 2:00 pm on March 22, 2022: member

Left some more nits :sweat_smile:

theStack force-pushed on Mar 22, 2022

theStack commented at 2:40 pm on March 22, 2022: member

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

removed the assertion for non-empty UTXO list (https://github.com/bitcoin/bitcoin/pull/24637#discussion_r832217228)
renamed helper method and split it up into create_self_transfer_multi/send_self_transfer_multi for the symmetry with existing helpers (https://github.com/bitcoin/bitcoin/pull/24637#discussion_r832218529 and #24637 (review))

The PR description was also adapted accordingly.

in test/functional/test_framework/wallet.py:202 in 5d0c26a8c7 outdated

197+        """
198+        tx = self.create_self_transfer_multi(from_node, utxos, **kwargs)
199+        txid = self.sendrawtransaction(from_node=from_node, tx_hex=tx.serialize().hex())
200+        return [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))]
201+
202+    def create_self_transfer_multi(self, from_node, utxos, *, num_outputs=1, fee_per_output=1000):

MarcoFalke commented at 4:00 pm on March 22, 2022:

nit

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

in test/functional/test_framework/wallet.py:191 in 5d0c26a8c7 outdated

187@@ -188,6 +188,41 @@ def send_to(self, *, from_node, scriptPubKey, amount, fee=1000):
188         txid = self.sendrawtransaction(from_node=from_node, tx_hex=tx.serialize().hex())
189         return txid, 1
190 
191+    def send_self_transfer_multi(self, from_node, utxos, **kwargs):

MarcoFalke commented at 4:03 pm on March 22, 2022:

nit:

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

Also, “utxos” can be renamed to “inputs” or “utxos_to_spend”.

MarcoFalke commented at 4:24 pm on March 22, 2022:

0    def send_self_transfer_multi(self, **kwargs):

or maybe even just

in test/functional/test_framework/wallet.py:200 in 5d0c26a8c7 outdated

195+
196+        Returns a list of newly created UTXOs, ordered by vout index.
197+        """
198+        tx = self.create_self_transfer_multi(from_node, utxos, **kwargs)
199+        txid = self.sendrawtransaction(from_node=from_node, tx_hex=tx.serialize().hex())
200+        return [self.get_utxo(txid=txid, vout=vout) for vout in range(len(tx.vout))]

MarcoFalke commented at 4:04 pm on March 22, 2022:

nit: Maybe return a dict similar to send_self_transfer?

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

MarcoFalke commented at 4:22 pm on March 22, 2022: member

(feel free to ignore)

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

test: use MiniWallet for mempool_package_onemore.py

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

2b6dd4e75b

theStack force-pushed on Mar 22, 2022

theStack commented at 5:54 pm on March 22, 2022: member

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

in test/functional/test_framework/wallet.py:228 in 2b6dd4e75b

223+
224+        # adapt output amounts (use fixed fee per output)
225+        inputs_value_total = sum([int(COIN * utxo['value']) for utxo in utxos_to_spend])
226+        outputs_value_total = inputs_value_total - fee_per_output * num_outputs
227+        for i in range(num_outputs):
228+            tx.vout[i].nValue = outputs_value_total // num_outputs

MarcoFalke commented at 11:06 am on March 24, 2022:

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

0        for o in tx.vout:
1            o.nValue = outputs_value_total // num_outputs

MarcoFalke commented at 1:25 pm on March 24, 2022:

Done in #24623

fanquake requested review from glozow on Mar 24, 2022

MarcoFalke approved

MarcoFalke commented at 11:36 am on March 24, 2022: member

ACK 2b6dd4e75b3ad2daff553fde018fe4c8f1187878 💾

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 2b6dd4e75b3ad2daff553fde018fe4c8f1187878 💾
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh0jgwAlqAJ870kGVFw8j59DsDnOgazr4Q3gZs3ev7+QPUDwABrwzfJggjUWYy8
 8G+fBs+4b/buewrlpMpLHMoAza3n1ib4yiR4Ayebbj65vyVhRVERBncLMlbnYDU7c
 9VQR0Fkuz5FTza9P7BuIegO3JPOXxPnJi5AKg8xSsIY7vZfNvXuwn74zyZqI14hCm
10iDG4JtKtyiYpL+Mct9UWD9mKBUlEW1KnESejKVj5eQTG5ww0TijlNCfQ7W+JTE3m
11ndwqmbuPI5RM7YqytsKuTpPc584nqr/zM1/AuIGM7aEM8BdwTW5StJCL6AuPL1B+
12wD3w7HVltleskkvmj2407xPZkiGanxzOE7L3Q5G0GiWDCJFYU6jqXbRoJ8ml/oPn
13b8kILOTtLvEqRNry8bOGiOAxTGZG8VkdsvRtjxBE/HLGtgX/avlN9ELPPvLE6GKD
14rY+isoIqlqqGGZfLegweVnCuLJVS0+1o8HJAgwPyhyejVXbcU1EACstphCNdMtd8
15s9QdQ3hV
16=mjSo
17-----END PGP SIGNATURE-----

MarcoFalke merged this on Mar 24, 2022

MarcoFalke closed this on Mar 24, 2022

theStack deleted the branch on Mar 25, 2022

sidhujag referenced this in commit 5a46099d8c on Apr 2, 2022

MarcoFalke referenced this in commit cd110cdd0e on Apr 11, 2022

sidhujag referenced this in commit 06add764f5 on Apr 11, 2022

luke-jr referenced this in commit 2e596b6cb5 on May 21, 2022

MarcoFalke referenced this in commit 38cbf43dee on Dec 5, 2022

sidhujag referenced this in commit 59b077f4e5 on Dec 5, 2022

DrahtBot locked this on Mar 25, 2023

test: use MiniWallet for mempool_package_onemore.py #24637