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

Zmq RPC in daemon #2044

Merged
merged 4 commits into from Sep 18, 2017

Conversation

@tewinget
Contributor

tewinget commented May 24, 2017

There are things that could still be improved, but I think it's in a good state to be merged.

EDIT: separated some changes into a different PR, now this PR depends #2184.

Show outdated Hide outdated src/cryptonote_basic/cryptonote_basic.h
Show outdated Hide outdated src/cryptonote_core/blockchain.cpp
Show outdated Hide outdated src/cryptonote_core/tx_pool.cpp
Show outdated Hide outdated src/cryptonote_core/tx_pool.cpp
Show outdated Hide outdated src/cryptonote_core/tx_pool.cpp
Show outdated Hide outdated src/rpc/zmq_server.cpp
Show outdated Hide outdated src/rpc/zmq_server.cpp
Show outdated Hide outdated src/rpc/zmq_server.cpp
running = false;
return;
}

This comment has been minimized.

@moneromooo-monero

moneromooo-monero May 27, 2017

Contributor

That looks racy.

@moneromooo-monero

moneromooo-monero May 27, 2017

Contributor

That looks racy.

This comment has been minimized.

@tewinget

tewinget Sep 5, 2017

Contributor

changed

@tewinget

tewinget Sep 5, 2017

Contributor

changed

Show outdated Hide outdated src/serialization/json_object.cpp
@fluffypony

This comment has been minimized.

Show comment
Hide comment
@fluffypony

fluffypony May 30, 2017

Collaborator

Needs rebasing, and check review comments

Collaborator

fluffypony commented May 30, 2017

Needs rebasing, and check review comments

@peronero

This comment has been minimized.

Show comment
Hide comment
@peronero

peronero Jun 10, 2017

I hate to be that guy, but what's going on here? Back-and-forth turnaround is seemingly on weeks-to-months timeframes and @tewinget seldom attends the dev meetings, despite this being under the FFS. The funding proposal is sparse in detail but this work seems to be more than 6 months late by the original estimate.

This is a pretty significant component - as I understand it, a hard dependency for functionality like block- and wallet- notify and in turn a roadblock for ecosystem infrastructure development.

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

There is, ostensibly, a dev meeting tomorrow where this can be discussed.

peronero commented Jun 10, 2017

I hate to be that guy, but what's going on here? Back-and-forth turnaround is seemingly on weeks-to-months timeframes and @tewinget seldom attends the dev meetings, despite this being under the FFS. The funding proposal is sparse in detail but this work seems to be more than 6 months late by the original estimate.

This is a pretty significant component - as I understand it, a hard dependency for functionality like block- and wallet- notify and in turn a roadblock for ecosystem infrastructure development.

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

There is, ostensibly, a dev meeting tomorrow where this can be discussed.

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Jun 10, 2017

Contributor
Contributor

tewinget commented Jun 10, 2017

@dternyak

This comment has been minimized.

Show comment
Hide comment
@dternyak

dternyak Jun 10, 2017

@tewinget Thank you for your responsible and courteous reply, as well as the commitment to have ZMQ ready by the end of the month.

I have been following ZMQ's development for a while now, and I, among many others, are excited that it's getting close 👍

dternyak commented Jun 10, 2017

@tewinget Thank you for your responsible and courteous reply, as well as the commitment to have ZMQ ready by the end of the month.

I have been following ZMQ's development for a while now, and I, among many others, are excited that it's getting close 👍

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Jun 10, 2017

@tewinget Thanks

@peronero

This comment has been minimized.

Show comment
Hide comment
@peronero

peronero Jun 11, 2017

Thanks @tewinget.

If I could make one more recommendation - and I really hate to be that guy again - but you might find it useful to break down the work into smaller milestones instead of aiming to deliver the whole thing 3 weeks from now.

Cheers

peronero commented Jun 11, 2017

Thanks @tewinget.

If I could make one more recommendation - and I really hate to be that guy again - but you might find it useful to break down the work into smaller milestones instead of aiming to deliver the whole thing 3 weeks from now.

