Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Wallet] Basic multiwallet support #2337

Merged
merged 27 commits into from
May 17, 2021
Merged

[Wallet] Basic multiwallet support #2337

merged 27 commits into from
May 17, 2021

Conversation

random-zebra
Copy link

Quite a bit of refactoring to bring us a little closer to upstream in the wallet/walletdb area, introducing basic support for multiple wallets.
The PIVX client can now be started with more than one -wallet argument (either as startup flags, or as multiple lines in pivx.conf). The wallets will be all loaded and kept separated, with individual balances, keys and received transactions.

Even though only the first wallet will be used in the GUI/RPC for the moment (selectable wallets will be added later), all other loaded wallets will remain synchronized to the node's current tip and update their internal data.

Bulk of changes coming from:

@random-zebra random-zebra added this to the 6.0.0 milestone Apr 23, 2021
@random-zebra random-zebra self-assigned this Apr 23, 2021
This was referenced Apr 27, 2021
Merged
Merged
@random-zebra random-zebra mentioned this pull request May 4, 2021
Merged
@random-zebra
Copy link
Author

Rebased on top of #2362, adding multiwallet auto-backup.

@random-zebra random-zebra force-pushed the 202104_multiwallet branch 2 times, most recently from 40c0418 to db7b27a Compare May 7, 2021 17:05
random-zebra added a commit that referenced this pull request May 11, 2021
@random-zebra
Merge #2362: [Refactoring] Move automatic-wallet-backup code out of i…
6fc3498 
…nit.cpp

4dfc8e0 [MOVE-ONLY] Init: move -resync interaction out of init-wallet (step 5) (random-zebra)
1270ef8 [Refactor] Break up Auto-backup monolithic code in init.cpp (random-zebra)
6ab142b [Refactoring] Move sysperms + enabled wallet check in wallet.cpp (random-zebra)
d8d723b [wallet] Introduce DEFAULT_DISABLE_WALLET (MarcoFalke)
d098e5b init: Get rid of fDisableWallet (random-zebra)

