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 --block-notify to monerod and --tx-notify to monero-wallet-{cli,rpc} #4333

Closed
wants to merge 1 commit into
base: master
from

Conversation

@moneromooo-monero
Copy link
Contributor

moneromooo-monero commented Sep 3, 2018

Those take a command line of the form "A [B]", with A being the
name (and optional path, if not in the caller's CWD, but fully
qualified path is recommended, avoids possible security issues)
to a program, and optional arguments. Any occurence of the two
character string "%s" will be replaced by the hash of the block
or transaction which triggered the notification.

block-notify is called when a new block is added onto the chain.

tx-notify is called when a new transaction happens with the
wallet as source and/or destination.

It is the notification program's responsibility to determine what
to do in those cases.

Note that this is asynchronous, so it is very possible that:

  • the notification programs will be run out of order
  • several events happen before the notification for the first one

A Windows port would be nice if someone wants to make one.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from 0417813 to 5774db7 Sep 3, 2018

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 3, 2018

If anyone has comments about security here, please say so.
Also, the tokenizing is dumb, so it'll be different from what a shell would do. Not sure if that's a real problem. Also no pipes obviously but if you need them, call a shell script.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Sep 5, 2018

Firstly, I would have thought this kind of notification would be either via 0mq or registering a callback URL via the JSON-RPC interface. I like this take on how to provide a notification, but given users are generally using the RPC, it would be nicer if this was part of the RPC as opposed to a different mechanism.

Secondly, if a wallet is spammed with micro txs, is there not the risk of a DDoS due to the forking and starting processes?

Lastly, I really don't get what is wrong with polling. It works fine. A poll every 2 minutes to get new blocks / txs is perfectly simple and what people use currently. This PR seems to just be adding something else that needs maintaining when there is no real need.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 5, 2018

I made that one because I found somewhere saying Bitcoin was doing it that way, and people have been asking for soimeting like Bitcoin.
vtnerd will do a RPC notification system AFAIK.

@lessless

This comment has been minimized.

Copy link

lessless commented Sep 9, 2018

@jtgrassie polling is not a viable solution at scale. If one have to manage a number of wallets then it will just become a resource drain.

Obviously launching new application is a middle-ground between nothing and 0mq/rpc, but hey - that definitely helps!

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Sep 9, 2018

@lessless

polling is not a viable solution at scale. If one have to manage a number of wallets then it will just become a resource drain.

Actually polling is almost certainly less resource intensive than forking, launching a new process and then the process doing something (likely making a TCP request). With polling, you only have to perform 1 RPC request to check for a new block every 2 minutes and then for say 10 wallets, thats 10 TCP requests to check for new transactions. The overhead of a TCP response is far lower than N forks and the new process launches (N would be as high as the number of incoming transactions on all wallets as opposed to a deterministic amount of polling TCP request/responses).

The most efficient method would be 0mq or a registered RPC callback, hence whilst this option maybe useful (and I have nothing against it being added), it's probably the least efficient option.

@danrmiller

This comment has been minimized.

Copy link
Contributor

danrmiller commented Sep 9, 2018

@jtgrassie Agreed 0mq would be ideal, but if you need to take action on an unconfirmed transaction as soon as possible, polling will be more frequent than every 2 minutes.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 9, 2018

I think adding pool txes might be useful. And also a reorg notification maybe. It starts to become complicated...

@lessless

This comment has been minimized.

Copy link

lessless commented Sep 9, 2018

@jtgrassie but with polling one will have to query every single wallet, while push-on-update will launch only minimum necessary amount notification application instances.
I can build a test case and benchmark it over the next weekend if I will be able to build Monero :)

PUB/SUB over 0mq feels like an optimal solution, JSON RPC callback is the second best imo.

@danrmiller block mining time in Monero is 2 mins afaik. What will be the reason to poll more often?

@monero-project indeed. thanks for giving it a try.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Sep 9, 2018

@moneromooo-monero

It starts to become complicated...

It does indeed! With the architecture of this PR for notifications, we could end up with a lot of command line parameters. Sorry moneromoo but this looks a bit rushed and ill thought out.

@danrmiller

but if you need to take action on an unconfirmed transaction as soon as possible, polling will be more frequent than every 2 minutes.

