test: run mempool_compatibility.py even with wallet disabled

mjdietzx commented at 6:52 pm on December 17, 2020: contributor

Another functional test rewritten as proposed in https://github.com/bitcoin/bitcoin/issues/20078

DrahtBot added the label Tests on Dec 17, 2020

in test/functional/mempool_compatibility.py:53 in 655673b57e outdated

54+        self.sync_all()
55 
56+        self.stop_node(1)
57         self.log.info("Add a transaction to mempool on old node and shutdown")
58-        old_tx_hash = old_node.sendtoaddress(recipient, 0.0001)
59+        old_tx_hash = old_wallet.send_self_transfer(from_node=old_node)['txid']

MarcoFalke commented at 3:11 pm on December 21, 2020:

no need to replace this. old nodes always have a wallet

in test/functional/mempool_compatibility.py:44 in 655673b57e outdated

44-        self.stop_node(1)
45+        old_node, new_node = self.nodes
46+        old_wallet = MiniWallet(old_node)
47+        new_wallet = MiniWallet(new_node)
48+
49+        # Add enough mature utxos to the wallets so that all txs spend confirmed coins.

MarcoFalke commented at 3:11 pm on December 21, 2020:

No need for this comment? There is only one inputs, so this is clear from reading the code.

MarcoFalke commented at 3:11 pm on December 21, 2020: member

Concept ACK

mjdietzx force-pushed on Dec 22, 2020

mjdietzx commented at 7:00 pm on December 22, 2020: contributor

The 1st commit https://github.com/bitcoin/bitcoin/pull/20688/commits/f2b7cb56c8cbec9c6a2a8ec88143756974f6860f is ready to go now

I realized that I could extend the test coverage a bit, see 2nd commit https://github.com/bitcoin/bitcoin/pull/20688/commits/6a1e9adeaea8a12a23ec4be5d59cec4fccd0c196. I know it’s not totally related to this PR. Maybe it seems more appropriate for a test like this to go in mempool_accept.py, but this test-case does fit in naturally to MempoolCompatibilityTest, and seems a little tricky to do in mempool_accept.py the way that test is setup.

What do you think is the best way forward? If you want it in this PR I can squash the commits, otherwise I’ll ditch the 2nd commit

mjdietzx force-pushed on Dec 22, 2020

in test/functional/mempool_compatibility.py:74 in 45238d7961 outdated

67@@ -69,7 +68,23 @@ def run_test(self):
68         self.stop_node(1)
69 
70         self.log.info("Move mempool.dat from new to old node")
71-        os.rename(new_node_mempool, old_node_mempool)
72+        shutil.copyfile(new_node_mempool, old_node_mempool)
73+
74+        # Coinbase is only valid in a block, not as a loose transaction.
75+        # Because the nodes have not been synced, old_node will not accept unbroadcasted tx to its mempool.

instagibbs commented at 1:58 am on December 24, 2020:

can you convert this “nodes have not been synced” statement into an assertion? May catch future test regressions, and is more self-explanatory

in test/functional/mempool_compatibility.py:73 in 45238d7961 outdated

67@@ -69,7 +68,23 @@ def run_test(self):
68         self.stop_node(1)
69 
70         self.log.info("Move mempool.dat from new to old node")
71-        os.rename(new_node_mempool, old_node_mempool)
72+        shutil.copyfile(new_node_mempool, old_node_mempool)
73+
74+        # Coinbase is only valid in a block, not as a loose transaction.

instagibbs commented at 2:01 am on December 24, 2020:

I don’t understand why this comment is here

edit: ah, because it’s a coinbase output being spent? Can we convert this into an assertion?

instagibbs commented at 2:03 am on December 24, 2020: member

concept ACK

mjdietzx commented at 5:48 pm on December 24, 2020: contributor

Hey @instagibbs thanks for the review. How does this look? https://github.com/bitcoin/bitcoin/pull/20688/commits/174da9f63a51747e7297949414edba6bf9d673dc. lmk if you have any ideas on how this can be done better - I know it doesn’t perfectly address your comments, but seemed like a good balance to me without adding too much code or extra stuff

instagibbs commented at 9:48 am on December 26, 2020: member

better thanks, you can squash the last two commits together :+1:

ACK otherwise

mjdietzx force-pushed on Dec 26, 2020

jonatack commented at 5:09 pm on December 29, 2020: member

ACK bff5aca27d0d66587c0155642a9c6cd88228ed29 code review, tested with bitcoind built with the wallet and without

in test/functional/mempool_compatibility.py:41 in bff5aca27d outdated

37@@ -33,13 +38,15 @@ def setup_network(self):
38             None,
39         ])
40         self.start_nodes()
41-        self.import_deterministic_coinbase_privkeys()
42+        self.init_wallet(0)  # only import_deterministic_coinbase_privkeys for old_node

MarcoFalke commented at 11:28 am on January 5, 2021:

Does this line need to change? If yes, why?

mjdietzx commented at 6:04 pm on January 5, 2021:

Good point - this change is not necessary. I got rid of it and force pushed

mjdietzx force-pushed on Jan 5, 2021

in test/functional/mempool_compatibility.py:83 in 88d0d9c4e9 outdated

84         self.start_node(0, ['-nowallet'])
85         assert old_tx_hash in old_node.getrawmempool()
86+        # Because the nodes haven't been synced, old_node doesn't have the block that contains the coinbase that unbroadcasted_tx spends.
87+        # As coinbases are only valid in a block, and not as loose txns, unbroadcasted_tx won't pass `MemPoolAccept::PreChecks`.
88+        assert_raises_rpc_error(-5, "Block not found", old_node.getblock, blockhash=block_hash)
89+        assert unbroadcasted_tx_hash not in old_node.getrawmempool()

MarcoFalke commented at 12:54 pm on January 7, 2021:

I don’t understand why this test is being changed. The only change should be replacing the bitcoin core wallet with miniwallet. If you want to do additional changes, they can be done in a separate commit or pull request.

mjdietzx force-pushed on Jan 7, 2021

test: run mempool_compatibility.py even with wallet disabled a7599c80eb

in test/functional/mempool_compatibility.py:71 in 71c0cb2048 outdated

70 
71-        self.log.info("Move mempool.dat from new to old node")
72+        self.log.info("Sync the nodes and move mempool.dat from new to old node and shutdown")
73+        self.start_node(0, ['-nowallet'])
74+        self.connect_nodes(0, 1)
75+        self.sync_blocks()

MarcoFalke commented at 6:03 pm on January 7, 2021:

Would be nice to move up the sync where the blocks are generated to not accidentally change what the test is testing and preserving previous test behavior

mjdietzx commented at 6:26 pm on January 7, 2021:

Good point, that will also reduce the diff. Done

mjdietzx force-pushed on Jan 7, 2021

MarcoFalke commented at 6:31 pm on January 7, 2021: member

review ACK a7599c80ebb9579df45e2d6ccf3168302cf42f03 didn’t test

MarcoFalke merged this on Jan 8, 2021

MarcoFalke closed this on Jan 8, 2021

DrahtBot locked this on Aug 16, 2022

test: run mempool_compatibility.py even with wallet disabled #20688