Pull request description:

  Simple refactoring. First two commits are coming from bitcoin#8768.
  Other commits break up a big chunk of code from init.cpp into more manageable functions in wallet/walletdb.

  This will make the implementation of auto-backups for multi-wallets cleaner (will now rebase #2337 on top of this one).

ACKs for top commit:
  furszy:
     All good, code review ACK 4dfc8e0.
  Fuzzbawls:
    ACK 4dfc8e0

Tree-SHA512: d87c8aa7bb5293cc9f15650d4329313a63af8f33f08b82dd8e119a75db3608b4c2535322ccae82b404a68801dba76d858236b9e0c09bc1800f7f7cc0e8f21383
@random-zebra
Copy link
Author

Rebased on master. Ready to go

@furszy
Copy link

furszy commented May 11, 2021

Since this needs another rebase due conflicts with #2275, would be good to rebase it on top of #2369, there shouldn't be any conflict, and we will get here a cleaner workflow and less deduplicated code for the auto-backup feature.

@random-zebra
Copy link
Author

random-zebra commented May 12, 2021 •
edited
Loading

Since this needs another rebase due conflicts with #2275, would be good to rebase it on top of #2369, there shouldn't be any conflict, and we will get here a cleaner workflow and less deduplicated code for the auto-backup feature.

I've rebased on master (there were only two trivial conflicts, one in the release notes).

I'd rather do the other way around, as #2369 needs to be fixed, and it is a marginal refactoring, which can be done at any time (the intention of #2362 was just to "move auto-backup out of the way" for now). Plus there are already #2341, #2351 and #2347 built on top of this one.

Finally, as commented on #2369, there is room for a cleaner refactoring/deduplication after #2351.

Btw... don't be discouraged by the number of commits / line changes in this chain of PRs: there are a lot of file touched, but the changes are pretty simple and repetitive 🙂

@random-zebra random-zebra mentioned this pull request May 12, 2021
Merged
Fuzzbawls
Fuzzbawls previously approved these changes May 15, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great! There are some user-facing strings that can be translated or have improved translations, but I've found instances of other such strings in unrelated areas of the code, so am putting together a dedicated PR based on top of this to specifically address the issue.

ACK ba1c581

@Fuzzbawls Fuzzbawls mentioned this pull request May 15, 2021
Merged
1 task
@random-zebra
Copy link
Author

Rebased due to conflicts with #2336 just merged

Fuzzbawls
Fuzzbawls previously approved these changes May 16, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 1018138 after rebase

random-zebra and others added 19 commits May 17, 2021 13:25
@random-zebra
RPC: Do all wallet access through new GetWalletForJSONRPCRequest
325baaa 
>>> inspired by bitcoin/bitcoin@d77ad6d

currently pwalletMain local variables shadow the global.
These will be renamed to pwallet in the next commit (kept separate for
ease of review)
@random-zebra
[Trivial] Rename pwalletMain --> pwallet for local variables in RPC
22f8507
@luke-jr @random-zebra
Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task
cc965fe 
unique per CWallet
@luke-jr @random-zebra
RPC: Pass on JSONRPCRequest metadata (URI/user/etc) for "help" method
687c2fd
@random-zebra
[MOVE-ONLY] Move wallet RPC declarations to rpcwallet.h
ca6a62d 
based on bitcoin/bitcoin@a435632
@luke-jr @random-zebra
Wallet/RPC: Use filename rather than CWallet pointer, for lockwallet
3bfa7d8 
RPCRunLater job name

The job name is logged, and could pose as an information leak to someone
attacking the process, helping them counteract ASLR protections
@luke-jr @random-zebra
Wallet: Sanitise -wallet parameter
100d67c
@random-zebra
[Wallet] Replace pwalletMain with a vector of wallet pointers
4bbad5c 
>>> adapts bitcoin/bitcoin@b124cf0
@random-zebra
[Refactor] Remove CWalletDBWrapper::GetUpdateCounter()
d6cf608
@random-zebra
[Tests] move pwalletMain to wallet test fixture + use smart pointer
d10acd5 
>>> inspired by bitcoin/bitcoin@49bd659
              + bitcoin/bitcoin@f94c2ec
@luke-jr @random-zebra
Wallet: Support loading multiple wallets if -wallet used more than once
b27dcfe
@luke-jr @random-zebra
Wallet: Move multiwallet sanity checks to CWallet::Verify, and do other
647fbc9 
checks on all wallets
@luke-jr @random-zebra
wallet: Include actual backup filename in recovery warning message
e6efa6b
@luke-jr @random-zebra
wallet: Base backup filenames on original wallet filename
60f9b4b
@random-zebra
wallet: Forbid -salvagewallet, -zapwallettxes, and -upgradewallet with
b6dbbf3 
multiple wallets

>>> backports bitcoin/bitcoin@9cbe8c8
@random-zebra
[Cleanup] Fix formatting in wallet and walletdb
36796f2
@random-zebra
[Doc] Add multiwallet section to release notes
dc596f3
@random-zebra
[Trivial] Add wallet filename to backup errors/warning
ff4cee0
@random-zebra
[Cleanup][Tests] Fix chainparams-change in librust tests
4734a84
@random-zebra
Copy link
Author

Code review ACK 1018138.
Needs a final rebase.

Done. Let's get this PR merged, so we can keep going, and rebase all the stuff that depends on it (#2341/#2347, #2351/#2369, #2388).

furszy
furszy approved these changes May 17, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code ACK 4734a84 after rebase and inclusion of 4734a84.

@random-zebra random-zebra merged commit 096a0f9 into PIVX-Project:master May 17, 2021
furszy added a commit that referenced this pull request May 21, 2021
@furszy
Merge #2388: [GUI] Ensure that all UI error/warning texts pass throug…
1e40e2b 
…h translation

6528ba3 [GUI] Ensure that all UI error/warning texts pass through translation (Fuzzbawls)

Pull request description:

  This adds translation where it was previously missing for some `UIError`
  and `UIWarning` calls. in the case that the string is a pointer, the
  base string has had translation added.

  Also parameterized some translation strings to filter out string
  literals that we DON'T want to be translated (eg '`-wallet`',
  '`-paytxfee`', etc)

  - [x] #2337

ACKs for top commit:
  random-zebra:
    utACK 6528ba3
  furszy:
    utACK 6528ba3 and merging..

Tree-SHA512: 2afe3435de794900555b413b40edab7b2fb8980a39edebcdc583e16e76a82af793ec4667fda41ad2dab421716045542938dd6f8fb38d8f1ee11d90a2ee9f2d2f
random-zebra added a commit that referenced this pull request Jun 9, 2021
@random-zebra
Merge #2351: [Wallet] Laggard wallet-related backports from btc 0.15
2d50b6e 
d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider)
3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider)
dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli)
20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky)
e411b70 [wallet] Fix leak in CDB constructor (João Barbosa)
f15aeea Change getmininginfo errors field to warnings (Andrew Chow)
c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra)
1d966ce Add warnings field to getblockchaininfo (Andrew Chow)
ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra)
e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra)
e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra)
2188c3e Move some static functions out of wallet.h/cpp (random-zebra)
f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra)
8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra)
900bbfa Reject invalid wallet files (João Barbosa)
a1f4e2a Reject duplicate wallet filenames (random-zebra)
ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky)
ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra)
37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra)
3955ee9 [Doc] Update release notes (random-zebra)
4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery)
1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery)
fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery)
cf4a90b Remove factor of 3 from definition of dust. (random-zebra)
a1c56fd [Policy] Introduce -dustrelayfee (random-zebra)
9fb29cc [Doc] Update multiwallet release notes (random-zebra)
379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra)
808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra)
f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery)
2e02006 Rename -usewallet to -rpcwallet (Alex Morcos)
a64440b Select wallet based on the given endpoint (Jonas Schnelli)
5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra)
b0fe92f Fix test_pivx circular dependency issue (random-zebra)
6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli)
7dd3916 Register wallet endpoint (Jonas Schnelli)
5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos)
41a7335 Remove unused variables (practicalswift)
5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky)
e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra)
54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra)
494ba64 [test] Add test for getmemoryinfo (random-zebra)
2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)
7d977ac Remove unused C++ code not covered by unit tests (random-zebra)
60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar)
3633d75 Initialize nRelockTime (Patrick Strateman)
3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra)
f219be9 Add safe flag to listunspent result (NicolasDorier)
0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra)
75c8c6d Disallow copy of CReserveKeys (Gregory Sanders)
8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra)

Pull request description:

  I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us.
  I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files).

  This changeset is based on top of
  - [x] #2337

  as it keeps going with the multiwallet implementation, adding:
  - RPC endpoint support
  - `listwallets` RPC
  - wallet name in `getwalletinfo` RPC
  - `wallet_multiwallet.py` functional test

  As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code.

  List of upstream PRs backported/adapted:

  - bitcoin#9906  : Disallow copy constructor CReserveKeys
  - bitcoin#9830  : Add safe flag to listunspent result
  - bitcoin#9993  : Initialize nRelockTime
  - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value
  - bitcoin#10075 : Remove unused C++ code not covered by unit tests
  - bitcoin#10257 : Add test for getmemoryinfo
  - bitcoin#10295 : Move some WalletModel functions into CWallet
  - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings
  - bitcoin#10522 : Remove unused variables
  - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet
  - bitcoin#10849 : Multiwallet: simplest endpoint support
  - bitcoin#9380  : Separate different uses of minimum fees (only dustRelayFee)
  - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit)
  - bitcoin#10883 : Rename -usewallet to -rpcwallet
  - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests
  - bitcoin#10893 : Avoid running multiwallet.py twice
  - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets
  - bitcoin#10931 : Fix misleading "method not found" multiwallet errors
  - bitcoin#10885 : Reject invalid wallets
  - bitcoin#11022 : Basic keypool topup
  - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp
  - bitcoin#11310 : Test listwallets RPC
  - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs
  - bitcoin#11492 : Fix leak in CDB constructor
  - bitcoin#11476 : Avoid opening copied wallet databases simultaneously
  - bitcoin#11590 : always show help-line of wallet encryption calls
  - bitcoin#11289 : Add wallet backup text to import* and add* RPCs

ACKs for top commit:
  furszy:
    re-re-utACK d5526bd.
  Fuzzbawls:
    ACK d5526bd

Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2
furszy added a commit that referenced this pull request Jun 13, 2021
@furszy
Merge #2341: [Wallet][RPC] Add subtract-fee-from-amount option in Cre…
e8d2cf0 
…ate[Fund]Transaction

a1bd590 [Doc] Document subtract-fee-from-amount RPC changes in the release notes (random-zebra)
6589429 wallet: first step generalizing CRecipient and ShieldedRecipient usage creating CRecipientBase class. (furszy)
1239506 [QA] Properly test change position in fundraw with sffa (random-zebra)
cc9ff09 [RPC] Add fSubtractFeeFromAmount to shieldsendmany (random-zebra)
65d3a80 [Refactoring] Subtract fee from destination when building shield tx (random-zebra)
cba898b [Refactoring] SendManyRecipient: use CRecipient for taddr + add sffa (random-zebra)
6fff9f5 [RPC] Add fSubtractFeeFromAmount to sendmany (transparent) (random-zebra)
7e30e3f [MOVE-ONLY] Move fundrawtransaction to rpcwallet (random-zebra)
499d62a [Tests] Add functional test for sffa in fundrawtransaction (random-zebra)
523b24d Subtract fee from amount (random-zebra)

Pull request description:

  Add a feature requested many times (#894, #1079, #2196, ...)

  Allow the user to deduct the fee from one or more of the transaction recipient amounts.
  The most common use-case is when sending the whole balance, or a selection of UTXOs, without getting any change back.
  This is already possible by selecting "after-fee-amount" in coin-control, but such metod is not user-friendly (e.g. in case of shield recipients, to get the correct value, the destinations must already be filled before opening coin-control and copying the "after-fee" value).
  Plus, this workaround is only available in the GUI.

  The subtract-fee option makes it easier, and enables it via the cli as well.

  Here it's exposed only to the RPC, both for transparent and shield recipients:
  - `sendtoaddress`
  - `sendmany`
  - `shieldsendmany`
  - `fundrawtransaction`

  GUI connection will be added in a successive PR (which will close those issues).

  Last commit is coming from bitcoin#10294

  Built on top of:
  - [x] #2337

ACKs for top commit:
  furszy:
    rebase + cherry-pick utACK a1bd590.
  Fuzzbawls:
    ACK a1bd590

Tree-SHA512: 7ed78f6ab8f1e9300d1468242e66016a4e1867f413256de23b7a60e103c2b628cdadcfc7998e8a7b252a7c2817d45261be108fad0eaa34d5ff892052c09b6e60
@furszy furszy mentioned this pull request Jun 15, 2021
Merged
furszy added a commit that referenced this pull request Jul 23, 2021
@furszy
Merge #2423: [Wallet] Introduce wallets directory configuration and e…
b04e1f5 
…xternal wallet files capabilities

056f4e8 [GUI] settings information widget setting the correct data directory. (furszy)
f765611 bugfix: Remove dangling wallet env instance (João Barbosa)
524103f Implement and connect CWallet::GetPathToFile to GUI. (furszy)
e7aa6bd [Refactor] First load all wallets, then back em (random-zebra)
91b112b [Refactor][Bug] Fix automatic backups, final code deduplication (random-zebra)
12a1e39 [BUG] Sanitize wallet name in GetUniqueWalletBackupName (random-zebra)
7aa251d wallet: Fix backupwallet for multiwallets (Daniel Kraft)
351d2c8 wallet_tests: mock wallet db. (furszy)
565abcd db: fix db path not removed from the open db environments map. (furszy)
4cae8dc test: Add unit test for LockDirectory  Add a unit test for LockDirectory, introduced in btc#11281. (W. J. van der Laan)
16b4651 util: Fix multiple use of LockDirectory  This commit fixes problems with calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD. (W. J. van der Laan)
9ae619a Test: Use specific testing setups for wallet_zkeys_tests tests (furszy)
d86cd4f wallet: Add missing check for backup to source wallet file. (furszy)
d9e1c6b Abstract VerifyWalletPath and connect it to init and GUI. (furszy)
23458ca GUI: Implement and connect WalletModel::getWalletPath(). (furszy)
c2d3a07 Create new wallet databases as directories rather than files (Russell Yanofsky)
daa7fe5 Allow wallet files not in -walletdir directory (Russell Yanofsky)
9b2dae1 Allow wallet files in multiple directories (furszy)
d36185a wallet: unify backup creation process. (furszy)
8b8725d wallet_tests: Use dummy wallet instance instead of wallet pointer. (furszy)
434ed75 Abstract LockDirectory into system.cpp (furszy)
6a0380a Make .walletlock distinct from .lock (MeshCollider)
d8539bb Generalise walletdir lock error message for correctness (MeshCollider)
ddcfd4a Enable test for wallet directory locking (furszy)
a238a8d Add a lock to the wallet directory (MeshCollider)
1dc2219 Don't allow relative -walletdir paths (Russell Yanofsky)
dcb43ea Create walletdir if datadir doesn't exist and correct tests (furszy)
03db5c8 Default walletdir is wallets/ if it exists (MeshCollider)
359b01d Add release notes for -walletdir and wallets/ dir (MeshCollider)
71a4701 Add -walletdir parameter to specify custom wallet dir (furszy)
5b31813 Use unique_ptr for dbenv (DbEnv) (practicalswift)
a1bef4f Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv)

Pull request description:

  > Adding more flexibility in where the wallets directory can be located. Before, the wallet database files were stored at the top level of the PIVX data directory. Now the location of the wallets directory can be overridden by specifying a `-walletdir=<path>` option where `<path>` can be an absolute path to a directory or directory symlink.
  >
  >Another advantage of this change is that if two wallets are located in the same directory, they will now use their own BerkeleyDB environments instead using a shared environment. Using a shared environment makes it difficult to manage and back up wallets separately because transaction log files will contain a mix of data from all wallets in the environment.

  Coming from:
  * bitcoin#11351 -> Refactor: Modernize disallowed copy constructors/assignment.
  * bitcoin#11466 -> Specify custom wallet directory with -walletdir param.
  * bitcoin#11726 -> Cleanups + nit fixes for -walletdir work.
  * bitcoin#11904 -> Add lock to the wallet directory.
  * bitcoin#11687 -> External wallet files support.
  * bitcoin#12166 -> Doc better -walletdir description.
  * bitcoin#12220 -> Error if relative -walletdir is specified.

  This finishes the work started in #2337, enabling all the commented tests inside `wallet_multiwallet.py` and more.

  PR built on top of #2369.

  Note: Test this properly.

ACKs for top commit:
  random-zebra:
    utACK 056f4e8
  Fuzzbawls:
    ACK 056f4e8

Tree-SHA512: 98ae515dd664f959d35b63a0998bd93ca3bcea30ca67caccd2a694a440d10e18f831a54720ede0415d8f5e13af252bc6048a820491863d243df70ccc9d5fa7c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy approved these changes

@Fuzzbawls Fuzzbawls Fuzzbawls left review comments

Assignees

@random-zebra random-zebra

Labels
RPC Upstream Wallet
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@random-zebra @furszy @Fuzzbawls @luke-jr