Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Adding BTC.com wallet #1357

Merged
merged 18 commits into from Mar 29, 2017

Conversation

Projects
None yet
5 participants
Contributor

AlejandroDeLaTorre commented Sep 5, 2016

Hello,

We'd like to add BTC.com's wallet to Bitcoin.org. Our former name was Blocktrail, but we've rebranded to BTC.com. Please let me know if I need to correct or add something more.

Thank you

@crwatkins crwatkins added the Wallets label Dec 2, 2016

@crwatkins crwatkins self-assigned this Dec 3, 2016

Contributor

wbnns commented Dec 3, 2016

@AlejandroDeLaTorre Hi, thanks for submitting this wallet to be potentially included on the site. This message is to confirm that it is currently under review as per Bitcoin.org's wallet inclusion criteria.

@wbnns wbnns self-assigned this Dec 9, 2016

Contributor

crwatkins commented Feb 24, 2017

I have reviewed the BTC.com wallet based on the current wallet requirements criteria and my evaluation is below. The summary is that I can recommend this wallet for listing.

However, this specific PR has numerous issues at this time, mostly clerical and syntactical, that need to be addressed before I can recommend merging. I will list those issues in a separate comment.

BTC.com Wallet

Android version 3.1.8

iOS version 3.1.7

Web commit 6c71dfd

Review Version 2017022401

NOTE The BTC.com wallet is a rebranding of the previously released Blocktrail wallet.

The wallet list is based on the personal evaluation of the maintainer(s) and regular contributors of this site, according to the criteria detailed below.

These requirements are meant to be updated and strengthened over time. Innovative wallets are exciting and encouraged, so if your wallet has a good reason for not following some of the rules below, please submit it anyway and we'll consider updating the rules.

Basic requirements:

  • Sufficient users and/or developers feedback can be found without concerning issues, or independent security audit(s) is available

PASS 10,000 Google Play downloads, 240 reviews, https://bitcointalk.org/index.php?topic=756279.0

  • No indication that users have been harmed considerably by any issue in relation to the wallet

PASS No indication with standard Google search and https://bitcointalk.org/index.php?topic=1180096.0, https://bitcointalk.org/index.php?topic=756279.0

  • No indication that security issues have been concealed, ignored, or not addressed correctly in order to prevent new or similar issues from happening in the future

PASS No indication. BTC.com is very responsive on Google Play reviews and https://bitcointalk.org/index.php?topic=756279.0

  • No indication that the wallet uses unstable or unsecure libraries

PASS No indication. Uses bitcoinjs and randombytes.

  • No indication that changes to the code are not properly tested

PASS No indication. Test code is at https://github.com/blocktrail/

  • Wallet was publicly announced and released since at least 3 months

PASS 14 September 2105: https://blog.blocktrail.com/2015/09/time-for-a-new-bitcoin-wallet/

  • No concerning bug is found when testing the wallet

PASS No concerning bugs

  • Website supports HTTPS and 301 redirects HTTP requests

PASS http://btc.com and http://wallet.btc.com both redirect to HTTPS.

PASS A+ rating for https://btc.com and https://www.btc.com

  • Website serving executable code or requiring authentication uses HSTS with a max-age of at least 180 days

PASS https://wallet.btc.com max-age is 186 days

  • The identity of CEOs and/or developers is public

PASS "About us" on btc.com links to https://www.bitmain.com/about

  • Avoid address reuse by displaying a new receiving address for each transaction in the wallet UI

PASS A new receiving address is displayed each time

  • Avoid address reuse by using a new change address for each transaction

PASS A new change address is used for each transaction

  • If private keys or encryption keys are stored online:
    • Refuses weak passwords (short passwords and/or common passwords) used to secure access to any funds, or provides an aggressive account lock-out feature in response to failed login attempts along with a strict account recovery process.

PASS Refuses weak passwords using very good zxcvbn password meter, and has a 25 try lockout count

NOTE Web password change in setting page does not refuse weak passwords

  • If user has no access over its private keys:

N/A

  • Provides 2FA authentication feature

  • Reminds the user to enable 2FA by email or in the main UI of the wallet

  • User session is not persistent, or requires authentication for spending

  • Provides account recovery feature

  • If user has exclusive access over its private keys:

    • Allows backup of the wallet

