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

Asyncio based on develop #261

Merged
merged 17 commits into from Sep 4, 2014
Merged

Conversation

mcelrath
Copy link

No description provided.

@mcelrath mcelrath mentioned this pull request Aug 26, 2014
@adamkrellenstein
Copy link
Member

Do you have any estimates on what the optimal value for Bitcoin Core's rpcthreads parameter is? Is it much higher than 4, say, for the initial DB synchronization?

@mcelrath
Copy link
Author

I suspect there's a bug in bitcoin core. I didn't see it use more than 4
threads even when I increased rpcthreads.

As for optimal, that comes down to disk speed, really. I think I was
substantially slowed down because my bitcoin database is on a spinning-rust
style drive (So bitcoind is I/O bound regardless of the number of threads). If
you have an SSD I suspect a higher value of rpcthreads would make it even
faster, since its less likely to be I/O bound and can then use multiple cores...

@robby-d
Copy link
Contributor

robby-d commented Aug 26, 2014

in API.rst, you are committing a conflict (thus the <<< >>> symbols)... can you add a commit to this to clean it up?

@robby-d
Copy link
Contributor

robby-d commented Aug 26, 2014

actually, looking at the commits, that is MY fault. Jeez! I can get that fixed once we merge this in

@mcelrath
Copy link
Author

Aaah that explains why I had so many problems with that file. ;-) I kept trying to remove the merge conflict...

@robby-d
Copy link
Contributor

robby-d commented Aug 26, 2014

yeah, looks like a merge gone bad on my part...if you can fix it, feel free to do so (the right version is the signature without the lock= parameter)

@mcelrath
Copy link
Author

Will do. I'm not entirely sure how this happened, and it might not be your
fault. I did lots of gymnastics trying to rebase on develop.

@@ -384,9 +385,10 @@ def sort_unspent_txouts(unspent, allow_unconfirmed_inputs):
return unspent

def private_key_to_public_key (private_key_wif):
# allowable_wif_prefixes = [
allowable_wif_prefixes = [ b'\x80', b'\xef' ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to fix this---these values should be set in config.py.

@adamkrellenstein
Copy link
Member

Can you rename aiorun()? Let's merge this in and do some more testing.

@mcelrath
Copy link
Author

Sure, how about {{{aio_wait_for()}}}?

@adamkrellenstein
Copy link
Member

I guess I'd prefer something like synch_run(), but of course it doesn't matter much. 'wait for' sounds like it wants a time argument.

@mcelrath
Copy link
Author

It does take an (optional) time argument. ;-)

I just wanted the name to somehow refer to the fact that it has to do with aiohttp and asyncio. It's your code, if you really want another name, tell me what to use.

@adamkrellenstein
Copy link
Member

No, sure, that makes sense. How about aio_run_synch()?

@mcelrath
Copy link
Author

Done.

@adamkrellenstein
Copy link
Member

See #235.

The only thing left is allowable_wif_prefixes, I think.

@mcelrath
Copy link
Author

I'm not sure what you have in mind there...

@adamkrellenstein
Copy link
Member

Just instead of hardcoding those two bytes in bitcoin.py, use config.ADDRESSVERSION_MAINNET, etc..

@adamkrellenstein
Copy link
Member

Rather:

if config.TESTNET:
    allowable_wif_prefixes = [config.PRIVATEKEY_VERSION_TESTNET,]
else:
    allowable_wif_prefixes = [config.PRIVATEKEY_VERSION_MAINNET,]

@mcelrath
Copy link
Author

Done. Note that this is due to an interface change in pycoin, and you'll have to upgrade it. (I didn't want to have to include this patch in my asyncio PR, but I don't know what version of pycoin you guys are running and I had to do this to get it to run)

@btcdrak
Copy link
Contributor

btcdrak commented Aug 29, 2014

It's safe to upgrade to pycoin 0.50 btw.

@@ -18,7 +18,7 @@ pytest==2.5.1

pycoin==0.26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line correct, then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want >= instead... For the windows build... Dont know if it will matter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad idea. If they later push a breaking change it would wreak havok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should the value be? 0.50?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using 0.50 with no problems in clearinghoused that was released 3 weeks ago. Looks like he's upgraded to 0.51 but there is no 0.51 tag so I cant tell if the changes are OK. 0.50 is definitely fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.51 has been tagged, it's safe to use.

@adamkrellenstein
Copy link
Member

@mcelrath, bump

@mcelrath
Copy link
Author

mcelrath commented Sep 3, 2014

@PhantomPhreak what do you need?

@adamkrellenstein
Copy link
Member

@mcelrath, can you fix the pycoin version to 0.51?

@mcelrath
Copy link
Author

mcelrath commented Sep 4, 2014

Sure thing, pushed.

adamkrellenstein pushed a commit that referenced this pull request Sep 4, 2014
Asyncio based on develop
@adamkrellenstein adamkrellenstein merged commit a3c9236 into CounterpartyXCP:develop Sep 4, 2014
@adamkrellenstein
Copy link
Member

@mcelrath, I'm having some trouble getting aiohttp to work with SSL, when that is enabled for Bitcoin Core. The only workaround I can figure out is passing connector=aiohttp.TCPConnector(force_close=True, verify_ssl=False) to aiohttp.request() in bitcoin.connect(). The error is: ssl.SSLError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:598)

@mcelrath
Copy link
Author

I recall having this problem, and fixing it (on a different project). Can you try aiohttp 0.9.1dev and see if it is already fixed there?

FWIW, I would not expose bitcoind to the external world if I were you. It's better to firewall it so it only accepts connections from localhost, in which case, SSL connections are just wasted CPU.

@mcelrath mcelrath deleted the asyncio branch September 11, 2014 01:27
@adamkrellenstein
Copy link
Member

Also, this message pops up sometimes randomly: poll 5.000 took 2.531 seconds

@mcelrath
Copy link
Author

I believe the poll message was fixed by Python 3.4.1.

@adamkrellenstein
Copy link
Member

I tried using SSL with the latest from the aiohttp GitHub repo. No dice.

I agree with you re: SSL, but the feature is there in Bitcoin Core, and some people (most notably exchanges) want it badly.

I'm running Python 3.4.1, and I still see the poll message.

@mcelrath
Copy link
Author

The "Attempt to decode JSON..." message is from aiohttp. So the fix is to file a PR against aiohttp removing the message, or get web servers to report the correct mine-type. ;-)

@adamkrellenstein
Copy link
Member

We worked around the JSON problem by just parsing the JSON externally.

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

Successfully merging this pull request may close these issues.

None yet

4 participants