Introduce disable-wallet / no-wallet mode. #2901

jgarzik · 2013-08-15T17:33:45Z

Goal: As one goal is to eventually separate wallet/GUI from public blockchain engine, it is useful to explore what such an engine would look like. As such, disablewallet=1 will enforce pwalletMain==NULL, or "no-wallet mode."

Many RPCs are disabled. getblocktemplate does continue to work, permitting no-wallet mining.

BitcoinPullTester · 2013-08-15T18:14:14Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/116c9582760e1fc37408ce74abd0769062aa9025 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

BitcoinPullTester · 2013-08-15T19:42:14Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/588e13c40f4e66b3d789f9b426ef173acc21d9ed for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

jgarzik · 2013-08-15T20:09:08Z

Completed review of all RPCs for require-wallet status, and fixed a couple bugs found in testing. disable-wallet mode should be working in bitcoind now. These are the RPCs that remain available:

addnode <node> <add|remove|onetry>
createmultisig <nrequired> <'["key","key"]'>
createrawtransaction [{"txid":txid,"vout":n},...] {address:amount,...}
decoderawtransaction <hex string>
getaddednodeinfo <dns> [node]
getbestblockhash
getblock <hash> [verbose=true]
getblockcount
getblockhash <index>
getconnectioncount
getdifficulty
getgenerate
gethashespersec
getinfo
getmininginfo
getpeerinfo
getrawmempool
getrawtransaction <txid> [verbose=0]
gettxout <txid> <n> [includemempool=true]
gettxoutsetinfo
help [command]
sendrawtransaction <hex string>
signrawtransaction <hex string> [{"txid":txid,"vout":n,"scriptPubKey":hex,"redeemScript":hex},...] [<privatekey1>,...] [sighashtype="ALL"]
stop
submitblock <hex data> [optional-params-obj]
validateaddress <bitcoinaddress>
verifychain [check level] [num blocks]
verifymessage <bitcoinaddress> <signature> <message>

jgarzik · 2013-08-15T20:10:25Z

Updating CreateNewBlock() to take a script, rather than a CReserveKey, should make it possible to enable getblocktemplate RPC.

jgarzik · 2013-08-15T21:06:26Z

disable-wallet mode now skips BDB environment setup, reducing startup RSS here by over 40MB (h/t @gmaxwell)

BitcoinPullTester · 2013-08-15T21:10:13Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/db2b19ee838cdab513a6503ddefc16d985b59e87 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

sipa · 2013-08-15T21:41:36Z

Interesting how little code changes were necessary.

sipa · 2013-08-15T22:31:16Z

Is there a good reason why this isn't intended to be merged? :p

jgarzik · 2013-08-15T22:37:55Z

Cannot be merged... as-is. I wouldn't mind merging a cleaned up version, which attention paid to indentation and such.

gmaxwell · 2013-08-15T22:39:23Z

Only argument I can make against it is that it's more configurations to test and getting the 40mbytes back from BDB on walletless nodes would be nice.

jgarzik · 2013-08-15T22:41:06Z

Also, my paranoid fear is that I missed a spot in review and testing, and someone will figure out a way to crash a node by triggering a wallet lookup somehow, somewhere.

sipa · 2013-08-15T22:41:54Z

All P2P-induced calls from main to wallet should go through the registration interface.

jgarzik · 2013-08-15T23:35:15Z

Code cleaned up, and perhaps merge-worthy.

jgarzik · 2013-08-15T23:39:46Z

TODO list:

getblocktemplate should be re-enabled, in some form (requires a key for the default txout script, due to CreateNewBlock calling convention)
I thought of another TODO item, then promptly forgot. Nevertheless, a bullet point list is not a list without at least two items.

luke-jr · 2013-08-16T06:14:16Z

Thoughts on just removing getwork, since it's useless with mainnet and getblocktemplate's use of CreateNewBlock doesn't need a generation transaction at all?

gmaxwell · 2013-08-16T06:20:17Z

Internal miner needs a generation transaction. :-/ But yea, removing getwork sounds great to me, except for the whole not solving the needing a generation transaction bit...

wtogami · 2013-08-17T12:13:19Z

Why are we keeping the internal miner at all?

gmaxwell · 2013-08-17T12:27:44Z

Because we have not provided an adequate replacement with the package, and it's commonly used and perfectly reasonable on testnet.

Diapolo · 2013-08-17T12:40:16Z

@gmaxwell Agreed, it's the only miner I use with Testnet as it's so damn easy to use and JUST works.

wtogami · 2013-08-18T05:51:07Z

With getblocktemplate and submitblock working, this type of node could still be used for p2pool or pools, with the payout address arbitrarily elsewhere for security. That is already possible today with normal nodes with a wallet, but this at least shrinks the memory requirement per p2pool node, which is great.

jgarzik · 2013-08-20T11:39:57Z

The mining-removal discussion is outside the scope of this pull request, and is better discussed in active mailing list threads or pull req #2905

getblocktemplate can work just fine in disablewallet mode; just needs additional work appended here. Hence "TODO"

laanwj · 2013-08-22T06:07:18Z

Nice. Incidentally this would also be the "no wallets loaded" case for multi wallet support.

Haven't tested yet, but from looking at the code: validateaddress does a IsMine check (dereferencing pWalletMain) when the address is valid. Will this crash?

Diapolo · 2013-08-24T13:36:13Z

src/init.cpp

-        if (r == CDBEnv::RECOVER_FAIL)
-            return InitError(_("wallet.dat corrupt, salvage failed"));
-    }
+    } // (1)


What does that (1) mean?

Look at the commits one-by-one. In the code movement commit, you see a "if (1) { ... }" change. This comment is a remnant of that change (that should be removed).

jgarzik · 2013-08-25T04:15:43Z

Rebased on top of #2928 and fixed @Diapolo 's nit.

getblocktemplate now works in no-wallet mode. OP updated accordingly. Should be merge ready, modulo another IsMine() review. I reviewed quickly based on @laanwj 's comment, but did not see the case. Will look more closely with brain fully engaged before saying it is 100% merge-ready :)

jgarzik · 2013-08-26T02:03:50Z

Rebased, and fixed the bug found by @laanwj

jgarzik · 2013-08-27T02:57:33Z

no-wallet mode is now ready for reviewing and merging.

gavinandresen · 2013-08-28T00:32:42Z

Nit / pet peeve: negative options, because I hate double-negatives (-disablewallet=0 means "yes I want a wallet please").

Suggest that the option be: -wallet= to mean "no wallet, please." (default right now is -wallet=wallet.dat).

laanwj · 2013-08-29T11:26:41Z

What about "-nowallet"?

Diapolo · 2013-08-29T13:43:41Z

@laanwj -wallet=0 IS -nowallet, which I would vote for as a name for the switch.

wtogami · 2013-09-08T10:56:04Z

I change my mind. I like -disablewallet the way it is now. -disablewallet is best as a distinct parameter for a specific purpose. It is more confusing to overload -wallet= with another possible meaning, and you don't want it to attempt load a wallet file of that name if you have a typo.

Bleh, before we bikeshed on the name, could folks please review the actual code?

Diapolo · 2013-09-08T11:19:15Z

As I said, -foo=0 is -nofoo for every parameter we use, why special case this one?

jgarzik · 2013-09-08T15:36:55Z

-nowallet is fine with me, as long as it does not break, or otherwise require contortions of, the existing wallet pathname support.

"-nowallet" seems quite natural, but a concern is that people were discussing -wallet=foo.dat -wallet=bar.dat and similar extensions for multiple wallet support.

I'm happy to go with whatever most people prefer.

sipa · 2013-09-28T20:35:33Z

src/bitcoinrpc.cpp

@@ -1034,6 +1037,8 @@ void ServiceConnection(AcceptedConnection *conn)
    const CRPCCommand *pcmd = tableRPC[strMethod];
    if (!pcmd)
        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
+    if (pcmd->reqWallet && !pwalletMain)
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");


I think "Method requires wallet" or something explicitly mentioning why it is disabled is more useful.

sipa · 2013-09-28T20:41:38Z

Code looks good, apart from a few nits (see inline), needs rebase though.

I don't feel strongly about -disablewallet or -nowallet or -wallet=. We just need to know how it may later integrate with multiwallet (which naturally extends nowallet).

jgarzik · 2013-10-02T15:20:39Z

Rebased. This is merge-ready, except for option-naming shed-painting.

BitcoinPullTester · 2013-10-04T07:24:01Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/5d4f3a1f0cbdf52257b41ea09d175c0018ad9434 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

gavinandresen · 2013-10-24T01:24:45Z

