[test] Refactor ZMQ test to use one address per notification type #11439

With this change it is possible to test the notifications independently, regardless of the publishing order, since each notification type is published to a different address.

Took the opportunity to do some cleanup with the help of:

• Subscriber class to keep the subscriber metadata and sequence state;
• ZMQTest.subscribe which creates and registers the new subscriber and setups the socket;
• ZMQTest.receive which does the normal asserts upon reading from the socket;

Also changed 10 to 5 blocks to speed up the test.

In #10286 the commit Fix zmq tests now that txn/blocks are unordered (04feed) can be removed after this is merged. cc @TheBlueMatt

Hmm, I removed the commit in question from #10286 (replaced with https://github.com/bitcoin/bitcoin/pull/10286/commits/fdfd31355bfd1401c60e1ff3ec581d8f3ff29291). Its really unclear to me what the ZMQ API is, even - its not defined anywhere if clients should or should not rely on the ordering here. The fact that it was possible for our test to implies to me that we should maybe keep the ordering consistent, but I'm open to someone doing the work to document an stable API with or without a consistent ordering.

There is no order guarantee and as such the clients can't and shouldn't rely on that - that's why the type is the first piece in each message. IMO we should avoid the ordering in the test because it is a bad example on how to subscribe and process the notifications.

After some discussion with @TheBlueMatt on IRC we came to the conclusion that there should be an improvement to the ZMQ interface documentation regarding all notifications, respective payloads and guarantees.

Concept ACK. I like the refactor and placing the repeated checking logic into its own method. A couple of nits inline.

However, this doesn't actually do what I was expecting, which is remove the assumptions about ordering. I was expecting the ZMQ subscribers to be able to receive the notifications asynchronously (perhaps using something like https://github.com/bitcoin/bitcoin/blob/master/contrib/zmq/zmq_sub.py) so that ordering of notifications doesn't matter.

@jnewbery I believe we don't want the loop right?

https://github.com/bitcoin/bitcoin/blob/dbc4ae03963014ab4b7957d62ba59dbd8f938c33/contrib/zmq/zmq_sub.py#L41

I like the Subscriber class and subscribe/receive methods here which eliminate so much boilerplate in the test. But I unless I'm missing something I think you need to go back to receiving the messages over a single channel instead of 4 channels if your actual goal is to make it "possible to test the notifications independently, regardless of the publishing order."

Otherwise it seems like this change is does the exact opposite of what you are claiming. If you are using a single channel, it's easy to read n messages, sort them, and make sure you've gotten the information you expected regardless of ordering. But if you have 4 channels, you either need to (1) create threads or coroutines or asynchronous callbacks to check incoming messages on each channel or (2) demultiplex 4 channels back into a single channel so you can take the sorting approach. Simplicity can be a matter of perspective, but I don't either of these approaches is simpler than just using a single channel.

Useful discussion of zmq notification ordering here: https://botbot.me/freenode/bitcoin-core-dev/2017-10-03/?msg=91859584&page=1

I still think this PR could be a useful simplification in the test code.

This is controversial and unneeded at the moment, as @TheBlueMatt said, the current order (although not documented) will not change and this test (as it is in master) helps to verify it.

Therefore I'm closing this and I'll submit another with the cleanup and another with improved ZMQ interface documentation.