Added CWalletManager class to support loading of multiple wallets. #2407

pull CodeShark wants to merge 2 commits into bitcoin:master from CodeShark:multiwallet-new changing 9 files +385 −84

CodeShark commented at 2:50 AM on March 24, 2013: contributor

This pull request is a minimal step in the direction of merging #2124.

The idea is to merge things incrementally rather than all at once.
BitcoinPullTester commented at 3:28 AM on March 25, 2013: none
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/74c341604961ea9611a6c024c0bdecb573f2361a for binaries and test log.

This could happen for one of several reasons:
1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.
BitcoinPullTester commented at 11:58 AM on March 30, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f48ace32517880418420f45e51b9a97caaa33ce8 for binaries and test log. This is an automated test script which runs test cases on each commit every time is updated. It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ and contact BlueMatt on freenode if something looks broken.
in src/walletmanager.cpp:None in f48ace3251 outdated
```
  76 | +    {
  77 | +        strErrors << _("Wallet name may only contain letters, numbers, and underscores.");
  78 | +        return false;
  79 | +    }
  80 | +    
  81 | +    ENTER_CRITICAL_SECTION(cs_WalletManager);
```
sipa commented at 4:27 PM on April 6, 2013:

I think you can better abstract the part between the ENTER_CRITICAL_SECTION and the LEAVE_CRITICAL_SECTION into a separate function that has a LOCK(cs_WalletManager) at the start. This also means you don't need to explicitly leave the section prematurely in case of errors.

CodeShark commented at 5:23 AM on April 12, 2013:

Yes, I realized this after I manually parsed the macros in sync.h. But this code is just copy/pasted from the original multiwallet pull request. I'll change it. Thanks for pointing it out.
CodeShark commented at 1:24 AM on July 1, 2013: contributor

This pull request has been refactored and reworked a little to improve error messaging.
Added CWalletManager class to support loading of multiple wallets. d726e56aa0
sipa commented at 1:02 PM on July 7, 2013: member

If you add a dependency on a boost library, please add it to the gitian descriptors too (though it seems overkill to me ofr this purpose).
Added gitian descriptor. 19f569cdd3
in src/walletmanager.cpp:None in 19f569cdd3
```
  19 | +
  20 | +vector<string> GetFilesAtPath(const boost::filesystem::path& _path, unsigned int flags)
  21 | +{
  22 | +    vector<string> vstrFiles;
  23 | +    if (!boost::filesystem::exists(_path))
  24 | +        throw runtime_error("Path does not exist.");
```
sipa commented at 10:15 PM on July 9, 2013:

A runtime error for this sounds very severe.

CodeShark commented at 9:03 AM on July 26, 2013:

It's not that severe if we allow wallets to be stored at arbitrary locations on the file system as in multiwallet-qt. It would be quite severe if _path happened to be the datadir :)

in src/walletmanager.cpp:None in 19f569cdd3

  41 | +            if (((flags & file_option_flags::REGULAR_FILES) && boost::filesystem::is_regular_file(pFile)) ||
  42 | +                ((flags & file_option_flags::DIRECTORIES) && boost::filesystem::is_directory(pFile)))
  43 | +#if defined (BOOST_FILESYSTEM_VERSION) && BOOST_FILESYSTEM_VERSION == 3
  44 | +                vstrFiles.push_back(pFile.filename().string());
  45 | +#else
  46 | +            vstrFiles.push_back(pFile.filename());

sipa commented at 10:15 PM on July 9, 2013:

Indentation problem.

CodeShark commented at 9:06 AM on July 26, 2013:

Also, should probably be BOOST_FILESYSTEM_VERSION >= 3. I hope they don't decide to break this again in version 4.

in src/walletmanager.h:None in 19f569cdd3

  16 | + */
  17 | +typedef std::map<std::string, boost::shared_ptr<CWallet> > wallet_map;
  18 | +class CWalletManager
  19 | +{
  20 | +protected:
  21 | +    static const boost::regex WALLET_NAME_REGEX;

sipa commented at 10:16 PM on July 9, 2013:

Do these need to be exposed?

CodeShark commented at 9:00 AM on July 26, 2013:

No, they don't. I'll move them into cpp file when I get a chance.

BitcoinPullTester commented at 4:31 PM on July 10, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/19f569cdd364592d91453fd246b11e44cef3940e 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.
kyledrake commented at 4:42 PM on August 3, 2013: none

Not sure if this is the appropriate place for this comment, but one feature request I wanted to recommend was to allow for the dynamic creation of wallets via RPC. That way you could add new ones without having to restart the daemon. I think people writing wallet interfaces might find that handy.
kyledrake commented at 4:43 PM on August 3, 2013: none

Also, this is great work, happy that you're tackling this hard problem. Thanks!
CodeShark commented at 12:51 PM on August 8, 2013: contributor

@Kyle r.e. dynamic creation of wallets via RPC: that was one of the main objectives of #2124 I think I've found a better approach to wallet management which I've been pursuing separate from these pull requests. I'll publish something on it soon.
gavinandresen commented at 1:30 AM on October 21, 2013: contributor

Needs rebase and response to code review comments.
laanwj commented at 12:44 PM on April 23, 2014: member

Like #2124, this pull request has fallen behind. Needs a lot of rebasing work. Please open a new pull or let me know in case anyone wants to pick this up.
laanwj closed this on Apr 23, 2014
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#2124 Dynamically Loadable Multiple Wallet Support Complete!!!