test, assumeutxo: Add test to ensure failure when mempool not empty

hernanmarino commented at 5:40 am on February 7, 2024: contributor

Add a test to ensure that loadtxoutset fails when the node’s mempool is not empty, as suggested by maflcko here: #27596 (review)

DrahtBot commented at 5:40 am on February 7, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, BrandonOdiwuor
Stale ACK	fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in test/functional/feature_assumeutxo.py:64 in 4a4d8ad3be outdated

60         self.rpc_timeout = 120
61         self.extra_args = [
62             [],
63             ["-fastprune", "-prune=1", "-blockfilterindex=1", "-coinstatsindex=1"],
64             ["-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1"],
65+            ["-persistmempool=0"]

maflcko commented at 8:17 am on February 7, 2024:

Could use one of the other nodes and restart?

hernanmarino commented at 8:10 pm on February 7, 2024:

Will do.

in test/functional/feature_assumeutxo.py:142 in 4a4d8ad3be outdated

140+        self.log.info("Test bitcoind should fail when mempool not empty.")
141+        node=self.nodes[3]
142+        self.log.info("Creating transaction in node 3's mempool")
143+        miniwallet = MiniWallet(node)
144+        tx = miniwallet.send_self_transfer(from_node=node)
145+

maflcko commented at 8:17 am on February 7, 2024:

0 
1        tx = self.miniwallet.send_self_transfer(from_node=node)

It already exists

hernanmarino commented at 7:04 pm on February 7, 2024:

Reusing self.mini_wallet was my first approach but the following error is produced:

 0File "./test/functional/feature_assumeutxo.py", line 143, in test_invalid_mempool_state
 1    tx = self.mini_wallet.send_self_transfer(from_node=node)
 2  File "./test/functional/test_framework/wallet.py", line 251, in send_self_transfer
 3    self.sendrawtransaction(from_node=from_node, tx_hex=tx['hex'])
 4  File "./test/functional/test_framework/wallet.py", line 371, in sendrawtransaction
 5    txid = from_node.sendrawtransaction(hexstring=tx_hex, maxfeerate=maxfeerate, **kwargs)
 6  File "./test/functional/test_framework/coverage.py", line 50, in __call__
 7    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 8  File "./test/functional/test_framework/authproxy.py", line 129, in __call__
 9    raise JSONRPCException(response['error'], status)
10test_framework.authproxy.JSONRPCException: bad-txns-inputs-missingorspent (-25)
112024-02-07T18:53:14.644000Z TestFramework (INFO): Stopping nodes

I believe this happens to the fact that self.mini_wallet is tied to node 0 (and its UTXOs) but node 0 uses most of them in run_test, and the other UTXOs left available from node 0 ’s mining and self transfers are not (and should not be) known to node 3.

Given this situation, i thought the simplest and most straigthforward way of inserting a tx in node 3 ’s mempool is usign a wallet tied to it. This tx will never be mined, but i need for it to be accepted in the mempool.

Am I missing something or there an alternetive (and simpler) way you can suggest?

maflcko commented at 10:22 am on February 8, 2024:

Ah right, the nodes are on different chains, so the utxo sets the miniwallet can use are different.

maflcko approved

in test/functional/feature_assumeutxo.py:142 in 4a4d8ad3be outdated

135@@ -135,6 +136,19 @@ def expected_error(log_msg="", error_msg=""):
136         rmtree(chainstate_snapshot_path)
137         self.start_node(0)
138 
139+    def test_invalid_mempool_state(self, dump_output_path):
140+        self.log.info("Test bitcoind should fail when mempool not empty.")
141+        node=self.nodes[3]
142+        self.log.info("Creating transaction in node 3's mempool")

fjahr commented at 1:02 pm on February 7, 2024:

nit: Could remove this log line

hernanmarino force-pushed on Feb 8, 2024

in test/functional/feature_assumeutxo.py:63 in 802194012d outdated

59@@ -60,7 +60,7 @@ def set_test_params(self):
60         self.extra_args = [
61             [],
62             ["-fastprune", "-prune=1", "-blockfilterindex=1", "-coinstatsindex=1"],
63-            ["-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1"],
64+            ["-persistmempool=0","-txindex=1", "-blockfilterindex=1", "-coinstatsindex=1"]

maflcko commented at 7:24 pm on February 8, 2024:

please don’t remove the trailing comma

in test/functional/feature_assumeutxo.py:149 in 802194012d outdated

144+
145+        # Attempt to load the snapshot on Node 2 and expect it to fail
146+        with node.assert_debug_log(expected_msgs=["[snapshot] can't activate a snapshot when mempool not empty"]):
147+            assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", node.loadtxoutset, dump_output_path)
148+
149+        self.restart_node(2, extra_args=[*self.extra_args[2]] )

maflcko commented at 7:25 pm on February 8, 2024:

why the extra_args juggling? Isn’t this set by default already?

hernanmarino commented at 8:26 pm on February 8, 2024:

Apparently extra_args are needed when calling restart_node. Removing it causes an assertion failure later in the code in run_test (I also confirmed this with the debugger.) But you are right that there was some extra juggling in the code with the * and [] , I fixed it in the latest commit push I’ve just submitted.

hernanmarino commented at 9:00 pm on February 8, 2024:

By debugging and digging deeper in the code, I see that the extra_args are preserved only if you call setup_nodes() or if you call add_nodes() with the extra_args parameter, something that was not done here by the original author (intentionally, I guess)

fjahr commented at 9:32 pm on February 8, 2024:

@hernanmarino you marked the other thread as resolved while I was writing, this change works and I think that’s what @maflcko had in mind: https://github.com/fjahr/bitcoin/commit/a6883b8919d7348bab56f6ebee0818912146ae52

hernanmarino commented at 6:08 pm on February 12, 2024:

Answered on the other thread :)

maflcko approved

maflcko commented at 7:27 pm on February 8, 2024: member

lgtm

hernanmarino force-pushed on Feb 8, 2024

in test/functional/feature_assumeutxo.py:149 in da13cc2c80 outdated

144+
145+        # Attempt to load the snapshot on Node 2 and expect it to fail
146+        with node.assert_debug_log(expected_msgs=["[snapshot] can't activate a snapshot when mempool not empty"]):
147+            assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", node.loadtxoutset, dump_output_path)
148+
149+        self.restart_node(2, extra_args=self.extra_args[2] )

maflcko commented at 8:13 pm on February 8, 2024:

0        self.restart_node(2 )

why is this needed?

hernanmarino commented at 8:28 pm on February 8, 2024:

Please see my answer to your other question.

fjahr commented at 9:26 pm on February 8, 2024:

There is an extra space after the extra_args option.

Since the -persistmempool=0 is only relevant to this one restart and not the rest of the tests I wouldn’t change the setup and instead do it like this, but how it’s done now is also ok.

Another small improvement might be to switch from node 2 to node 0. None of the args from node 2 are needed for this test, so using node 0, that has no args, may cause less confusion.

0        # Restart to purge the transaction from the mempool again
1        self.restart_node(0, extra_args=["-persistmempool=0"])

fjahr commented at 9:27 pm on February 8, 2024:

Actually, when you switch to use node 0 you can remove this line entirely because node 0 is not used to load snapshots in the rest of the test.

maflcko commented at 8:28 am on February 9, 2024:

If node 0 can load the snapshot, that sounds like a bug, because node 0 created the snapshot, no?

maflcko commented at 10:17 am on February 9, 2024:

I checked this and the test would fail if the early return errors would be shuffled. node0 returns:

0 - 2024-02-09T10:15:58.291593Z (mocktime: 2011-02-02T23:17:17Z) [httpworker.3] [validation.cpp:5392] [PopulateAndValidateSnapshot] [snapshot] activation failed - work does not exceed active chainstate

hernanmarino commented at 6:06 pm on February 12, 2024:

Thanks for testing that @maflcko. I was thinking about this exact possbility when @fjahr suggested his idea a few days ago. I think it ’s better to leave node 2 as a test subject.

test, assumeutxo: Add test to ensure failure when mempool not empty 8d20602e55

maflcko commented at 10:17 am on February 9, 2024: member

lgtm ACK da13cc2c8010cbccf37324cca4403cb346ecdd30

Thanks

maflcko requested review from fjahr on Feb 12, 2024

fjahr commented at 12:30 pm on February 12, 2024: contributor

utACK da13cc2c8010cbccf37324cca4403cb346ecdd30

I still think my suggestions here would be an improvement but are not strictly needed.

DrahtBot removed review request from fjahr on Feb 12, 2024

maflcko added this to the milestone 27.0 on Feb 12, 2024

hernanmarino commented at 6:11 pm on February 12, 2024: contributor

utACK da13cc2

I still think my suggestions here would be an improvement but are not strictly needed.

Thanks for mentioning that, I think that leaving node 2 was a better alternative becuase it makes this a cleaner and more test , and avoids the errors that might arise in future code changes (mentioned by @maflcko and others that might arise) but i was struggling to find a solution and / or follow your suggestion somehow. It’s nice that we can all agree in this way, thanks.

hernanmarino force-pushed on Feb 12, 2024

hernanmarino commented at 6:27 pm on February 12, 2024: contributor

Fixed the last nit: Pushed to remove the extra space mentioned by @fjahr https://github.com/bitcoin/bitcoin/compare/da13cc2c8010cbccf37324cca4403cb346ecdd30..8d20602e555dfe026b421363ee32cfca17c674d8

BrandonOdiwuor commented at 6:41 pm on February 12, 2024: contributor

Concept ACK

maflcko commented at 9:03 am on February 13, 2024: member

Please don’t invalidate two ACKs for a single space style issue. If you care about the style, it is your responsibility to take care of before opening the pull request, or before pushing the code. You can use any auto-formatter of your choice.

re-ACK 8d20602e555dfe026b421363ee32cfca17c674d8

DrahtBot requested review from fjahr on Feb 13, 2024

DrahtBot requested review from BrandonOdiwuor on Feb 13, 2024

BrandonOdiwuor approved

BrandonOdiwuor commented at 9:45 am on February 13, 2024: contributor

ACK 8d20602e555dfe026b421363ee32cfca17c674d8

fanquake merged this on Feb 13, 2024

fanquake closed this on Feb 13, 2024

test, assumeutxo: Add test to ensure failure when mempool not empty #29394

Code Coverage

Reviews