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

add getzmqnotifications rpc #295

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

antonilol
Copy link
Contributor

this rpc gives information about the active zmq publishers of bitcoin core

currently get_zmq_notifications returns a Vec directly from bitcoind, but a structure like getindexinfo could also be used because the possible values of the "type" field are known

@stevenroose
Copy link
Collaborator

Is this call new? I can't find it here https://developer.bitcoin.org/search.html?q=getzmqnotifications.

Could you also add an entry to the integration test? So that we can see for which versions of core it works.

@antonilol
Copy link
Contributor Author

antonilol commented Apr 19, 2023

it was added in 0.17 (changelog), it is the last command in the help page (bitcoin-cli help) but i cant find it on developer.bitcoin.org
i will add the test later today

@stevenroose
Copy link
Collaborator

Ah it might be one of the hidden RPCs that are meant for developers. Adding the test should be enough :)

@antonilol
Copy link
Contributor Author

this test now will only check if bitcoind returns an empty array because no zmq subscribers are configured in the test. it will check if the rpc is available but not if the struct gets successfully deserialized

can change this later today

@antonilol antonilol force-pushed the getzmqnotifications branch from 702a074 to c219c61 Compare April 19, 2023 18:35
@antonilol
Copy link
Contributor Author

can this be reviewed/merged? i will rebase in a second

@antonilol antonilol force-pushed the getzmqnotifications branch 2 times, most recently from 8d99fbd to d4983f5 Compare June 22, 2023 21:13
@antonilol
Copy link
Contributor Author

i am still interested in this change, can this be reviewed/merged?

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK d4983f5

@tcharding
Copy link
Member

@apoelstra can you take a look at this please, and ack/merge?

@0xB10C
Copy link
Contributor

0xB10C commented Apr 18, 2024

Code Review ACK d4983f5

@antonilol antonilol force-pushed the getzmqnotifications branch 2 times, most recently from 0088e71 to 89945c1 Compare April 24, 2024 09:02
@antonilol antonilol force-pushed the getzmqnotifications branch from 89945c1 to a9be191 Compare April 24, 2024 09:23
@antonilol
Copy link
Contributor Author

antonilol commented Apr 24, 2024

the ci will fail on a dependency that doesnt compile anymore on 1.48 (3+ years old), i dont think this is related to this pr because i did not change any dependencies, it did not fail on the tests ran on master 5 months ago so i assume dependencies updated in the mean time and use functions stabilized after 1.48

the ci runs on my fork (antonilol#1) because it will not auto run here

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a9be191

@apoelstra
Copy link
Member

Will go ahead and merge. On master we need to pin some more deps (or even include a lockfile for CI, ideally) and also there are a bunch of clippy warnings here.

@apoelstra
Copy link
Member

Actually @tcharding can you re-ACK first?

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK a9be191

@apoelstra apoelstra merged commit d44ab83 into rust-bitcoin:master Apr 25, 2024
9 of 10 checks passed
@antonilol antonilol deleted the getzmqnotifications branch April 25, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants