Bitcoin Forum

Bitcoin => Development & Technical Discussion => Topic started by: Gavin Andresen on May 06, 2011, 03:36:24 PM



Title: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Gavin Andresen on May 06, 2011, 03:36:24 PM
This has been lingering for months, and got bogged down in discussions of some nifty new mega-efficient binary protocol for stuff.  That hasn't happened.  So:  https://github.com/bitcoin/bitcoin/pull/198

This adds these new RPC commands:

monitortx/monitorblocks: POST JSON-RPC to a URL when new wallet transactions or blocks are received.
listmonitored: list URLS that will be POSTed to
getblock: get information about a block, given depth in main chain.

monitortx posts the same information you get from gettransaction.
monitorblock/getblock posts:
Code:
{
    "hash" : "00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048",
    "blockcount" : 1,
    "version" : 1,
    "merkleroot" : "0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098",
    "time" : 1231469665,
    "nonce" : 2573394689,
    "difficulty" : 1.00000000,
    "tx" : [
        "0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098"
    ],
    "hashprevious" : "000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f",
    "hashnext" : "000000006a625f06636b8bb6ac7b960a8d03705d1ace08b1a19da3fdcc99ddbd"
}




Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Pieter Wuille on May 22, 2011, 05:06:42 PM
Any good reason not to merge this?


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Gavin Andresen on May 23, 2011, 08:11:50 PM
The only reason I can think of is it relies on the boost::xpressive regular expression parsing .hpp, and that slows down the build.

I did refactor most of this into a rpcmonitor.cpp file; rpc.cpp was getting huge, and was taking a ton of memory and time to compile.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: error on May 23, 2011, 08:15:09 PM
Ooooo, I really could use monitortx and monitorblock.