Cheers

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Jun 11, 2017

Contributor
Contributor

tewinget commented Jun 11, 2017

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Jun 25, 2017

Contributor

Apart from squashing any compiler bugs on various platforms and choosing a version of libzmq to bring in-source and doing that (assuming we still want to do so), I believe all of the comments here have been addressed. Testing is a bit tedious until I've got the client side of things back in good shape -- look for that in the next two days. If you're dying to test things sooner than that, PM me on IRC and I'll get back to you as soon as I see the message.

Contributor

tewinget commented Jun 25, 2017

Apart from squashing any compiler bugs on various platforms and choosing a version of libzmq to bring in-source and doing that (assuming we still want to do so), I believe all of the comments here have been addressed. Testing is a bit tedious until I've got the client side of things back in good shape -- look for that in the next two days. If you're dying to test things sooner than that, PM me on IRC and I'll get back to you as soon as I see the message.

@peronero

This comment has been minimized.

Show comment
Hide comment
@peronero

peronero Jul 1, 2017

OK, so I believe we're in a spot where these components can be reviewed and iterated upon if need be, and the notify stuff is impending separately.

Thanks @tewinget

peronero commented Jul 1, 2017

OK, so I believe we're in a spot where these components can be reviewed and iterated upon if need be, and the notify stuff is impending separately.

Thanks @tewinget

Show outdated Hide outdated src/rpc/daemon_handler.cpp
for (const crypto::hash& h : bwt.block.tx_hashes)
{
bwt.transactions.emplace(h, *tx_it);
tx_it++;

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Jul 8, 2017

Contributor

Should still error out if tx_it goes to end()

@moneromooo-monero

moneromooo-monero Jul 8, 2017

Contributor

Should still error out if tx_it goes to end()

This comment has been minimized.

@tewinget

tewinget Jul 10, 2017

Contributor

noted.

@tewinget

tewinget Jul 10, 2017

Contributor

noted.

Show outdated Hide outdated src/rpc/daemon_handler.cpp
Show outdated Hide outdated src/rpc/daemon_handler.cpp
Show outdated Hide outdated src/rpc/daemon_handler.cpp
outputs_for_amount outputs;
};
struct peer

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Jul 8, 2017

Contributor

This needs to shed ip assumption, see the new network_address class

@moneromooo-monero

moneromooo-monero Jul 8, 2017

Contributor

This needs to shed ip assumption, see the new network_address class

This comment has been minimized.

@tewinget

tewinget Jul 10, 2017

Contributor

since the wallet doesn't use that RPC call, I hadn't touched it up yet. Will deal with that when I go through to check all the other calls I've implemented for changes.

@tewinget

tewinget Jul 10, 2017

Contributor

since the wallet doesn't use that RPC call, I hadn't touched it up yet. Will deal with that when I go through to check all the other calls I've implemented for changes.

Show outdated Hide outdated src/rpc/zmq_server.cpp
Show outdated Hide outdated src/rpc/zmq_server.cpp
Show outdated Hide outdated src/rpc/zmq_server.cpp
Show outdated Hide outdated src/serialization/json_object.cpp
@NanoAkron

This comment has been minimized.

Show comment
Hide comment
@NanoAkron

NanoAkron Jul 10, 2017

Contributor

Can't some of the more general fixes (e.g. to blockchain.cpp) be submitted as separate PRs, leaving only the ZMQ RPC stuff to go in?

Contributor

NanoAkron commented Jul 10, 2017

Can't some of the more general fixes (e.g. to blockchain.cpp) be submitted as separate PRs, leaving only the ZMQ RPC stuff to go in?

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Jul 11, 2017

Contributor
Contributor

tewinget commented Jul 11, 2017

@moneromooo-monero

This comment has been minimized.

Show comment
Hide comment
@moneromooo-monero

moneromooo-monero Jul 11, 2017

Contributor

