test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2106-testMiniWallet changing 3 files +88 −94

MarcoFalke commented at 11:05 AM on June 10, 2021: member
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 :art:
fanquake added the label Tests on Jun 10, 2021
MarcoFalke force-pushed on Jun 10, 2021
MarcoFalke force-pushed on Jun 10, 2021
theStack commented at 11:24 AM on June 10, 2021: member

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).
test: Add txin.sequence option to MiniWallet faff3f35b7

test: Remove unused generate() from test

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

fab871f649

test: Use MiniWallet in test_no_inherited_signaling RBF test fab7e99c2a
test: Run pep-8 on touched test
Can be reviewed with --ignore-all-space
fa7d71f270
MarcoFalke force-pushed on Jun 10, 2021
in test/functional/feature_rbf.py:572 in fa7d71f270
```
 576 | -        # Send tx from which to conflict outputs later
 577 | -        base_txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10"))
 578 | -        self.nodes[0].generate(1)
 579 | -        self.sync_blocks()
 580 | +        wallet = MiniWallet(self.nodes[0])
 581 | +        wallet.scan_blocks(start=76, num=1)
```
mjdietzx commented at 6:07 PM on June 11, 2021:

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?

MarcoFalke commented at 9:14 AM on June 12, 2021:

The constant is derived from https://github.com/bitcoin/bitcoin/blob/96f828ba4d69f8acadf335ee79fa8f06cd33c0a6/test/functional/test_framework/test_framework.py#L736-L748
in test/functional/feature_rbf.py:81 in fa7d71f270
```
  80 | @@ -78,10 +81,7 @@ def skip_test_if_missing_module(self):
  81 |          self.skip_if_no_wallet()
```
mjdietzx commented at 6:08 PM on June 11, 2021:

Did you forget to remove this now that the test can be run without the wallet compiled?

MarcoFalke commented at 9:14 AM on June 12, 2021:

Can it?

mjdietzx commented at 2:34 PM on June 12, 2021:

Nope 👍
in test/functional/feature_rbf.py:622 in fa7d71f270
```
 666 |          # BIP 125 :
 667 |          # 1. The original transactions signal replaceability explicitly or through inheritance as described in the above
 668 |          # Summary section.
 669 | -        # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_txid`) does.
 670 | +        # The original transaction (`optout_child_tx`) doesn't signal RBF but its parent (`optin_parent_tx`) does.
 671 |          # The replacement transaction (`replacement_child_tx`) should be able to replace the original transaction.
```
mjdietzx commented at 6:33 PM on June 11, 2021:

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

MarcoFalke commented at 9:18 AM on June 12, 2021:

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 commented at 6:55 PM on June 11, 2021: contributor

Concept ACK: MiniWallet good
mjdietzx commented at 2:46 PM on June 12, 2021: contributor

Tested ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a thanks for the explanations, nicely done
DrahtBot commented at 7:48 AM on June 16, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #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 approved
theStack commented at 11:57 AM on June 18, 2021: member

ACK fa7d71f270b89c9d06230d4ff262646f9ea29f4a 🍷

Follow-up idea based on discussion #22210 (review): 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.
MarcoFalke merged this on Jun 19, 2021
MarcoFalke closed this on Jun 19, 2021
MarcoFalke deleted the branch on Jun 19, 2021
sidhujag referenced this in commit ea43647e15 on Jun 20, 2021
MarcoFalke referenced this in commit da1c0c64fd on Jul 30, 2021
gwillen referenced this in commit cfa325712a on Jun 1, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#22209 getmempoolentry returns "incorrect" bip125-replaceable status #22330 test: use MiniWallet for simple doublespend sub-test in feature_rbf.py

test: Use MiniWallet in test_no_inherited_signaling RBF test #22210

Conflicts