test: run mempool_reorg.py even with wallet disabled

DariusParvin commented at 3:39 am on February 15, 2021: contributor

Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.

As part of this PR I created a new method in MiniWallet, create_self_transfer, to return a raw tx (without broadcasting it) and its associated utxo.

DariusParvin force-pushed on Feb 15, 2021

DrahtBot added the label Tests on Feb 15, 2021

DrahtBot commented at 8:44 am on February 15, 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:

#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

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.

in test/functional/test_framework/wallet.py:88 in a96c58aad6 outdated

84+    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, locktime=0):
85+        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
86+
87+        tx_hex, new_utxo = self.create_self_transfer(fee_rate=fee_rate, from_node=from_node, utxo_to_spend=utxo_to_spend, locktime=locktime)
88+
89+        tx_info = from_node.testmempoolaccept([tx_hex])[0]

glozow commented at 1:41 pm on February 16, 2021:

this is not needed anymore i believe

stackman27 commented at 4:11 pm on February 22, 2021:

I believe tx_info is being reused used in line 92

stackman27 commented at 4:31 pm on February 22, 2021:

Also is there a need to recheck testmempoolaccept? Every time we call send_self_transfer it checks the mempool acceptance through create_self_transfer so, can we just return proper values and remove the additional check?

DariusParvin commented at 5:12 am on April 5, 2021:

good catch! I replaced it with testmempoolaccept with FromHex to get the txid and wtxid.

in test/functional/mempool_reorg.py:58 in 2ff5034eb1 outdated

65+        utxo = wallet.get_utxo(txid=coinbase_txids[0])
66+        timelock_tx, _ = wallet.create_self_transfer(
67+            from_node=self.nodes[0],
68+            utxo_to_spend=utxo,
69+            locktime=self.nodes[0].getblockcount() + 2
70+            )

glozow commented at 1:45 pm on February 16, 2021:

nit

0        timelock_tx, _ = wallet.create_self_transfer(
1            from_node=self.nodes[0],
2            utxo_to_spend=utxo,
3            locktime=self.nodes[0].getblockcount() + 2
4         )

in test/functional/test_framework/wallet.py:136 in 2ff5034eb1 outdated

