test: Remove dead code from interface_zmq test #30942

1. 
   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.
2. test: Remove dead code from interface_zmq ed1af8386b
3. 
   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	tdb3

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

4. DrahtBot added the label Tests on Sep 21, 2024
5. in test/functional/interface_zmq.py:492 in ed1af8386b

   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?
6. tdb3 approved
7. 
   tdb3 commented at 7:27 pm on September 22, 2024: contributor

   CR and light test ACK ed1af8386b34eafed7a2d634ab96d23d6732e5bf

   Thanks for simplifying the test.

8. maflcko requested review from instagibbs on Sep 23, 2024
9. bitcoin deleted a comment on Sep 23, 2024
10. bitcoin deleted a comment on Sep 23, 2024
11. in test/functional/interface_zmq.py:498 in ed1af8386b

    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)
    

12. l0rinc approved
13. 
    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).

Contributors
fjahr DrahtBot tdb3 l0rinc

Review Requested
instagibbs

Labels
Tests