test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

maflcko · 2021-06-10T11:05:35Z

This comes with nice benefits:

Less code and complexity
Test can be run without wallet compiled in

Also add some additional checks for getmempoolentry (#22209) and other cleanups 🎨

theStack · 2021-06-10T11:24:47Z

Concept ACK

Not strictly related to this PR, but: the parameter locktime is currently unused in send_self_transfer, i.e. it is not passed on to create_self_transfer. Now that this part is touched in this PR, this could also be addressed (e.g. in the first commit).

This is already done by the test framework in setup_nodes()

Can be reviewed with --ignore-all-space

mjdietzx · 2021-06-11T18:07:54Z

test/functional/feature_rbf.py

-        self.nodes[0].generate(1)
-        self.sync_blocks()
+        wallet = MiniWallet(self.nodes[0])
+        wallet.scan_blocks(start=76, num=1)


Where does the hardcoded 76 come from? It seems to me you just need a mature utxo here? If that's the case and I'm not missing anything, could you do something like: start=blockheight - COINBASE_MATURITY?

The constant is derived from

bitcoin/test/functional/test_framework/test_framework.py

Lines 736 to 748 in 96f828b

# Create a 199-block-long chain; each of the 3 first nodes

# gets 25 mature blocks and 25 immature.

# The 4th address gets 25 mature and only 24 immature blocks so that the very last

# block in the cache does not age too much (have an old tip age).

# This is needed so that we are out of IBD when the test starts,

# see the tip age check in IsInitialBlockDownload().

gen_addresses = [k.address for k in TestNode.PRIV_KEYS][:3] + [ADDRESS_BCRT1_P2WSH_OP_TRUE]

assert_equal(len(gen_addresses), 4)

for i in range(8):

cache_node.generatetoaddress(

nblocks=25 if i != 7 else 24,

address=gen_addresses[i % len(gen_addresses)],

)

test/functional/feature_rbf.py

mjdietzx · 2021-06-11T18:33:20Z

test/functional/feature_rbf.py

        # Summary section.
-        # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_txid`) does.
+        # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_tx`) does.
        # The replacement transaction (`replacement_child_tx`) should be able to replace the original transaction.


Is my understanding here correct?

a) testmempoolaccept returns that replacement_child_tx is allowed even though it is rejected w/ 'txn-mempool-conflict'
b) getmempoolentry returns that optout_child_tx is 'bip125-replaceable' - as you descbribe in #22209 even though it is not

So does anything need to be done to fix a and b?

Another question I have (which maybe I should just try out). But, I wonder what would happen if you tried something like https://github.com/bitcoin/bitcoin/pull/22210/files#diff-260c479178e3e99a16798aeed39f400676894392864784eda6c207716704398dR584-R589 after optout_child_tx was already sent

a) No, it is also rejected by testmempoolaccept, see mempool_valid=False for replacement_child_tx above.
b) correct

So does anything need to be done to fix a and b?

Maybe. But a fix should be done outside a test-only change.

practicalswift · 2021-06-11T18:55:16Z

Concept ACK: MiniWallet good

mjdietzx · 2021-06-12T14:46:15Z

Tested ACK fa7d71f thanks for the explanations, nicely done

DrahtBot · 2021-06-16T07:48:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: refactor: various (de)serialization helpers cleanups/improvements #22257 (test: refactor: use FromHex helper for msg serialization from hex, remove ToHex helper by theStack)

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.

theStack

ACK fa7d71f 🍷

Follow-up idea based on discussion #22210 (comment): This will probably not the last test that needs the block number of the first output to ADDRESS_BCRT1_P2WSH_OP_TRUE (i.e. suitable for MiniWallet in its default mode of MiniWalletMode.ADDRESS_OP_TRUE). In the future it may makes sense to put the magic number 76 into a constant.

…ing RBF…

