test: Remove dead code from interface

fjahr commented at 8:12 pm on September 21, 2024: contributor

The loop removed here appears to be effectively dead code: In case get_raw_seq is behind zmq_mem_seq the loop runs and tries to get a more recent (higher) number for get_raw_seq. However, the exact number of get_raw_seq is asserted in the line above: assert_equal(get_raw_seq, 6). If the loop would actually achieve its purpose this assert would need to be racy. This does not seem to be the case and 6 appears to be the final number. zmq_mem_seq however does take some time to catch up (if it were continue to be updated). But this is not handled by the loop and does not seem to be relevant at this point in the test. The backlog is consumed a bit later in another loop that handles this correctly already.

DrahtBot commented at 8:12 pm on September 21, 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	l0rinc, tdb3

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

DrahtBot added the label Tests on Sep 21, 2024

in test/functional/interface_zmq.py:492 in ed1af8386b outdated

489@@ -490,12 +490,6 @@ def test_mempool_sync(self):
490         mempool_view = set(mempool_snapshot["txids"])
491         get_raw_seq = mempool_snapshot["mempool_sequence"]
492         assert_equal(get_raw_seq, 6)

tdb3 commented at 7:24 pm on September 22, 2024:

Thought about this assert being turned into a loop (retrying until get_raw_seq is 6), but it doesn’t seem to fail currently, and the sync_all() call above might be helping to ensure this.

l0rinc commented at 11:39 am on September 24, 2024:

is it always num_txs + 1? If so, could we assert that instead?

fjahr commented at 7:44 pm on September 29, 2024:

get_raw_seq is always 6 and num_txs is always 5, so we can write it like this. Not sure if this is an improvement but it doesn’t hurt so I have changed it.

tdb3 approved

tdb3 commented at 7:27 pm on September 22, 2024: contributor

CR and light test ACK ed1af8386b34eafed7a2d634ab96d23d6732e5bf

Thanks for simplifying the test.

maflcko requested review from instagibbs on Sep 23, 2024

bitcoin deleted a comment on Sep 23, 2024

in test/functional/interface_zmq.py:498 in ed1af8386b outdated

489@@ -490,12 +490,6 @@ def test_mempool_sync(self):
490         mempool_view = set(mempool_snapshot["txids"])
491         get_raw_seq = mempool_snapshot["mempool_sequence"]
492         assert_equal(get_raw_seq, 6)
493-        # Snapshot may be too old compared to zmq message we read off latest
494-        while zmq_mem_seq >= get_raw_seq:
495-            sleep(2)
496-            mempool_snapshot = self.nodes[0].getrawmempool(mempool_sequence=True)
497-            mempool_view = set(mempool_snapshot["txids"])
498-            get_raw_seq = mempool_snapshot["mempool_sequence"]

l0rinc commented at 11:26 am on September 24, 2024:

Would it make sense to assert that this is never true anymore?

0        assert(zmq_mem_seq < get_raw_seq)

fjahr commented at 7:45 pm on September 29, 2024:

Added as belt and suspender

l0rinc approved

l0rinc commented at 11:39 am on September 24, 2024: contributor

Tested with

0pip3 install pyzmq --break-system-packages
1git clean -fxd
2cmake -B build -DWITH_ZMQ=ON && cmake --build build -j10
3python3 build/test/functional/interface_zmq.py

passed on master; changed the affected part in the build folder to:

0assert(zmq_mem_seq < get_raw_seq)
1while zmq_mem_seq >= get_raw_seq:
2    raise Exception("Shouldn't happen")

still passed, no exception - seems like dead code indeed, unless the behavior is not as deterministic as we think.

Please consider adding an assertion in place of this so that we can see if it turns out that there’s a race condition here (where ZMQ messages are processed faster than the mempool snapshot updates - if I understood the problem correctly).

fanquake referenced this in commit 39219fe145 on Sep 25, 2024

fjahr force-pushed on Sep 29, 2024

l0rinc approved

l0rinc commented at 7:43 pm on September 29, 2024: contributor

ACK eb20d8263b0da828624261eb9df0f9cc1f4f9f96

DrahtBot requested review from tdb3 on Sep 29, 2024

test: Remove dead code from interface_zmq c4dc81f9c6

in test/functional/interface_zmq.py:492 in eb20d8263b outdated

494-        while zmq_mem_seq >= get_raw_seq:
495-            sleep(2)
496-            mempool_snapshot = self.nodes[0].getrawmempool(mempool_sequence=True)
497-            mempool_view = set(mempool_snapshot["txids"])
498-            get_raw_seq = mempool_snapshot["mempool_sequence"]
499+        assert_equal(get_raw_seq, num_tx + 1)

l0rinc commented at 7:45 pm on September 29, 2024:

should likely be num_txs + 1

fjahr commented at 7:46 pm on September 29, 2024:

fixed

fjahr force-pushed on Sep 29, 2024

DrahtBot added the label CI failed on Sep 29, 2024

DrahtBot commented at 7:46 pm on September 29, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30824044440

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc commented at 7:47 pm on September 29, 2024: contributor

ACK c4dc81f9c6980964f63b9ad5166cd4cfaa86f3e6

tdb3 approved

tdb3 commented at 7:50 pm on September 29, 2024: contributor

CR re ACK c4dc81f9c6980964f63b9ad5166cd4cfaa86f3e6

DrahtBot removed the label CI failed on Sep 29, 2024

bitcoin deleted a comment on Sep 30, 2024

fanquake merged this on Oct 28, 2024

fanquake closed this on Oct 28, 2024

test: Remove dead code from interface_zmq test #30942

Code Coverage

Reviews