NanoAkron was not asking to remove them, but to PR them separately. Which is a good idea usually (at least commit them separately, which is what I try to do). With this particular massive patch set though, I think it's past the point where it can be made to look nice anyway. Let it be the last one to be ^_^, and let's get it merged once the missing recent (and not so recent) changes are also in the 0MQ RPC :)

Contributor

moneromooo-monero commented Jul 11, 2017

NanoAkron was not asking to remove them, but to PR them separately. Which is a good idea usually (at least commit them separately, which is what I try to do). With this particular massive patch set though, I think it's past the point where it can be made to look nice anyway. Let it be the last one to be ^_^, and let's get it merged once the missing recent (and not so recent) changes are also in the 0MQ RPC :)

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Jul 11, 2017

Contributor
Contributor

tewinget commented Jul 11, 2017

@peronero

This comment has been minimized.

Show comment
Hide comment
@peronero

peronero Jul 21, 2017

@fluffypony @luigi1111 @iamsmooth @moneromooo-monero

It's 3 weeks into the next month and 10 days since last message from @tewinget and this is still not merged.

I will not be here for the dev meeting but if not merged by then I strongly recommend that the community finds someone else to complete this work as per the 2nd option above - billing against the remaining funds from this FFS and disbursing the difference, if any, tewinget's way.

It's ridiculous that volunteers have to chase down what is effectively a $2000/hr developer to complete work that is more than half a year late.

peronero commented Jul 21, 2017

@fluffypony @luigi1111 @iamsmooth @moneromooo-monero

It's 3 weeks into the next month and 10 days since last message from @tewinget and this is still not merged.

I will not be here for the dev meeting but if not merged by then I strongly recommend that the community finds someone else to complete this work as per the 2nd option above - billing against the remaining funds from this FFS and disbursing the difference, if any, tewinget's way.

It's ridiculous that volunteers have to chase down what is effectively a $2000/hr developer to complete work that is more than half a year late.

@NanoAkron

This comment has been minimized.

Show comment
Hide comment
@NanoAkron

NanoAkron Jul 23, 2017

Contributor

Could you squash commits to remove things like 'macros, yay' please?

Contributor

NanoAkron commented Jul 23, 2017

Could you squash commits to remove things like 'macros, yay' please?

@barnyardanimal

This comment has been minimized.

Show comment
Hide comment
@barnyardanimal

barnyardanimal Jul 31, 2017

This would be a lot easier to review if broken into smaller pieces. Can we have another update from @tewinget on his effort to clean things up?

barnyardanimal commented Jul 31, 2017

This would be a lot easier to review if broken into smaller pieces. Can we have another update from @tewinget on his effort to clean things up?

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Aug 1, 2017

Contributor
Contributor

tewinget commented Aug 1, 2017

@moneromooo-monero

This comment has been minimized.

Show comment
Hide comment
@moneromooo-monero

moneromooo-monero Aug 1, 2017

Contributor

I think cleaning up commits helps a lot, because you then have N commits of size X, instead of one large amorphous blob of size N*X, because it can't really be considered small parts. That said, I'm ready to accept this PR as a large set of disparate commits, as long as it's the last one. But some cleanup would be most welcome (especially rolling any fixes/cleanups back into the commit which introduced them if that applies here).

Contributor

moneromooo-monero commented Aug 1, 2017

I think cleaning up commits helps a lot, because you then have N commits of size X, instead of one large amorphous blob of size N*X, because it can't really be considered small parts. That said, I'm ready to accept this PR as a large set of disparate commits, as long as it's the last one. But some cleanup would be most welcome (especially rolling any fixes/cleanups back into the commit which introduced them if that applies here).

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Aug 1, 2017

Contributor
Contributor

tewinget commented Aug 1, 2017

@expez

This comment has been minimized.

Show comment
Hide comment
@expez

expez Aug 12, 2017

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

Two months ago we tried 1).

Would you be OK with giving 2) a try @tewinget?

expez commented Aug 12, 2017

I would recommend either:

1 - getting committment from tewinget to complete this work on some sort of firm timetable
2 - having someone familiar with the codebase (like @moneromooo-monero ) estimate the remaining effort and reallocating the difference from the 1000XMR left to another developer

Two months ago we tried 1).

Would you be OK with giving 2) a try @tewinget?

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Aug 12, 2017

Contributor
Contributor

tewinget commented Aug 12, 2017

@dternyak

This comment has been minimized.

Show comment
Hide comment
@dternyak

dternyak Aug 25, 2017

@tewinget It appears there are a few outstanding comments by @moneromooo-monero that have gone unaddressed.

Are you planning on addressing these?

dternyak commented Aug 25, 2017

@tewinget It appears there are a few outstanding comments by @moneromooo-monero that have gone unaddressed.

Are you planning on addressing these?

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Aug 25, 2017

Contributor
Contributor

tewinget commented Aug 25, 2017

@dternyak

This comment has been minimized.

Show comment
Hide comment
@dternyak

dternyak Aug 25, 2017

Great -- thank you.

dternyak commented Aug 25, 2017

Great -- thank you.

@NanoAkron

This comment has been minimized.

Show comment
Hide comment
@NanoAkron

NanoAkron Sep 3, 2017

Contributor

It appears the last commit was about 10 weeks ago. Is this in a state to merge? The 1000XMR is now around a quarter of a million dollars...

Contributor

NanoAkron commented Sep 3, 2017

It appears the last commit was about 10 weeks ago. Is this in a state to merge? The 1000XMR is now around a quarter of a million dollars...

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Sep 4, 2017

Contributor
Contributor

tewinget commented Sep 4, 2017

@NanoAkron

This comment has been minimized.

Show comment
Hide comment
@NanoAkron

NanoAkron Sep 4, 2017

Contributor

@tewinget Good to hear! Have you addressed all of @moneromooo-monero's concerns above though? There still seem to be 6 open discussions - would you at least comment on them so we can hear your thoughts?

Contributor

NanoAkron commented Sep 4, 2017

@tewinget Good to hear! Have you addressed all of @moneromooo-monero's concerns above though? There still seem to be 6 open discussions - would you at least comment on them so we can hear your thoughts?

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Sep 4, 2017

Contributor
Contributor

tewinget commented Sep 4, 2017

tewinget added some commits Apr 6, 2017

Refactor some things into more composable (smaller) functions
This commit refactors some of the rpc-related functions in the
Blockchain class to be more composable.  This change was made
in order to make implementing the new zmq rpc easier without
trampling on the old rpc.

New functions:
  Blockchain::get_num_mature_outputs
  Blockchain::get_random_outputs
  Blockchain::get_output_key
  Blockchain::get_output_key_mask_unlocked

  Blockchain::find_blockchain_supplement (overload)

functions which previously had this functionality inline now call these
functions as necessary.
json serialization for rpc-relevant monero types
Structured {de-,}serialization methods for (many new) types
which are used for requests or responses in the RPC.

New types include RPC requests and responses, and structs which compose
types within those.

# Conflicts:
#	src/cryptonote_core/blockchain.cpp
Fix various oversights/bugs in ZMQ RPC server code
- Add some RPC commands (and touch up a couple others)
- some bounds checking
- some better pointer management
- const correctness and error handling

-- Thanks @vtnerd for type help with serialization and CMake changes
@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Sep 5, 2017

Contributor

@NanoAkron thanks for reminding me I hadn't replied to those comments. I have now.

Contributor

tewinget commented Sep 5, 2017

@NanoAkron thanks for reminding me I hadn't replied to those comments. I have now.

@moneromooo-monero

This comment has been minimized.

Show comment
Hide comment
@moneromooo-monero

moneromooo-monero Sep 7, 2017

Contributor

I'd like this to be merged shortly after we are sure the release isn't borked.

Contributor

moneromooo-monero commented Sep 7, 2017

I'd like this to be merged shortly after we are sure the release isn't borked.

@fluffypony

Reviewed

@fluffypony fluffypony merged commit 0299cb7 into monero-project:master Sep 18, 2017

3 of 13 checks passed

buildbot/monero-static-alpine-3.5-x86_64 Build done.
Details
buildbot/monero-static-debian-armv8 Build done.
Details
buildbot/monero-static-dragonflybsd-amd64 Build done.
Details
buildbot/monero-static-freebsd64 Build done.
Details
buildbot/monero-static-openbsd-amd64 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-arm7 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
buildbot/monero-static-osx-10.10 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details

fluffypony added a commit that referenced this pull request Sep 18, 2017

Merge pull request #2044
0299cb7 Fix various oversights/bugs in ZMQ RPC server code (Thomas Winget)
7798602 json serialization for rpc-relevant monero types (Thomas Winget)
5c1e08f Refactor some things into more composable (smaller) functions (Thomas Winget)
9ac2ad0 DRY refactoring (Thomas Winget)
@hyc

This comment has been minimized.

Show comment
Hide comment
@hyc

hyc Sep 18, 2017

Contributor

A few other problems with this PR. The code now depends on libzmq and a C++ wrapper for it <zmq.hpp> but there is no corresponding CMake test for this. Also this dependency needs to be listed in the README.md. As of now the build fails without explanation on various systems that don't have zmq installed by default. (E.g. #2471 )

Contributor

hyc commented Sep 18, 2017

A few other problems with this PR. The code now depends on libzmq and a C++ wrapper for it <zmq.hpp> but there is no corresponding CMake test for this. Also this dependency needs to be listed in the README.md. As of now the build fails without explanation on various systems that don't have zmq installed by default. (E.g. #2471 )

@danrmiller

This comment has been minimized.

Show comment
Hide comment
@danrmiller

danrmiller Sep 19, 2017

Contributor

Do these undefined references in libzmq.a when using the wrapper from https://github.com/zeromq/cppzmq pretty much mean the bindings don't match the libs in the packages? Or something else?

I'm sure lots of users would prefer to be able to use the libs packaged by their OS, so I hope that will end up working.

https://build.getmonero.org/builders/monero-static-dragonflybsd-amd64/builds/1346/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-ubuntu-amd64/builds/2328/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-win64/builds/2280/steps/compile/logs/stdio

Contributor

danrmiller commented Sep 19, 2017

Do these undefined references in libzmq.a when using the wrapper from https://github.com/zeromq/cppzmq pretty much mean the bindings don't match the libs in the packages? Or something else?

I'm sure lots of users would prefer to be able to use the libs packaged by their OS, so I hope that will end up working.

https://build.getmonero.org/builders/monero-static-dragonflybsd-amd64/builds/1346/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-ubuntu-amd64/builds/2328/steps/compile/logs/stdio
https://build.getmonero.org/builders/monero-static-win64/builds/2280/steps/compile/logs/stdio

@tewinget

This comment has been minimized.

Show comment
Hide comment
@tewinget

tewinget Sep 19, 2017

Contributor
Contributor

tewinget commented Sep 19, 2017

@danrmiller danrmiller referenced this pull request Sep 20, 2017

Closed

OS X build error #2497

@jtgrassie

This comment has been minimized.

Show comment
Hide comment
@jtgrassie

jtgrassie Sep 20, 2017

Contributor

Sorry but latest (4.2.2) does not seem to include the cpp bindings (as installed via MacPorts and Homebrew on OSX). For OSX you have to install cppzmq too.

Contributor

jtgrassie commented Sep 20, 2017

Sorry but latest (4.2.2) does not seem to include the cpp bindings (as installed via MacPorts and Homebrew on OSX). For OSX you have to install cppzmq too.

@Sexual

This comment has been minimized.

Show comment
Hide comment
@Sexual

Sexual Sep 25, 2017

Does this also add support for ZMQ pub/sub such as bitcoin's zmqpubhashtx. I couldn't seem to find any references to it in the code, so I'm doubtful.

Sexual commented Sep 25, 2017

Does this also add support for ZMQ pub/sub such as bitcoin's zmqpubhashtx. I couldn't seem to find any references to it in the code, so I'm doubtful.

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