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.
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.
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.
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>
Updating CreateNewBlock() to take a script, rather than a CReserveKey, should make it possible to enable getblocktemplate RPC.
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.
Interesting how little code changes were necessary.
Is there a good reason why this isn't intended to be merged? :p
Cannot be merged... as-is. I wouldn't mind merging a cleaned up version, which attention paid to indentation and such.
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.
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.
All P2P-induced calls from main to wallet should go through the registration interface.
Code cleaned up, and perhaps merge-worthy.
TODO list:
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?
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...
Why are we keeping the internal miner at all?
Because we have not provided an adequate replacement with the package, and it's commonly used and perfectly reasonable on testnet.
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.
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"
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?
621 | + return InitError(_("wallet.dat corrupt, salvage failed")); 622 | } 623 | - if (r == CDBEnv::RECOVER_FAIL) 624 | - return InitError(_("wallet.dat corrupt, salvage failed")); 625 | - } 626 | + } // (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).
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 :)
no-wallet mode is now ready for reviewing and merging.
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).
What about "-nowallet"?
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?
As I said, -foo=0 is -nofoo for every parameter we use, why special case this one?
-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.
1036 | @@ -1034,6 +1037,8 @@ void ServiceConnection(AcceptedConnection *conn) 1037 | const CRPCCommand *pcmd = tableRPC[strMethod]; 1038 | if (!pcmd) 1039 | throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found"); 1040 | + if (pcmd->reqWallet && !pwalletMain) 1041 | + 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.
853 | @@ -848,92 +854,97 @@ bool AppInit2(boost::thread_group& threadGroup) 854 | 855 | // ********************************************************* Step 8: load wallet 856 | 857 | - uiInterface.InitMessage(_("Loading wallet...")); 858 | + if (fDisableWallet) { 859 | + uiInterface.InitMessage(_("Wallet disabled..."));
Pet peeve: don't use "..." when it's not about an ongoing operation.
Agreed, even if this is a minor thing I would use Wallet disabled! here.
+1 for Wallet disabled!
1015 | + pwalletMain->ScanForWalletTransactions(pindexRescan, true); 1016 | + printf(" rescan %15"PRI64d"ms\n", GetTimeMillis() - nStart); 1017 | + pwalletMain->SetBestChain(CBlockLocator(pindexBest)); 1018 | + nWalletDBUpdated++; 1019 | + } 1020 | + } // (1)
This (1) seems incorrect?
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).
Rebased. This is merge-ready, except for option-naming shed-painting.
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.
-disablewallet is a fine shade of paint for the shed; I think you should rebase, sanity test one last time, and merge.
https://github.com/wtogami/bitcoin/commits/btc-0.8.5-disablewallet Disable Wallet for Bitcoin 0.8.5
Please s/Wallet disabled.../Wallet disabled!/
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
s/Wallet disabled.../Wallet disabled!/
s/Wallet disabled.../Wallet disabled!/