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

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

Closed
wants to merge 2 commits into from

Conversation

@jmcorgan
Copy link
Contributor

jmcorgan commented Feb 11, 2014

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:

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

This comment has been minimized.

Copy link
Member

sipa commented Feb 11, 2014

Sorry, I misclicked, please ignore.

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

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

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

This comment has been minimized.

Copy link
Member

sipa commented Feb 11, 2014

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

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

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

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 11, 2014

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

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

@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

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 11, 2014

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

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

This has been squashed to a single commit.

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 11, 2014

nice!

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Feb 11, 2014

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

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Feb 11, 2014

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

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Mar 1, 2014

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

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Mar 31, 2014

Rebased off master to accommodate rpc call table reorg conflict.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jun 7, 2014

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

This comment has been minimized.

Copy link

leofidus commented Jun 8, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Jun 8, 2014

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

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jun 29, 2014

Minor rebase to accommodate CTransaction and POW refactoring.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jul 1, 2014

Minor rebase to accommodate #4415 merge.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jul 7, 2014

Minor rebase to accommodate smart fees merge.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jul 14, 2014

Minor rebase for rpcserver.cpp changes.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Jul 30, 2014

Minor rebase to accommodate #4593 changes.

@jmcorgan

This comment has been minimized.

Copy link
Contributor Author

jmcorgan commented Aug 12, 2014

Minor rebase to accommodate RPC server help categorization.

sipa and others added 2 commits Feb 11, 2014
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
@btcdrak

This comment has been minimized.

Copy link
Member

btcdrak commented Oct 26, 2014

Needs rebase

@dexX7

This comment has been minimized.

Copy link
Contributor

dexX7 commented Nov 26, 2014

@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

This comment has been minimized.

Copy link
Member

luke-jr commented Nov 26, 2014

I wouldn't expect orphaned transactions to be removed from the index...?

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Nov 26, 2014

They're kept in -txindex as well.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Nov 26, 2014

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

This comment has been minimized.

Copy link
Contributor

dexX7 commented Nov 26, 2014

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

This comment has been minimized.

Copy link
Member

btcdrak commented Dec 12, 2014

We have ported this to the current master branch of bitcoin if anyone is still interested viacoin/viacoin#19 by @reorder

@ghost

This comment has been minimized.

Copy link

ghost commented Dec 30, 2014

i hope this hasn't been shelved

return true;
}

bool FindTransactionsByDestination(const CTxDestination &dest, std::set<CExtDiskTxPos> &setpos) {

This comment has been minimized.

Copy link
@ghost

ghost Dec 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?

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

This comment has been minimized.

Copy link
@davecgh

davecgh Feb 5, 2015

Contributor

searchrawtransactions

@btcdrak

This comment has been minimized.

Copy link
Member

btcdrak commented Apr 5, 2015

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

This comment has been minimized.

Copy link
Contributor

dexX7 commented Apr 5, 2015

@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

This comment has been minimized.

Copy link

reorder commented Apr 6, 2015

@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

This comment has been minimized.

Copy link
Contributor

dexX7 commented Apr 6, 2015

@reorder: I'm not really familiar with how the address index is used in the context of XCP, thus the curiosity. Also: CounterpartyXCP/counterparty-lib#418 (comment).. :)

@reorder

This comment has been minimized.

Copy link

reorder commented Apr 6, 2015

@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

This comment has been minimized.

Copy link
Member

btcdrak commented Apr 6, 2015

@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

This comment has been minimized.

Copy link
Contributor

dexX7 commented Apr 6, 2015

Thanks for the follow up.

@btcdrak btcdrak mentioned this pull request Jul 9, 2015
@deweller

This comment has been minimized.

Copy link

deweller commented Jul 13, 2015

@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

This comment has been minimized.

Copy link
Contributor

dexX7 commented Jul 13, 2015

@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

This comment has been minimized.

Copy link

deweller commented Jul 14, 2015

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

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jul 23, 2015

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

This comment has been minimized.

Copy link

ghost commented Jul 24, 2015

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

This comment has been minimized.

Copy link
Contributor

jgarzik commented Sep 15, 2015

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 jgarzik closed this Sep 15, 2015
@jmcorgan jmcorgan deleted the jmcorgan:addrindex branch Sep 19, 2015
@rnicoll rnicoll mentioned this pull request Oct 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.