PASS Initial backup of wallet to PDF document is allowed.

  • Restoring wallet from backup is working

PASS Wallet recovery tool at https://recovery.blocktrail.com/ was successfully tested with a lost password recovery scenario

NOTE Standalone recovery tool is at https://github.com/blocktrail/wallet-recovery-tool

  • Source code is public and kept up to date under version control system

PASS Source code is public and kept up to date at https://github.com/blocktrail

  • If user has no access to some of the private keys in a multi-signature wallet:
    • Provides 2FA authentication feature

PASS Provides TOTP 2FA (RFC 6238) on web wallet for login and send

NOTE 2FA is required on mobile login, but not for each send as on the web

  • Reminds the user to enable 2FA by email or in the main UI of the wallet

PASS Web wallet provides daily reminders to enable 2FA.

NOTE Mobile wallet uses PINs for spending, not 2FA.

  • User session is not persistent, or requires authentication for spending

PASS Web wallet requires password and optionally 2FA for spending

PASS Mobile wallet requires PIN

  • Gives control to the user over moving their funds out of the multi-signature wallet

  • For hardware wallets:

N/A

  • Uses the push model (computer malware cannot sign a transaction without user input)
  • Protects the seed against unsigned firmware upgrades
  • Supports importing custom seeds
  • Provides source code and/or detailed specification for blackbox testing if using a closed-source Secure Element

Optional criteria (some could become requirements):

  • Received independent security audit(s)

NOTE No known independent security audits

  • Does not show "received from" Bitcoin addresses in the UI

PASS Does not show "received from" addresses

  • Uses deterministic ECDSA nonces (RFC 6979)

PASS A transaction signed by the BTC.com wallet was duplicated and signed with custom code using pybitcointools which is RFC 6979 based. The signatures match.

  • Provides a bug reporting policy on the website

PASS "Contact us" link on btc.com

  • If user has no access over its private keys:

N/A

  • Full reserve audit(s)

  • Insurance(s) against failures on their side

  • Reminds the user to enable 2FA in the main UI of the wallet

  • If user has exclusive access over its private keys:

    • Supports HD wallets (BIP32)

PASS Uses a BIP32 path of M/0'/0/i for the wallet and server key with a path of M/0/0/i for the backup key.

  • Provides users with step to print or write their wallet seed on setup

PASS Provides a PDF instruction document with seeds

  • Uses a strong KDF and key stretching for wallet storage and backups

PASS Uses PBKDF2 HMAC-SHA512 with 35k rounds

  • On desktop platform:
    • Encrypt the wallet by default

N/A

  • For hardware wallets:
    • Prevents downgrading the firmware

N/A

Contributor

crwatkins commented Feb 24, 2017

@AlejandroDeLaTorre thanks for the PR. There are a number of issues that you might want to address before we can merge.

  1. Regretfully choose-your-wallet.html has changed. Could you rebase your PR to resolve the conflict?
  2. Under ios: you should list only os: -ios and not list -android
  3. Likewise under android: you should only list -android and not -ios
  4. Your text: references don't match the tag in en.yml. For consistency, I would suggest that all your text references are for btccomwallet (without the dot) and that you use this tag in en.yml.
  5. Since it has been a few months, you may want to submit a new screenshot if anything has changed.
  6. You probably want your title: to be BTC.com Wallet and not btc.comwallet
  7. In your titleshort: you may want to drop "Wallet" and leave it just BTC.com for consistency with most other wallets listed. This is up to you.
  8. Your validation: is inconsistent between the umbrella mobile listing and the individual ios and android listings. These need to match. I believe you want them all to be validation: "checkfailvalidationcentralized"
  9. In your text in en.yml "Multi-Signature" should be spelled "multi-signature"
  10. In your text, you may want to leave out the word "Bitcoin" in "BTC.com Bitcoin Wallet" to match your iOS and Android store branding. Up to you.
  11. In your text, the word "unparalleled" seems a little awkward. Take a look at the other text strings and see if you might want to change that.
  12. Lastly, could you reword your text to remove the word "we" since in this case it would refer to bitcoin.org? Maybe something along the lines of "... so your private keys stay with you."
Contributor

schildbach commented Feb 24, 2017

