mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Should MasterCore wallet be separate by default? #238

Open zathras-crypto opened 9 years ago

zathras-crypto commented 9 years ago

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:

On the con side there are:

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 commented 9 years ago

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:

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

because blocks can be stored out of order

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

dexX7 commented 9 years ago

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

zathras-crypto commented 9 years ago

I see I see yep very true - Bitcoin 0.10 would need MasterCore 0.1.0 On 13/12/2014 12:16 AM, "dexX7" notifications@github.com 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 https://github.com/mastercoin-MSC/mastercore/issues/238#issuecomment-66770999 .

zathras-crypto commented 9 years ago

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

any thoughts?

dacoinminster commented 9 years ago

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 notifications@github.com 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 https://github.com/mastercoin-MSC/mastercore/issues/238#issuecomment-66929687 .

dexX7 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

zathras-crypto commented 9 years ago

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

dexX7 commented 9 years ago

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 commented 9 years ago

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" notifications@github.com 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 https://github.com/mastercoin-MSC/mastercore/issues/238#issuecomment-68613567 .

m21 commented 9 years ago

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

zathras-crypto commented 9 years ago

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.