Dynamically Loadable Multiple Wallet Support Complete!!! #2124
Conversation
|
It's a work in progress and can certainly be improved. I welcome all comments, suggestions, criticisms, and witty insults. |
|
Would it be better or possible to use references instead of pointers in your code? |
|
Possible, sure. Better, why? |
|
The sanity test failed because the test suite uses the old RPC function prototype. I was still able to build and run using
|
|
This may be an opportunity to move away from the wallets being called "wallet.dat", which makes it all the more easier for malware to steal the contents. |
|
You can now specify additional wallets in the config file or via command line arguments: or A default wallet called "" in the RPC and using file "wallet.dat" is always loaded, as to not break compatibility with the master branch. Therefore, additional wallets should neither be called "default" nor "wallet". The wallet will be stored in a file called wallet-foo.dat. If the wallet doesn't yet exist, it will be created the first time the wallet is loaded. |
=============================
A new global structure called pWalletMap has been added. Upon initialization, multiple wallets can be loaded.
A new CWallet* parameter has been added to the RPC functions. Functions which do not use a wallet simply ignore it. In addition, a new field has been added to CRPCCommand that tells us whether or not the function uses a wallet.
Two new RPC methods have been added:
1) listwallets - Returns an array containing wallet names.
2) usewallet - Prefix existing wallet RPC calls with usewallet <walletname> to use a particular wallet. If no wallet is specified, the default wallet is used.
example: bitcoind usewallet foo listreceivedbyaddress 0 true
TODO:
1) Get wallet names from bitcoin.conf in init.cpp.
2) Change help RPC so that the message does not depend on wallet state (i.e. walletpassphrase RPC call)
…to test_bitcoin.cpp
================================================================= You can now specify additional wallets in the config file or via command line arguments: usewallet=foo or bitcoind -usewallet=foo A default wallet called "default" in the RPC and using file "wallet.dat" is always loaded, as to not break compatibility with the master branch. Therefore, additional wallets should neither be called "default" nor "wallet" nor anything else that might conflict with other filenames in the .bitcoin directory. The wallet will be stored in a file called foo.dat. If the wallet doesn't yet exist, it will be created the first time bitcoind is run. TODO: Ensure filename collisions cannot occur. Perhaps allow the wallet files to be arbitrarily named rather than tied to their identifier.
…ons are synched automatically without having to call SyncWithWallets() explicitly. This is important when sending between two wallets in a single bitcoind instance so that the receiving wallet is alerted of the transaction immediately. Removed calls to SyncWithWallets() in ProcessMessage() since SyncWithWallets() is already called by tx.AcceptToMemoryPool(). SyncWithWallets() is still called explicitly from CBlock::ConnectBlock() since the transaction never goes through the mempool in this case.
… methods. Help always returns usage info now.
|
The getinfo RPC method now returns an array of wallets each with wallet-specific information. This change is, unfortunately, not backwards compatible. However, it doesn't really seem to make sense to make this call wallet-specific. And returning an array of wallets seems to be a very useful feature. |
|
An alternative would be to move that data out of getinfo and into a getwallet info. Part of the reason to do that is that the wallet outputs in getinfo can be rather slow already, and doing them for N wallets won't help matters. (Though lets see if anyone else has an opinion, I could be on drugs here) |
|
I was thinking listwallets should maybe show all this information with a verbose option. Without the verbose option, it would just give a list of the wallet names. |
|
Hm. From the perspective of the RPC as a CLI the use of verbose flags isn't especially discoverable— for something basic like 'get your balances' that is probably worth exposing at the top level. ::shrugs:: |
|
What about making the full info the default and having an option to shorten it? That way interactive users get relevant information while application developers seeking to improve performance have an option to do so if they don't need all that information. Of course we can just have two distinct RPC methods...but I'd rather avoid having too many RPC calls when the semantics are similar. |
|
I do think that the wallet information should be completely removed from getinfo, though. Anyhow, this is the kind of stuff that's really easy to implement and change up front but becomes a serious problem to change once people start using it to build applications. I'd also like to hear several more opinions on this before committing to anything specific. |
|
Another issue I'm wondering about is programmatic access to the config file. We could just append the new wallets to the end of the file whenever they are created - but it would be nice to be able to have tags for sections. Presumably whenever a user creates a new wallet they want it to be loaded at next startup unless they explicitly remove it from the config file. |
|
Multiple wallet support came into my head as I was doing #2075 but I left it aside in favor of trying to move the code in the direction of further modularization improvements between the RPC interface and the wallet and the wallet database as a prerequisite. I put off coding there since I expected something like this to come along and because commentary on the pull request died away. I can't review the code in detail now but this capability needs to happen. Passing CWallet * around is an improvement for sure, definitely better than another global. I'm finding it kinda cringeworth-brittle from an OO perspective, though. It would be lovely to see new RPC functions implemented in ways that drag suitable amounts of code that depends strongly on the wallet's internal representation out along with them the appropriate changes to the wallet object. Have you tried merging my code? I'll have to look at that later. Multiple wallets are useful for any number of reasons. Certainly the Bitcoin-only accountants and would-be tax collectors of the future aren't going to be running n instances of bitcoind on their desktops. Ultimate I think the wallet should be decoupled from the rest of the satoshi client. Pushing standarized interfaces down to wallet-object level is somewhere on the road to getting there. You seem to be addressing some modularization issues in other areas though, too. |
|
As for the OO stuff, I would like to abstract things further and define a listener interface, a transaction store interface, and a key store interface. Technically speaking, a signing agent doesn't even need to store complete transactions. Private keys and unspent outpoints are all it needs. Furthermore, a listening agent could send alerts to other components without the need to store any transactions. For instance, it could just provide filtering. Having said that, in the interest of seeing these pull requests merged with bitcoin/bitcoin master ASAP, I'm willing to do things incrementally and use whatever structures already exist for now. However, I'd be very interested in giving all this some deeper thought and coming up with a serious reorganization for some later version. As for now, at the very least it is possible to encapsulate the existing CWallet class much better - and I applaud your efforts. |
| result = pcmd->actor(params, false); | ||
| if (pcmd->unlocked) { | ||
| result = pcmd->actor(pWallet, params, false); | ||
| } | ||
| else { | ||
| LOCK2(cs_main, pwalletMain->cs_wallet); |
There was a problem hiding this comment.
This will need to lock the appropriate wallet (if any), not pwalletMain.
|
In general: I'm very much in favor of having native support for multiple wallets. Wallet stuff hasn't been a priority for some time, but if necessary I'd like to help to get this working. Regarding the idea of separating wallets for block chain processing: sure, that's definitely where we need to go in the future. Ideally, I think the code evolves to separate wallet and the rest along a clean interface in a first step indeed. In a second step, we can maybe make them separate processes started from the same binary or even just separate binaries with shared codebase. The final aim should be entirely separate things, either communicating via some private "wallet interconnect protocol" (where several wallets on a trusted network connect to a single validation service), or even turning the wallet processes into standalone SPV clients (with their own blockheader-chain), that can either connect directly to the network, or can connect to a server provide block/tx validation service, simply via the P2P protocol. |
… rather than iterating through the wallet map in init.
|
Hmm, still having some issues with the default wallet not unloading at shutdown. Will need to go over the shutdown sequence to make sure all threads relinquish their pointers to it. |
|
Just to set expectations: This big a change, in this critical a part of the codebase, will need:
See https://github.com/bitcoin/QA for the lastest QA process. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1c45edce2fbe76f7e146ee12c1df80c66c5fa8f9 for binaries and test log. |
|
Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/6c7f86ae371f955371fba05c7f747e4659cd39c8 for test log. This pull does not merge cleanly onto current master |
|
How i can install bitcoind whit Dynamically Loadable Multiple Wallet ? |
|
Rebase needed. |
|
This is awesome work. I hope it makes it way to BTC soon. I've been playing with it for a bit now. Here are the issues I am working through.
So far these are the only issues I have found. Thanks again for the hard work. I'll share what I can when as I work through these changes. I am still new and trying to catchup as quick as possible. Any hints are appreciated it. Happy Bitcoining! |
|
I also found that sign message didn't work with other wallets. I have found a solution to all of these. I can do a pull request to this version, if this actually has a chance to be part of btc. I don't have time to rebase though. Thanks! |
|
Is it already implemented? |
|
No, the pull request was not kept up to date. I needs a lot of rebasing to apply to 0.9.x, which Gavin already noted 5 months ago. |
|
I'm going to close this. It has diverged too much from the current code base and no one seems to be interested in rebasing it. In case anyone ever wants to have a shot at implementing multi-wallet I'll refer to to these code changes, as in principle they are good but they just arrived at the wrong time and were not kept in sync long enough to be tested properly and merged. |
|
Noone can re-base this ? Amazing that this did not become the standard |
bitcoind now supports loading more than one wallet at once.
A singleton object of type CWalletManager now exists. It handles dynamic loading/unloading and synchronization of wallets and allows different parts of the application to access wallets by name.
A new CWallet* parameter has been added to the RPC functions. Functions which do not use a wallet simply ignore it. In addition, a new field has been added to CRPCCommand that tells us whether or not the function can be called on a wallet.
Wallet-specific information has been removed from RPC method getinfo. Instead, getinfo just reports how many
wallets are currently loaded. Detailed wallet info is now available via the listwallets method.
Four new RPC methods have been added:
Returns an array containing wallet information.
Allows you to apply existing RPC commands to different wallets.
A default wallet named the empty string is always loaded and is used if calls are made without usewallet.
Example: bitcoind usewallet foo listreceivedbyaddress 0 true
(thanks, gmaxwell, for the idea)
Dynamically loads an existing wallet file wallet-.dat.
If no wallet file exists a new wallet is created. The default wallet file is always called wallet.dat.
To specify additional wallets at startup, use option -usewallet=foo -usewallet=bar etc...as detailed here: #2124 (comment) and CodeShark@9d201cf
TODO: