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 12: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 🐐

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

Signature:

</details>

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:

            script_pubkey = ( b"6a4d0200"  # OP_RETURN OP_PUSH2 512 bytes
             + b"01" * 512 )
            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 👜

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

Signature:

</details>

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