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

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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?

            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

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

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

    assert(zmq_mem_seq < get_raw_seq)
while zmq_mem_seq >= get_raw_seq:
    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

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  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
  28. fanquake merged this on Oct 28, 2024
  29. fanquake closed this on Oct 28, 2024
  30. TheCharlatan referenced this in commit a73b2bd0f0 on Nov 14, 2024
  31. bug-castercv502 referenced this in commit fdcc066ca0 on Sep 28, 2025
  32. knst referenced this in commit 5343be30f0 on Oct 22, 2025
  33. bitcoin locked this on Oct 28, 2025
Contributors
fjahrDrahtBottdb3l0rinc


instagibbs

Labels
Tests
Linked (view graph)
#30946 doc: correct the zmq automatic build info
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: 2026-05-02 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me