I presume monitortx is sending info on transactions received but not yet incorporated into a block?


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: error on May 25, 2011, 10:23:19 AM
Please do clean this up and merge it (it doesn't apply cleanly to 0.3.22 as is, thanks to some namespace issues).


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: ninjaneo on May 25, 2011, 06:15:54 PM
Yes this looks like a great feature, yay for pushing opposed to polling.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: ninjaneo on May 25, 2011, 07:51:03 PM
I actually got this code compiled under g++, pulled it into the 3.22 codebase, and made my edits from there.

The git diff is available here:

http://pastebin.com/wwtDCErJ


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: khal on May 26, 2011, 07:59:11 PM
"monitortx" could be really usefull for merchants who wait for payments to process orders. This works like a bank call to confirm a credit card transaction.

Quote
I presume monitortx is sending info on transactions received but not yet incorporated into a block?
If this is the case, a call to "gettransaction" will be needed to check the desired number of confirmations :p


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: weavejester on May 26, 2011, 08:06:50 PM
This would be incredibly useful for me, and would make a lot of work I was planning on doing around polling unnecessary.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: weavejester on May 28, 2011, 11:55:41 PM
I actually got this code compiled under g++, pulled it into the 3.22 codebase, and made my edits from there.

The git diff is available here:

http://pastebin.com/wwtDCErJ


I'm still getting some problems getting this pull request to compile against master, even after applying the diff. Could you possibly publish your git branch?


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: error on June 02, 2011, 09:29:04 AM
This needed a lot of hackery to apply to current git. You can find my patch here. I would still clean it up further before putting it in Bitcoin since I basically just moved a whole class into rpc.h and that's hardly elegant.

http://dl.dropbox.com/u/24777749/0001-New-RPC-calls-monitortx-monitorblocks-listmonitored.patch


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Gavin Andresen on June 23, 2011, 12:39:33 AM
I took error's work and further tweaked so it works (and is rebased against) latest git head.

... but I'm not 100% happy with it. I'm not sure it properly handles block chain re-orgs and dependent orphan transactions. Would be nice to write some tests to exercise those edge cases, and figure out what it SHOULD do in those cases.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: MrJoshua on August 09, 2011, 02:03:16 PM
If I understand correctly this patch will allow web application servers running bticoind to confirm payments without an active receiving wallet.

I would like to reiterate that this is an important feature. Since the mybitcoin debacle there is now a deadlock for merchant services between the risk of a server side wallet, and the risk of 3rd party merchant services. I am shocked to find out that getreceivedbyaddress does not give a correct value for any address except those in your wallet. This means that a receiving wallet must be left on the server and it will receive ALL bitcoin income for the service. Even with code to regularly sweep the account, this is clearly an unacceptable security risk.  On the other hand all 3rd party solutions are even riskier.

Conscientious merchants simply can't move forward with bitcoin without this patch.

j


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Furyan on August 09, 2011, 05:37:18 PM
I took error's work and further tweaked so it works (and is rebased against) latest git head.

... but I'm not 100% happy with it. I'm not sure it properly handles block chain re-orgs and dependent orphan transactions. Would be nice to write some tests to exercise those edge cases, and figure out what it SHOULD do in those cases.


Gavin,

Yes, I initially included this patch in my build of bitcoind but decided not to use it for exactly the reasons you posit. I realized it would lead to apparent block confirmations that ended up being orphaned blocks, and/or reporting transactions that would not ultimately be confirmed.  In other words - both monitors can lead to a false sense of security.

Sigh :( Unfortunately I don't have the time to exercise the edge cases. 

I may in the future, offer a bounty to someone who does exercise those cases, and writes the appropriate code to deal with them.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: error on August 09, 2011, 07:08:23 PM
If I understand correctly this patch will allow web application servers running bticoind to confirm payments without an active receiving wallet.

I would like to reiterate that this is an important feature. Since the mybitcoin debacle there is now a deadlock for merchant services between the risk of a server side wallet, and the risk of 3rd party merchant services. I am shocked to find out that getreceivedbyaddress does not give a correct value for any address except those in your wallet. This means that a receiving wallet must be left on the server and it will receive ALL bitcoin income for the service. Even with code to regularly sweep the account, this is clearly an unacceptable security risk.  On the other hand all 3rd party solutions are even riskier.

Conscientious merchants simply can't move forward with bitcoin without this patch.

j

This is part of the reason I stopped using this patch myself. At the moment I am trying to stabilize blkmond, which provides a realtime view of the whole Bitcoin network, including all transaction activity, but likes to stop running unexpectedly. It can notify of payments received at an address without access to the wallet containing the corresponding key, but not if it's crashed.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: vector76 on August 09, 2011, 11:10:13 PM
Seems the natural solution would to be to have an "address watch list" that can maintain balances at addresses without the private key anywhere in sight.  I would have expected the wallet to use something along those lines internally anyway, but not sure how hard it would be to expose that functionality with arbitrary addresses and no private keys.


Title: 25btc bounty!
Post by: BitcoinLocator on August 09, 2011, 11:34:09 PM
We need these features for our applications, so were are prepared to put our money where are mouth is.  We will pay a 25btc bounty for the integration of these features.  To be split between the key participants in making this patch a reality.



Title: Re: 25btc bounty!
Post by: kjj on August 10, 2011, 12:30:05 AM
We need these features for our applications, so were are prepared to put our money where are mouth is.  We will pay a 25btc bounty for the integration of these features.  To be split between the key participants in making this patch a reality.

This thread covers several different things.  Exactly which features are you offering the bounty for?


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: BitcoinLocator on August 10, 2011, 12:51:38 AM
This thread covers several different things.  Exactly which features are you offering the bounty for?

Well it's the entire pull request we where thinking of (i.e. push based notification for transactions and blocks, plus generic getblock).  So anyone who contributes to the finalizing of this pull request, once it's actually integrated with the client.

After some additional consideration we are not offering this bounty for a solution that is not integrated with bitcoind.  The goal is to get the feature we want and promote it for inclusion in the client so the community gets the benefit as well without the need for other tools.

However we would definitely pay at least a partial bounty to an interim solution that is client integrated, but not necessarily this complete set. Such as pull based equivalents, or the ability to check arbitrary addresses for payments.

 


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: kjj on August 10, 2011, 01:35:52 AM
From reading your other posts, I get the impression that you want to know about addresses that are not in the wallet.  I don't think this patch gives you that.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: MrJoshua on August 10, 2011, 01:42:25 AM
From reading your other posts, I get the impression that you want to know about addresses that are not in the wallet.  I don't think this patch gives you that.

Oh doesn't it?  That was my understanding, as posted above, and no one has corrected me on that yet.

j


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: kjj on August 10, 2011, 02:57:08 AM
My C++ is really rusty, and I'm reading the diff out of context, but I believe that this patch just lets you add URLs to two lists, one that gets POSTs for every new block that comes in, and one that gets POSTs for every transaction that comes in related to the running wallet.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: 2112 on August 10, 2011, 04:13:32 AM
... but I'm not 100% happy with it. I'm not sure it properly handles block chain re-orgs and dependent orphan transactions. Would be nice to write some tests to exercise those edge cases, and figure out what it SHOULD do in those cases.
In addition to the above this patch doesn't seem to have any mechanism for garbage collecting stale notification callbacks. From the cursory look I would guess that it doesn't even try to be idempotent, i.e. one could add multiple identical callback URLs with no limit that I could see.


Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: BitcoinLocator on August 11, 2011, 05:55:46 PM
My C++ is really rusty, and I'm reading the diff out of context, but I believe that this patch just lets you add URLs to two lists, one that gets POSTs for every new block that comes in, and one that gets POSTs for every transaction that comes in related to the running wallet.

I hope this is not the case, and it doesn't make sense.  If you're receiving blocks complete with all transactions and you have addresses in your POST queue for monitortx why would you go out of your way to filter results based on the wallet?  It's an extra step to reduce functionality.



Title: Re: [PULL] monitortx monitorblocks listmonitored getblock
Post by: Furyan on September 08, 2011, 12:22:27 PM
Gavin,

I have been running with this patch for a few weeks now (decided to go ahead and see how big the problem might be).

So, orphaned blocks received from the network definitely do get posted via the monitorBlock workflow.  However, when bitcoin eventually re-orgs the block chain to replace the orphaned blocks with the real block from the longest chain, those blocks are not being posted.  For now I have a process that runs once in awhile to detect these cases and updates the values stored in the db, but I'm wondering if something can be modified in the patch to address it.

Interestingly, if the orphaned block is one of my own, I actually want to retain the old value for tracking purposes.

With regard to dependent orphaned transactions, I haven't run into those yet so can't comment.