2 minute pooling is for the new block check, not necessarily for incoming txs (which could be higher frequency if needed). Not sure why anyone would want to know about unconfirmed txs anyway. Mining pool servers (which need to know about new blocks and txs) simply poll. Online wallets do the same and polling works fine in both these use cases.

I just cant see this yet as anything other than fixing a problem (somewhat inefficiently) that doesn't exist, rather than improving on an existing mechanism (e.g. adding a 0mq or RPC registered callback solution - which actually would be an improvement to polling).

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Sep 9, 2018

@lessless

but with polling one will have to query every single wallet

Yes of course, but with multiple wallets actively receiving transactions, you could be spawning many more processes (which requires much more overhead) than multiple TCP requests. At least with polling you know exactly how many requests are needed as it's fixed to the amount of wallets. Spawning an unknown number of processes is harder to manage. Remember, this PR notifies via first forking, then launching a user supplied process, then (optionally) waiting for that process to end. That's a lot of overhead vs cheap TCP polling.

@danrmiller

This comment has been minimized.

Copy link
Contributor

danrmiller commented Sep 9, 2018

@jtgrassie I agree that it does start to get complicated, and agree the better solution is 0mq or RPC registered callback.

I'm just mentioning that the 2 cases you mention, mining pools and online wallets, both frequently personally tell me they have great pain around polling. Another use case I am personally familiar with where polling is not meeting the users' needs is web shopping, people who have paid you want to see you acknowledge that payment as soon as possible, and there does reach a point where polling hurts.

Anyway, again I'm not disagreeing, but want to clarify that polling is not working fine for several projects.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Sep 9, 2018

@danrmiller

I'm just mentioning that the 2 cases you mention, mining pools and online wallets, both frequently personally tell me they have great pain around polling.

Which pools / wallets? I know of no pool implementation that polling is a problem for example.

Another use case I am personally familiar with where polling is not meeting the users' needs is web shopping, people who have paid you want to see you acknowledge that payment as soon as possible, and there does reach a point where polling hurts.

Polling for a purchase is simple though. You can poll seconds after the tx (where it will be unconfirmed) and later to check it's confirmed (so you can ship product). The later check would be based on how many block confirmations the seller is comfortable with - it's deterministic (X * 2 minutes).

but want to clarify that polling is not working fine for several projects.

What projects? I know of none that don't work fine with polling. Please specify. My hunch is projects that the developers can't wrap their head around how best to poll. The mining pool servers operate fine, payment processors, online wallets... please clarify.

I still cannot see how adding something worse than polling makes sense to add when we all agree on a method that would be better (0mq / RPC callback).

@danrmiller

This comment has been minimized.

Copy link
Contributor

danrmiller commented Sep 9, 2018

Which pools / wallets? I know of no pool implementation that polling is a problem for example.

I'll let you know on IRC so I'm not publicly opening people up to criticism who haven't directly involved themselves in it.

I still cannot see how adding something worse than polling makes sense to add when we all agree on a method that would be better (0mq / RPC callback).

True, I take your point, yes. Thanks.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from 5774db7 to d446041 Sep 9, 2018

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 9, 2018

I think adding pool txes might be useful.

It now does.

@xiphon

This comment has been minimized.

Copy link
Contributor

xiphon commented Sep 9, 2018

@moneromooo-monero

also a reorg notification maybe

Lets trigger --block-notify for both events (new block and reorg). Could be renamed to --blockchain-notify.

@jtgrassie

Which pools / wallets? I know of no pool implementation that polling is a problem for example.

Right. Mining pools don't have any problems polling the daemon.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 9, 2018

You always push at least a block after you pop N (at least without user command to pop manually). It was for the wallet that it might be interesting, if a payment gets reverted.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Sep 28, 2018

Anyone else has an opinion/argument about this ? If not, it'll get in soon.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from d446041 to da43b03 Sep 28, 2018

namespace tools
{

Notify::Notify(const char *spec)

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor
Notify::Notify(const std::string &spec)
{
  boost::split(args, spec, boost::is_any_of(" "));
...

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor

The main problem is that it won't handle paths containing spaces.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Sep 28, 2018

Contributor

Yes, I indicated in the commit message the tokenizing is dumb. I don't mind that. It can always be improved later, and you can use a symlink if it really is a bother.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Sep 28, 2018

Contributor

Hmm, actually looks like I didn't, having just looked. But the intent was there :D Proper tokenizing is not trival.

@@ -1634,6 +1650,7 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
m_callback->on_money_received(height, txid, tx, td.m_amount, td.m_subaddr_index);
}
total_received_1 += extra_amount;
notify = true;

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor

Indentation

@@ -3552,6 +3553,9 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
get_difficulty_for_next_block(); // just to cache it
invalidate_block_template_cache();

if (m_block_notify)

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor
auto notify = m_block_notify;
if (notify)
{
  notify->notify(epee::string_tools::pod_to_hex(id).c_str());
}

This comment has been minimized.

@jtgrassie

jtgrassie Sep 28, 2018

Contributor

Any reason why? shared_ptr has an operator bool which checks if a pointer is set.

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor

Thread safety.
PS: edited

This comment has been minimized.

@jtgrassie

jtgrassie Sep 28, 2018

Contributor

How does get have any more thread safety than operator bool? Both are thread safe.

This comment has been minimized.

@xiphon

xiphon Sep 28, 2018

Contributor

Please, check the code snippet once again, i updated it as i noted in the previous reply.

This comment has been minimized.

@jtgrassie

jtgrassie Sep 29, 2018

Contributor

Other functions are making use of CRITICAL_REGION_LOCAL (locking).

This comment has been minimized.

@xiphon

xiphon Sep 29, 2018

Contributor

Haha, don't be so full of butthurt. Shit happens.

Sure, i will point to any problem i will identify in the code.
That is what is called code review, FYI.

This comment has been minimized.

@jtgrassie

jtgrassie Sep 29, 2018

Contributor

Not butthurt at all. Genuinely pleased things get reviewed and debated before they get merged - it's critically important. That's why I submit code, participate in code reviews of others and support the community in all ways I can.

In the case of this class, it turns out it uses those CRITICAL_... macros for locking to get thread safety. For consistency, this variable should ideally follow suit I guess.

This comment has been minimized.

@xiphon

xiphon Sep 29, 2018

Contributor

If we are goin' to keep shared_ptr and add a mutex, there won't be any point to use shared_ptr exactly in this case then. Will have to replace shared_ptr with unique_ptr and use a mutex to maintain unique_ptr's thread-safety.

And that's obviously would be a self-made shared_ptr parody. That's what shared_ptr actually do.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Sep 29, 2018

Contributor

I wonder if the copy operator is atomic...

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from da43b03 to da34601 Sep 28, 2018

if (!wait)
return 0;

while (1)

This comment has been minimized.

@vtnerd

vtnerd Sep 29, 2018

Contributor

What happens if ctrl-c is pressed in this loop? If SIGTERM is caught, is this potentially stuck until a -9 kill? I don't think there is a generic shutdown flag to check sadly, so I'm not sure what to do about this.

This comment has been minimized.

@vtnerd

vtnerd Sep 29, 2018

Contributor

On further thought - the wait variable is never set to true. Should that option and code supporting it be dropped, until there is a need for it?

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Sep 29, 2018

Contributor

If it happens during the syscall, you should get EINTR back. If not, it might loop indeed. wait is never used currently, it just seemed like a useful thing to have. It can be removed if it's the general consensus.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from da34601 to c06f07a Sep 29, 2018

add --block-notify to monerod and --tx-notify to monero-wallet-{cli,rpc}
Those take a command line of the form "A [B]", with A being the
name (and optional path, if not in the caller's CWD, but fully
qualified path is recommended, avoids possible security issues)
to a program, and optional arguments. Any occurence of the two
character string "%s" will be replaced by the hash of the block
or transaction which triggered the notification.

Tokenization is barebones. If you want things like pipes, calls
to paths with spaces, etc, then use a script (though exec time
will suffer).

block-notify is called when a new block is added onto the chain.

tx-notify is called when a new transaction happens with the
wallet as source and/or destination.

It is the notification program's responsibility to determine what
to do in those cases.

Note that this is asynchronous, so it is very possible that:
- the notification programs will be run out of order
- several events happen before the notification for the first one

A Windows port would be nice if someone wants to make one.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:notify branch from c06f07a to 7340300 Sep 29, 2018

@@ -3552,6 +3553,10 @@ bool Blockchain::handle_block_to_main_chain(const block& bl, const crypto::hash&
get_difficulty_for_next_block(); // just to cache it
invalidate_block_template_cache();

std::shared_ptr<tools::Notify> block_notify = m_block_notify;

This comment has been minimized.

@xiphon

xiphon Sep 29, 2018

Contributor

@moneromooo-monero @jtgrassie
Regarding #4333 (comment)

Had to revisit the standard once again. Unfortunately, yes, std::shared_ptr copy constructor is not atomic. There are std::atomic_load and std::atomic_store overloads for this case in c++11.

But they are going to be deprecated in c++20 and the semantic will be changed to std::atomic<std::shared_ptr<T>>. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0718r2.html#3.3

I bet we don't want to deal with different compilation issues tied to these changes.

We have no choice but to introduce a kind of manual locking.

This comment has been minimized.

@moneromooo-monero

moneromooo-monero Sep 29, 2018

Contributor

This is set once at program start though. In practice it can't race.

@fluffypony
Copy link
Collaborator

fluffypony left a comment

Reviewed

fluffypony added a commit that referenced this pull request Sep 29, 2018

Merge pull request #4333
7340300 add --block-notify to monerod and --tx-notify to monero-wallet-{cli,rpc} (moneromooo-monero)

fluffypony added a commit that referenced this pull request Sep 29, 2018

Merge pull request #4333
7340300 add --block-notify to monerod and --tx-notify to monero-wallet-{cli,rpc} (moneromooo-monero)

@fluffypony fluffypony closed this Sep 30, 2018

@DinoStray

This comment has been minimized.

Copy link

DinoStray commented Oct 17, 2018

How to use this new config parameter?
Any test case?
I use it in config file:
block-notify='bash /root/scripts/new_block_callback.sh'

But nothing happened.
In /root/scripts/new_block_callback.sh:

echo "hi" >> /root/lyl

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Oct 17, 2018

@DinoStray The parameter cannot contain spaces right now (see above comment). If just using a bash script, you don't need the bash bit anyway. Just pass:
--block-notify='bash /root/scripts/new_block_callback.sh', assuming of course the script starts with a #!/bin/bash.

@xiphon

This comment has been minimized.

Copy link
Contributor

xiphon commented Oct 18, 2018

@jtgrassie
also you have to specify the full path to executable #4633 (comment)

@shrikus

This comment has been minimized.

Copy link

shrikus commented Dec 4, 2018

I have alot of defunct threads with notify (one for each block):

11888 ?        SLl    0:07 /usr/local/bin/monerod --config-file /root/monero/monero.conf --detach --pidfile /run/monero.pid
11941 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11948 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11950 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11952 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11954 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11959 ?        Z      0:00  \_ [block-monero.sh] <defunct>
11977 ?        Z      0:00  \_ [block-monero.sh] <defunct>
12395 ?        Z      0:00  \_ [block-monero.sh] <defunct>
12983 ?        Z      0:00  \_ [block-monero.sh] <defunct>

monero.conf:

block-notify=/usr/local/bin/block-monero.sh %s

/usr/local/bin/block-monero.sh:

#!/bin/bash
curl -s --connect-timeout 20 -X PUT "http://127.0.0.1:8000/api/block?hash=$@"

Notification script working as well, my api is receiving notifications, but i have no ideas how to fix those annoying defuncts.

I also tried to add --block-notify='/usr/local/bin/block-monero.sh %s', and --block-notify='/bin/bash /usr/local/bin/block-monero.sh' with same results.

@oneevil

This comment has been minimized.

Copy link

oneevil commented Dec 7, 2018

@moneromooo-monero
The bug described above is seen on my servers too.
Please, fix this bug.

@KraxCZ

This comment has been minimized.

Copy link

KraxCZ commented Jan 10, 2019

I have an application that gets notified about new transactions using --tx_notify.
Upon notification the application "decodes" the transaction using RPC get_transfer_by_txid. It seems that each call triggers new notifications about all transactions that are not mined yet. My application gets in loop (notification -> query -> notification ....)
My question: is that triggering an intended behavior? Or is it just a side affect or maybe... bug?

@jtgrassie

This comment has been minimized.

Copy link
Contributor

jtgrassie commented Jan 13, 2019

@KraxCZ I can confirm this. Please open an issue.

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