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.
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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]
this is not needed anymore i believe
I believe tx_info is being reused used in line 92
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?
good catch! I replaced it with testmempoolaccept with FromHex to get the txid and wtxid.
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 | + )
nit
timelock_tx, _ = wallet.create_self_transfer(
from_node=self.nodes[0],
utxo_to_spend=utxo,
locktime=self.nodes[0].getblockcount() + 2
)
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)
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
asserted to be correct
There is an assertion: assert_equal(tx_info['vsize'], vsize)
Concept ACK, very nice!
Concept ACK
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
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)
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.
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.
Sorry, there was a bug. If you rebase, the bug should fix itself.
yes, it worked! I updated the PR to use miniwallet.scan_blocks starting from block 76.
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)
nit: shouldn't the append be after sendrawtx as per this comment
🤔
switched it around!
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
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']
done!
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 :)
@DariusParvin are you planning on following up with the review comments here?
@fanquake Yes, thanks for prompting me, I'll push updates this weekend!
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.
@stackman27 thanks for your review! I added logs instead of comments, as you suggested.
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)]
b = [self.nodes[0].getblockhash(n) for n in range(first_block, first_block+4)]
Any reason you didn't fix this as well on the latest force push?
sorry i didn't see this comments, i'll fix it now!
just pushed it. Thanks for the review
cr ACK a856630c27cd28186d4e39dd1129f0cae8149261
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.
The test is still skipped when wallet is disabled. I think you need to delete:
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
@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
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),
nit: s/invalidatblock/invalidateblock Might as well fix the typo
thanks! by the way, should I be squashing commits when making small fixes to the PR or will they get squashed at merge time?
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.
ACK 65dd482ba8c87443e4c27dc28f2027625ca43005
Reviewed code, test passes (and not skipped) with wallet disabled.
Re-ACK 163e8881e88a798462a60b5232a6b4ff62e1969a
s/disable/disabled/ in commit message 163e8881e88a798462a60b5232a6b4ff62e1969a
- 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
thanks @MarcoFalke! typo fixed and rebased
cr ACK a3f0cbf82ddae2dd83001a9cc3a7948dcfb6fa47