Adds transaction index by address (updates #2802 to current master)

jmcorgan commented at 7:40 pm on February 11, 2014: contributor

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:

0./bitcoin-cli searchrawtransactions 1KwDYMJMS4xq3ZEWYfdBRwYG2fHwhZsipa

(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.

sipa closed this on Feb 11, 2014

sipa reopened this on Feb 11, 2014

sipa commented at 7:43 pm on February 11, 2014: member

Sorry, I misclicked, please ignore.

You’ll need to get rid of the merge commits though.

jmcorgan commented at 7:45 pm on February 11, 2014: contributor

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.

sipa commented at 7:45 pm on February 11, 2014: member

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.

jmcorgan commented at 7:47 pm on February 11, 2014: contributor

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.

luke-jr commented at 7:48 pm on February 11, 2014: member

@jmcorgan Just cherry-pick the original commits on top of master. Or commit with –date=… –author=…

jmcorgan commented at 7:49 pm on February 11, 2014: contributor

@luke-jr: unfortunately, I can’t cherry-pick like that; the original commits were a year ago and way too much has changed.

luke-jr commented at 7:50 pm on February 11, 2014: member

@jmcorgan Just resolve the conflicts like you had to for this..

jmcorgan commented at 7:54 pm on February 11, 2014: contributor

@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.

jmcorgan commented at 8:03 pm on February 11, 2014: contributor

This has been squashed to a single commit.

ghost commented at 8:18 pm on February 11, 2014: none

nice!

sipa commented at 8:21 pm on February 11, 2014: member

Wanting this to implement a wallet still sounds like a horrible idea. It’s awfully inefficient, and will only work with confirmed transactions.

jmcorgan commented at 8:37 pm on February 11, 2014: contributor

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.

jmcorgan commented at 6:02 pm on March 1, 2014: contributor

Rebased off 0.9.0rc2, minor conflict in main.cpp.

jmcorgan commented at 2:58 pm on March 31, 2014: contributor

Rebased off master to accommodate rpc call table reorg conflict.

jmcorgan commented at 4:30 pm on June 7, 2014: contributor

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.

leofidus commented at 3:00 am on June 8, 2014: none

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?

laanwj commented at 7:41 am on June 8, 2014: member

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.

jmcorgan commented at 2:53 am on June 29, 2014: contributor

Minor rebase to accommodate CTransaction and POW refactoring.

jmcorgan commented at 4:51 pm on July 1, 2014: contributor

Minor rebase to accommodate #4415 merge.

jmcorgan commented at 5:08 pm on July 7, 2014: contributor

Minor rebase to accommodate smart fees merge.

jmcorgan commented at 0:25 am on July 14, 2014: contributor

Minor rebase for rpcserver.cpp changes.

jmcorgan commented at 4:03 pm on July 30, 2014: contributor

Minor rebase to accommodate #4593 changes.

laanwj added the label UTXO Db and Indexes on Jul 31, 2014

jmcorgan commented at 8:30 am on August 12, 2014: contributor

Minor rebase to accommodate RPC server help categorization.

Squashed commit of the following, plus fixups

to carry through the development to current master:

commit 4790f3c823a33fae44b82ef7962372e38b1b0131
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 fd867c7dbc1b5ad971dd308069257e7db5e11ac1
Author: Pieter Wuille <pieter.wuille@gmail.com>
Date:   Sat Jan 12 00:48:29 2013 +0100

    Encapsulate CLevelDB iterators cleanly

079503c6ac

Fix file descriptor leak

Originally reported by dexX7

609a0a7831

jmcorgan commented at 1:40 pm on August 18, 2014: contributor

Minor rebase to accommodate #4656.

BitcoinPullTester commented at 7:54 pm on August 18, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3652_609a0a7831e2dea70f32ddb1603593a3a9437701/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

luke-jr commented at 12:12 pm on August 20, 2014: member

@gmaxwell mentioned this was buggy - hope he can elaborate.

jmcorgan commented at 7:30 pm on August 22, 2014: contributor

@luke-jr @gmaxwell Context?

reorder referenced this in commit 44f764d524 on Aug 25, 2014

reorder referenced this in commit 9181c51cee on Aug 25, 2014

btcdrak referenced this in commit baaa625498 on Aug 25, 2014

jgarzik commented at 3:24 pm on October 13, 2014: contributor

(1) Still waiting on @gmaxwell comment? (2) Intersects with #5048 ?

btcdrak commented at 8:52 pm on October 26, 2014: contributor

Needs rebase

dexX7 commented at 7:26 pm on November 26, 2014: contributor

@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.

luke-jr commented at 8:23 pm on November 26, 2014: member

I wouldn’t expect orphaned transactions to be removed from the index…?

sipa commented at 8:26 pm on November 26, 2014: member

They’re kept in -txindex as well.

gmaxwell commented at 8:34 pm on November 26, 2014: contributor

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..

dexX7 commented at 8:54 pm on November 26, 2014: contributor

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.

btcdrak commented at 8:53 pm on December 12, 2014: contributor

We have ported this to the current master branch of bitcoin if anyone is still interested https://github.com/viacoin/viacoin/pull/19 by @reorder

ghost commented at 2:06 pm on December 30, 2014: none

i hope this hasn’t been shelved

in src/main.cpp: in 609a0a7831

1092+    }
1093+    hashBlock = header.GetHash();
1094+    return true;
1095+}
1096+
1097+bool FindTransactionsByDestination(const CTxDestination &dest, std::set<CExtDiskTxPos> &setpos) {

unknown commented at 5:52 pm on December 30, 2014:

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?

in src/rpcclient.cpp: in 609a0a7831

71@@ -72,6 +72,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
72     { "createrawtransaction", 1 },
73     { "signrawtransaction", 1 },
74     { "signrawtransaction", 2 },
75+    { "searchrawtransaction", 2 },

davecgh commented at 4:31 pm on February 5, 2015:

searchrawtransactions

btcdrak commented at 10:16 pm on April 5, 2015: contributor

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 master since 0.10 is now maintenance only afaik.

dexX7 commented at 10:50 pm on April 5, 2015: contributor

@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”?

reorder commented at 6:48 am on April 6, 2015: none

@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.

dexX7 commented at 7:33 am on April 6, 2015: contributor

@reorder: I’m not really familiar with how the address index is used in the context of XCP, thus the curiosity. Also: https://github.com/CounterpartyXCP/counterpartyd/pull/418#issuecomment-64180770.. :)

reorder commented at 7:54 am on April 6, 2015: none

@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”.

btcdrak commented at 8:41 am on April 6, 2015: contributor

@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.

dexX7 commented at 8:47 am on April 6, 2015: contributor

Thanks for the follow up.

deweller commented at 9:51 pm on July 13, 2015: none

@btcdrak

Here is a picture of results from running https://gist.github.com/deweller/b823eb3f76f84071b761 for 90 minutes this afternoon: This is on a t2.large instance with an SSD EBS drive.

untitled_spreadsheet_-_google_sheets

Link to the data:

https://docs.google.com/spreadsheets/d/1esEy16D4szsf5ovZBAD927-1hUef3YJxgUe2Zgk7ikA/edit#gid=0

dexX7 commented at 10:34 pm on July 13, 2015: contributor

@deweller: so what did you learn, based on this data?

There are a few things to keep in mind here:

a great number of results is empty, because there is no transaction history yet (> 50 %)
the delay grows proportionally to the number of results

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.

deweller commented at 0:50 am on July 14, 2015: none

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 getrawtransaction RPC call. So we ran some tests to see what kind of response we were getting from queries to the address index.

I’ve posted that data here for discussion and any clues on how to narrow the problem down further.

jgarzik commented at 5:53 pm on July 23, 2015: contributor

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.

ghost commented at 6:28 pm on July 24, 2015: none

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.

jgarzik commented at 5:14 pm on September 15, 2015: contributor

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.

jgarzik closed this on Sep 15, 2015

jmcorgan deleted the branch on Sep 19, 2015

DrahtBot locked this on Sep 8, 2021

Adds transaction index by address (updates #2802 to current master) #3652