Yes, "unparalleled" seems to imply that no other wallet can be as good as this. This is the kind of superlative we agreed to not use a couple of years ago, because obviously it's always wrong.

@wbnns wbnns removed the Under Review label Mar 3, 2017

Contributor

AlejandroDeLaTorre commented Mar 7, 2017

Okay thank you all for the feedback, updating now.

Contributor

wbnns commented Mar 8, 2017

@AlejandroDeLaTorre Hello, it looks like there is still a merge conflict in _templates/choose-your-wallet.html - could you please resolve it?

Contributor

crwatkins commented Mar 13, 2017

@AlejandroDeLaTorre thanks for the update! I did a quick glance at it and you might want to check out the following:

  1. The new logo is too big. I think the old logo was the right size.

  2. Would you mind changing text: "btc.comwallet" to text: "btccomwallet" without the dot? Maybe someone can tell us this is unnecessary, but I think we might have had some syntax issues in the past. (No one sees this label on the website.) It will also need to be changed in en.yml.

  3. Could you change Our features include, HD and multi-signature technology, multi-platform,... to Features include HD and multi-signature technology, multi-platform,... removing the "Our" and the comma?

Thanks

Contributor

AlejandroDeLaTorre commented Mar 13, 2017

ok thanks working on it right now.

Contributor

crwatkins commented Mar 13, 2017

@AlejandroDeLaTorre Amost there!

  1. In en.yml, you need to have your tag walletbtc.com: be walletbtccom: without the dot to match.

  2. I think you introduced a typo by adding the characters "gre"

validation: "checkfailvalidationcentralizedgre"

Contributor

crwatkins commented Mar 13, 2017

utACK
I concur with the current scoring and content in this PR.

lacksfish commented Mar 14, 2017 edited

utACK. Have been using the wallet myself and it is working very well for quite some time now.

Contributor

wbnns commented Mar 14, 2017 edited

Hello, it's my understanding that BTC.com is owned by Bitmain, which runs Antpool. Yesterday it was noted by Bloomberg in a quote from Jihan Wu, the cofounder of Bitmain, that the entire pool will be switching to Bitcoin Unlimited (which among other issues currently has a remote crash DoS flaw running live in production).

@AlejandroDeLaTorre Could you please let us know how these types of decisions / slated changes might affect BTC.com wallet users?

@lacksfish I noted on your GitHub profile that it appears you are also part of BTC.com. Could you please share your thoughts as well?

@wbnns wbnns added On Hold and removed Changes Requested labels Mar 14, 2017

Contributor

AlejandroDeLaTorre commented Mar 16, 2017

Thank you for the question, Will.

We actively seek to discuss any wallet concerns, upgrades and issues with other bitcoin wallet providers/companies through initiatives like s3nd. s3nd is set to occur sometime next month in Berlin and as I understand will be a monthly or bi-monthly meetup. We find this initiative to be a step in the right direction, for we believe in dialogue and in the spirit of working together to reach our common goal i.e increasing the usage of bitcoin.

If some novelty occurs to the Bitcoin network we will be discussing with other wallet providers to see what the best plan of action will be for all our software users.

Our highest concern is the safety and security of our users and only wish to provide an excellent wallet for them.

We only intend to work together with the whole wallet ecosystem to find the best plan of action if anything might happen to bitcoin in the future.

Contributor

AlejandroDeLaTorre commented Mar 16, 2017

@lacksfish joined our team last week, but if he has anything to add he should :)

Contributor

AlejandroDeLaTorre commented Mar 23, 2017

Hey all,

Could we please move this process forward? We have gone through the current wallet requirements criteria (thank you Craig Watkins) and there is an evaluation provided.

We have all worked really hard to provide a (hopefully) excellent wallet to the highest of our ability. Even before Bitmain came into the picture.

We are all bitcoiners here!

Thank you

Contributor

wbnns commented Mar 27, 2017

@AlejandroDeLaTorre Hello, thanks for the responsiveness and also for the additional thoughts.

Unless others object, this will be merged on Wednesday, March 29th.

@wbnns wbnns added Merge Scheduled and removed On Hold labels Mar 27, 2017

@wbnns wbnns merged commit ee1a4e4 into bitcoin-dot-org:master Mar 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment