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

w0xlt · 2022-09-02T16:02:05Z

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.

kouloumos

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 · 2022-09-03T08:25:39Z

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

kouloumos

Looks good, some minor comments

test/functional/rpc_packages.py

DrahtBot · 2022-09-03T15:14:25Z

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.

glozow

Looks good, thanks for working on this!

test/functional/rpc_packages.py

test/functional/test_framework/wallet.py

test/functional/rpc_packages.py

test/functional/test_framework/wallet.py

glozow

ACK 17cad44

hernanmarino

Approach ACK

pablomartin4btc

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.

kouloumos

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

kouloumos · 2022-11-22T09:00:08Z

test/functional/rpc_packages.py

+        address = node.get_deterministic_priv_key().address
+        garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {address: 1})


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

Suggested change

address = node.get_deterministic_priv_key().address

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

address = self.wallet.getaddress()

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.

kouloumos · 2022-11-22T15:15:19Z

test/functional/rpc_packages.py

+            {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"},
+            {"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"}


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.

bitcoin/test/functional/mempool_package_limits.py

Lines 116 to 118 in 85892f7

testres_too_long = node.testmempoolaccept(rawtxs=package_hex)

for txres in testres_too_long:

assert_equal(txres["package-error"], "package-mempool-limits")

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.

bitcoin/test/functional/mempool_accept.py

Lines 49 to 55 in 85892f7

def check_mempool_result(self, result_expected, *args, **kwargs):

"""Wrapper to check result of testmempoolaccept on node_0's mempool"""

result_test = self.nodes[0].testmempoolaccept(*args, **kwargs)

for r in result_test:

r.pop('wtxid') # Skip check for now

assert_equal(result_expected, result_test)

assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state

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

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.

kouloumos · 2022-11-22T15:22:31Z

test/functional/rpc_packages.py

-        # 2 parent 1 child CPFP. First parent pays high fees, second parent pays 0 fees and is
-        # fee-bumped by the child.


comment good

rserranon · 2022-11-22T17:03:15Z

Approach ACK

w0xlt force-pushed the issue_25965 branch from e5accec to 41773da Compare September 2, 2022 16:06

fanquake requested a review from glozow September 2, 2022 16:31

fanquake added the Tests label Sep 2, 2022

kouloumos reviewed Sep 2, 2022

View reviewed changes

w0xlt force-pushed the issue_25965 branch 2 times, most recently from 941ae7f to 10edc9c Compare September 3, 2022 08:23

kouloumos reviewed Sep 3, 2022

View reviewed changes

w0xlt force-pushed the issue_25965 branch 2 times, most recently from df7e74b to 4d4298f Compare September 3, 2022 20:19

DrahtBot mentioned this pull request Sep 4, 2022

policy: nVersion=3 and Package RBF #25038

Closed

glozow reviewed Sep 6, 2022

View reviewed changes

test/functional/rpc_packages.py Outdated Show resolved Hide resolved

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

w0xlt force-pushed the issue_25965 branch from 4d4298f to b21329d Compare September 10, 2022 00:08

fanquake mentioned this pull request Sep 22, 2022

remove create_child_with_parents, make_chain, and create_raw_chain from test_framework/wallet #25965

Closed

glozow reviewed Sep 29, 2022

View reviewed changes

test: refactor RPCPackagesTest to use MiniWallet

17cad44

w0xlt force-pushed the issue_25965 branch from b21329d to 17cad44 Compare October 4, 2022 01:38

DrahtBot mentioned this pull request Nov 4, 2022

policy: Ephemeral anchors #26403

Closed

glozow reviewed Nov 17, 2022

View reviewed changes

glozow requested a review from kouloumos November 17, 2022 19:35

hernanmarino reviewed Nov 21, 2022

View reviewed changes

pablomartin4btc reviewed Nov 21, 2022

View reviewed changes

kouloumos reviewed Nov 22, 2022

View reviewed changes

glozow merged commit 00c3236 into bitcoin:master Nov 28, 2022

w0xlt deleted the issue_25965 branch November 28, 2022 18:43

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2022

Merge bitcoin#25986: test: refactor RPCPackagesTest to use MiniWallet

c329008

bitcoin locked and limited conversation to collaborators Nov 28, 2023

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

		{"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"},
		{"txid": tx1["txid"], "wtxid": tx1["wtxid"], "package-error": "conflict-in-package"}

	testres_too_long = node.testmempoolaccept(rawtxs=package_hex)
	for txres in testres_too_long:
	assert_equal(txres["package-error"], "package-mempool-limits")

	def check_mempool_result(self, result_expected, args, *kwargs):
	"""Wrapper to check result of testmempoolaccept on node_0's mempool"""
	result_test = self.nodes[0].testmempoolaccept(args, *kwargs)
	for r in result_test:
	r.pop('wtxid') # Skip check for now
	assert_equal(result_expected, result_test)
	assert_equal(self.nodes[0].getmempoolinfo()['size'], self.mempool_size) # Must not change mempool state

		# 2 parent 1 child CPFP. First parent pays high fees, second parent pays 0 fees and is
		# fee-bumped by the child.

test: refactor RPCPackagesTest to use MiniWallet #25986

test: refactor RPCPackagesTest to use MiniWallet #25986

Uh oh!

Conversation

w0xlt commented Sep 2, 2022

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Sep 3, 2022

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Sep 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

kouloumos Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

rserranon commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

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

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

DrahtBot commented Sep 3, 2022 •

edited

Loading