[Tests] Wallet encryption functional tests #10551

achow101 commented at 7:35 PM on June 7, 2017: member

Added a functional test which tests the encryptwallet, walletpassphrase, walletpassphrasechange, and walletlock RPCs

fanquake added the label Tests on Jun 7, 2017

jonasschnelli commented at 6:22 AM on June 8, 2017: contributor

Meh. Those code parts are already tested in bumpfee / keypool / fundrawtransaction / wallet-dump. IMO we don't need another test for this.

Or where are the improvements?

achow101 commented at 7:04 AM on June 8, 2017: member

The improvements are that walletpassphrasechange is now tested and that these functionality are explicitly being tested. The usage of these RPCs in other tests are more for doing stuff with testing other stuff rather than actually testing the behavior of these RPCs themselves.

jonasschnelli commented at 7:07 AM on June 8, 2017: contributor

I would recommend to add the missing test to an existing wallet test and maybe rename the test file (so the name may make more sense). It's just my opinion, I think we should try to combine tests (if it makes sense) to avoid unnecessary test-bootstraps/runs.

in test/functional/wallet-encryption.py:12 in 3f35a87737 outdated

   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import (
   9 | +    assert_equal,
  10 | +    assert_raises_jsonrpc,
  11 | +)
  12 | +import os

jnewbery commented at 3:18 PM on June 8, 2017:

os and shutil are imported but not used.

achow101 commented at 5:54 PM on June 8, 2017:

done

