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

Added -txnotify to call a script when a transaction is received #2364

Closed
wants to merge 7 commits into from

Conversation

Bobalot
Copy link

@Bobalot Bobalot commented Mar 13, 2013

More or less copied code from the -blocknotify and -walletnotify, but for all accepted transactions.

Is there a good reason not to implement this? As far as i can tell any DOS attack will be limited by the rate limiting of free transactions and even then process spawn limits can be set on any unix system.

As long as the script does something simple, such as adding the txid to a queue, then there will be little chance of the number of spawned processess building up. This is very useful to people who would otherwise have to rely on constant polling or centralised services such as blockchain.info websockets.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/67f82f019b8e73efa07cf2ed478fb53d3c8d51b1 for binaries and test log.

@Diapolo
Copy link

Diapolo commented Mar 21, 2013

As long as this consists of that many commits I'm rather sure no core dev will give you an ACK, so you should squash everything into one commit.

@gavinandresen
Copy link
Contributor

RE: "Is there a good reason not to implement this?"

The attitude for the core client is the the opposite: Is there a good reason TO add this?

Unless you have a compelling use case for this, I'd rather leave it out. Less is generally better, less code to review for security issues, fewer bugs, ...

@gmaxwell
Copy link
Contributor

@Bobalot Can you walk me through what problem this solves that isn't solved equally well by just polling once a second (to a few seconds)?

@jgarzik
Copy link
Contributor

jgarzik commented Mar 24, 2013

Per-tx events are going to average at least once per second. For this patch -- yuck -- you are continually running new OS processes. It is far more efficient to combine stock bitcoind with https://github.com/jgarzik/pynode/tree/mini-node if you simply want to see all transactions that are accepted (and relayed) by bitcoind.

Therefore this patch and polling are both the wrong solutions IMO.

@gmaxwell
Copy link
Contributor

My point about polling was that new transactions are coming in at more than once per second, so simply polling every few seconds will be less work, no dos exposure, and can still give acceptable responsiveness.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4a5e4de2adeb3dc7ddc2d0cd9214c43714bd80fe 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.

@Bobalot
Copy link
Author

Bobalot commented Apr 5, 2013

I guess you're right, launching a new process is a terrible thing to do on every transaction. This was really just an ugly hack, having found mini-node it seems using that would be a much easier and safer way to achieve what I wanted.

@gavinandresen it isn't really a good idea to implement this, but the same can be said for -blocknotify and -walletnotify, if they only create further code to review.

@gmaxwell you're right this would be easier by polling, but i've found using pynode is even easier.

@sipa
Copy link
Member

sipa commented Apr 5, 2013

Yes, I consider -blocknotify and -walletnotify also borderline ugly and inefficiently, but this is just too resource-intensive for what it gains. The 0mq support could do things like this much better, imho.

@sipa
Copy link
Member

sipa commented Apr 12, 2013

No consensus, closing.

@sipa sipa closed this Apr 12, 2013
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants