test: refactor `RPCPackagesTest` to use `MiniWallet`

w0xlt commented at 4:02 pm on September 2, 2022: contributor

This PR refactors RPCPackagesTest to use MiniWallet and removes create_child_with_parents, make_chain, and create_raw_chain from test_framework/wallet, as requested in #25965.

Close #25965.

w0xlt force-pushed on Sep 2, 2022

fanquake requested review from glozow on Sep 2, 2022

fanquake added the label Tests on Sep 2, 2022

kouloumos commented at 5:42 pm on September 2, 2022: contributor

The reasoning behind the removal of those functions was that the functionality of those methods can now be replicated with the existing MiniWallet methods.

I believe that your implementation doesn’t fully utilize the MiniWallet methods. See #25379 where I did the first part of refactoring mempool_package_limits.py which was the other test that was using those functions.

w0xlt force-pushed on Sep 3, 2022

w0xlt commented at 8:25 am on September 3, 2022: contributor

@kouloumos Thanks for the tip. I changed all tests to use MiniWallet methods in 10edc9c0d7b686808c040204103da01ed386a3c9.

in test/functional/rpc_packages.py:41 in 10edc9c0d7 outdated

37@@ -48,33 +38,34 @@ def assert_testres_equal(self, package_hex, testres_expected):
38         shuffled_testres = [testres_expected[i] for i in shuffled_indeces]
39         assert_equal(shuffled_testres, self.nodes[0].testmempoolaccept(shuffled_package))
40 
41+    def create_tx_chain(self, chain_length=25):

kouloumos commented at 10:28 am on September 3, 2022:

Maybe this could become part of the MiniWallet methods as create_self_transfer_chain (see send_self_transfer_chain) to be also used at mempool_package_limits.py.

See also #21800 (review) for chain_length=25

w0xlt commented at 8:03 pm on September 3, 2022:

Done in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:52 in 10edc9c0d7 outdated

47+            tx = self.wallet.create_self_transfer(utxo_to_spend=chaintip_utxo)
48+            chaintip_utxo = tx["new_utxo"]
49+            chain_hex.append(tx["hex"])
50+            chain_txns.append(tx["tx"])
51+
52+        return (chain_hex, chain_txns)

kouloumos commented at 10:31 am on September 3, 2022:

See #21800 (review) about tuple. Also iirc we are mostly returning dictionaries in most of the other functions.

w0xlt commented at 8:03 pm on September 3, 2022:

Done in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:58 in 10edc9c0d7 outdated

66-                "scriptPubKey": coinbase["vout"][0]["scriptPubKey"],
67-            })
68+        self.wallet = MiniWallet(self.nodes[0])
69 
70+        self.generate(self.wallet, COINBASE_MATURITY + 100)  # blocks generated for inputs
71+        self.wallet.rescan_utxos()

kouloumos commented at 10:34 am on September 3, 2022:

Since recently rescan_utxos() is called at the end of generate. https://github.com/bitcoin/bitcoin/blob/ea67232cdb80c4bc3f16fcd823f6f811fd8903e1/test/functional/test_framework/wallet.py#L171

w0xlt commented at 8:04 pm on September 3, 2022:

Done in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:65 in 10edc9c0d7 outdated

79-            rawtx = node.createrawtransaction([{"txid": coin["txid"], "vout": 0}],
80-                {self.address : coin["amount"] - Decimal("0.0001")})
81-            signedtx = node.signrawtransactionwithkey(hexstring=rawtx, privkeys=self.privkeys)
82-            assert signedtx["complete"]
83-            testres = node.testmempoolaccept([signedtx["hex"]])
84+            tx = self.wallet.create_self_transfer(fee_rate=Decimal("0.0001"))["tx"]

kouloumos commented at 10:38 am on September 3, 2022:

create_self_transfer returns the hex, so you could avoid tx.serialize().hex()

0            tx_hex = self.wallet.create_self_transfer(fee_rate=Decimal("0.0001"))["hex]

w0xlt commented at 8:04 pm on September 3, 2022:

Done in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:128 in 10edc9c0d7 outdated

139-                                           {self.address : coin["amount"] - Decimal("0.999")})
140-        tx_high_fee_signed = node.signrawtransactionwithkey(hexstring=tx_high_fee_raw, privkeys=self.privkeys)
141-        assert tx_high_fee_signed["complete"]
142-        tx_high_fee = tx_from_hex(tx_high_fee_signed["hex"])
143-        testres_high_fee = node.testmempoolaccept([tx_high_fee_signed["hex"]])
144+        coin = self.wallet.get_utxo()

kouloumos commented at 10:52 am on September 3, 2022:

Where is this used?

w0xlt commented at 8:04 pm on September 3, 2022:

Removed in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:173 in 10edc9c0d7 outdated

191-        tx_child_b = tx_from_hex(rawtx_b)
192-        tx_child_b.wit.vtxinwit = [CTxInWitness()]
193-        tx_child_b.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
194-        tx_child_b_hex = tx_child_b.serialize().hex()
195-        assert not node.testmempoolaccept([tx_child_b_hex])[0]["allowed"]
196+        # # Child B

kouloumos commented at 10:54 am on September 3, 2022:

nit: double #

w0xlt commented at 9:13 pm on September 3, 2022:

Done in https://github.com/bitcoin/bitcoin/commit/df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:224 in 10edc9c0d7 outdated

235-                package_hex.append(txhex)
236-                parents_tx.append(tx)
237-                values.append(value)
238-                parent_locking_scripts.append(parent_locking_script)
239-            child_hex = create_child_with_parents(node, self.address, self.privkeys, parents_tx, values, parent_locking_scripts)
240-            # Package accept should work with the parents in any order (as long as parents come before child)

kouloumos commented at 10:55 am on September 3, 2022:

I think this comment is still useful.

Same for the rest of the comments that you deleted later in the code, just writing it here to avoid multiple comments.

w0xlt commented at 8:05 pm on September 3, 2022:

It was unintentionally removed. Fixed it in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677.

in test/functional/rpc_packages.py:311 in 10edc9c0d7 outdated

325         # Check that each result is present, with the correct size and fees
326-        for i in range(num_parents + 1):
327-            tx = package_txns[i]
328+        for package_tx in package_txns:
329+            tx = package_tx['tx']
330             wtxid = tx.getwtxid()

kouloumos commented at 11:07 am on September 3, 2022:

create_self_transfer already returns wtxid, maybe it makes sense for create_self_transfer_multi to also return it.

w0xlt commented at 8:05 pm on September 3, 2022:

Done in df7e74b4e4d1b67b4b9d226ba00ffc21548d5677. Thanks for the review.

kouloumos commented at 11:11 am on September 3, 2022: contributor

Looks good, some minor comments

DrahtBot commented at 3:14 pm on September 3, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, pablomartin4btc, kouloumos
Approach ACK	hernanmarino, rserranon

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:

#26403 ([POLICY] Ephemeral anchors by instagibbs)
#25038 (policy: nVersion=3 and Package RBF by glozow)

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.

w0xlt force-pushed on Sep 3, 2022

in test/functional/rpc_packages.py:97 in 4d4298f353 outdated

100+                "value": coinbase["vout"][0]["value"],
101+                "scriptPubKey": coinbase["vout"][0]["scriptPubKey"],
102+                "vout": 0,
103+                "height": 0
104+            }
105+        self.generate(node, COINBASE_MATURITY, sync_fun=self.no_op)

glozow commented at 3:42 pm on September 6, 2022:

Maybe generate all the coins at the beginning so we don’t need to multiple chains of 100 blocks for coinbase maturity?

w0xlt commented at 0:11 am on September 10, 2022:

Done in b21329d50639fc383d92c75ab7f20187de4ae0e9.

in test/functional/test_framework/wallet.py:294 in 4d4298f353 outdated

289@@ -291,6 +290,8 @@ def create_self_transfer_multi(
290                 height=0,
291             ) for i in range(len(tx.vout))],
292             "txid": txid,
293+            "wtxid": tx.getwtxid(),
294+            "vsize": tx.get_vsize(),

glozow commented at 3:47 pm on September 6, 2022:

I don’t think it’s necessary to add these fields since the caller can get them from the CTransaction object… though I don’t feel that strongly about it.

w0xlt commented at 0:11 am on September 10, 2022:

Done in https://github.com/bitcoin/bitcoin/commit/b21329d50639fc383d92c75ab7f20187de4ae0e9.

glozow commented at 3:48 pm on September 6, 2022: member

Looks good, thanks for working on this!

w0xlt force-pushed on Sep 10, 2022

in test/functional/rpc_packages.py:57 in b21329d506 outdated

59+                "value": coinbase["vout"][0]["value"],
60                 "scriptPubKey": coinbase["vout"][0]["scriptPubKey"],
61-            })
62+                "vout": 0,
63+                "height": 0
64+            }

glozow commented at 10:35 am on September 29, 2022:

There’s no reason not to construct this coin inside test_independent. You can use node.getblock(blockhash=node.getblockhash(height=1), verbosity=2)["tx"][0] to get the coinbase from that specific block, but also why not just use create self transfer?