… feature_rbf.py aa02c64 test: use MiniWallet for simple doublespend test in feature_rbf.py (Sebastian Falbesoner) a3f6397 test: feature_rbf.py: make MiniWallet instance available for all sub-tests (Sebastian Falbesoner) 84c8747 test: remove unneeded initialization code in feature_rbf.py (Sebastian Falbesoner) Pull request description: This PR's goal is to prepare the functional test `feature_rbf.py` for more MiniWallet usage. It first gets rid of unused initialization code (I guess that was needed at times when the nodes were still in IBD at the start of tests?), then makes the MiniWallet instance introduced in #22210 available for all sub-tests, and finally, uses that instance in the first sub-test `test_simple_doublespend`. Note that the same idea of replacing the `make_utxo` calls with MiniWallet can be also applied to other sub-tests too; this just serves as a first proof-of-concept. ACKs for top commit: MarcoFalke: re-ACK aa02c64 🌯 Tree-SHA512: 2902dd15d493d83b0790028c92d14fbd99ca05ace704c7011fb38261ce6517aeb810ef9f360fcb701d95887975b6a2911cfe538858d38fceb2c1c2a40afdbe6b

fanquake added the Tests label Jun 10, 2021

maflcko force-pushed the 2106-testMiniWallet branch 2 times, most recently from fa621d8 to fa59d50 Compare June 10, 2021 11:19

MarcoFalke added 4 commits June 10, 2021 13:38

test: Add txin.sequence option to MiniWallet

faff3f3

test: Remove unused generate() from test

fab871f

This is already done by the test framework in setup_nodes()

test: Use MiniWallet in test_no_inherited_signaling RBF test

fab7e99

test: Run pep-8 on touched test

fa7d71f

Can be reviewed with --ignore-all-space

maflcko force-pushed the 2106-testMiniWallet branch from fa59d50 to fa7d71f Compare June 10, 2021 11:39

mjdietzx reviewed Jun 11, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 16, 2021

test: refactor: various (de)serialization helpers cleanups/improvements #22257

Merged

theStack approved these changes Jun 18, 2021

View reviewed changes

maflcko merged commit e172ea8 into bitcoin:master Jun 19, 2021

maflcko deleted the 2106-testMiniWallet branch June 19, 2021 06:51

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 20, 2021

Merge bitcoin#22210: test: Use MiniWallet in test_no_inherited_signal…

ea43647

…ing RBF…

theStack mentioned this pull request Jun 23, 2021

test: use MiniWallet for simple doublespend sub-test in feature_rbf.py #22330

Merged

maflcko mentioned this pull request May 31, 2022

CVE-2021-31876 child transactions do not inherit rbf signaling #25252

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge e172ea8 into merged_master (Bitcoin PR bitcoin/bitcoin#22210)

cfa3257

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

Uh oh!

maflcko commented Jun 10, 2021

Uh oh!

theStack commented Jun 10, 2021

Uh oh!

mjdietzx Jun 11, 2021

Uh oh!

maflcko Jun 12, 2021

Uh oh!

Uh oh!

mjdietzx Jun 11, 2021

Uh oh!

maflcko Jun 12, 2021

Uh oh!

practicalswift commented Jun 11, 2021

Uh oh!

mjdietzx commented Jun 12, 2021

Uh oh!

DrahtBot commented Jun 16, 2021 •

edited

Loading

Uh oh!

theStack left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	# Create a 199-block-long chain; each of the 3 first nodes
	# gets 25 mature blocks and 25 immature.
	# The 4th address gets 25 mature and only 24 immature blocks so that the very last
	# block in the cache does not age too much (have an old tip age).
	# This is needed so that we are out of IBD when the test starts,
	# see the tip age check in IsInitialBlockDownload().
	gen_addresses = [k.address for k in TestNode.PRIV_KEYS][:3] + [ADDRESS_BCRT1_P2WSH_OP_TRUE]
	assert_equal(len(gen_addresses), 4)
	for i in range(8):
	cache_node.generatetoaddress(
	nblocks=25 if i != 7 else 24,
	address=gen_addresses[i % len(gen_addresses)],
	)

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

Uh oh!

Conversation

maflcko commented Jun 10, 2021

Uh oh!

theStack commented Jun 10, 2021

Uh oh!

mjdietzx Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjdietzx Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2021

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 11, 2021

Uh oh!

mjdietzx commented Jun 12, 2021

Uh oh!

DrahtBot commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jun 16, 2021 •

edited

Loading