Remove python-bitcoinaddress dependency p.2 (issue #2867)

[techtonik]

Previous PR #2885 got accidentally merged, so this is a continuation.

[techtonik]

This is ready to be reviewed. This code for bitcoin address validation is written by Gavin Andresen (unlike the code used at https://pypi.python.org/pypi/python-bitcoinaddress), so it is tested and ported to multiple languages, and there is no reason not to trust it. If there is consensus to keep it as external dependency, I can make a package.

[Changaco]

That bitcoin module looks terrible, it doesn't follow PEP8 at all.

[techtonik]

@Changaco I intentionally left it consistent with original code, so that it is possible to compare it to original and ensure that it was not seriously messed with and modifications are visible.

In this case application security is more important than consistency of application code style, and on module level consistency is preserved, which is also what PEP8 says:
http://legacy.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

So it does follow PEP8 in this way. =)

[techtonik]

Good check would be to dump all bitcoin addresses from live db and run utils/bitcoin.py -i <address> over them. It should fail with non-0 status if some address can not be validated.

[techtonik]

Rebased.

[chadwhitacre]

IRC

[chadwhitacre]

@techtonik The purpose of this PR is to get the GPL out of our codebase, yes?

[chadwhitacre]

"Reach license clarity" #2881

[chadwhitacre]

All current participant.bitcoin_address values pass the validation check (the tail call trims noise resulting from my .psqlrc):

[gratipay] $ echo "copy (select bitcoin_address from participants where bitcoin_address > '') to stdout" \
    | heroku pg:psql \
    | tail -n +4 \
    | xargs -n1 ./env/bin/python gratipay/utils/bitcoin.py -i $0 \
    | grep invalid
[gratipay] $

[chadwhitacre]

Let's remove that code entirely instead of making it optional.

[chadwhitacre]

I intentionally left it consistent with original code, so that it is possible to compare it to original and ensure that it was not seriously messed with and modifications are visible.

We should at least trim trailing whitespace.

[chadwhitacre]

If there is consensus to keep it as external dependency, I can make a package.

This isn't a bad idea. Surely there are others who want a non-GPL bitcoin library. I say +1 on this route ... then we can clean up the code. :-)

[techtonik]

@whit537, the primary purpose became to use approved and security checked code for bitcoin address validation. The secondary is to remove dependency that conflicts with new setuptools (and prevent Vagrant bootstrapping). Removing GPL dependency just adds to that.

Cleaning up the code...

[techtonik]

All current participant.bitcoin_address values pass the validation check

Awesome. =)

[chadwhitacre]

the primary purpose became to use approved and security checked code for bitcoin address validation.

Why does python-bitcoinaddress not satisfy this condition? Is it not approved? Or not security-checked? Or ... ?

The secondary is to remove dependency that conflicts with new setuptools (and prevent Vagrant bootstrapping).

Ah, yes. Now I remember: #2327.

Removing GPL dependency just adds to that.

I'm right that we want to not use GPL code, right? We can't use GPL code without releasing our code as GPL, right?

[chadwhitacre]

@techtonik The Travis error looks like the one we were seeing after #2899. Did you rebase on master while it was in an inconsistent state?

[techtonik]

Why does python-bitcoinaddress not satisfy this condition? Is it not approved? Or not security-checked? Or ... ?

It was copied from http://paddy3118.blogspot.nl/2012/11/some-identities-for-python-inttobytes.html and I don't know who is this guy, where this code was used, and if somebody from crypto experts reviewed it.

I'm right that we want to not use GPL code, right? We can't use GPL code without releasing our code as GPL, right?

It is not that clear, but I am sure FSF wants people to think so. I know for sure that LGPL is 100% fine to use, AGPL is 100% not ok. GPL must be somewhere in the middle, but it is better avoid speculations and don't use it.

[techtonik]

@techtonik The Travis error looks like the one we were seeing after #2899. Did you rebase on master while it was in an inconsistent state?

Yes. It is now green though after the last commit.

[chadwhitacre]

IRC re: GPL

[chadwhitacre]

Let's also reference this PR for convenience to document alterations:

#2886

[techtonik]

Agreed. Done.

[chadwhitacre]

That bitcoin module looks terrible[...]

What do you expect from an academic? ;-) It was written by Gavin Andresen, so while it might look terrible, I think we can have a high degree of trust in it (I believe that's what I hear @techtonik suggesting).