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

New blockchain tools #418

Merged
merged 22 commits into from
Nov 16, 2014
Merged

New blockchain tools #418

merged 22 commits into from
Nov 16, 2014

Conversation

ouziel-slama
Copy link
Contributor

Goals of this PR:

  • have only one required method for each "blockchain service provider": search_raw_transaction(). This make the integration of another services very easy.
  • make counterpartyd multisig usable with jmcorgan's fork OR insight+bitcoind
  • have an "unified" method to get utxos for monosig and multisig transactions.

Notes:

  1. Insight needs to be updated with the last version that contains this commit: bitpay/insight-api@63db916

  2. jmcorgan don't return unconfirmed transactions. There is two solutions I think:

  1. I propose in this PR to remove blockr and sochain from the blockchainmodule. They return a lot of timeout error and I don't see any good reason to maintain this 2 providers here.

@adamkrellenstein
Copy link
Member

ACK, except:
2) I think we need to check the mempool.
3) I wish we could do this, but I don't think it's feasible right now.

And what are we going to do if we want to switch to btcd and they don't support search_raw_transactions? Do we just stick with Insight?

@adamkrellenstein
Copy link
Member

See btcsuite/btcd#185 (comment)

@btcdrak
Copy link
Contributor

btcdrak commented Nov 10, 2014

You might find this interesting.

We merged jcmorgan's code into viacoin, I cant remember if we made any improvements, but anyway, insight is simply not reliable. We then wrote drivers for clearinghoused and clearblockd to query viacoind directly.

Anyway, we never considered making an upstream PR simply because it involved using a nonstandard daemon.

https://github.com/ClearingHouse/clearinghoused/blob/master/lib/blockchain/addrindex.py
https://github.com/ClearingHouse/clearblockd/blob/master/lib/blockchain/addrindex.py

I am sure you can find the other references.

The only part I am not sure of is there were claims by Greg Maxwell that jmcorgan's fork is not complete and returns false results but we have not experienced any problems. This is the patch we applied to viacoin viacoin/viacoin@baaa625

@ouziel-slama
Copy link
Contributor Author

And what are we going to do if we want to switch to btcd and they don't support search_raw_transactions? Do we just stick with Insight?

yes, btcd+insight or btcd+jmcorgan..
the question is, if bitcoind merge addrindex and btcd don't support addresses index, do we switch to btcd ?

See btcsuite/btcd#185 (comment)

I think many other people will do the same request as we do.. maybe even someone will do a PR..

@btcdrak , thank you for the informations! It's, in fact, great to control completely the backend, but I'am optimistic about the fact that bitcoind or btcd will integrate soon an addresses index. This is a recurring request from a lot of developers.

@btcdrak
Copy link
Contributor

btcdrak commented Nov 10, 2014

@JahPowerBit I wouldnt hold your breath for it being integrated into bitcoind any time soon unfortunately. I did try to get someone to work on the patch, but also they seem not to want to add more indexes and instead wait to build out a plugin system for bitcoind. IMO addrindex is a super important patch. You should all lobby for it bitcoin/bitcoin#3652

@adamkrellenstein adamkrellenstein changed the title New blockchain New blockchain tools Nov 12, 2014
@davecgh
Copy link

davecgh commented Nov 14, 2014

I have seen an address index requested a few times, so I would gladly accept a pull request to btcd that provides a flag to enable it. As I mentioned in the referenced issue, I like the idea and see the benefits of it. The only concern I have is that it needs to be an option that is disabled by default due to the rather largish amount of additional space it consumes which is not that useful for non-developers.

Unfortunately, I know there is a list long enough right now that those of us at Conformal won't get to work on it in the short term. However, if there is anyone willing to tackle adding it in a pull request, I would certainly work with them to make it happen.

@adamkrellenstein
Copy link
Member

@JahPowerBit, what about 2) and 3)?

@ouziel-slama
Copy link
Contributor Author

@adamkrellenstein,
2) 3c4e78a
3) 7db1c1c and 06e238a

Conflicts:
	test/conftest.py
adamkrellenstein added a commit that referenced this pull request Nov 16, 2014
@adamkrellenstein adamkrellenstein merged commit 4b83ac6 into develop Nov 16, 2014
@adamkrellenstein adamkrellenstein deleted the new_blockchain branch November 16, 2014 15:21
@dexX7
Copy link

dexX7 commented Nov 24, 2014

@JahPowerBit @btcdrak @adamkrellenstein:
I did not fully look at your code, but you should be aware that searchrawtransactions captures and stores all transactions and does not remove orphaned ones. It took me a moment to realize, so I hope I can save you some time. :p

Orphans have, for obvious reasons, a blockhash that references a block that is not part of the main chain, but also the time and blocktime field is missing and confirmations is always 0.

Example:

[{
  "txid": "9be9d3e8d35c0df8b4e092b0643adbcee57de10449f1e965e8a2300d3dec01ce",
  ...
  "blockhash": "00000000000000000b067f6c8d7e7c9ef40c6664859226a8c4157b17b5278dc6",
  "confirmations": 0
}, {
  "txid": "9be9d3e8d35c0df8b4e092b0643adbcee57de10449f1e965e8a2300d3dec01ce",
  ...
  "blockhash": "00000000000000001a21e2f02faedbc960b4298f57809a20429f680d9d89710f",
  "confirmations": 9969,
  "time": 1411057105,
  "blocktime": 1411057105
}]

@ouziel-slama
Copy link
Contributor Author

@dexX7, thank you very much for the information. I just did a PR to skip unconfirmed transactions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants