test: use MiniWallet to simplify mempool_package

kouloumos commented at 2:33 pm on June 15, 2022: contributor

While wallet.py includes the MiniWallet class and some helper methods, it also includes some methods that have been moved there without having any direct relation with the MiniWallet class. Specifically make_chain, create_child_with_parents and create_raw_chain methods that were extracted from rpc_packages.py at f8253d69d6f02850995a11eeb71fedc22e6f6575 in order to be used on both mempool_package_limits.py and rpc_packages.py.

Since that change, due to the introduction of additional methods in MiniWallet, the functionality of those methods can now be replicated with the existing MiniWallet methods and simultaneously simplify those tests by using the MiniWallet.

This PR’s goals are

to simplify the mempool_package_limits.py functional tests with usage of the MiniWallet.
to make progress towards the removal of the make_chain, create_child_with_parents and create_raw_chain methods of wallet.py.

For the purpose of the aforementioned goals, a helper method MiniWallet.send_self_transfer_chain is introduced and method bulk_transaction has been integrated in create_self_transfer* methods using an optional target_weight option.

fanquake added the label Tests on Jun 15, 2022

DrahtBot commented at 0:28 am on June 21, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in test/functional/mempool_package_limits.py:32 in 2b55bd364d outdated

52+        self.wallet = MiniWallet(self.nodes[0])
53+        # the pre-mined test framework chain contains coinbase outputs to the
54+        # MiniWallet's default address in blocks 76-100 (see method
55+        # BitcoinTestFramework._initialize_chain())
56+        self.wallet.rescan_utxos()
57+        self.generate(self.wallet, 100) # more mature UTXOs are needed

MarcoFalke commented at 6:35 am on June 21, 2022:

I am pretty sure none of the utxos are mature, so the comment doesn’t apply.

so maybe this can be removed?

If not, you may just mine 200 blocks in one go instead of using cached/non-cached?

kouloumos commented at 8:21 am on June 21, 2022:

It matures the cached blocks, so I could phrase that in better way. But you are right, I changed it to mine the neccesary blocks on a clean chain.

MarcoFalke commented at 8:32 am on June 21, 2022:

blocks 76-100 with a height of 200 should already be mature, no?

kouloumos commented at 8:45 am on June 21, 2022:

Yes but because those 25 blocks were not enough for the tests, I also used the 24 initially immature blocks by generating another 100 blocks. But what you suggested is more elegant thus I changed it to that.

kouloumos force-pushed on Jun 21, 2022

kouloumos force-pushed on Jun 28, 2022

kouloumos commented at 2:02 pm on June 28, 2022: contributor

Forced-pushed to take advantage of the logic introduced in https://github.com/bitcoin/bitcoin/pull/25445

in test/functional/mempool_package_limits.py:344 in 52424d27e0 outdated

368@@ -486,50 +369,21 @@ def test_desc_size_limits(self):
369         high_fee = Decimal("0.0021") # 10 sats/vB

MarcoFalke commented at 11:51 am on August 1, 2022:

52424d27e0990cc3b77ca5d6e45f045cbe1eaa60: This is wrong. Now it is 20sat/vB

kouloumos commented at 3:39 pm on August 1, 2022:

I’ve now fixed that with a division on the method call.

in test/functional/test_framework/wallet.py:354 in 52424d27e0 outdated

329+                height=0,
330+            ) for i in range(len(tx.vout))],
331+            "txid": txid,
332+            "hex": tx_heavy.serialize().hex(),
333+            "tx": tx_heavy,
334+        }

MarcoFalke commented at 11:52 am on August 1, 2022:

52424d27e0990cc3b77ca5d6e45f045cbe1eaa60: Any reason to not make this a target_weight=0 option on create_self_tx to avoid all the duplicated logic?

kouloumos commented at 3:55 pm on August 1, 2022:

Good idea. I’ve now created _bulk_tx because the logic is both in create_self_transfer and create_self_transfer_multi

in test/functional/mempool_package_limits.py:26 in 52424d27e0 outdated

22-)
23 from test_framework.util import (
24     assert_equal,
25 )
26-from test_framework.wallet import (
27-    bulk_transaction,

MarcoFalke commented at 11:59 am on August 1, 2022:

Can remove all of these functions now?

kouloumos commented at 12:16 pm on August 1, 2022:

create_child_with_parents and make_chain are still used in rpc_packages.py from which I anticipate to remove on a follow-up PR.

in test/functional/test_framework/wallet.py:336 in 1c9243b40d outdated

332@@ -333,6 +333,19 @@ def bulk_transaction(self, tx, target_weight):
333             "tx": tx_heavy,
334         }
335 
336+    def send_self_transfer_chain(self, *, from_node=None, chain_length, utxo_to_spend=None):

MarcoFalke commented at 12:02 pm on August 1, 2022:

1c9243b40db5746575ed55d09774872f0a98b24a: from_node should be a required arg, like in the other send* methods.

MarcoFalke approved

MarcoFalke commented at 12:04 pm on August 1, 2022: member

lgtm 1c9243b40db5746575ed55d09774872f0a98b24a 🐐

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3lgtm 1c9243b40db5746575ed55d09774872f0a98b24a 🐐
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjMzgv8Ck6nSSmgAatlxCMG837PArkz0UTGPh4ek3WbN73Mn1dLmSCBsvlzbSVG
 84yQlx+vMpDNUrJO07nU7fVszAKK5+BOFUcd1pElpZbIoOWHIudbDdEuOeOIDjuKC
 9368yHXtcXZUnx4ZUGJvC11LtkH2eZxjrwt70iVWqoNWMz2Wj3dNS01VGOkAN6Bb2
108wUWoNYwgm+iTNJIsQnzPuc9I7qjhILvhcbMTXKX/NQQi4StPq+5nBRFIXSePSuD
11ApxcDIGIB2s3cYu/SswObSHtCtRKt/gaG0Xd7OkOMZ8bpNPU0vWiRAFtRn5OhKj7
122csGoyU1prVU52njdCYaC/EAiyWumqBmw/4CtffajgZ4UJluPhvn2ZBjAJDq9DAb
13SiFfaITnyqEA327CahBPIkyTXmL15HC0zipUDjs0U5YYLby2z05h6i9htpQv+iCJ
14eH1N+nmixN9aA6v1gkXNEs7qVcVWWLhLUuatmxzgaEr5pdhn+7701yNirhqu004l
15JrbTuRmo
16=dEET
17-----END PGP SIGNATURE-----

bitcoin deleted a comment on Aug 1, 2022

kouloumos force-pushed on Aug 1, 2022

MarcoFalke commented at 2:47 pm on August 1, 2022: member

Also, should be rebased

DrahtBot added the label Needs rebase on Aug 1, 2022

kouloumos force-pushed on Aug 1, 2022

kouloumos commented at 3:58 pm on August 1, 2022: contributor

Rebased and addressed @MarcoFalke comments.

~~The failing CI task is also on master, it seems not to be related with the changes.~~

in test/functional/test_framework/wallet.py:115 in 48afe03354 outdated

110+        assert_greater_than_or_equal(target_weight, tx.get_weight())
111+        while tx.get_weight() < target_weight:
112+            script_pubkey = "6a4d0200"  # OP_RETURN OP_PUSH2 512 bytes
113+            for _ in range(512):
114+                script_pubkey = script_pubkey + "01"
115+            tx.vout.append(CTxOut(0, bytes.fromhex(script_pubkey)))

MarcoFalke commented at 4:02 pm on August 1, 2022:

0            script_pubkey = ( b"6a4d0200"  # OP_RETURN OP_PUSH2 512 bytes
1             + b"01" * 512 )
2            tx.vout.append(CTxOut(0, script_pubkey))

nit?

kouloumos commented at 4:14 pm on August 1, 2022:

Done

test: use MiniWallet to simplify mempool_package_limits.py tests

Moved `bulk_transaction` into MiniWallet class as `_bulk_tx` private
helper method to be used when the newly added `target_weight` option is
passed to `create_self_transfer*`

1d6b438ef0

test: MiniWallet: add `send_self_transfer_chain` to create chain of txns

With this new method, a chain of transactions can be created. This
method is introduced to further simplify the mempool_package_limits.py
tests.

f2f6068b69

kouloumos force-pushed on Aug 1, 2022

DrahtBot removed the label Needs rebase on Aug 1, 2022

MarcoFalke commented at 1:43 pm on August 2, 2022: member

ACK f2f6068b69b1b532db92b276f024c89b56f38294 👜

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK f2f6068b69b1b532db92b276f024c89b56f38294 👜
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjJEAwAs/RUpTTagS6DLfFVOT2d+FAmEcS+/m1s5438bxGA9pb/lhVYxxYlOTvM
 8wtgbP2N1G496b2wmgzRWZ8LPdI4JvUmzgP4+f9fvPO2jXo6+bLZFfLBnVvhbybVn
 9keiIkU38nUmohPlQ2OZvNfXFxrf3Ij1jQDauEcg2wk4fjog7WPFK3tkA6Js338bE
10/GzH7hmy/pNMMTZcsX6etEwxT+zOfit/PKo4b7+VD1R41lkSg1SacUdMbUph13Lw
11bVtPG450XzW0U2MSS8iPyG81y1Wnlcvtj2Z8wO5MsSuJbcoYT5/haDjsMssKDHF9
12YpRhKbRVYeejJny9xIqrtGP56WIjIdGrJ7h3I14vmn9zqeu4qvW9nUeUN7VHyEiw
13dTb8YDFwTjmI6h2XzVA2OBltMB4ZlUJqvL59CO6DOHKwzObtWh/Y5ox2GKqEzEUz
14QXBwKrNA3g+5jKAJYfpRy6c8OTTsN4elrnl45T5QmVXe5npyStD3TMPoZJhCASgI
153a/nCGHb
16=PXPl
17-----END PGP SIGNATURE-----

MarcoFalke commented at 1:44 pm on August 2, 2022: member

cc @glozow @theStack

glozow commented at 1:49 pm on August 2, 2022: member

Concept ACK

I was just thinking it would be nice to get rid of these functions that are only used in package functional tests and use MiniWallet instead, thanks for working on this!

MarcoFalke merged this on Aug 3, 2022

MarcoFalke closed this on Aug 3, 2022

theStack commented at 11:58 am on August 3, 2022: contributor

Post-merge concept and light code-review ACK f2f6068b69b1b532db92b276f024c89b56f38294 🌇

kouloumos deleted the branch on Sep 2, 2022

bitcoin locked this on Sep 2, 2023

test: use MiniWallet to simplify mempool_package_limits.py tests #25379

Conflicts