From 3199610ad3b93b849f2cb55a8ed3a39a32bbdffc Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 10 Jul 2019 15:44:26 -0400 Subject: [PATCH 1/4] Place out args at the end for CreateWallet --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/wallet.cpp | 2 +- src/wallet/wallet.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 87ef58ee96..b7618e1259 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2685,7 +2685,7 @@ static UniValue createwallet(const JSONRPCRequest& request) std::string error; std::string warning; WalletCreationStatus status; - std::shared_ptr wallet = CreateWallet(*g_rpc_interfaces->chain, request.params[0].get_str(), error, warning, status, passphrase, flags); + std::shared_ptr wallet = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, warning, status); if (status == WalletCreationStatus::CREATION_FAILED) { throw JSONRPCError(RPC_WALLET_ERROR, error); } else if (status == WalletCreationStatus::ENCRYPTION_FAILED) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e53b433ca8..a4e3a23476 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -160,7 +160,7 @@ std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& return LoadWallet(chain, WalletLocation(name), error, warning); } -std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status, const SecureString& passphrase, uint64_t wallet_creation_flags) +std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status) { // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7e2230554d..9db4b2c7d9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -55,7 +55,7 @@ enum WalletCreationStatus { ENCRYPTION_FAILED }; -std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status, const SecureString& passphrase, uint64_t wallet_creation_flags); +std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status); //! Default for -keypool static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000; From d6649d16b57e20b05075f1c80d0de7ff32cca1a4 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 10 Jul 2019 15:46:02 -0400 Subject: [PATCH 2/4] Use strong enum for WalletCreationStatus --- src/wallet/wallet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9db4b2c7d9..059528106f 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -49,7 +49,7 @@ std::vector> GetWallets(); std::shared_ptr GetWallet(const std::string& name); std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning); -enum WalletCreationStatus { +enum class WalletCreationStatus { SUCCESS, CREATION_FAILED, ENCRYPTION_FAILED From ba1f128d6c117a63d5d904b3956551bd83405ec9 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 10 Jul 2019 17:51:39 -0400 Subject: [PATCH 3/4] Return error for ignored passphrase through disable private keys option --- src/wallet/rpcwallet.cpp | 4 ++-- src/wallet/wallet.cpp | 27 +++++++++++++------------- src/wallet/wallet.h | 2 +- test/functional/wallet_createwallet.py | 3 +++ 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b7618e1259..a600331ba0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2684,8 +2684,8 @@ static UniValue createwallet(const JSONRPCRequest& request) std::string error; std::string warning; - WalletCreationStatus status; - std::shared_ptr wallet = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, warning, status); + std::shared_ptr wallet; + WalletCreationStatus status = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, warning, wallet); if (status == WalletCreationStatus::CREATION_FAILED) { throw JSONRPCError(RPC_WALLET_ERROR, error); } else if (status == WalletCreationStatus::ENCRYPTION_FAILED) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a4e3a23476..804c5c619d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -160,7 +160,7 @@ std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& return LoadWallet(chain, WalletLocation(name), error, warning); } -std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status) +WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, std::shared_ptr& result) { // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); @@ -174,39 +174,40 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureStri WalletLocation location(name); if (location.Exists()) { error = "Wallet " + location.GetName() + " already exists."; - status = WalletCreationStatus::CREATION_FAILED; - return nullptr; + return WalletCreationStatus::CREATION_FAILED; } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. std::string wallet_error; if (!CWallet::Verify(chain, location, false, wallet_error, warning)) { error = "Wallet file verification failed: " + wallet_error; - status = WalletCreationStatus::CREATION_FAILED; - return nullptr; + return WalletCreationStatus::CREATION_FAILED; + } + + // Do not allow a passphrase when private keys are disabled + if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + error = "Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled."; + return WalletCreationStatus::CREATION_FAILED; } // Make the wallet std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, location, wallet_creation_flags); if (!wallet) { error = "Wallet creation failed"; - status = WalletCreationStatus::CREATION_FAILED; - return nullptr; + return WalletCreationStatus::CREATION_FAILED; } // Encrypt the wallet if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { if (!wallet->EncryptWallet(passphrase)) { error = "Error: Wallet created but failed to encrypt."; - status = WalletCreationStatus::ENCRYPTION_FAILED; - return nullptr; + return WalletCreationStatus::ENCRYPTION_FAILED; } if (!create_blank) { // Unlock the wallet if (!wallet->Unlock(passphrase)) { error = "Error: Wallet was encrypted but could not be unlocked"; - status = WalletCreationStatus::ENCRYPTION_FAILED; - return nullptr; + return WalletCreationStatus::ENCRYPTION_FAILED; } // Set a seed for the wallet @@ -220,8 +221,8 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureStri } AddWallet(wallet); wallet->postInitProcess(); - status = WalletCreationStatus::SUCCESS; - return wallet; + result = wallet; + return WalletCreationStatus::SUCCESS; } const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 059528106f..e36703f2c1 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -55,7 +55,7 @@ enum class WalletCreationStatus { ENCRYPTION_FAILED }; -std::shared_ptr CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, WalletCreationStatus& status); +WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::string& warning, std::shared_ptr& result); //! Default for -keypool static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000; diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index c17949a2f6..294f90a0fa 100755 --- a/test/functional/wallet_createwallet.py +++ b/test/functional/wallet_createwallet.py @@ -119,5 +119,8 @@ class CreateWalletTest(BitcoinTestFramework): # Empty passphrase, error assert_raises_rpc_error(-16, 'Cannot encrypt a wallet with a blank password', self.nodes[0].createwallet, 'w7', False, False, '') + self.log.info('Using a passphrase with private keys disabled returns error') + assert_raises_rpc_error(-4, 'Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.', self.nodes[0].createwallet, wallet_name='w8', disable_private_keys=True, passphrase='thisisapassphrase') + if __name__ == '__main__': CreateWalletTest().main() From e967cae8fac84ec7a89a3a853a83d8193ac3308e Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 12 Jul 2019 14:06:55 -0400 Subject: [PATCH 4/4] Use switch on status in RpcWallet --- src/wallet/rpcwallet.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index a600331ba0..ae610ec07d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2686,12 +2686,14 @@ static UniValue createwallet(const JSONRPCRequest& request) std::string warning; std::shared_ptr wallet; WalletCreationStatus status = CreateWallet(*g_rpc_interfaces->chain, passphrase, flags, request.params[0].get_str(), error, warning, wallet); - if (status == WalletCreationStatus::CREATION_FAILED) { - throw JSONRPCError(RPC_WALLET_ERROR, error); - } else if (status == WalletCreationStatus::ENCRYPTION_FAILED) { - throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error); - } else if (status != WalletCreationStatus::SUCCESS) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed"); + switch (status) { + case WalletCreationStatus::CREATION_FAILED: + throw JSONRPCError(RPC_WALLET_ERROR, error); + case WalletCreationStatus::ENCRYPTION_FAILED: + throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error); + case WalletCreationStatus::SUCCESS: + break; + // no default case, so the compiler can warn about missing cases } UniValue obj(UniValue::VOBJ);