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

Should MasterCore wallet be separate by default? #238

Open
zathras-crypto opened this issue Dec 12, 2014 · 17 comments
Open

Should MasterCore wallet be separate by default? #238

zathras-crypto opened this issue Dec 12, 2014 · 17 comments
Labels

Comments

@zathras-crypto
Copy link

Hey all,

With the tag of 0.0.9 coming up and this including the first windows UI, I think we stand to get an increase in user base.

I'd like to open the discussion on the default wallet. We've touched on this briefly before with views on both sides, so I wanted to get the discussion going and reach a consensus.

The topic is simply that of "what do we use for our default wallet when we startup?".

There are two possible options:

  1. We re-use the users bitcoin wallet.dat by default
  2. We use a separate mastercore mpwallet.dat by default

Now code is a non-issue (it is quite literally a one line change) so the debate is a simple one of want we think is best.

On the pro side there are:

  • Minimizes risk surface (bugs only affect funds trusted to MasterCore, not affect all of users Bitcoin)
  • Minimizes trust required ("trust us with your MP tokens" is an easier sell than "trust us with your MP tokens and your Bitcoins")

On the con side there are:

  • Requires user to backup another wallet
  • Any MP tokens in bitcoin wallet would not be visible

Could you guys please weigh in?

Also worth noting is that if we do default to our own separate mpwallet.dat, it's a simple parameter to override that and use the bitcoin wallet.dat (--wallet=wallet.dat).

I like the idea of a separate wallet purely for the reduced risk surface and I don't really see the cons as obstacles (especially when you can just load the bitcoin wallet with a param if you desired and as for backups, well if a user installs Litecoin they have to backup their Litecoin wallet, I see no problem at all with the same applying to MasterCoin). The question is one of default behavior - please discuss :)

Thanks
Z

@dexX7
Copy link

dexX7 commented Dec 12, 2014

Very interesting idea actually. Even though I'm not sure, if there is much risk at all, it could indeed be a "selling point" as you stated: "you can run this and literally your bitcoin wallet will be unaffected". ;)

I see some user experience problems though:

  • "What, you're messing around with my wallet.dat file? [I know you don't] This is probably really dangerous!"
  • Consider the practical use-case. The user has some coins in his wallet, switches to Master Core and starts with zero coins. What can he do to fund his new wallet?

In general, I like the concept, so it's more about how to do this smoothly. Is there a chance to fire a popup on the first startup which asks the user, if he likes to use a new wallet? I know that Bitcoin-Qt asks on first startup for a datadir location, at least recently, not sure, if it's in 0.9.3, and on Windows. Maybe this could be used as basis?

As a side note: I'm not a huge fan of the startup warning, especially with the big red error indicating icon on Ubuntu, but I see enough reason to fire one. In this context, I also suggest to warn only once, but not on every startup.

@zathras-crypto
Copy link
Author

Thanks for the feedback :)

"What, you're messing around with my wallet.dat file? [I know you don't] This is probably really dangerous!"

Absolutely, we have to convey the message "we supply a default --wallet=mpwallet.dat parameter to Core to use a new wallet file" or something - since it's an existing bitcoin function perhaps that buys us a little faith that we're not messing with the wallet files... If conveying that's problematic then yeah this would be a non-starter...

Consider the practical use-case. The user has some coins in his wallet, switches to Master Core and starts with zero coins. What can he do to fund his new wallet?

Perhaps I'm just too familiar with things haha... I actually run like this myself on some boxes as I like to poke about with changes to vanilla Bitcoin before I make them to MasterCore sometimes, Bitcoin Core uses wallet.dat and Master Core uses mpwallet.dat (funds not an issue I just close Master Core, open Bitcoin Core, send some funds to the other wallet, close, reopen Master Core) but then again perhaps we'd have issues with people trying to run them simultaneously (big no-no we're not setup for shared blockchain usage, locks would hopefully prevent a mess, though good test if anyone has time).

Completely agree has to be easy for user, a startup dialog would be good we'd need to look into where/how Bitcoin Core stores settings (eg the datadir it requests on startup) and whether they're available to us at the time we define the wallet file - if so should be reasonably straight forward to just ask the user what they want to do.

Also re startup warning, that's been the agreed approach - fire once and ack, if/when we change wording/text can reset that ack flag and redisplay - just a case of coding something to store the fact it's been acknowledged - can be touching a flat file or perhaps some form of settings as mentioned above.

@dexX7
Copy link

dexX7 commented Dec 12, 2014

Just as a quick note: Bitcoin Core 0.10.0 is almost ready with headers-first and this is IIRC incompatible to earlier versions, because blocks can be stored out of order.

Edit:

Blocks will be stored on disk out of order (in the order they are
received, really), which makes it incompatible with some tools or
other programs. Reindexing using earlier versions will also not work
anymore as a result of this.

http://sourceforge.net/p/bitcoin/mailman/message/32921390/

@zathras-crypto
Copy link
Author

because blocks can be stored out of order

Transactions still have an index in a block which once mined are fixed right?

@dexX7
Copy link

dexX7 commented Dec 12, 2014

Yes, sure, but I think the point was: you cannot use the datadir for both.

@zathras-crypto
Copy link
Author

I see I see yep very true - Bitcoin 0.10 would need MasterCore 0.1.0
On 13/12/2014 12:16 AM, "dexX7" [email protected] wrote:

Yes, sure, but I think the point was: you cannot use the datadir for both.


Reply to this email directly or view it on GitHub
#238 (comment)
.

@zathras-crypto
Copy link
Author

@m21 @CraigSellars @dacoinminster @DavidJohnstonCEO @faiz @marv-engine @adam

any thoughts?

@dacoinminster
Copy link

If we are installing software on their PC where their bitcoin wallet is,
they have to trust us, because we could be using a keylogger. I don't think
using a different wallet file helps with that issue. Maybe it helps if they
are worried about us doing something accidentally to their BTC . . .

I think I would prefer to handle BTC and MSC. Don't feel strongly about it
if others disagree though.

On Sun, Dec 14, 2014 at 1:06 PM, zathras-crypto [email protected]
wrote:

@m21 https://github.com/m21 @CraigSellars
https://github.com/CraigSellars @dacoinminster
https://github.com/dacoinminster @DavidJohnstonCEO
https://github.com/DavidJohnstonCEO @faiz https://github.com/faiz
@marv-engine https://github.com/marv-engine @adam
https://github.com/adam

any thoughts?


Reply to this email directly or view it on GitHub
#238 (comment)
.

@dexX7
Copy link

dexX7 commented Dec 16, 2014

The "-choosedatadir" dialog looks actually pretty nice:

datadir

It lives in qt/intro.cpp, but I'm still not sure about the practial use. If there were "profiles" (e.g. "mastercoin tokens", "bitcoin savings wallet", "bitcion tips", ...) which could be switched back and forth, I would love it. But right now, regarding something more simple, I think my imagination is too limited. ;)

@zathras-crypto:
Remembering a value turned out to be very easy as it seems: QSettings - Basic Usage

It might be possible to do something like:

// Project wide key:value storage
QSettings settings;
// Get earlier acknowledgement and skip startup warning, if true
bool bSkipWarning  = settings.value("bWarningAcknowledged", false);
// Otherwise show a warning about potential coin loss
if (!bSkipWarning) {
    bool bHideWarningNextTime = false;
    // And ask, if the user likes to hide it in the future
    bHideWarningNextTime = ShowWarningAndStoreAcknowledgement();
    // Finally store the user's decision
    settings.setValue("bWarningAcknowledged", bHideWarningNextTime);
}

I didn't test it and this, but void setValue(const QString & key, const QVariant & value) and QVariant value(const QString & key, const QVariant & defaultValue = QVariant()) const) look nice to deal with.

@m21
Copy link

m21 commented Jan 2, 2015

The user always needs Bitcoins to be able to send Mastercoins.
Thus if they don't wish to have all their networth in one wallet they are juggling separate wallet.dat files already (wallet-1btc.dat & wallet-1000btc.dat for instance).
I really don't wish to add wallet-management functions to MasterCore (the wallet is neither MC's core functionality nor Bitcoin Core's core functionality anyway); I believe with --wallet & --datadir and the above need for BTC to always exist within our wallet sufficient solutions already exist.

@zathras-crypto
Copy link
Author

Sure sure, this is purely a question of default.

--wallet=wallet.dat we use the bitcoin wallet by default.
--wallet=mpwallet.dat we use a fresh mastercoin wallet by default.

Both are unbelievably simple - we're not adding wallet management or anything like that (unless I've missed something), purely (IIRC) it's just a question of whether the default wallet filename on first run is wallet.dat (existing) or mpwallet.dat (new). I think we were talking about a dialog to select as a possibility too.

But yes, no extra functions - just quite literally (already tested) changing a single line of code to change the default; std::string strWalletFile = GetArg("-wallet", "wallet.dat");. wallet.dat is passed as default argument, so we can set said default arg to whatever we like without heavy work.

Thanks
Z

@dexX7
Copy link

dexX7 commented Jan 3, 2015

@zathras-crypto: how about a forced "-chosedatadir" on first startup with a slightly changed text?

@zathras-crypto
Copy link
Author

--wallet uses the same blockchain, choosing a new datadir would mean storing a second copy of the blockchain.

@dexX7
Copy link

dexX7 commented Jan 3, 2015

Sorry, let me rephrase: how about using the "-chosedatadir" dialog as basis to ask for a wallet.dat location? It could be combined with the "warning".

@zathras-crypto
Copy link
Author

Ah I see - yeah I think this was mentioned before - I'm all for that - if
everyone's happy with that approach I'll put it into the UI :)
On 04/01/2015 10:18 AM, "dexX7" [email protected] wrote:

Sorry, let me rephrase: how about using the "-chosedatadir" dialog as
basis to ask for a wallet.dat location?


Reply to this email directly or view it on GitHub
#238 (comment)
.

@m21
Copy link

m21 commented Jan 26, 2015

Seems like you'll be prompting them to potentially download another copy of blockchain? Yikes :(

@zathras-crypto
Copy link
Author

Not at all mate :)

--datadir would use a second copy of the blockchain, but I'm proposing overriding the default --wallet parameter not --datadir - this still uses the same datadir (blockchain) but just changes the wallet filename (I originally suggested from wallet.dat to mpwallet.dat). One wallet.dat for OmniCore, another wallet.dat for Bitcoin Core both sharing the same blockchain - potentially increase user adoption if users don't have to trust our code with all their BTC as well as all their Omni Protocol tokens. I think we should at least give them the option, defaults are subjective/debatable absolutely.

@m21 m21 added the Wallet label Jan 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants