RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest by luke-jr · Pull Request #8775 · bitcoin/bitcoin

luke-jr · 2016-09-21T09:56:50Z

Part of the refactorings needed for basic multiwallet (#8694)

jonasschnelli · 2016-09-21T13:07:21Z

I don't like the coupling and the #ifdef ENABLE_WALLET in rpc/server.cpp|.h.
I'd recommend to keep the CRPCRequestInfo wallet-free.

I think we should pack the request path (URI) into the CRPCRequestInfo and or informations about the authentication (in case we want to distinct wallets based on authentication).
Then I think there should be a method in wallet.cpp (or in rpcwallet.cpp) that maps a CWallet * pointer from a given URI, Auth-Info or the complete CRPCRequestInfo instance.

Instead of the CWallet *& pwallet = reqinfo.wallet; there could be then something like CWallet *pwallet = CWallets::getWalletFromRequest(reqinfo)

jonasschnelli · 2016-09-21T13:09:42Z

General ConceptACK on a CRPCRequestInfo for the RPC table commands.
Maybe it could also include the UniValue params and fHelp?

paveljanik · 2016-09-21T16:17:01Z

src/rpc/misc.cpp

Yes, pwallet is an alias to reqinfo.wallet which is of type CWallet*.

luke-jr · 2016-09-21T20:40:26Z

I think we should pack the request path (URI) into the CRPCRequestInfo and or informations about the authentication (in case we want to distinct wallets based on authentication).
Then I think there should be a method in wallet.cpp (or in rpcwallet.cpp) that maps a CWallet * pointer from a given URI, Auth-Info or the complete CRPCRequestInfo instance.

That sounds nice, but greatly complicates the implementation. :(

maflcko · 2016-10-19T15:16:41Z

I think this can be closed after #8788?

jonasschnelli · 2016-10-19T15:28:54Z

Closing in favor of merged #8788

Wrapped in CRPCRequestInfo to avoid gratuitous #ifdef ENABLE_WALLET everywhere Github-Pull: bitcoin#8775 Rebased-From: 80f4ab7b5f404d20bed41ec70182e6fc0990205b

Github-Pull: bitcoin#8775 Rebased-From: 9279b515815e58df5bd796b0570e388ff42fb84b

…nique per CWallet Github-Pull: bitcoin#8775 Rebased-From: 041f4e06a664b7e31ba77bbfbb2f7bc8d04174ca

luke-jr · 2016-10-25T09:14:49Z

Rebased and refactored based on @jonasschnelli 's idea.

laanwj · 2016-10-25T11:18:26Z

Makes sense, utACK ab5ce98

laanwj · 2016-10-25T11:19:35Z

src/rpc/misc.cpp


 using namespace std;

+CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest&);


Not sure how this can pass the build w/ --disable-wallet, but this should be bracketed with #ifdef ENABLE_WALLET

luke-jr · 2016-11-12T01:44:44Z

Rebased and addressed nit

Github-Pull: bitcoin#8775 Similar-To: 7de55733c5690350c48ec94660b5be56fbb5eb39

instagibbs · 2017-01-04T18:25:55Z

utACK 7de55733c5690350c48ec94660b5be56fbb5eb39

Github-Pull: bitcoin#8775 Rebased-From: d77ad6d

…nique per CWallet Github-Pull: bitcoin#8775 Rebased-From: 2e518e3

Github-Pull: bitcoin#8775 Rebased-From: bf8a04a

Github-Pull: bitcoin#8775 Rebased-From: ad15734

Github-Pull: bitcoin#8775 Rebased-From: a435632

…equest where appropriate Github-Pull: bitcoin#8775 Rebased-From: 86be48a

…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 Github-Pull: bitcoin#8775 Rebased-From: 9756be3

Github-Pull: bitcoin#8775 Rebased-From: d678771

jtimon · 2017-03-23T20:31:22Z

By the way, I reviewed this partially after merged when rebasing #9845 (making it mostly pointless since most was done here already).

adneerav · 2019-02-05T07:39:27Z

Hello @luke-jr ,

Is there any way where we can increase the number of wallets to be in loaded state at same time.As rightnow its like if I load wallet more than 100 or 150 daemon is getting crashed/stopped. I am trying to load/unload wallet dynamically using RPC.

What i tried is incraesing the set_lk_max_lockers to 400000 and also added
set_lg_regionmax 1048576 But still same things I am facing.

Reflink for above setup : https://docs.oracle.com/cd/E17276_01/html/api_reference/C/set_lk_max_lockers_parameter.html
https://bitcoin.stackexchange.com/a/70737

So is there any way or configuration or any changes which can increase and allow more number of wallets to be loaded ?

maflcko added RPC/REST/ZMQ Wallet labels Sep 21, 2016

paveljanik reviewed Sep 21, 2016

View reviewed changes

jonasschnelli mentioned this pull request Sep 22, 2016

[RPC] Give RPC commands more information about the RPC request #8788

Merged

luke-jr force-pushed the multiwallet_prefactor_rpc branch 2 times, most recently from 7697a0e to df2c0c5 Compare October 18, 2016 12:20

jonasschnelli closed this Oct 19, 2016

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

RPC/Wallet: Access wallet via CRPCRequestInfo only

cb370e3

Github-Pull: bitcoin#8775 Rebased-From: 9279b515815e58df5bd796b0570e388ff42fb84b

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task u…

06e4c90

…nique per CWallet Github-Pull: bitcoin#8775 Rebased-From: 041f4e06a664b7e31ba77bbfbb2f7bc8d04174ca

laanwj reopened this Oct 25, 2016

luke-jr force-pushed the multiwallet_prefactor_rpc branch 2 times, most recently from 06bd5d0 to ab5ce98 Compare October 25, 2016 08:50

laanwj reviewed Oct 25, 2016

View reviewed changes

luke-jr force-pushed the multiwallet_prefactor_rpc branch 2 times, most recently from 5cce71a to d6bc295 Compare November 12, 2016 01:44

luke-jr changed the title ~~RPC refactoring: Never access wallet directly, only via new CRPCRequestInfo~~ RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest Nov 12, 2016

luke-jr force-pushed the multiwallet_prefactor_rpc branch from d6bc295 to 7de5573 Compare December 29, 2016 13:20

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 31, 2016

RPC: Pass on CRPCRequestInfo metadata (wallet) for "help" method

41383de

Github-Pull: bitcoin#8775 Similar-To: 7de55733c5690350c48ec94660b5be56fbb5eb39

luke-jr mentioned this pull request Jan 3, 2017

Basic multiwallet support #8694

Merged

luke-jr force-pushed the multiwallet_prefactor_rpc branch from 7de5573 to 7c65786 Compare January 5, 2017 22:52

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

RPC: Do all wallet access through new GetWalletForJSONRPCRequest

e6aa16c

Github-Pull: bitcoin#8775 Rebased-From: d77ad6d

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task u…

b7adec2

…nique per CWallet Github-Pull: bitcoin#8775 Rebased-From: 2e518e3

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

Reformat touched lines with C++11

212a0f5

Github-Pull: bitcoin#8775 Rebased-From: bf8a04a

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

RPC: Pass on JSONRPCRequest metadata (URI/user/etc) for "help" method

9283eaf

Github-Pull: bitcoin#8775 Rebased-From: ad15734

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

Move wallet RPC declarations to rpcwallet.h

2115207

Github-Pull: bitcoin#8775 Rebased-From: a435632

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

More tightly couple EnsureWalletIsAvailable with GetWalletForJSONRPCR…

7a5499c

…equest where appropriate Github-Pull: bitcoin#8775 Rebased-From: 86be48a

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 7, 2017

Wallet: Sanitise -wallet parameter

6689b85

Github-Pull: bitcoin#8775 Rebased-From: d678771

ryanofsky mentioned this pull request Jun 15, 2017

[wallet] [tests] Add listwallets RPC, include wallet name in getwalletinfo and add multiwallet test #10604

Merged

PastaPastaPasta mentioned this pull request Nov 13, 2018

[WIP] Backport multiwallet functionality dashpay/dash#2439

Closed

PastaPastaPasta mentioned this pull request Jan 25, 2019

Backports 0.15 pr4 dashpay/dash#2646

Merged

random-zebra mentioned this pull request Apr 23, 2021

[Wallet] Basic multiwallet support PIVX-Project/PIVX#2337

Merged


		using namespace std;

		CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest&);

Conversation

luke-jr commented Sep 21, 2016

Uh oh!

jonasschnelli commented Sep 21, 2016

Uh oh!

jonasschnelli commented Sep 21, 2016

Uh oh!

paveljanik Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Sep 21, 2016

Uh oh!

maflcko commented Oct 19, 2016

Uh oh!

jonasschnelli commented Oct 19, 2016

Uh oh!

luke-jr commented Oct 25, 2016

Uh oh!

laanwj commented Oct 25, 2016

Uh oh!

laanwj Oct 25, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Nov 12, 2016

Uh oh!

instagibbs commented Jan 4, 2017

Uh oh!

jtimon commented Mar 23, 2017

Uh oh!

adneerav commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants