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.

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

RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest #8775

Merged
merged 9 commits into from Mar 3, 2017

Conversation

@luke-jr
Member

luke-jr commented Sep 21, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 21, 2016

Member

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)

Member

jonasschnelli commented Sep 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 21, 2016

Member

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

Member

jonasschnelli commented Sep 21, 2016

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 21, 2016

Member

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. :(

Member

luke-jr commented Sep 21, 2016

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. :(

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 19, 2016

Member

I think this can be closed after #8788?

Member

MarcoFalke commented Oct 19, 2016

I think this can be closed after #8788?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 19, 2016

Member

Closing in favor of merged #8788

Member

jonasschnelli commented Oct 19, 2016

Closing in favor of merged #8788

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

RPC: Allow function signature to include CWallet reference
Wrapped in CRPCRequestInfo to avoid gratuitous #ifdef ENABLE_WALLET everywhere

Github-Pull: #8775
Rebased-From: 80f4ab7

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

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

@laanwj laanwj reopened this Oct 25, 2016

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 25, 2016

Member

Rebased and refactored based on @jonasschnelli 's idea.

Member

luke-jr commented Oct 25, 2016

Rebased and refactored based on @jonasschnelli 's idea.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 25, 2016

Member

Makes sense, utACK ab5ce98

Member

laanwj commented Oct 25, 2016

Makes sense, utACK ab5ce98

Show outdated Hide outdated src/rpc/misc.cpp
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 12, 2016

Member

Rebased and addressed nit

Member

luke-jr commented Nov 12, 2016

Rebased and addressed nit

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

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

@luke-jr luke-jr referenced this pull request Jan 3, 2017

Merged

Basic multiwallet support #8694

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs
Member

instagibbs commented Jan 4, 2017

utACK 7de5573

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 5, 2017

Member

Minor change: Forward-declared CWallet even for non-wallet builds so it can be used in a pointer type, avoiding unnecessary casting.

Member

luke-jr commented Jan 5, 2017

Minor change: Forward-declared CWallet even for non-wallet builds so it can be used in a pointer type, avoiding unnecessary casting.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
Show outdated Hide outdated src/rpc/misc.cpp
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 7, 2017

Member

So in some cases in the RPC you get the wallet pointer from json but then the check if it's available is far below. This is begging for someone to insert code that uses a potentially null pointer between to two and doesn't notice because their function doesn't get tested with a missing wallet. I would recommend moving the creation of that local pointer down to the point where you're going to test it.

Alternatively or in addition, rename GetWalletForJSONRPCRequest to TryGetWalletForJSONRPCRequest and make GetWalletForJSONRPCRequest a wrapper for it that throws when it fails?

Other than this nit that perhaps getting the pointer and testing it are too separated in some cases, utACK.

Member

gmaxwell commented Jan 7, 2017

So in some cases in the RPC you get the wallet pointer from json but then the check if it's available is far below. This is begging for someone to insert code that uses a potentially null pointer between to two and doesn't notice because their function doesn't get tested with a missing wallet. I would recommend moving the creation of that local pointer down to the point where you're going to test it.

Alternatively or in addition, rename GetWalletForJSONRPCRequest to TryGetWalletForJSONRPCRequest and make GetWalletForJSONRPCRequest a wrapper for it that throws when it fails?

Other than this nit that perhaps getting the pointer and testing it are too separated in some cases, utACK.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 7, 2017

Member

I liked the TryGetWalletForJSONRPCRequest/GetWalletForJSONRPCRequest refactor idea, but I don't see any clean way to do it without changing the help behaviours.

So I moved the one case EnsureWalletIsAvailable was delayed up, and removed the blank line between them and GetWalletForJSONRPCRequest.

Member

luke-jr commented Jan 7, 2017

I liked the TryGetWalletForJSONRPCRequest/GetWalletForJSONRPCRequest refactor idea, but I don't see any clean way to do it without changing the help behaviours.

So I moved the one case EnsureWalletIsAvailable was delayed up, and removed the blank line between them and GetWalletForJSONRPCRequest.

@TheBlueMatt

Please do not tag the 4th commit "Trivial". We usually define trivial as doesnt touch any code.

As for the 6th commit: please do not do this. nothing in src/wallet is built when ENABLE_WALLET is off, so generally src/wallet/* should never be included outside of src/wallet/*

Aside from the (partial-revert) of "Move wallet RPC declarations to rpcwallet.h" and the printing of the raw pointers to debug log, this looks good to me at d9aff6e.

void ImportAddress(const CBitcoinAddress& address, const string& strLabel);
void ImportScript(const CScript& script, const string& strLabel, bool isRedeemScript)
void ImportAddress(CWallet*, const CBitcoinAddress& address, const string& strLabel);
void ImportScript(CWallet * const pwallet, const CScript& script, const string& strLabel, bool isRedeemScript)

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

This comment has been minimized.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

I'm not seeing function signatures changes in c68b5a8 RPC: Do all wallet access through new GetWalletForJSONRPCRequest so the two commits do seem distinct currently.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

I'm not seeing function signatures changes in c68b5a8 RPC: Do all wallet access through new GetWalletForJSONRPCRequest so the two commits do seem distinct currently.

@@ -1126,7 +1129,7 @@ struct tallyitem
}
};
UniValue ListReceived(const UniValue& params, bool fByAccounts)
UniValue ListReceived(CWallet * const pwallet, const UniValue& params, bool fByAccounts)

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ListReceived after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

@TheBlueMatt

TheBlueMatt Jan 7, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ListReceived after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

Show outdated Hide outdated src/wallet/rpcwallet.cpp
Show outdated Hide outdated src/wallet/rpcwallet.cpp
Show outdated Hide outdated src/wallet/rpcwallet.cpp
Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -112,13 +112,17 @@ UniValue getinfo(const JSONRPCRequest& request)
class DescribeAddressVisitor : public boost::static_visitor<UniValue>
{
public:
CWallet * const pwallet;

This comment has been minimized.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Seems like this could be a pointer to a const CWallet. Same for some other cases. Would suggest using const CWallet* instead of CWallet* where possible for more safety and clarity.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Seems like this could be a pointer to a const CWallet. Same for some other cases. Would suggest using const CWallet* instead of CWallet* where possible for more safety and clarity.

void ImportAddress(const CBitcoinAddress& address, const string& strLabel);
void ImportScript(const CScript& script, const string& strLabel, bool isRedeemScript)
void ImportAddress(CWallet*, const CBitcoinAddress& address, const string& strLabel);
void ImportScript(CWallet * const pwallet, const CScript& script, const string& strLabel, bool isRedeemScript)

This comment has been minimized.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

I'm not seeing function signatures changes in c68b5a8 RPC: Do all wallet access through new GetWalletForJSONRPCRequest so the two commits do seem distinct currently.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note that there are a ton of uses of pwalletMain in ImportScript after the first commit ("RPC/Wallet: Pass CWallet as pointer to helper functions") which are fixed in the next ("RPC: Do all wallet access through new GetWalletForJSONRPCRequest") when they belong in the first.

I'm not seeing function signatures changes in c68b5a8 RPC: Do all wallet access through new GetWalletForJSONRPCRequest so the two commits do seem distinct currently.

Show outdated Hide outdated src/rpc/server.h
if (pwalletMain) {
obj.push_back(Pair("walletversion", pwalletMain->GetVersion()));
obj.push_back(Pair("balance", ValueFromAmount(pwalletMain->GetBalance())));
if (pwallet) {

This comment has been minimized.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note to reviewers: This commit (d77ad6d RPC: Do all wallet access through new GetWalletForJSONRPCRequest) is trivial to review if you configure your diff tool to ignore the pwallet->pwalletmain renames (much shorter and no changed lines, only inserted).

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

Note to reviewers: This commit (d77ad6d RPC: Do all wallet access through new GetWalletForJSONRPCRequest) is trivial to review if you configure your diff tool to ignore the pwallet->pwalletmain renames (much shorter and no changed lines, only inserted).

Show outdated Hide outdated src/wallet/rpcwallet.cpp
Show outdated Hide outdated src/wallet/rpcwallet.cpp
Show outdated Hide outdated src/rpc/misc.cpp
@@ -234,6 +235,9 @@ UniValue validateaddress(const JSONRPCRequest& request)
return ret;
}
// Needed even with !ENABLE_WALLET, to pass (ignored) pointers around
class CWallet;

This comment has been minimized.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

This doesn't actually seem to be needed since CWallet is also forward declared in init.h, but maybe it is better to keep it.

@ryanofsky

ryanofsky Feb 27, 2017

Contributor

This doesn't actually seem to be needed since CWallet is also forward declared in init.h, but maybe it is better to keep it.

@laanwj laanwj self-assigned this Mar 2, 2017

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2017

Member

Great PR. I think we should get this in as soon as possible.
utACK d678771

Member

jonasschnelli commented Mar 2, 2017

Great PR. I think we should get this in as soon as possible.
utACK d678771

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 3, 2017

Member

utACK d678771

Member

laanwj commented Mar 3, 2017

utACK d678771

@laanwj laanwj merged commit d678771 into bitcoin:master Mar 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 3, 2017

Merge #8775: RPC refactoring: Access wallet using new GetWalletForJSO…
…NRPCRequest

d678771 Wallet: Sanitise -wallet parameter (Luke Dashjr)
9756be3 Wallet/RPC: Use filename rather than CWallet pointer, for lockwallet RPCRunLater job name (Luke Dashjr)
86be48a More tightly couple EnsureWalletIsAvailable with GetWalletForJSONRPCRequest where appropriate (Luke Dashjr)
a435632 Move wallet RPC declarations to rpcwallet.h (Luke Dashjr)
ad15734 RPC: Pass on JSONRPCRequest metadata (URI/user/etc) for "help" method (Luke Dashjr)
bf8a04a Reformat touched lines with C++11 (Luke Dashjr)
2e518e3 Move nWalletUnlockTime to CWallet::nRelockTime, and name timed task unique per CWallet (Luke Dashjr)
d77ad6d RPC: Do all wallet access through new GetWalletForJSONRPCRequest (Luke Dashjr)
eca550f RPC/Wallet: Pass CWallet as pointer to helper functions (Luke Dashjr)

Tree-SHA512: bfd592da841693390e16f83b451503eb5cedb71208089aa32b3fc45e973555584a3ed7696dd239f6409324464d565dacf0f3d0e36e8e13ae6a7843848465f960

@fanquake fanquake moved this from In progress to Done in Multiwallet support Mar 3, 2017

@jtimon jtimon referenced this pull request Mar 6, 2017

Closed

RPC: cleanups in rpc/server #9845

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

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

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

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

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

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

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

More tightly couple EnsureWalletIsAvailable with GetWalletForJSONRPCR…
…equest where appropriate

Github-Pull: #8775
Rebased-From: 86be48a

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

Wallet/RPC: Use filename rather than CWallet pointer, for lockwallet …
…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: #8775
Rebased-From: 9756be3

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Mar 23, 2017

Member

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

Member

jtimon commented Mar 23, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment