search

mastercoin-MSC / mastercore

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

Encapsulate code that uses wallet with #ifdef ENABLE_WALLET #189

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

This compiles and works with with --disable-wallet (or --without-wallet).

ghost commented 9 years ago

I know @achamely needs this... can he test it out and report back on if he has any issues building and using this PR?

zathras-crypto commented 9 years ago

Is this not disabling a significant portion of mastercore code (like the block handlers)?

achamely commented 9 years ago

I was originally misinterpreting the option set. At this time i don't believe I need this.

dexX7 commented 9 years ago

@zathras-crypto: the handlers are never called and Master Core not initialized. Seems like this is enough. Ubuntu 14.04 builds and runs fine with a clean ./configure --without-wallet. The RPC calls are already flagged as "wallet only".

This is a minimal patch - I guess a clean solution would be something like --enable-mastercore.

m21 commented 9 years ago

This (kind of) removes (most) MasterCore functionality from the binary -- making it a "pure" Bitcoin Core binary. Why do we need this? :)

dexX7 commented 9 years ago

Haha that's a great question. :D I don't know - maybe to have an intermediate solution that doesn't break everything, if one compiles MC with --disable-wallet which is currently a legit build option? Discouraging doing so by removing a few lines in the readme isn't really a guard against it.

dexX7 commented 9 years ago

I saw zathras #188 and this was the obvious next step.

That aside, on a mid-timeframe I believe MC might as well be build without wallet and simply provide options to broadcast and encode transactions which don't necessarily require a wallet.

m21 commented 9 years ago

Running MasterCore Without-Wallet is probably a valid scenario. This patch disables MasterCore, not quite the same. :) Table it?

dexX7 commented 9 years ago

Table it?

Sorry, language barrier. Can you rephrase please?

dacoinminster commented 9 years ago

Verb "table" in this case means "postpone consideration of" ( https://www.google.com/#q=define:table)

On Thu, Oct 30, 2014 at 10:32 AM, dexX7 notifications@github.com wrote:

Table it?

Sorry, language barrier. Can you rephrase please?

— Reply to this email directly or view it on GitHub https://github.com/mastercoin-MSC/mastercore/pull/189#issuecomment-61134085 .

dexX7 commented 9 years ago

Well, the immediate goal is not to create a Master Core that is able to run without the wallet component and still provides full functionality, but to guard against the case where one builds MC with --disable-wallet which currently results in a bunch of errors while building. I consider a Master Core without Master Core features as less harmful than a bunch of build errors without explanation.

m21 commented 9 years ago

MasterCore without wallet functionality is not supported right now. I believe it's a valid use case, but we haven't put any thought into it. So a failing build while attempting building Without-Wallet is the correct behavior. At least better than building a non-working MasterCore instead. That would be misleading, sorry. (I meant to close this patch if we are in agreement.)

dexX7 commented 9 years ago

MasterCore without wallet functionality is not supported right now.

It is. That's what this PR is for.

walletless_1

walletless_2

walletless_3

m21 commented 9 years ago

This patch disables all MasterProtocol functionality. How could that be a useful feature of MasterCore? :)

dexX7 commented 9 years ago

The lack of RPC calls that work without wallet is a whole different story imho and your question is like asking "what use has --disable-wallet for one who wants to maintain a wallet"?

Maybe this is the wrong approach. Could you modify the makefiles so building with --disable-wallet isn't available anymore? OTOH if there should be a wallet-less MC version at some point in the future, I believe this is the starting point nevertheless.

m21 commented 9 years ago

Somehow we are not understanding each other :| I don't like --wallet-disable to perform a --disable-MasterCore, and compile mastercored and totally confuse everybody. I don't believe we know how to do MasterCore w/o a wallet (yet -- spent no time on that).

ghost commented 9 years ago

Hm, it seems like this issue might be avoided with a clear message in the readme:

**Limitations:
-disable-wallet is not supported, but might be in a future release

is a code solution really needed here?

dexX7 commented 9 years ago

Gotcha, then the right approach on top is to remove the option --wallet-disable altogether imho, but it's still sort of a different topic.

... clear message in the readme

This would be great, yes.

... is a code solution really needed here?

Imho yes, because documentation is only one aspect. Almost every RPC call has checks in place to make sure user input is within the allowed range and so on and I assume you would agree that a note in the doc such as "please submit only valid input" isn't suffficient.

... totally confuse everybody.

Are you saying a bunch of errors is less confusing? I would assume MC is broken - which errors sort of imply.

zathras-crypto commented 9 years ago

Since this PR disables the block handlers this means the state of the Master Protocol won't be accurate internally in Core, so we're effectively removing the entire purpose for the software - I do not see much value in expending much effort to enable that - could we just disable that build option?

dexX7 commented 9 years ago

Yes, removing the build option would be great. A note in the documentation in addition maybe too, because it might not be obivious that MC requires the wallet.

ghost commented 9 years ago

I think we should close this no? IMO disabling the MP functionality just for wallet support isn't the right way forward. Perhaps open a separate issue to discuss, but I think this can be closed

m21 commented 9 years ago

I will update the README file to state that --without-wallet does not work.