Skip to content

qa: Test ZMQ notification after chain reorg #16404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 3 commits into from
Aug 26, 2019
Merged

qa: Test ZMQ notification after chain reorg #16404

maflcko merged 3 commits into from
Aug 26, 2019

Conversation

@promag
Copy link
Contributor

@promag promag commented Jul 17, 2019

No description provided.

@promag promag mentioned this pull request Jul 17, 2019
Closed
@fanquake fanquake added the Tests label Jul 17, 2019
@promag promag force-pushed the 2019-07-zmq-reorg branch 4 times, most recently from 910474b to e11aa5a Compare July 17, 2019 22:34
kallewoof
kallewoof reviewed Aug 5, 2019
View reviewed changes
test/functional/interface_zmq.py Outdated
Copy link
Contributor

@kallewoof kallewoof Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it normal to import stuff in the middle of a file in python? Don't see it often. And you could remove the one below in test_reorg if you just moved this to top..

Copy link
Contributor Author

@promag promag Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already the case. It's like this to not break if zmq module is not available. See skip_if_no_py3_zmq() above.

Copy link
Contributor

@kallewoof kallewoof Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense..

Copy link
Member

@maflcko maflcko Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can move the import zmq right after the line skip_if_no_bitcoind_zmq? :)

Copy link
Contributor Author

@promag promag Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? It's then available to other scopes? I'll try.

Copy link
Contributor Author

@promag promag Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove import zmq and keep the one you said I get NameError: name 'zmq' is not defined.

@laanwj
Copy link
Member

laanwj commented Aug 5, 2019

ACK e11aa5a6f2683741567f0aaa08d3b198d219913f

maflcko
maflcko reviewed Aug 5, 2019
View reviewed changes
maflcko
maflcko reviewed Aug 5, 2019
View reviewed changes
@promag promag force-pushed the 2019-07-zmq-reorg branch from e11aa5a to dae02dc Compare August 5, 2019 14:23
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 14, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag promag force-pushed the 2019-07-zmq-reorg branch from dae02dc to abdfc5e Compare August 19, 2019 00:26
@promag
Copy link
Contributor Author

promag commented Aug 19, 2019

Rebased.

@promag
Copy link
Contributor Author

promag commented Aug 25, 2019

IMO this is ready to merge.

@maflcko
Copy link
Member

maflcko commented Aug 26, 2019

Indeed. Thanks for the ping

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 26, 2019
Merge bitcoin#16404: qa: Test ZMQ notification after chain reorg
af4100c 
abdfc5e qa: Test ZMQ notification after chain reorg (João Barbosa)
aa2622a qa: Refactor ZMQ test (João Barbosa)
6bc1ff9 doc: Add note regarding ZMQ block notification (João Barbosa)

Pull request description:

Top commit has no ACKs.

Tree-SHA512: b93237adc8c84b3aa72ccc28097090eabcb006cf408083218bebf6fec703bd0de2ded80b6879e77096872e14ba9402a6d3f923b146a54d4c4e41dcb862c3e765
@maflcko maflcko merged commit abdfc5e into bitcoin:master Aug 26, 2019
@promag promag deleted the 2019-07-zmq-reorg branch August 26, 2019 15:31
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 27, 2019
@sidhujag
Merge bitcoin#16404: qa: Test ZMQ notification after chain reorg
0ff9761 
abdfc5e qa: Test ZMQ notification after chain reorg (João Barbosa)
aa2622a qa: Refactor ZMQ test (João Barbosa)
6bc1ff9 doc: Add note regarding ZMQ block notification (João Barbosa)

Pull request description:

Top commit has no ACKs.

Tree-SHA512: b93237adc8c84b3aa72ccc28097090eabcb006cf408083218bebf6fec703bd0de2ded80b6879e77096872e14ba9402a6d3f923b146a54d4c4e41dcb862c3e765
@maflcko
Copy link
Member

maflcko commented Aug 27, 2019

This test fails on travis

@promag
Copy link
Contributor Author

promag commented Aug 27, 2019

The test should run pretty fast, on my system around 1s:

2019-08-27T20:59:50.749000Z TestFramework (INFO): Test the getzmqnotifications RPC
2019-08-27T20:59:51.603000Z TestFramework (INFO): Stopping nodes

But on one travis failure it takes around 60s to fail:

2019-08-27T17:18:36.885000Z TestFramework (INFO): Test the getzmqnotifications RPC
2019-08-27T17:19:37.373000Z TestFramework (ERROR): Unexpected exception caught during testing

and looking at the error:

zmq.error.Again: Resource temporarily unavailable

I assume the receive timeouts due to:

        socket.set(zmq.RCVTIMEO, 60000)

I suspect it happens because by the time the message is published there is no subscriber connected - this is called a "slow joiner" in http://zguide.zeromq.org/py:all#sockets-and-patterns - and I think this happens because we are restart_node after socket.connect. I'll submit a PR to invert this order to see if it stops happening, otherwise we'll have to make sure the subscriber is ready before publishing messages.

@promag promag mentioned this pull request Aug 27, 2019
Merged
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 3, 2020
@promag @majcosta
[backport#16404] doc: Add note regarding ZMQ block notification
a9f7ee1 
Summary:
bitcoin/bitcoin@6bc1ff9

Partial backport of Core [[bitcoin/bitcoin#16404 | PR16404]]

Test Plan:
  read it

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7342
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 3, 2020
@promag @majcosta
[backport#16404] qa: Refactor ZMQ test
ce68479 
Summary:
bitcoin/bitcoin@aa2622a

Partial backport of Core [[bitcoin/bitcoin#16404 | PR16404]] and [[https://github.com/bitcoin/bitcoin/pull/16740| PR16740]] (PR16404 on its own is broken and will not pass the tests otherwise, sorry)

notes:

~~* test looks to be wrong. socket.connect() is running before there is a node with the proper `-zmqxx=xx` CLI parameters set, so I moved it accordingly.~~

Test Plan:
  ninja
  ./test/functional/test_runner.py interface_zmq

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7343
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 3, 2020
@promag @majcosta
[backport#16404] qa: Test ZMQ notification after chain reorg
14653a0 
Summary:
bitcoin/bitcoin@abdfc5e

---

Depends on D7343

Concludes backport of Core [[bitcoin/bitcoin#16404 | PR16404]] and [[https://github.com/bitcoin/bitcoin/pull/16740| PR16740]] (PR16404 on its own is broken and will not pass the tests otherwise, sorry)

notes:

~~* ditto as the note for D7343 re: callsite of socket.connect(address)~~

Test Plan:
  ./test/functional/test_runner.py interface_zmq

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7344
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Dec 15, 2021
@vijaydasmp
Merge bitcoin#16404: qa: Test ZMQ notification after chain reorg
cf43f40 
abdfc5e qa: Test ZMQ notification after chain reorg (João Barbosa)
aa2622a qa: Refactor ZMQ test (João Barbosa)
6bc1ff9 doc: Add note regarding ZMQ block notification (João Barbosa)

Pull request description:

Top commit has no ACKs.

Tree-SHA512: b93237adc8c84b3aa72ccc28097090eabcb006cf408083218bebf6fec703bd0de2ded80b6879e77096872e14ba9402a6d3f923b146a54d4c4e41dcb862c3e765
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+2 more reviewers

@kallewoof kallewoof kallewoof left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@promag @laanwj @DrahtBot @maflcko @kallewoof @fanquake