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

pull jmcorgan wants to merge 2 commits into bitcoin:master from jmcorgan:addrindex changing 11 files +391 −78
  1. 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.

  2. sipa closed this on Feb 11, 2014

  3. sipa reopened this on Feb 11, 2014

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

  5. 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.
  6. 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.
  7. 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.
  8. 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=…
  9. 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.
  10. luke-jr commented at 7:50 pm on February 11, 2014: member
    @jmcorgan Just resolve the conflicts like you had to for this..
  11. 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.
  12. jmcorgan commented at 8:03 pm on February 11, 2014: contributor
    This has been squashed to a single commit.
  13. ghost commented at 8:18 pm on February 11, 2014: none
    nice!
  14. 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.
  15. 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.
  16. jmcorgan commented at 6:02 pm on March 1, 2014: contributor
    Rebased off 0.9.0rc2, minor conflict in main.cpp.
  17. jmcorgan commented at 2:58 pm on March 31, 2014: contributor
    Rebased off master to accommodate rpc call table reorg conflict.
  18. 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.
  19. 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?
  20. 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.

  21. jmcorgan commented at 2:53 am on June 29, 2014: contributor
    Minor rebase to accommodate CTransaction and POW refactoring.
  22. jmcorgan commented at 4:51 pm on July 1, 2014: contributor
    Minor rebase to accommodate #4415 merge.
  23. jmcorgan commented at 5:08 pm on July 7, 2014: contributor
    Minor rebase to accommodate smart fees merge.
  24. jmcorgan commented at 0:25 am on July 14, 2014: contributor
    Minor rebase for rpcserver.cpp changes.
  25. jmcorgan commented at 4:03 pm on July 30, 2014: contributor
    Minor rebase to accommodate #4593 changes.
  26. laanwj added the label UTXO Db and Indexes on Jul 31, 2014
  27. jmcorgan commented at 8:30 am on August 12, 2014: contributor
    Minor rebase to accommodate RPC server help categorization.
  28. 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
  29. Fix file descriptor leak
    Originally reported by dexX7
    609a0a7831
  30. jmcorgan commented at 1:40 pm on August 18, 2014: contributor
    Minor rebase to accommodate #4656.
  31. 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.
  32. luke-jr commented at 12:12 pm on August 20, 2014: member
    @gmaxwell mentioned this was buggy - hope he can elaborate.
  33. jmcorgan commented at 7:30 pm on August 22, 2014: contributor
    @luke-jr @gmaxwell Context?
  34. reorder referenced this in commit 44f764d524 on Aug 25, 2014
  35. reorder referenced this in commit 9181c51cee on Aug 25, 2014
  36. btcdrak referenced this in commit baaa625498 on Aug 25, 2014
  37. jgarzik commented at 3:24 pm on October 13, 2014: contributor
    (1) Still waiting on @gmaxwell comment? (2) Intersects with #5048 ?
  38. btcdrak commented at 8:52 pm on October 26, 2014: contributor
    Needs rebase
  39. 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.
  40. luke-jr commented at 8:23 pm on November 26, 2014: member
    I wouldn’t expect orphaned transactions to be removed from the index…?
  41. sipa commented at 8:26 pm on November 26, 2014: member
    They’re kept in -txindex as well.
  42. 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..
  43. 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.

  44. 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
  45. ghost commented at 2:06 pm on December 30, 2014: none
    i hope this hasn’t been shelved
  46. 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?

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

  49. 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”?
  50. 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.
  51. 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.. :)
  52. 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”.
  53. 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.
  54. dexX7 commented at 8:47 am on April 6, 2015: contributor
    Thanks for the follow up.
  55. 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

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

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

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

  59. 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.
  60. 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.
  61. jgarzik closed this on Sep 15, 2015

  62. jmcorgan deleted the branch on Sep 19, 2015
  63. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-23 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me