[Test] Tests for zmqpubrawtx and zmqpubrawblock

achow101 commented at 1:02 am on June 8, 2017: member

fanquake added the label Tests on Jun 8, 2017

fanquake commented at 7:21 am on June 8, 2017: member

Both linux builds timed out after ~48 minutes.

jnewbery commented at 2:30 pm on June 8, 2017: member

I suspect that this timeout is due to the python3-zmq Context not closing cleanly at the end of the zmq_test.py test. See https://stackoverflow.com/questions/17140417/termination-of-python-script-while-using-zeromq-with-dead-server for example.

I’m working on a branch that fixes that behaviour here: https://github.com/jnewbery/bitcoin/tree/zmqtestimprovements . It’s currently building in Travis. If that works, you might want to try rebasing this PR on top of that branch to see if it resolves this for you.

jnewbery commented at 3:05 pm on June 8, 2017: member

Opened #10555 to fix the python zmq issue. @achow101 - if rebasing on that resolves your travis issue, could you review/ACK that PR?

achow101 force-pushed on Jun 8, 2017

achow101 commented at 6:51 pm on June 8, 2017: member

Rebased on top of #10555

MarcoFalke closed this on Jun 18, 2017

MarcoFalke commented at 12:26 pm on June 18, 2017: member

Reopen. Was closed due to false positive regex match by GitHub.

MarcoFalke reopened this on Jun 18, 2017

achow101 force-pushed on Jun 18, 2017

achow101 commented at 5:26 pm on June 18, 2017: member

Rebased after #10555 merge.

achow101 force-pushed on Jun 19, 2017

achow101 commented at 9:55 pm on August 15, 2017: member

rebased

achow101 force-pushed on Aug 15, 2017

in test/functional/test_framework/util.py:16 in eeac6e6079 outdated

12@@ -13,6 +13,7 @@
13 import random
14 import re
15 import time
16+import hashlib

jnewbery commented at 8:13 pm on August 23, 2017:

nit: why not placed alphabetically like the other imports?

in test/functional/test_framework/util.py:292 in eeac6e6079 outdated

288@@ -288,6 +289,13 @@ def connect_nodes_bi(nodes, a, b):
289     connect_nodes(nodes[a], b)
290     connect_nodes(nodes[b], a)
291 
292+def sha256d(byte_str):

jnewbery commented at 8:13 pm on August 23, 2017:

Please place this in the ‘Utility Functions’ section, rather than ‘Node Functions’

in test/functional/zmq_test.py:72 in eeac6e6079 outdated

68@@ -63,25 +69,49 @@ def _zmq_test(self):
69         body = msg[1]
70         msgSequence = struct.unpack('<I', msg[-1])[-1]
71         assert_equal(msgSequence, 0)  # must be sequence 0 on hashtx
72+        txhash = body

jnewbery commented at 8:14 pm on August 23, 2017:

why not just assign txhash on line 69. There’s no reason to have body in this section.

in test/functional/zmq_test.py:104 in eeac6e6079 outdated

100+        body = msg[1]
101+        msgSequence = struct.unpack('<I', msg[-1])[-1]
102+        assert_equal(msgSequence, 0) #must be sequence 0 on rawblock
103+        
104+        # Check the hash of the rawblock's header matches generate
105+        assert_equal(genhashes[0], bytes_to_hex_str(sha256d(body[:80]))) #blockhash from generate must be equal to the hash of the block header

jnewbery commented at 8:15 pm on August 23, 2017:

No need for the duplicated comment

in test/functional/zmq_test.py:149 in eeac6e6079 outdated

141@@ -107,8 +142,17 @@ def _zmq_test(self):
142         hashZMQ = bytes_to_hex_str(body)
143         msgSequence = struct.unpack('<I', msg[-1])[-1]
144         assert_equal(msgSequence, blockcount + 1)
145-
146+        
147+        msg = self.zmqSubSocket.recv_multipart()
148+        topic = msg[0]
149+        body = msg[1]
150+        hashedZMQ = ""

jnewbery commented at 8:16 pm on August 23, 2017:

No need for this line

in test/functional/zmq_test.py:144 in eeac6e6079 outdated

146+        
147+        msg = self.zmqSubSocket.recv_multipart()
148+        topic = msg[0]
149+        body = msg[1]
150+        hashedZMQ = ""
151+        assert_equal(topic, b"rawtx")

jnewbery commented at 8:16 pm on August 23, 2017:

I’d move this assert to immediately after the topic = ... line (like in the previous tests in this script).

jnewbery changes_requested

jnewbery commented at 8:20 pm on August 23, 2017: member

An unfortunate aspect of this test is that it assumes an order for ZMQ notifications (tx > rawtx > hashblock > rawblock). I don’t think that’s necessary or should be asserted in the test. However, you’ve just extended what’s already here, so concept ACK.

Please run a linter over this and fix up the warnings. There are a bunch of nits (unused imports, trailing whitespace, etc).

A few more nits inline.

achow101 force-pushed on Aug 23, 2017

achow101 commented at 11:50 pm on August 23, 2017: member

Nits addresses (I think) and rebased (there were merge conflicts).

