Adds transaction index by address (updates #2802 to current master) #3652
Conversation
|
Sorry, I misclicked, please ignore. You'll need to get rid of the merge commits though. |
|
I don't mind squashing all of these into a new single commit; however, this will mean that your original contributions will show up as done by me instead. If that is not a problem, I'll do this right now and update the PR. |
|
Also, I don't understand how you can call it a "lite" wallet if it requires a fully-indexed full node behind the scenes. That's exactly why I hate this. |
|
Poor terminology. I have a personal need to have this index, and I also already run multiple copies of bitcoind fully indexed for other reasons. |
|
@jmcorgan Just cherry-pick the original commits on top of master. Or commit with --date=... --author=... |
|
@luke-jr: unfortunately, I can't cherry-pick like that; the original commits were a year ago and way too much has changed. |
|
@jmcorgan Just resolve the conflicts like you had to for this.. |
|
@luke-jr The conflicts were extensive and spread over many files, including through file renames and code movement to new binaries (like bitcoin-cli). I think I'll just squash this whole branch to a commit, make sipa the author, and go with it. It's not like I wrote any of the actual features, just the drudge work to get them current. |
|
This has been squashed to a single commit. |
|
nice! |
|
Wanting this to implement a wallet still sounds like a horrible idea. It's awfully inefficient, and will only work with confirmed transactions. |
|
Well, it does what I need, that's why I spent the time and effort to refresh it. Maybe it is useful to others, maybe not--you guys decide. If it doesn't get merged I'll try to keep it fresh on my repo as master changes. |
|
Rebased off 0.9.0rc2, minor conflict in main.cpp. |
|
Rebased off master to accommodate rpc call table reorg conflict. |
|
Rebased on current master with minor fixups (func usage, switch away from boost::int64_t). Also, there is now a addrindex-0.9.2 branch in my repo with the same functionality that is rebased on v0.9.2rc2. |
|
so the main reason against implementing this is that there is no equivalent index over the utxo set, so people would start depending on a fully indexed blockchain? |
|
Right, there are other tools that can be used to index the entire block chain if you need to, for example for forensic reasons. One of these is Bitpay's insight. Anything that relies on indexing the whole block chain doesn't belong in Bitcoin Core. These indices could be anything, depending on the kind of queries you want to do. So use the right tool for the job. From mailing list discussion a similar index for the UTXO set would be acceptable. It would be a much smaller index and only relies on data that is needed anyway. |
|
Minor rebase to accommodate CTransaction and POW refactoring. |
|
Minor rebase to accommodate #4415 merge. |
|
Minor rebase to accommodate smart fees merge. |
|
Minor rebase for rpcserver.cpp changes. |
|
Minor rebase to accommodate #4593 changes. |
|
Minor rebase to accommodate RPC server help categorization. |
to carry through the development to current master: commit 4790f3c Author: Pieter Wuille <pieter.wuille@gmail.com> Date: Fri Jan 11 23:34:08 2013 +0100 Add address-based index 1) Maintain a salt to perturbate the address index (protection against collisions). 2) Add support for address index entries in the block index, and maintain those if -addrindex is specified. It indexes the use of every >8-byte data push in output script or consumed script - or in case of no such push, the entire script. 3) Add a searchrawtransactions RPC call, which can look up raw transactions by address. commit fd867c7 Author: Pieter Wuille <pieter.wuille@gmail.com> Date: Sat Jan 12 00:48:29 2013 +0100 Encapsulate CLevelDB iterators cleanly
Originally reported by dexX7
|
@gmaxwell: I'm very interested in learning about potential bugs here as well. The only issue that I noticed, besides a fixed FD leak, and one should be aware of: orphaned transactions are not rermoved from the index. |
|
I wouldn't expect orphaned transactions to be removed from the index...? |
|
They're kept in -txindex as well. |
|
What I observed was missing transactions. But I can't tell why they were missing... e.g. perhaps they were inserted and reorged out. I'm absolutely sure they were missing, since ... thats easy to reliably test.. |
|
Bad choice of words: returning and keeping orphaned transactions is fine and not an issue per se, but it wasn't something I expected. Thanks for information. |
|
We have ported this to the current |
|
i hope this hasn't been shelved |
| return true; | ||
| } | ||
|
|
||
| bool FindTransactionsByDestination(const CTxDestination &dest, std::set<CExtDiskTxPos> &setpos) { |
There was a problem hiding this comment.
This function is giving problems to compile on the latest master branch, particularly
const CKeyID *pkeyid = boost::get(&dest);
anyone know how to re-write it so it compiles?
|
I dont know if there is still interest in this PR. I have it adapted for bitcoin 0.10 here https://github.com/btcdrak/bitcoin/tree/addrindex It's been used in deployment now for many months, including by Counterparty without any problems or missing transactions. @gmaxwell unless there is a concrete example of it missing data out, I would suggest it's been well tested to date. Counterparty has been stress testing it considerably in their Counterwallet web-wallet and have not reported any problems. I assume if it was wanted it would have to be rebased to |
|
@btcdrak: it's great that you maintain the branch and I'm sure there are a few using it. For a longer time the Counterparty integration did not filter orphaned transactions, and just used all of them, and this was basically unnoticed, until pointed out, if I recall correctly. So when you say "stress tested", does this equal "really tested" or "there were no complaints so far"? |
|
@dexX7 Counterparty (and other users of addrindex) rely on 'confirmations' field of transaction record to be non-zero, which only holds if transaction block is in mapBlockIndex. I am not sure how are reorgs handled by current Counterparty code but it used to reparse blockchain tail and invalidate orphaned transactions. |
|
@reorder: I'm not really familiar with how the address index is used in the context of XCP, thus the curiosity. Also: CounterpartyXCP/counterparty-core#418 (comment).. :) |
|
@dexX7 I was actually assuming XCP logic is similar to what we have done in Clearinghouse/Viacoin using the same addrindex code: with Viacoin extremely short blocktime there happens a reorg or two every hour so handling it can indeed be considered "stress tested". |
|
@dexX7 Counterparty have relied exclusively on this patch for 5 months in production processing thousands of requests per day in counterwallet.co, I believe this counts as stress testing. In Viacoin's ClearingHouse we have relied on this patch for about 8 months and as reorder explains we have a lot more reorgs than the bitcoin blockchain. This is why I am confident that the patch, as it exists in my bitcoin fork for 0.10, works. |
|
Thanks for the follow up. |
|
Here is a picture of results from running https://gist.github.com/deweller/b823eb3f76f84071b761 for 90 minutes this afternoon: Link to the data: https://docs.google.com/spreadsheets/d/1esEy16D4szsf5ovZBAD927-1hUef3YJxgUe2Zgk7ikA/edit#gid=0 |
|
@deweller: so what did you learn, based on this data? There are a few things to keep in mind here:
I'm not sure, what exactly you want to test, but you're probably better off, if you'd exclude empty results, and messure delays in time/byte or similar. |
|
We have been experiencing issues with delayed transaction creations in counterparty. The problem became significantly worse during the recent spam attacks on the bitcoin network. Our preliminary tests pointed toward bitcoind responding very slowly to a I've posted that data here for discussion and any clues on how to narrow the problem down further. |
|
Leaning towards closing this - pinging interested parties here for comment. Rationale: For whatever reason, this seems stuck and not getting merged. That is not a judgement on the goal or code quality or a NAK, simply an observation over time. |
|
I integrated this into my alt and find it very useful, i may try get time and submit a refactored version. IMO it is a feature well worth the effort as it reduces reliance on web explorers for information, maybe label it as an advanced option much like coin-control. |
|
Closing for aforementioned non-technical reasons. I'm hoping that someone will come along, volunteer as a maintainer of this change, and work to shepherd this in. |
This branch updates the original transaction index by address patch by sipa in #2802 to work with the current master.
To get here, successive portions of the master branch were merged into the addrindex branch, resolving merge conflicts along the way. There was no need to change any of sipa's original code, other than to accommodate things like splitting out bitcoin-cli or other code reorganizations. This did take a lot of merges; in retrospect it might have been faster to just re-implement the changes starting with the current master. Nonetheless, doing it this way preserves all the history (and blame) and allows backtracking along the branch to see what fixups were needed.
I've tested with with txindex=1 and addrindex=1 in bitcoin.conf (using -reindex on cli), and am able to make queries with bitcoin-cli:
(output suppressed)
In the original pull request, there was some debate about whether the reference client should include this capability, for fear that some would come to rely on it; I don't see the concern and in fact it will allow me personally to implement some lite wallet functionality I've been wanting to pursue.
Also, it may be useful in helping deal with the current TX malleability issues by helping to identify actual transactions to/from addresses instead of just by txid.