w0xlt commented at 1:47 am on October 4, 2022:

node.getblock(blockhash=node.getblockhash(height=1), verbosity=2)["tx"][0] returns a UTXO that does not need a signature, as the UTXO are being generated for a ADDRESS_OP_TRUE miniwallet.

The same applies to create_self_transfer.

The reason why this code is not inside test_independent is #25986 (review).

in test/functional/rpc_packages.py:112 in b21329d506 outdated

133         assert_equal(testres_bad_sig, self.independent_txns_testres + [{
134             "txid": tx_bad_sig.rehash(),
135-            "wtxid": tx_bad_sig.getwtxid(), "allowed": False,
136+            "wtxid": tx_bad_sig.getwtxid(),
137+            "allowed": False,
138             "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)"

glozow commented at 10:39 am on September 29, 2022:

These changes aren’t necessary, as createrawtransaction is not a wallet RPC and we want a tx with no signature. It’s a little weird to have MiniWallet sign it and then manually edit the tx to remove the signature.

w0xlt commented at 1:41 am on October 4, 2022:

Done in https://github.com/bitcoin/bitcoin/commit/17cad448516a6906ff637593ab57df332fade5d2.

in test/functional/rpc_packages.py:166 in b21329d506 outdated

185 
186         self.log.info("Testmempoolaccept with entire package, should work with children in either order")
187-        testres_multiple_ab = node.testmempoolaccept(rawtxs=[parent_signed["hex"], tx_child_a_hex, tx_child_b_hex])
188-        testres_multiple_ba = node.testmempoolaccept(rawtxs=[parent_signed["hex"], tx_child_b_hex, tx_child_a_hex])
189+        testres_multiple_ab = node.testmempoolaccept(rawtxs=[parent_tx["hex"], child_a_tx["hex"], child_b_tx["hex"]])
190+        testres_multiple_ba = node.testmempoolaccept(rawtxs=[parent_tx["hex"], child_a_tx["hex"], child_b_tx["hex"]])

glozow commented at 10:46 am on September 29, 2022:

You have reversed the order of the transactions here, so this is submitting the same package twice and no longer testing that it works in either order.

w0xlt commented at 1:41 am on October 4, 2022:

Fixed in https://github.com/bitcoin/bitcoin/commit/17cad448516a6906ff637593ab57df332fade5d2.

in test/functional/rpc_packages.py:242 in b21329d506 outdated

307+        testres_replaceable = node.testmempoolaccept([replaceable_tx["hex"]])
308         assert_equal(testres_replaceable, [
309-            {"txid": replaceable_tx.rehash(), "wtxid": replaceable_tx.getwtxid(),
310-            "allowed": True, "vsize": replaceable_tx.get_vsize(), "fees": { "base": fee }}
311+            {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"],
312+            "allowed": True, "vsize": replaceable_tx['tx'].get_vsize(), "fees": { "base": fee }}

glozow commented at 10:49 am on September 29, 2022:

Prefer double quotes for consistency

w0xlt commented at 1:40 am on October 4, 2022:

Done in https://github.com/bitcoin/bitcoin/commit/17cad448516a6906ff637593ab57df332fade5d2.

in test/functional/rpc_packages.py:288 in b21329d506 outdated

310-            "allowed": True, "vsize": replaceable_tx.get_vsize(), "fees": { "base": fee }}
311+            {"txid": replaceable_tx["txid"], "wtxid": replaceable_tx["wtxid"],
312+            "allowed": True, "vsize": replaceable_tx['tx'].get_vsize(), "fees": { "base": fee }}
313         ])
314 
315-        # Replacement transaction is identical except has double the fee

glozow commented at 10:50 am on September 29, 2022:

Why delete the comment?

w0xlt commented at 1:40 am on October 4, 2022:

Was inadvertently. Done in 17cad448516a6906ff637593ab57df332fade5d2.

in test/functional/rpc_packages.py:304 in b21329d506 outdated

330         ])
331 
332         self.log.info("Test that packages cannot conflict with mempool transactions, even if a valid BIP125 RBF")
333-        node.sendrawtransaction(signed_replaceable_tx["hex"])
334-        testres_rbf_single = node.testmempoolaccept([signed_replacement_tx["hex"]])
335-        # This transaction is a valid BIP125 replace-by-fee

glozow commented at 10:50 am on September 29, 2022:

Why delete the comment?

w0xlt commented at 1:40 am on October 4, 2022:

Was inadvertently. Done in 17cad448516a6906ff637593ab57df332fade5d2.

in test/functional/test_framework/wallet.py:340 in b21329d506 outdated

336@@ -338,6 +337,19 @@ def sendrawtransaction(self, *, from_node, tx_hex, maxfeerate=0, **kwargs):
337         self.scan_tx(from_node.decoderawtransaction(tx_hex))
338         return txid
339 
340+    def create_self_transfer_chain(self, *, chain_length):

glozow commented at 10:53 am on September 29, 2022:

Nice, but needs documentation. You can copy paste the docstring from make_chain to here.

w0xlt commented at 1:39 am on October 4, 2022:

Done in 17cad448516a6906ff637593ab57df332fade5d2.

test: refactor `RPCPackagesTest` to use `MiniWallet` 17cad44851

w0xlt force-pushed on Oct 4, 2022

glozow commented at 7:35 pm on November 17, 2022: member

ACK 17cad448516a6906ff637593ab57df332fade5d2

glozow requested review from kouloumos on Nov 17, 2022

hernanmarino commented at 5:15 pm on November 21, 2022: contributor

Approach ACK

pablomartin4btc commented at 8:07 pm on November 21, 2022: member

tested ACK 17cad44; went thru all changes and recommendations from @kouloumos & @glozow; also went up to #20833 to get a bit of background of the origin and purpose of these tests.

in test/functional/rpc_packages.py:92 in 17cad44851

106         self.assert_testres_equal(self.independent_txns_hex, self.independent_txns_testres)
107 
108         self.log.info("Test an otherwise valid package with an extra garbage tx appended")
109-        garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {self.address: 1})
110+        address = node.get_deterministic_priv_key().address
111+        garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {address: 1})

kouloumos commented at 9:00 am on November 22, 2022:

nit: get_deterministic_priv_key is not often used alongside the MinWallet is used. Why not use its own method?

0        address = self.wallet.getaddress()
1        garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {address: 1})

Also, if you decide to touch this, garbage_tx_hex follows better the naming scheme.

in test/functional/rpc_packages.py:225 in 17cad44851

278+        testres = node.testmempoolaccept([tx1["hex"], tx1["hex"]])
279         assert_equal(testres, [
280-            {"txid": tx1.rehash(), "wtxid": tx1.getwtxid(), "package-error": "conflict-in-package"},
281-            {"txid": tx1.rehash(), "wtxid": tx1.getwtxid(), "package-error": "conflict-in-package"}
282+            {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"},
283+            {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"}

kouloumos commented at 3:15 pm on November 22, 2022:

nit: I believe that is for the best when similar implementation details follow similar checks in code across functional tests. Although I believe this is important, I consider it a nit as this is not code you’ve written as part of this refactor.

What I am referring to, is the assertion after a testmepoolaccept(). In a similar example at mempool_package_limits, the implementation logic differs a bit and imo improves the readability by not checking for (w)txid. https://github.com/bitcoin/bitcoin/blob/85892f77c98c7a08834a06d52af3eb474275afd8/test/functional/mempool_package_limits.py#L116-L118

Just a note that a similar assertion-of-testmepoolaccept() logic is already a method as part of another test. I’ve tried to implement it as part of this test, but it ended up not worth it. https://github.com/bitcoin/bitcoin/blob/85892f77c98c7a08834a06d52af3eb474275afd8/test/functional/mempool_accept.py#L49-L55

*This assertion-of-testmepoolaccept() pattern can be observed in multiple occasions, this highlights just one of them.

glozow commented at 11:47 am on November 28, 2022:

Agree doing similar checks each time is good and having a helper like check_mempool_result() is a way to achieve that. Fine for a future PR.

in test/functional/rpc_packages.py:388 in 17cad44851

328     def test_submit_cpfp(self):
329         node = self.nodes[0]
330         peer = node.add_p2p_connection(P2PTxInvStore())
331 
332-        # 2 parent 1 child CPFP. First parent pays high fees, second parent pays 0 fees and is
333-        # fee-bumped by the child.

kouloumos commented at 3:22 pm on November 22, 2022:

comment good

kouloumos commented at 3:26 pm on November 22, 2022: contributor

ACK 17cad448516a6906ff637593ab57df332fade5d2 looks good to me, just some minor nits.

rserranon commented at 5:03 pm on November 22, 2022: none

Approach ACK

glozow merged this on Nov 28, 2022

glozow closed this on Nov 28, 2022

w0xlt deleted the branch on Nov 28, 2022

sidhujag referenced this in commit c329008a55 on Dec 1, 2022

bitcoin locked this on Nov 28, 2023

test: refactor RPCPackagesTest to use MiniWallet #25986

Reviews

Conflicts

test: refactor `RPCPackagesTest` to use `MiniWallet` #25986