jnewbery commented at 10:57 pm on September 6, 2017: member

Tested ACK ba0b4f1c0a2c47f2f8c342686c94c6e358ea4efe, but needs rebase.

achow101 commented at 11:07 pm on September 6, 2017: member

Rebased

achow101 force-pushed on Sep 6, 2017

jnewbery commented at 11:53 pm on September 6, 2017: member

Tested ACK a3696dcec45f11f202c1666084c4aa1f8fabd37f

in test/functional/test_framework/util.py:183 in a3696dcec4 outdated

179@@ -179,6 +180,13 @@ def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=N
180     assert_greater_than(timeout, time.time())
181     raise RuntimeError('Unreachable')
182 
183+def sha256d(byte_str):

promag commented at 11:54 pm on September 6, 2017:

Nit, rename to hash256 to resemble CHash256?

achow101 commented at 3:44 pm on September 18, 2017:

Done

in test/functional/zmq_test.py:45 in a3696dcec4 outdated

37@@ -37,9 +38,12 @@ def setup_nodes(self):
38         self.zmqSubSocket.set(zmq.RCVTIMEO, 60000)
39         self.zmqSubSocket.setsockopt(zmq.SUBSCRIBE, b"hashblock")
40         self.zmqSubSocket.setsockopt(zmq.SUBSCRIBE, b"hashtx")
41+        self.zmqSubSocket.setsockopt(zmq.SUBSCRIBE, b"rawblock")
42+        self.zmqSubSocket.setsockopt(zmq.SUBSCRIBE, b"rawtx")
43         ip_address = "tcp://127.0.0.1:28332"
44         self.zmqSubSocket.connect(ip_address)
45-        self.extra_args = [['-zmqpubhashtx=%s' % ip_address, '-zmqpubhashblock=%s' % ip_address], []]
46+        self.extra_args = [['-zmqpubhashtx=%s' % ip_address, '-zmqpubhashblock=%s' % ip_address,

promag commented at 11:57 pm on September 6, 2017:

Ultra nit, sort arguments.

achow101 commented at 3:44 pm on September 18, 2017:

Done

promag commented at 0:05 am on September 7, 2017: member

In the future we could (partially) change to one socket per notification. As it is, the order matters and requires checking for topic when iterating.

promag commented at 0:14 am on September 7, 2017: member

Just saw @jnewbery comment above:

An unfortunate aspect of this test is that it assumes an order for ZMQ notifications (tx > rawtx > hashblock > rawblock).

Can be fixed with one socket per notification.

achow101 force-pushed on Sep 18, 2017

jnewbery commented at 3:51 pm on September 18, 2017: member

Can be fixed with one socket per notification. @promag - sounds like a very sensible change. This PR simply extends what’s already there so I think we should merge this as is. I’d be happy to review a follow-up PR if you want to implement that.

promag commented at 4:06 pm on September 18, 2017: member

@jnewbery agree. As for the follow up, I think it could be added somewhere in TestFramework something to abstract the sockets creation, to ease testing subscribers. As it is, it’s too much verbose for testing purpose.

in test/functional/test_framework/util.py:183 in d30a9ab27c outdated

179@@ -179,6 +180,13 @@ def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=N
180     assert_greater_than(timeout, time.time())
181     raise RuntimeError('Unreachable')
182 
183+def hash256(byte_str):

promag commented at 5:52 pm on September 18, 2017:

Move before hex_str_to_bytes.

achow101 commented at 2:08 am on September 19, 2017:

Done

promag commented at 5:53 pm on September 18, 2017: member

utACK.

Tests for zmqpubrawtx and zmqpubrawblock d3677ab757

achow101 force-pushed on Sep 19, 2017

jnewbery commented at 1:34 pm on September 19, 2017: member

@promag

As for the follow up, I think it could be added somewhere in TestFramework something to abstract the sockets creation

If you’re thinking of abstracting away the ZMQ interface, I think it would be better to add a new class in test_node.py to be owned by the TestNode class, like how I added a TestNodeCLI class in #10798

However, the ZMQ interface is only used in this one test script, and I think it’s best to keep it that way, so I think it’s best to leave the implementation in the zmq_test.py file.

We’ve wondered off-topic a bit for this PR. If you’re interested in pursuing this further, feel free to ping me on IRC or open a separate issue.

achow101 commented at 4:15 pm on September 28, 2017: member

Can this be merged?

MarcoFalke merged this on Sep 29, 2017

MarcoFalke closed this on Sep 29, 2017

MarcoFalke referenced this in commit 9c3c9cdae3 on Sep 29, 2017

MarcoFalke referenced this in commit f11dce1d20 on Oct 3, 2017

MarcoFalke referenced this in commit 7e17432986 on Oct 3, 2017

MarcoFalke referenced this in commit 6fcbafb177 on Oct 3, 2017

MarcoFalke referenced this in commit 1e5b901a48 on Oct 3, 2017

MarcoFalke referenced this in commit e4605d9dd4 on Oct 3, 2017

codablock referenced this in commit 7ddd169212 on Sep 24, 2019

barrystyle referenced this in commit 8163f186e2 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

[Test] Tests for zmqpubrawtx and zmqpubrawblock #10552