in test/functional/wallet-encryption.py:6 in 3f35a87737 outdated

   0 | @@ -0,0 +1,64 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2016 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test Wallet encryption"""
   6 | +

jnewbery commented at 3:19 PM on June 8, 2017:

nit: prefer PIP8 import order (std library, followed by 3rd party library, followed by project imports)

achow101 commented at 5:54 PM on June 8, 2017:

done

in test/functional/wallet-encryption.py:23 in 3f35a87737 outdated

  18 | +    def __init__(self):
  19 | +        super().__init__()
  20 | +        self.setup_clean_chain = True
  21 | +        self.num_nodes = 1
  22 | +
  23 | +    def run_test (self):

jnewbery commented at 3:21 PM on June 8, 2017:

no space before parens for function parameters please.

achow101 commented at 5:54 PM on June 8, 2017:

done

in test/functional/wallet-encryption.py:24 in 3f35a87737 outdated

  19 | +        super().__init__()
  20 | +        self.setup_clean_chain = True
  21 | +        self.num_nodes = 1
  22 | +
  23 | +    def run_test (self):
  24 | +        tmpdir = self.options.tmpdir

jnewbery commented at 3:22 PM on June 8, 2017:

I don't think this local variable is needed. You can just use self.options.tmpdir directly in the call to start_node() below.

achow101 commented at 5:54 PM on June 8, 2017:

done

in test/functional/wallet-encryption.py:30 in 3f35a87737 outdated

  25 | +        passphrase = "WalletPassphrase"
  26 | +        passphrase2 = "SecondWalletPassphrase"
  27 | +        
  28 | +        # Make sure the wallet isn't encrypted first
  29 | +        address = self.nodes[0].getnewaddress()
  30 | +        privkey = self.nodes[0].dumpprivkey(address)

jnewbery commented at 3:24 PM on June 8, 2017:

Is it possible to assert that privkey is a legitimate privkey here? You're asserting everything against it further down, but you never assert that privkey is actually a private key. This test would still pass if, for example, dumpprivkey() always returns an empty string.

achow101 commented at 5:53 PM on June 8, 2017:

I suppose it would be possible to do a prefix and length check on it. Alternatively a private key could be hard-coded and imported into the wallet. But actually checking that the private key is legitimate, I'm not sure that that can be done.

jnewbery commented at 5:57 PM on June 8, 2017:

Presumably it'd be possible to check that it's a base58 string of the correct length? That should be enough. I think we should at least detect the case where the return value is obviously not a private key.

achow101 commented at 6:08 PM on June 8, 2017:

I added a prefix check and length check since there isn't actually anything to do a base58 check in the tests.

in test/functional/wallet-encryption.py:34 in 3f35a87737 outdated

  29 | +        address = self.nodes[0].getnewaddress()
  30 | +        privkey = self.nodes[0].dumpprivkey(address)
  31 | +        
  32 | +        # Encrypt the wallet
  33 | +        self.nodes[0].encryptwallet(passphrase)
  34 | +        self.stop_node(0)

jnewbery commented at 3:28 PM on June 8, 2017:

This call to stop_node() is not needed (and may be dangerous). the encryptwallet RPC causes bitcoind to stop. Really what you need to do here is wait for the bitcoind process to terminate rather than call stop_node(). See https://github.com/bitcoin/bitcoin/blob/9c248e39f2807a7f89e555e99cc55cb2de1ad335/test/functional/fundrawtransaction.py#L455 for example.

stop_node() will assert if the bitcoind process no longer exists, so your current code is a bit racy. If the call to encryptwallet causes the node to stop before stop_node() is called, then the test will assert and fail.

achow101 commented at 5:54 PM on June 8, 2017:

done

in test/functional/wallet-encryption.py:45 in 3f35a87737 outdated

  40 | +        # Check that walletpassphrase works
  41 | +        self.nodes[0].walletpassphrase(passphrase, 10)
  42 | +        assert_equal(privkey, self.nodes[0].dumpprivkey(address))
  43 | +        
  44 | +        # Check that the timeout is right
  45 | +        time.sleep(10)

jnewbery commented at 3:30 PM on June 8, 2017:

Consider reducing the timeout in the call to walletpassphrase to shorten this test. Alternatively, you could use mocktime to avoid waiting for 10 seconds.

achow101 commented at 5:54 PM on June 8, 2017:

done

jnewbery commented at 3:35 PM on June 8, 2017: member

Looks good. I suggest you use a tool like flake8 or pylint to catch common style issues like unused imports, trailing whitespace, etc. There's a bunch of trailing whitespace in the new test which I haven't commented on individually.

Longer term, I think wallet encryption should be handled by an external tool (#8745), but even if that happens, it still makes sense to have tests for bitcoind unlocking the encrypted walled.

jnewbery commented at 4:04 PM on June 8, 2017: member

Those code parts are already tested in bumpfee / keypool / fundrawtransaction / wallet-dump. IMO we don't need another test for this.

Only just saw this comment. I agree with @achow101 that the calls to encryptwallet in those tests is incidental and it makes sense to have directed tests of encrypting/decrypting and changing passphrases. It may make sense to add it to an existing test, but on the other hand I'm not too concerned about adding a new test file.

There's a balance here: adding new test cases conceptually separates the different test objectives, and also allows tests to be run in parallel, but test setup/teardown is a non-negligable part of the time taken to run the tests.

achow101 force-pushed on Jun 8, 2017

Tests for wallet encryption stuff

Added a functional test which tests the encryptwallet, walletpassphrase, walletpassphrasechange, and walletlock RPCs

ec98b78e1e

achow101 force-pushed on Jun 8, 2017

jonasschnelli commented at 6:28 PM on June 8, 2017: contributor

There's a balance here: adding new test cases conceptually separates the different test objectives, and also allows tests to be run in parallel, but test setup/teardown is a non-negligable part of the time taken to run the tests.

Fair point.

jnewbery commented at 7:51 PM on June 8, 2017: member

Looks good to me! Tested ACK ec98b78e1e1ce6bd9e6189788f7689aebfbf343c

laanwj commented at 1:27 PM on June 14, 2017: member

ACK ec98b78

laanwj merged this on Jun 14, 2017

laanwj closed this on Jun 14, 2017

laanwj referenced this in commit 6702617c86 on Jun 14, 2017

PastaPastaPasta referenced this in commit 2b62003476 on Jul 5, 2019

PastaPastaPasta referenced this in commit 3426d207df on Jul 5, 2019

PastaPastaPasta referenced this in commit 81580005c8 on Jul 6, 2019

PastaPastaPasta referenced this in commit 85ff9f7a76 on Jul 8, 2019

PastaPastaPasta referenced this in commit 75f4230fb4 on Jul 9, 2019

PastaPastaPasta referenced this in commit 61ca895c4e on Jul 9, 2019

barrystyle referenced this in commit 5e2537aab5 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021