59-        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
60+    def create_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None, locktime=0):
61+        """Create a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
62         self._utxos = sorted(self._utxos, key=lambda k: k['value'])
63         utxo_to_spend = utxo_to_spend or self._utxos.pop()  # Pick the largest utxo (if none provided) and hope it covers the fee
64         vsize = Decimal(96)

glozow commented at 1:53 pm on February 16, 2021:

Not sure if this is relevant to this PR or previous one, but I’m surprised the vsize is hard-coded? It should be programmatically obtained or asserted to be correct after it’s constructed. There’s a get_vsize() for CTransactions in test_framework.messages

MarcoFalke commented at 3:37 pm on February 16, 2021:

asserted to be correct

There is an assertion: assert_equal(tx_info['vsize'], vsize)

glozow commented at 1:56 pm on February 16, 2021: member

Concept ACK, very nice!

felipsoarez commented at 2:11 pm on February 16, 2021: none

Concept ACK

in test/functional/mempool_reorg.py:18 in 2ff5034eb1 outdated

15+from test_framework.wallet import MiniWallet
16 
17 class MempoolCoinbaseTest(BitcoinTestFramework):
18     def set_test_params(self):
19         self.num_nodes = 2
20+        self.setup_clean_chain = True

MarcoFalke commented at 4:54 pm on February 16, 2021:

Generating blocks takes a few seconds in valgrind, so I am thinking if this test may benefit from using miniwallet.scan_blocks (to be introduced in #21200)

DariusParvin commented at 5:13 am on April 5, 2021:

I had a go but I don’t think it helps in this case since the pre-mined chain doesn’t send the coinbase transaction to self._address. I made it little bit faster though by only mining 100 blocks instead of 200.

MarcoFalke commented at 7:01 am on April 20, 2021:

It should. Specifically the last 25 mature blocks (including block 100). And the last 25 immature blocks (including block 200), but you don’t need those.

MarcoFalke commented at 7:13 am on April 29, 2021:

Sorry, there was a bug. If you rebase, the bug should fix itself.

DariusParvin commented at 1:11 am on May 17, 2021:

yes, it worked! I updated the PR to use miniwallet.scan_blocks starting from block 76.

in test/functional/test_framework/wallet.py:89 in 2ff5034eb1 outdated

85+        """Create and send a tx with the specified fee_rate. Fee may be exact or at most one satoshi higher than needed."""
86+
87+        tx_hex, new_utxo = self.create_self_transfer(fee_rate=fee_rate, from_node=from_node, utxo_to_spend=utxo_to_spend, locktime=locktime)
88+
89+        tx_info = from_node.testmempoolaccept([tx_hex])[0]
90+        self._utxos.append(new_utxo)

stackman27 commented at 4:07 pm on February 22, 2021:

nit: shouldn’t the append be after sendrawtx as per this comment 🤔

DariusParvin commented at 5:13 am on April 5, 2021:

switched it around!

in test/functional/test_framework/wallet.py:81 in 2ff5034eb1 outdated

77+        if tx_info['allowed']:
78+            assert_equal(tx_info['vsize'], vsize)
79+            assert_equal(tx_info['fees']['base'], fee)
80+
81+        new_utxo = {'txid': tx_info['txid'], 'vout': 0, 'value': send_value}
82+        return tx_hex,  new_utxo

stackman27 commented at 4:29 pm on February 22, 2021:

I’m not very sure the intuition behind this but, can we just add everything in one variable and use key/value pairs to retrieve things? Seems cleaner to me and wouldn’t have to include dead _ every time I create a tx. For instance instead of doing spend_101_raw, _ = wallet.create_self_transfer(from_node=self.nodes[0], utxo_to_spend=utxo_101) we can simplt do spend_101_raw= wallet.create_self_transfer(from_node=self.nodes[0], utxo_to_spend=utxo_101)['tx_hex']

DariusParvin commented at 5:13 am on April 5, 2021:

done!

stackman27 commented at 4:44 pm on February 22, 2021: contributor

Concept ACK. mempool_reorg looks really good except, is there a reason why we aren’t using self.log.info(...)? I always thought of them to be really helpful while running tests. Therefore, i think it’ll be really nice to add the logs :)

fanquake commented at 8:50 am on April 1, 2021: member

@DariusParvin are you planning on following up with the review comments here?

DariusParvin commented at 8:58 pm on April 2, 2021: contributor

@fanquake Yes, thanks for prompting me, I’ll push updates this weekend!

DariusParvin force-pushed on Apr 5, 2021

DariusParvin commented at 6:05 am on April 20, 2021: contributor

I’ve addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

DrahtBot added the label Needs rebase on Apr 29, 2021

DariusParvin force-pushed on May 17, 2021

DrahtBot removed the label Needs rebase on May 17, 2021

DariusParvin force-pushed on May 17, 2021

DariusParvin commented at 1:50 am on May 17, 2021: contributor

@stackman27 thanks for your review! I added logs instead of comments, as you suggested.

in test/functional/mempool_reorg.py:42 in a856630c27 outdated

51+        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_2 and spend_2_1
52+        # 3. Indirect (coinbase and child both in chain) : spend_3 and spend_3_1
53         # Use invalidatblock to make all of the above coinbase spends invalid (immature coinbase),
54         # and make sure the mempool code behaves correctly.
55-        b = [self.nodes[0].getblockhash(n) for n in range(101, 105)]
56+        b = [self.nodes[0].getblockhash(n) for n in range(76, 80)]

MarcoFalke commented at 7:20 am on May 17, 2021:

0        b = [self.nodes[0].getblockhash(n) for n in range(first_block, first_block+4)]

MarcoFalke commented at 3:39 pm on May 17, 2021:

Any reason you didn’t fix this as well on the latest force push?

DariusParvin commented at 5:08 pm on May 17, 2021:

sorry i didn’t see this comments, i’ll fix it now!

DariusParvin commented at 5:10 pm on May 17, 2021:

just pushed it. Thanks for the review

MarcoFalke approved

MarcoFalke commented at 7:26 am on May 17, 2021: member

cr ACK a856630c27cd28186d4e39dd1129f0cae8149261

michaelfolkson commented at 11:29 am on May 17, 2021: contributor

I’ve addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

Is this still waiting on #20874? Given Marco has reviewed this and not #20874 presumably this is closer to merging? 😅

edit: #20874 needs a rebase so yeah I think this is closer to merging. Obvious Concept ACK too btw.

michaelfolkson commented at 12:27 pm on May 17, 2021: contributor

The test is still skipped when wallet is disabled. I think you need to delete:

0def skip_test_if_missing_module(self):
1        self.skip_if_no_wallet()

DariusParvin force-pushed on May 17, 2021

DariusParvin commented at 3:25 pm on May 17, 2021: contributor

@michaelfolkson oops! I removed those lines. And yes, since #21762 where create_self_transfer was added, this PR is relatively independent of everything else, so it’s not waiting on anything. Cheers

DariusParvin force-pushed on May 17, 2021

DariusParvin force-pushed on May 18, 2021

in test/functional/mempool_reorg.py:40 in 65dd482ba8 outdated

48-        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_102 and spend_102_1
49-        # 3. Indirect (coinbase and child both in chain) : spend_103 and spend_103_1
50+        # 1. Direct coinbase spend  :  spend_1
51+        # 2. Indirect (coinbase spend in chain, child in mempool) : spend_2 and spend_2_1
52+        # 3. Indirect (coinbase and child both in chain) : spend_3 and spend_3_1
53         # Use invalidatblock to make all of the above coinbase spends invalid (immature coinbase),

michaelfolkson commented at 11:26 am on May 27, 2021:

nit: s/invalidatblock/invalidateblock Might as well fix the typo

DariusParvin commented at 8:49 pm on May 27, 2021:

thanks! by the way, should I be squashing commits when making small fixes to the PR or will they get squashed at merge time?

michaelfolkson commented at 10:17 am on May 28, 2021:

On this project we squash commits prior to review and merging. Obviously if there is a reason to have multiple commits then don’t squash but in this case I don’t think there should be a separate commit for a typo.

michaelfolkson commented at 12:05 pm on May 27, 2021: contributor

ACK 65dd482ba8c87443e4c27dc28f2027625ca43005

Reviewed code, test passes (and not skipped) with wallet disabled.

DariusParvin force-pushed on May 28, 2021

michaelfolkson commented at 5:49 pm on May 29, 2021: contributor

Re-ACK 163e8881e88a798462a60b5232a6b4ff62e1969a

DrahtBot added the label Needs rebase on May 31, 2021

MarcoFalke commented at 11:32 am on May 31, 2021: member

s/disable/disabled/ in commit message 163e8881e88a798462a60b5232a6b4ff62e1969a

DariusParvin force-pushed on May 31, 2021

test: run mempool_reorg.py even with wallet disabled

- run mempool_reorg.py even when the wallet is not compiled
- add `locktime` argument to `create_self_transfer` and `send_self_transfer`
- use more logs instead of comments

a3f0cbf82d

DariusParvin force-pushed on May 31, 2021

DrahtBot removed the label Needs rebase on May 31, 2021

DariusParvin commented at 5:41 pm on May 31, 2021: contributor

thanks @MarcoFalke! typo fixed and rebased

MarcoFalke commented at 1:04 pm on June 1, 2021: member

cr ACK a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47

MarcoFalke merged this on Jun 1, 2021

MarcoFalke closed this on Jun 1, 2021

sidhujag referenced this in commit 2b04a4c46a on Jun 1, 2021

gwillen referenced this in commit f46e4fdf55 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: run mempool_reorg.py even with wallet disabled #21178

Conflicts