Skip to content
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.

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

doc: Improve ZMQ documentation #23471

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

mbildwic
Copy link
Contributor

@mbildwic mbildwic commented Nov 9, 2021

This PR intends to clarify:

. when ZMQ notifications occur
. the message structure of each topic

Closes #23452 (comment)

@fanquake fanquake added the Docs label Nov 9, 2021
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK

The description looks good and helps the reader understand the topics in better detail. I agree with these changes. However, there is a minor nit that needs to be addressed.

doc/zmq.md Outdated Show resolved Hide resolved
Copy link
Contributor

@0xB10C 0xB10C left a comment

Choose a reason for hiding this comment

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

Concept ACK on improving the documentation. Thanks for picking this up!

I left a few comments below that I think should be addressed.

doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
@mbildwic mbildwic changed the title doc: Add more information about ZMQ topics doc: Improve ZMQ documentation Nov 12, 2021
@mbildwic
Copy link
Contributor Author

Thanks for review @0xB10C and @shaavan . I addressed your suggestions.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

ACK 079576e

doc/zmq.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK 079576e

Changes since my last review:

  • The definitions of topics are expanded upon. They are now much more detailed and help to understand these terms better.
  • A note is added explaining the difference in the representation of 32-byte hashes and RPC interface and block explorer.

There are no critical grammatical mistakes in the updated PR, and I find the expanded description to be logically sound and easy to understand. Great Work @mbildwic!

@tylerchambers
Copy link
Contributor

Thanks! This clears things up.

doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
doc/zmq.md Outdated Show resolved Hide resolved
@mbildwic mbildwic force-pushed the doc_add_zmq_desc branch 2 times, most recently from 717d863 to 8ce9f74 Compare November 16, 2021 23:22
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK 8ce9f74

Changes since my last review:

  • Text explaining that the topics are ZMQ multipart has been added for all four topics now.
  • Proper explanation of the trigger event has been added for hashtx and hashblock topics. This is done to make the description more robust if there is an ordering change of topics in the future.
  • String describing the syntax of rawtx and rawblock is added, along with the previously added syntax of hashtx and hashblock.

I agree with the updated changes.

However, I think the description has become too lengthy, with a lot of repeated content in succession. I don’t exactly know how to tackle this. Maybe bolding or italicizing the main difference between topics might help, but I don’t know if that would be the right approach.

@0xB10C
Copy link
Contributor

0xB10C commented Nov 23, 2021

ACK 8ce9f74.

If more people agree that bold or italic text would help (as mentioned in #23471 (review)), then please add it. I don't mind but don't think it's crucial.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK, thanks for working on this!

doc/zmq.md Outdated Show resolved Hide resolved
@0xB10C 0xB10C mentioned this pull request Nov 29, 2021
3 tasks
Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

This PR adds solid clarifications about the ZMQ topics that will be very helpful for all potential users of the ZMQ interface -- this would have also saved me quite some time in the past. Kudos for expicitely mentioning the fact that rawtx/hashtx notifications appear both when added to the mempool or in new blocks. LGTM!

ACK 9544ab6

@maflcko maflcko merged commit 6c04b50 into bitcoin:master Dec 14, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 14, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZMQ documentation unclear on what info notifications provide.
9 participants