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

Replacing MultiBit Classic with MultiBit HD #887

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
10 participants

The outcome of this pull request is to replace MultiBit Classic with MultiBit HD.

Here's a screen shot of the outcome, all being well:

screen shot 2015-06-12 at 15 19 54

@harding harding added the Wallets label Jun 12, 2015

@harding harding self-assigned this Jun 12, 2015

Contributor

saivann commented Jun 12, 2015

@bitcoin-solutions Congratulation for the release! I will let others comment on the new app. Regarding the pull request:

Is there a missing comma here, perhaps? (I'm only unsure about it)

With integrated TREZOR and Tor support it synchronizes...
With integrated TREZOR and Tor support, it synchronizes...

Can you delete the previous images?

img/screenshots/multibit.png
img/wallet/multibit.png

checkpassprivacybasic should be used instead of checkgoodprivacyimproved. Currently, only full nodes or wallets using a local full node by default get the best score.

Otherwise the other changes all LGTM (commit 553f15a).

Contributor

greenaddress commented Jun 12, 2015

I noticed this comment on reddit.

Given "It's practically a rewrite", does it mean this PR will wait the "standard" amount of time (obviously I'm taking into account that the review work is done by volunteer and there is no time guarantees) ?

Last, given BRIT by default automatically adds outputs to users transactions (even if slightly randomly) wouldn't it severely weaken its privacy?

I also noticed in some cases BRIT may send these outputs to hard coded addresses, I am not sure that's great for privacy either.

jim618 commented Jun 12, 2015

@greenaddess There is more information about our privacy policy here:
https://beta.multibit.org/privacy.html

Whenever there is more public information there is some loss of privacy. With BRIT the loss of privacy is roughly equivalent to making a payment to a known address (for instance an organisation's donation address).
As adding a transaction output to one of the BRIT addresses is easily spoofed it does not actually indicate that the user is using MultiBit HD. You could be using another wallet and just spoof a payment.

We cycle the BRIT addresses for new wallets daily. We typically get about 1000 downloads a day of MultiBIt Classic so let's use that figure for expected number of downloads a day for MultiBit HD. If a user makes a payment to a BRIT address (that isn't spoofed) then the set of BRIT addresses they use is shared by a 1000 other users. What identification do we have on that set of 1000 users ? We have a BRIT wallet id, which isn't linked in any way to a user's wallet (it's generated by trapdoor functions from the seed phrase which isn't stored anywhere).

We don't link BRIT wallet id to IP addresses and as we don't collect any user login info it's obviously not connected to that.

Compare that situation to a centralised server situation (Electrum, Copay, Mycelium, GreenAddress) where ALL transactions are connected intimately to a user. It's much much less information.

You are correct, the failover situation fails over to a small set of addresses but of course that means that the cohort in which the user is in is a much larger anonymity set.

edit: grammar

Contributor

harding commented Jun 12, 2015

@greenaddress Thanks for your comment.

We (@saivann, @crwatkins, and myself) have been discussing relaxing (or removing) the requirement that wallets be three months out of beta before we list them. The problem isn't that a three-month wait is a bad idea but, rather, that different authors release wallets with different amounts of alpha/beta testing, so it tends to penalize authors who perform extensive beta testing.

In addition, it just occured to me that Bitcoin Core's Help → About still says, "This is experimental software", so I guess it's technically failing this "requirement".

As you may recall, we were willing to waive that for GreenBits because your existing user/fan base allowed you to collect a lot of public reviews in short order---and it's the public reviews that we really need to discover non-obvious problems.

In the case of MultiBit HD (and CoPay in #888), it had a long public beta process that helped work out many of the bugs. The plan is for @crwatkins to give it his usual thorough review---which he started several weeks ago and is, I believe, finalizing now. If there are no problems or objections, we'll give it the usual 72 hours for public comment and then merge it.

Regarding BRIT and similar fees (such as Electrum with the 2FA oracle plugin), we don't have any criteria preventing us from listing wallets like that, nor any score to convey the possible reduction in privacy to the user. If you would like to suggest such a requirement or score, please feel free to open a separate issues.

@saivann I've just pushed the changes that you requested earlier. Good spot with the comma.

Ready for review.

Contributor

greenaddress commented Jun 12, 2015

@jim618 Thanks! I already had a look and is not the privacy policy that worried me is just the idea of extra output in transactions in a SPV wallet, even if you promise to not have logs or anything like that.

@harding Fair enough, makes sense. I am not sure I will open an issue to add a new score unless others thinks it may be worthwhile. Thanks

Contributor

schildbach commented Jun 12, 2015

I was the one who proposed the "wait peroid" criterium initially. I'm not sure how it evolved since then, but originally it wasn't about alpha, beta or release, but about how long the source code had been published in an auditable form. Some wallets like bc.i reset their commit history which made it impossible to review only the changes. So we made this criterium for new wallets and wallets that reset their source history.

Contributor

luke-jr commented Jun 12, 2015

With BRIT the loss of privacy is roughly equivalent to making a payment to a known address (for instance an organisation's donation address).

This is not an acceptable standard, and I believe falls under the policy's address reuse clause. At present, address reuse only reduces the privacy score (which means this PR needs to change it), but I think we should make avoiding reuse mandatory, at least for built-in mechanisms (eg, change and BRIT) - that's a topic for another issue, but I think the MultiBit HD team ought to strongly consider making the (trivial) changes to fix this issue regardless.

You are correct, the failover situation fails over to a small set of addresses but of course that means that the cohort in which the user is in is a much larger anonymity set.

It seems to me BRIT might be possible to avoid entirely by using stealth addresses, also solving the privacy issues entirely?

Contributor

harding commented Jun 12, 2015

@schildbach I think we're talking about two different things, although lots of people get them confused.

  1. The requirements policy that we only review if a "Wallet was publicly announced and released since at least 3 months." This is what I'm referring to, and what I think @greenaddress is referring to.
  2. The open source transparency score that is only applied to wallets after their source code has been public, and not had past commits revised, for six months. This is what I think you're talking about it. We still use this score and there are no plans to change it (that I know of).
Contributor

harding commented Jun 12, 2015

Good catch, @luke-jr! For some reason I hadn't thought about the reuse of addresses in BRIT as actual address reuse---but it is.

I think we should make avoiding reuse mandatory, at least for built-in mechanisms (eg, change and BRIT)

We were planning to open a pull to do that as soon as MultiBit HD was merged, as we thought MultiBit Classic was our last wallet incompatible with that policy. However, it now looks like Electrum 2.x is doing goofy things with change address reuse, as well as the issue you've raised here with MultiBit HD.

jim618 commented Jun 12, 2015

@luke-jr Note that the addition of BRIT addresses can be easily avoided by the user.

In the Preferences | Fees screen (shown below) the user can very easily send, say, a dollar (4.4 millis) to the MultiBit donation address.
4.4 millis would give the user 440 sends with no BRIT transaction outputs added. That's a long time for the average bitcoin user.

We have made it very easy for users who are sensitive about transaction outputs NOT being added to their transactions to do so. Users can decide for themselves what level of privacy they are comfortable with.

Screenshot:
screen shot 2015-06-12 at 22 56 45

On your technical solution : it is always easy to say something is trivial to do if you're not the one doing it ! We initially had an address generation scheme (not stealth addresses but similar) and it creates problems on redemption as you start having a very large number of addresses to track.

Fully documented, tested and production ready pull requests to MultiBit HD are of course welcome. :-)

We have added in a version number into the BRIT messages so that they can be upgraded as options and algorithms improve - this is BRIT version 1.

Contributor

luke-jr commented Jun 12, 2015

In the Preferences | Fees screen (shown below) the user can very easily send, say, a dollar (4.4 millis) to the MultiBit donation address.

And this doesn't reuse addresses?

We initially had an address generation scheme (not stealth addresses but similar) and it creates problems on redemption as you start having a very large number of addresses to track.

Uh, this sounds like a general MultiBit HD problem? Tracking a large number of addresses should not be a problem... And unnecessary if you do it the stealth address way, I think?

Contributor

harding commented Jun 12, 2015

@jim618 "donate to us in advance, or your privacy may be reduced later" --- doesn't seem like a friendly policy. However, that doesn't really matter here---it's our policy to score wallets based on their default settings.

Since MultiBit HD does appear to reuse addresses, with the consequent privacy-harming effects, I think it would be most correct to change the checkpassprivacyaddressrotation score back to a fail score until the problem is fixed.

Also, note that we do still want to make it a requirement that listed wallets don't reuse addresses, but I don't know when that will happen. We'll be sure to give you plenty of warning before making that change.

jim618 commented Jun 13, 2015

A few points to note:

  • MultiBit HD does indeed pass the current policy on address reuse as it is an HD wallet with rolling change address. If you use existing criteria for decisions, as opposed to what someone thinks they should be, then MultiBit HD should be classified as checkpassprivacyaddressrotation
  • I think we are agreed that the privacy leak from BRIT is no more than someone donating to a known address. (I would argue that it is less because we deliberately create an anonymity set of around 1000 to obfuscate things. A typical CoinShuffle anonymity set is 50 so we have an order of magnitude better behaviour than that).
  • The default behaviour of lots of wallets is to suggest a donation address (All the Electrum servers I have ever connected to have a tip address). The only difference to BRIT in this respect is the level of compulsion being used. This is an exercise in the study of human action (Praxeology) which is fascinating but off topic I think here.
  • The central nub of this discussion is 'address reuse'. How are addresses being reused ?
    In a single user's wallet we will give them in a BRIT exchange 50 fee addresses, chosen from a very much larger set. Typically a user uses one address per 20 sends. The address used is chosen at random but they will make approximately 1000 sends before they personally have any significant address reuse.
    That's just sends so we are talking about a lot of bitcoin activity.
    The same set of 50 addresses is given to all users that create a new wallet on a given day.
    This is the anonymity set I mentioned before - it is by design as it gives a very high level of plausible deniablity to any particular transaction output on any particular transaction.
    We've thought about the aggregation aspects of privacy and feel that an order of magnitude better behaviour than proposed mixing algorithms is a good level of privacy protection.
  • Stealth addresses are NOT compatible with SPV so aren't a magic bullet. We like SPV as it is the best way to scale. We don't want 6000 nodes - our design brief is to engineer something that works up to a 1,000,000 nodes.
  • BRIT has been in the public domain for a very long time now. There's an announcement blog here:
    https://multibit.org/blog/2014/04/11/multibit-hd-brit.html . We've had all the code in the public domain from day one and have had 8 betas for the code. That is a whole year to solicit comments. The appropriate time to make suggestions for improvements is during the design phase, not at roll-out.

edit: spelling, added links

Contributor

gurnec commented Jun 13, 2015

Considering the descriptions as they are currently written, even with BRIT's address re-use I personally find MultiBit HD to be much closer to the first description than the second:

This wallet makes it harder to spy on your balance and payments by rotating addresses. You should still take care to use a new Bitcoin address each time you request payment.

vs.

This wallet makes it easy for anyone to spy on your balance and payments because it reuses the same addresses.

Is a new score required? "This wallet makes it hard, but not impossible, to spy on your balance and payments. If you believe you'll be making thousands of payments from your wallet, another alternative may provide better privacy."?

Contributor

saivann commented Jun 13, 2015

@gurnec @jim618 Maybe I'm missing something, but if there is a tiny fixed transaction output included in each transaction, it's quite easy to track which addresses are owned by the same user despite no address reuse, unless everyone was using MultiBit. Am I mistaken?

I agree that anything that easily lets everyone track everyone's payments is worth creating requirements, ideally in a near future. This said, I don't think this should be a blocking issue for replacing MultiBit. BRIT cannot be worse than current MultiBit with address reuse. It's just a matter of choosing the most appropriate score IMO.

Contributor

gary-rowe commented Jun 14, 2015

@saivann The tiny fixed output isn't attached to every spend transaction.

The wallet keeps track internally of each spend that has been made from it. As each spend is made the wallet allocates 1000 satoshis towards a running total. The user sees this each time they create a spend:

Payment Confirmation

so it's not hidden anywhere.

When between 15 to 25 spends have been made a single output is added to the transaction at a random position for between 15000 and 25000 satoshis (less a small random number "discount" to avoid obvious multiples) to a BRIT address. The fee counter then resets back to zero, a new interval of between 15 and 25 is made and the process repeats.

In all cases the user will generate their own addresses using the BIP32 mechanism which ensures (within reasonable statistical measures) no duplicate addresses which I think meets the requirement of the address reuse score.

As Jim pointed out earlier, this is version 1 of BRIT. We intend to make changes based on feedback from the community.

EDIT: We don't subtract a random fee any more. User feedback was that because of the way the running total was shown it was rather confusing to suddenly see a lesser value.

jim618 commented Jun 14, 2015

@gurnec I think the idea of having another score as you suggest is a good one.

The whole raison d'etre for the classifications is to make it easy for non-technical people to make informed choices. There is a privacy impact of using BRIT so users should be given a non-technical explanation.

Contributor

saivann commented Jun 14, 2015

Well then I think I agree with @gurnec, checkpassprivacyaddressrotation seems more accurate at a first glance. I'm not sure if the new score, at least as currently suggested, is different enough to be meaningful. But if others think so, I am fine with it.

Contributor

crwatkins commented Jun 14, 2015

As I wrote just last week in regard to another wallet: "In general, I'm hesitant to create new scoring or we are likely to approach as many scores as wallets." Each new wallet these days is innovative and has new features that don't fit every category perfectly which regularly causes a new score to be suggested during review. We're currently discussing nuances that are very important, yet the details of which will likely be lost on our target audience. I'm not opposed to tweaking our existing scoring to be more inclusive or more correct, but I would prefer to stick with fewer, higher level scores rather than a separate score for every new wallet.

jim618 commented Jun 14, 2015

@crwatkins Agreed - too many categories don't really help the user make a decision

Contributor

crwatkins commented Jun 15, 2015

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

I concur with the scoring in the pull request.

MultiBit HD

Version 0.1

Review version 2015061401

The wallet list is based on the personal evaluation of the maintainer(s) and regular contributors of this site, according to the criterias 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.

NOTE MultiBit HD supports Trezor hardware which was reviewed

NOTE MultiBit HD supports Tor which was not tested

NOTE MultiBit HD supports BIP70 which was not tested

Basic requirements:

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

PASS No concerning issues were found during public beta period. Previous MultiBit Classic version is hugely popular (as of March 2014 there were 1.5 million downloads) https://multibit.org/blog/2014/03/10/multibit-downloads-reach-1.5m.html

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

PASS There have been many reports of lost and stolen coins at https://github.com/jim618/multibit/issues/ however most seem to not be be any fault of the wallet. One report that decidedly resulted in lost coins is described here https://multibit.org/blog/2015/02/02/wallet-forensics.html It seems most likely that MultiBit Classic allowed the import of a bad address which was subsequently used. It can be argued whether it is worse to ask to use a bad address or whether to accept it when asked (in any case, MultiBit no longer accepts addresses for import).

  • 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. The developers are very responsive.

  • No indication that the wallet uses unstable or unsecure libraries

PASS Uses bitcoinj

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

PASS Testing routines can be found on github, and issues are tracked and reviewed/tested before closing on github.

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

PASS With an open public beta running since Feb 2015 and with open source code and issue tracking fully public since Aug 2014, the 3 month requirement is waived.

  • No concerning bug is found when testing the wallet

PASS No concerning bug was found

  • Website supports HTTPS and 301 redirects HTTP requests

PASS multibit.org

PASS multibit.org Grade B (for weak ciphers)

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

PASS max-age=356 days

  • The identity of CEOs and/or developers is public

PASS Developers are well known https://multibit.org/community.html

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

N/A No central MultiBit server holds encryption keys

  • 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 BIP39 “wallet words” can be used to fully recover funds

PASS Wallet performs snapshot backups to a user supplied “cloud backup” directory

  • Restoring wallet from backup is working

PASS Used BIP39 phrase to restore funds using Hive Wallet.

PASS Used “cloud backup” snapshot and BIP39 phrase to fully restore wallet (with wallet metadata such as contacts and notes)

PASS Used BIP39 phrase to recover password

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

PASS https://github.com/bitcoin-solutions/multibit-hd

  • If user has no access to some of the private keys in a multi-signature wallet:

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
  • 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 criterias (some could become requirements):

  • Received independent security audit(s)

FAIL No known audits

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

PASS Uses a new change address

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

PASS Uses a new address each time

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

PASS Does not show received from address

  • Uses deterministic ECDSA nonces (RFC 6979)

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

  • Provides a bug reporting policy on the website

PASS https://multibit.org/help.html and github issue reporting https://github.com/bitcoin-solutions/multibit-hd/issues

  • If user has no access over its private keys:

N/A

  • Full reserve audit(s)
  • Insurrance(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 BIP32 path (m/0’/c/i) with standard BIP39 phrase (verified)

NOTE When using Trezor, the BIP44 path is used

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

PASS Provides user one step to write down “wallet words” and requires another step to confirm

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

PASS Uses scrypt to derive encryption key

  • On desktop platform:
    • Encrypt the wallet by default

PASS Wallet is encrypted by default with password using AES 256

NOTE There is no complexity imposed or suggested on the password

  • For hardware wallets:

N/A

  • Prevents downgrading the firmware
Contributor

harding commented Jun 15, 2015

I just read @crwatkins's final review, and I'm reminded how great MultiBit HD is compared to most of the wallets we list. So how about this for a compromise regarding address reuse:

  1. We let MultiBit HD keep the current pass score for address rotation. I think we're certainly at fault for using phrasing that failed to account for non-change automated address reuse.
  2. At some later date, we work on better describing what Bitcoin.org expects with regard to not reusing addresses. We can continue the technical part of this discussion then.

If this is agreeable and there are no other objections, let's look at merging sometime Thursday.

I also want to take a moment to specially thank @schildbach and @greenaddress for participating in this discussion. I get really excited when I see wallet authors helping to examine other wallets.

Contributor

luke-jr commented Jun 15, 2015

@harding ACK

Contributor

gary-rowe commented Jun 15, 2015

Thanks to everyone for taking the time to look over MultiBit HD and offering up your suggestions - we really do appreciate the feedback.

I talked it over with Jim and we're both happy with a Thursday merge and we'll liaise with @harding in order to make the transition smooth by email/IRC as convenient.

@harding harding closed this in 24e4083 Jun 18, 2015

harding added a commit that referenced this pull request Jun 18, 2015

Merge PRs #887, #900, #902
- #887: Replacing MultiBit Classic with MultiBit HD
- #900: Community: Add Bitcoin Embasy Amsterdam
- #902: Update Mycelium description

@crwatkins crwatkins referenced this pull request Jun 25, 2015

Closed

Add Copay wallet #888

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