-disablewallet is a fine shade of paint for the shed; I think you should rebase, sanity test one last time, and merge.

wtogami · 2013-10-27T23:04:23Z

https://github.com/wtogami/bitcoin/commits/btc-0.8.5-disablewallet
Disable Wallet for Bitcoin 0.8.5

Please s/Wallet disabled.../Wallet disabled!/

wtogami · 2013-10-29T23:39:38Z

https://bitcointalk.org/index.php?topic=320695.0
If end-users want to help testing of this patch, a backport is included in this build of Bitcoin 0.8.5

wtogami · 2013-11-01T22:44:43Z

Rebase
disablewallet=1 needs a GUI error message if someone tries it with bitcoin-qt
s/Wallet disabled.../Wallet disabled!/

KobuderaRoninShinobi · 2013-11-02T07:52:00Z

s/Wallet disabled.../Wallet disabled!/

sipa · 2013-11-09T17:34:07Z

@jgarzik Can you rebase please?

laanwj · 2013-11-12T14:00:12Z

See #3240

This was referenced Aug 20, 2013

Remove embedded CPU miner and eliminate wallet dependency for getblocktemplate #2917

Closed

getblocktemplate without a wallet #2918

Closed

jgarzik mentioned this pull request Aug 24, 2013

CreateNewBlock / getblocktemplate cleaning #2928

Merged

Diapolo reviewed Aug 24, 2013
View reviewed changes

sipa reviewed Sep 28, 2013
View reviewed changes

Jeff Garzik added 2 commits October 2, 2013 11:15

init.cpp: cosmetic indent changes, preparing for no-wallet mode

520cc05

Add -disablewallet option, to disable wallet support (and BDB)

5d4f3a1

wtogami mentioned this pull request Oct 25, 2013

Disable Wallet Mode for Litecoin litecoin-project/litecoin#78

Merged

laanwj mentioned this pull request Nov 12, 2013

Introduce disable-wallet / no-wallet mode (rebased) #3240

Merged

laanwj closed this Nov 12, 2013

jgarzik deleted the no-wallet branch August 24, 2014 04:20

IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this pull request Jun 4, 2019

Implement isolate_node/reconnect_isolated_node in tests (bitcoin#2901)

357b727

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Introduce disable-wallet / no-wallet mode. #2901

Introduce disable-wallet / no-wallet mode. #2901

Conversation

jgarzik commented Aug 15, 2013

BitcoinPullTester commented Aug 15, 2013

BitcoinPullTester commented Aug 15, 2013

jgarzik commented Aug 15, 2013

jgarzik commented Aug 15, 2013

jgarzik commented Aug 15, 2013

BitcoinPullTester commented Aug 15, 2013

sipa commented Aug 15, 2013

sipa commented Aug 15, 2013

jgarzik commented Aug 15, 2013

gmaxwell commented Aug 15, 2013

jgarzik commented Aug 15, 2013

sipa commented Aug 15, 2013

jgarzik commented Aug 15, 2013

jgarzik commented Aug 15, 2013

luke-jr commented Aug 16, 2013

gmaxwell commented Aug 16, 2013

wtogami commented Aug 17, 2013

gmaxwell commented Aug 17, 2013

Diapolo commented Aug 17, 2013

wtogami commented Aug 18, 2013

jgarzik commented Aug 20, 2013

laanwj commented Aug 22, 2013

Diapolo Aug 24, 2013

Choose a reason for hiding this comment

jgarzik Aug 24, 2013

Choose a reason for hiding this comment

jgarzik commented Aug 25, 2013

jgarzik commented Aug 26, 2013

jgarzik commented Aug 27, 2013

gavinandresen commented Aug 28, 2013

laanwj commented Aug 29, 2013

Diapolo commented Aug 29, 2013

wtogami commented Sep 8, 2013

Diapolo commented Sep 8, 2013

jgarzik commented Sep 8, 2013

sipa Sep 28, 2013

Choose a reason for hiding this comment

sipa commented Sep 28, 2013

jgarzik commented Oct 2, 2013

BitcoinPullTester commented Oct 4, 2013

gavinandresen commented Oct 24, 2013

wtogami commented Oct 27, 2013

wtogami commented Oct 29, 2013

wtogami commented Nov 1, 2013

KobuderaRoninShinobi commented Nov 2, 2013

sipa commented Nov 9, 2013

laanwj commented Nov 12, 2013