test: Remove dead code from interface_zmq test #30942

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2024-10-zmq-test changing 1 files +2 −7
  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. 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.

  3. DrahtBot added the label Tests on Sep 21, 2024
  4. 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.
  5. tdb3 approved
  6. tdb3 commented at 7:27 pm on September 22, 2024: contributor

    CR and light test ACK ed1af8386b34eafed7a2d634ab96d23d6732e5bf

    Thanks for simplifying the test.

  7. maflcko requested review from instagibbs on Sep 23, 2024
  8. bitcoin deleted a comment on Sep 23, 2024
  9. bitcoin deleted a comment on Sep 23, 2024
  10. 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
  11. l0rinc approved
  12. 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).

  13. fanquake referenced this in commit 39219fe145 on Sep 25, 2024
  14. fjahr force-pushed on Sep 29, 2024
  15. l0rinc approved
  16. l0rinc commented at 7:43 pm on September 29, 2024: contributor
    ACK eb20d8263b0da828624261eb9df0f9cc1f4f9f96
  17. DrahtBot requested review from tdb3 on Sep 29, 2024
  18. test: Remove dead code from interface_zmq c4dc81f9c6
  19. 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
  20. fjahr force-pushed on Sep 29, 2024
  21. DrahtBot added the label CI failed on Sep 29, 2024
  22. 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.

  23. l0rinc commented at 7:47 pm on September 29, 2024: contributor
    ACK c4dc81f9c6980964f63b9ad5166cd4cfaa86f3e6
  24. tdb3 approved
  25. tdb3 commented at 7:50 pm on September 29, 2024: contributor
    CR re ACK c4dc81f9c6980964f63b9ad5166cd4cfaa86f3e6
  26. DrahtBot removed the label CI failed on Sep 29, 2024
  27. bitcoin deleted a comment on Sep 30, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me