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

0mq - Zero MQ Support. #2415

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@ghost

ghost commented Mar 27, 2013

Using a publisher socket to publish three things when they are
happening in the Bitcoin client:

  1. New transactions
  2. New blocks
  3. New ipaddresses

This way there are no need to ask the bitcoin client if something
new has happen. You will be notified!

To compile:

make -f makefile.unix USE_ZMQ=1

To run:

bitcoind -zmqpubbind="ipc://.bitcoin.pub"

A Python client that will print all the information:

[CODE]
import zmq

def main():

context = zmq.Context()
socket = context.socket(zmq.SUB)
socket.setsockopt(zmq.SUBSCRIBE, "")
socket.connect("ipc://.bitcoin.pub")

while True:
    msg = socket.recv()
    print "%s" % (msg[8:])

if name == 'main':
main()
[/CODE]

For more commandline options, see bitcoind --help.

Fredrik Danerklint
0mq - Zero MQ Support.
Using a publisher socket to publish three things when they are
happening in the Bitcoin client:

1) New transactions
2) New blocks
3) New ipaddresses

This way there are no need to ask the bitcoin client if something
new has happen. You will be notified!

To compile:

USE_ZMQ=1 make -f makefile.unix

To run:

bitcoind -zmqpubbind="ipc://.bitcoin.pub"

A Python client that will print all the information:

[CODE]
import zmq

def main():

    context = zmq.Context()
    socket = context.socket(zmq.SUB)
    socket.setsockopt(zmq.SUBSCRIBE, "")
    socket.connect("ipc://.bitcoin.pub")

    while True:
        msg = socket.recv()
        print "%s" % (msg[8:])

if __name__ == '__main__':
    main()
[/CODE]

For more commandline options, see bitcoind --help.
@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Mar 27, 2013

I certainly like stuff like this over the forking...

What is the usecase for the IP addresses?

This removes the fIsInitialDownload test around the forking blocknotify— that test is incorrect to begin with (it can make you miss reorgs if the blocks have particular weird times), but without it blocknotify is a forkbomb. :)

@ghost

This comment has been minimized.

ghost commented Mar 27, 2013

The IP address is just to tell when the Bitcoin client has gotten a new IP address from the network.

I moved the test for 'fIsInitialDownload' so it only evaluates once.(two times in the master code at the moment if you take a look).

Hmm. There will be scenarios were you would like to have the blocks announced during 'fIsInitialDownload' and when you don't. Will make an parameter for that.

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Mar 27, 2013

Ah, missed that curly removal.

In any case that doesn't change the point that IsInitial is not a reliable test, go look at how it's implemented. The way that it's setup there— in theory— a miner could construct a reorg that doesn't trigger the notice. This might be bad if you're depending on being told about the reorg in order to abort shipping a good or whatever.

@jgarzik

This comment has been minimized.

Contributor

jgarzik commented Mar 27, 2013

+1 for 0mq support in general. Reading now...

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 28, 2013

I like this. Delivering events over 0mq for block/transaction/etc
notifications is much more elegant and efficient than launching commands.

@petertodd

This comment has been minimized.

Contributor

petertodd commented Mar 28, 2013

Note that 0mq is LGPL, which is a bit more restrictive than the MIT/BSD license that bitcoin-qt is currently licensed under. Specifically LGPL requires you to distribute binaries that can be relinked if the user decides to change the source of the LGPL library. Not an issue with dynamic linking, but static linking is problematic. Other than some graphics assets - see assets-attribution.txt - this would be a new requirement for bitcoin-qt.

Having said that, personally I'm happy to ACK the change; users should not be using a copy of bitcoin-qt if the source code isn't available to them.

@gavinandresen

This comment has been minimized.

Contributor

gavinandresen commented Mar 28, 2013

Nice!
Can you put the trivial python client in contrib/0mq/ ?

Also:
0mq is available in MacPorts, does version matter?
zmq @3.2.2 devel/zmq
zmq20 @2.0.11 devel/zmq20
zmq22 @2.2.0 devel/zmq22

Fredrik Danerklint
Added a parameter to publish new blocks during the state
'IsInitialDownload' in main.cpp.

Did notice a document called doc/coding.txt. Oups.

To compile with 0mq:
--------------------
make -f makefile.unix USE_ZMQ=1

To compile without 0mq:
-----------------------
make -f makefile.unix USE_ZMQ=0

or

make -f makefile.unix
@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Mar 28, 2013

@petertodd I was aware of the licensing— I don't think it's problematic. We have LGPL (QT) and advertising-clause BSD (OpenSSL) in parts of the system which are more essential... ZeroMQ can at least just be left out by someone practicing extreme licensing confusion. GMP for modular inversion will be a bigger question in the future.

@ghost

This comment has been minimized.

ghost commented Mar 28, 2013

@gavinandresen

Yes, the version does matter. Use the version 3.2.2 (which is the latest stable one).

Quote from 0mq: "We recommend this release to anyone developing new applications with ØMQ"

I will include example client(s) under contrib/0mq in later commitments.

@petertodd

This comment has been minimized.

Contributor

petertodd commented Mar 28, 2013

@gmaxwell Yup, it's a niche case for it to actually matter, but we should document it clearly for the lawyers; might as well put the info about our dependencies and associated license obligations in assets-attribution.txt or a similar file. (yes, I'm volunteering to write that up)

Fredrik Danerklint added some commits Mar 28, 2013

Fredrik Danerklint
Implemented a reply socket, which means that there is now
a RPC Server with 0MQ!

It's running in a seperate thread.

All rpc command(s) should work, since we are calling the same
functions which the current RPC Server is using.

An example in Lua is under contrib/0mq/bitcoin-zmq_reqrep.lua.

Start with:

'bitcoind -zmqrepbind="ipc://.bitcoin.rep"'

and then run the example client.
@BitcoinPullTester

This comment has been minimized.

BitcoinPullTester commented Mar 30, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7c05592af68dd7d1ec9219c1afe76207797f3214 for binaries and test log.
This is an automated test script which runs test cases on each commit every time is updated.
It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
and contact BlueMatt on freenode if something looks broken.

@ghost

This comment has been minimized.

ghost commented Mar 30, 2013

@ BitcoinPullTester

You need to build with make -f makefile.unix USE_ZMQ=1

to actually know if this pull request builds or not. You do need to have libzmq installed before that.

@mikehearn

This comment has been minimized.

Contributor

mikehearn commented Apr 1, 2013

The idea is neat, but there doesn't seem to be much documentation on how to use it or what form the messages take. For instance you call the message "new block" but that's not correct is it, it's "new best block" - you won't see blocks that connect to a side chain and if a re-org happens (I think) you will only see the new top block and not the intervening blocks published on the queue. Or did I misunderstand the code?

@ghost

This comment has been minimized.

ghost commented Apr 1, 2013

@mikehearn

What the publisher does is to send new messages when something happens in the bitcoin client. You don't have to ask if something has happened, you already know that.

Under 'contrib/0mq/bitcoin-zmq.py' is an example client of how the publisher part works and their filter defined.

Please run this and take a look of what the output is. (Hint: all json)

To your other questions that depends on where in the source code that happens. If it's in the main.cpp - SetBestChain, then yes if not, tell me where and I will add that as well.

@luke-jr

This comment has been minimized.

Member

luke-jr commented Apr 8, 2013

Needs rebase.

@mikehearn

This comment has been minimized.

Contributor

mikehearn commented Apr 8, 2013

Don't get me wrong, new best block isn't a bad thing to notify on. New side blocks probably aren't that interesting for most users anyway. Just pointing out that the naming/docs could be updated.

@TripleSpeeder

This comment has been minimized.

Contributor

TripleSpeeder commented Apr 12, 2013

This makes a LOT of usecases possible. I use an older fork of bitcoind, based on even older implementation by Gavin, exactly for this purpose - Have an automatic callback whenever a transaction and/or block is coming in (See my fork on github).

One suggestion:
Add an option to get the "incoming transaction" notification only when it is for the own wallet. I think this would be a frequent usecase, so the receiving end does not need to do boilerplate code, probably involving additional rpc calls, to find out if the tx is actually interesting.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 15, 2013

Some comments after code review:

  • There is no authentication for requests over ZMQ, do I get this right?
  • When compiling with USE_ZMQ, enabling it should be an option, not be done by default. This allows builders of Linux distributions (for example) to build an executable with all options enabled, and leaves it up to the end user to enable it or not.
}
} else
//we are only sleeping if we don't have anything to do.
usleep(10000);

This comment has been minimized.

@laanwj

laanwj Apr 15, 2013

Member

Do you need this usleep? If zeromq has a way to block XXms waiting for a request, I'd prefer that. It would result in less latency when responding to requests and means the process doesn't have to wake up unnecessarily.

@ghost

This comment has been minimized.

ghost commented Apr 15, 2013

@laanwj

The usleep is only running if there is nothing for the thread to do. If we have a lot of request coming in, we will process this until the queue is empty. The reason why I have done this is to be able to shutdown the bitcoin process.

0mq does not any authentication method. That's correct. Only allow it from trusted sources.

The default value for USE_ZMQ is zero (0) in the makefile.unix. That's why you need USE_ZMQ=1 to be able to compile it with 0mq support.

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 15, 2013

You seem to be completely misunderstanding me

  1. I know why you added the usleep, so my suggestion is to use a "blocking read" for the same time instead of an usleep (which doesn't process messages when they come in). Not an unbounded blocking wait, of course, that would indeed make it impossible to exit the client.

  2. I agree with "allow it from trusted sources only". How does this work? Is there some form of access control at all? Or can everyone on localhost connect to it by default?

  3. I understand that. My suggestion is to make it possible to compile with USE_ZMQ=1 but disable zeromq, so that it is possible to compile binaries with zeromq support compiled in, distribute them, and leave it up to the user to enable it or not. Enabling it should not only depend on a compile-time option.

@jgarzik

This comment has been minimized.

Contributor

jgarzik commented May 30, 2013

Agree with @laanwj comments.

Let's go those addressed, and get this merged.

@doublec

This comment has been minimized.

doublec commented May 31, 2013

Would it be faster to split this into two pull requests? The first to do the pub/sub notifications of transactions, blocks and ip addresses. The second for the zmq based RPC server. The reason for splitting the latter is there is likely to be much discussion over authentication which would bog down the acceptance of the notification patch,

I've been using my own zmq patch for notification of blocks in my mining pool which works very similar to this one and it has been operating well for the past couple of years.

@luke-jr

This comment has been minimized.

Member

luke-jr commented Jul 17, 2013

@Fredan Are you going to fix this up?

@jgarzik

This comment has been minimized.

Contributor

jgarzik commented Aug 25, 2013

Ping @Fredan. Also, ping @doublec in case you are motivated to split this up, and get 0mq support moving again.

@laanwj

This comment has been minimized.

Member

laanwj commented Aug 29, 2013

Splitting it into two sounds like a good idea. I suppose the notifications can be unauthenticated.

@gavinandresen

This comment has been minimized.

Contributor

gavinandresen commented Oct 21, 2013

Closing this because of inactivity. If you have time to rework this so it merges cleanly against the current tree, please open a new pull request (and link to the discussion in this closed request, if it is relevant).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment