Skip to content

Commit 7195fa7

Browse files
jonatackjonatack
committed
test: Tool wallet test coverage for unexpected writes to wallet
This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in bitcoin#15608: - wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing. 2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 3. Add some logging for sanity checking. ------ Changes after rebase: 5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion. 6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround. 7. Change the added logging from info to debug level. 8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue.
1 parent 3bf2b3a commit 7195fa7

File tree

1 file changed

+86
-1
lines changed

1 file changed

+86
-1
lines changed

‎test/functional/tool_wallet.py‎

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,17 @@
44
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
55
"""Test bitcoin-wallet."""
66

7+
import hashlib
8+
import os
9+
import stat
710
import subprocess
811
import textwrap
912

1013
from test_framework.test_framework import BitcoinTestFramework
1114
from test_framework.util import assert_equal
1215

16+
BUFFER_SIZE = 16 * 1024
17+
1318
class ToolWalletTest(BitcoinTestFramework):
1419
def set_test_params(self):
1520
self.num_nodes = 1
@@ -33,9 +38,27 @@ def assert_raises_tool_error(self, error, *args):
3338
def assert_tool_output(self, output, *args):
3439
p = self.bitcoin_wallet_process(*args)
3540
stdout, stderr = p.communicate()
36-
assert_equal(p.poll(), 0)
3741
assert_equal(stderr, '')
3842
assert_equal(stdout, output)
43+
assert_equal(p.poll(), 0)
44+
45+
def wallet_shasum(self):
46+
h = hashlib.sha1()
47+
mv = memoryview(bytearray(BUFFER_SIZE))
48+
with open(self.wallet_path, 'rb', buffering=0) as f:
49+
for n in iter(lambda : f.readinto(mv), 0):
50+
h.update(mv[:n])
51+
return h.hexdigest()
52+
53+
def wallet_timestamp(self):
54+
return os.path.getmtime(self.wallet_path)
55+
56+
def wallet_permissions(self):
57+
return oct(os.lstat(self.wallet_path).st_mode)[-3:]
58+
59+
def log_wallet_timestamp_comparison(self, old, new):
60+
result = 'unchanged' if new == old else 'increased!'
61+
self.log.debug('Wallet file timestamp {}'.format(result))
3962

4063
def test_invalid_tool_commands_and_args(self):
4164
self.log.info('Testing that various invalid commands raise with specific error messages')
@@ -51,6 +74,18 @@ def test_tool_wallet_info(self):
5174
# Stop the node to close the wallet to call the info command.
5275
self.stop_node(0)
5376
self.log.info('Calling wallet tool info, testing output')
77+
#
78+
# TODO: Wallet tool info should work with wallet file permissions set to
79+
# read-only without raising:
80+
# "Error loading wallet.dat. Is wallet being used by another process?"
81+
# The following lines should be uncommented and the tests still succeed:
82+
#
83+
# self.log.debug('Setting wallet file permissions to 400 (read-only)')
84+
# os.chmod(self.wallet_path, stat.S_IRUSR)
85+
# assert(self.wallet_permissions() in ['400', '666']) # Sanity check. 666 because Appveyor.
86+
# shasum_before = self.wallet_shasum()
87+
timestamp_before = self.wallet_timestamp()
88+
self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
5489
out = textwrap.dedent('''\
5590
Wallet info
5691
===========
@@ -61,6 +96,20 @@ def test_tool_wallet_info(self):
6196
Address Book: 3
6297
''')
6398
self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
99+
timestamp_after = self.wallet_timestamp()
100+
self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
101+
self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
102+
self.log.debug('Setting wallet file permissions back to 600 (read/write)')
103+
os.chmod(self.wallet_path, stat.S_IRUSR | stat.S_IWUSR)
104+
assert(self.wallet_permissions() in ['600', '666']) # Sanity check. 666 because Appveyor.
105+
#
106+
# TODO: Wallet tool info should not write to the wallet file.
107+
# The following lines should be uncommented and the tests still succeed:
108+
#
109+
# assert_equal(timestamp_before, timestamp_after)
110+
# shasum_after = self.wallet_shasum()
111+
# assert_equal(shasum_before, shasum_after)
112+
# self.log.debug('Wallet file shasum unchanged\n')
64113

65114
def test_tool_wallet_info_after_transaction(self):
66115
"""
@@ -73,6 +122,9 @@ def test_tool_wallet_info_after_transaction(self):
73122
self.stop_node(0)
74123

75124
self.log.info('Calling wallet tool info after generating a transaction, testing output')
125+
shasum_before = self.wallet_shasum()
126+
timestamp_before = self.wallet_timestamp()
127+
self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
76128
out = textwrap.dedent('''\
77129
Wallet info
78130
===========
@@ -83,9 +135,22 @@ def test_tool_wallet_info_after_transaction(self):
83135
Address Book: 3
84136
''')
85137
self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
138+
shasum_after = self.wallet_shasum()
139+
timestamp_after = self.wallet_timestamp()
140+
self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
141+
self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
142+
#
143+
# TODO: Wallet tool info should not write to the wallet file.
144+
# This assertion should be uncommented and succeed:
145+
# assert_equal(timestamp_before, timestamp_after)
146+
assert_equal(shasum_before, shasum_after)
147+
self.log.debug('Wallet file shasum unchanged\n')
86148

87149
def test_tool_wallet_create_on_existing_wallet(self):
88150
self.log.info('Calling wallet tool create on an existing wallet, testing output')
151+
shasum_before = self.wallet_shasum()
152+
timestamp_before = self.wallet_timestamp()
153+
self.log.debug('Wallet file timestamp before calling create: {}'.format(timestamp_before))
89154
out = textwrap.dedent('''\
90155
Topping up keypool...
91156
Wallet info
@@ -97,21 +162,41 @@ def test_tool_wallet_create_on_existing_wallet(self):
97162
Address Book: 0
98163
''')
99164
self.assert_tool_output(out, '-wallet=foo', 'create')
165+
shasum_after = self.wallet_shasum()
166+
timestamp_after = self.wallet_timestamp()
167+
self.log.debug('Wallet file timestamp after calling create: {}'.format(timestamp_after))
168+
self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
169+
assert_equal(timestamp_before, timestamp_after)
170+
assert_equal(shasum_before, shasum_after)
171+
self.log.debug('Wallet file shasum unchanged\n')
100172

101173
def test_getwalletinfo_on_different_wallet(self):
102174
self.log.info('Starting node with arg -wallet=foo')
103175
self.start_node(0, ['-wallet=foo'])
104176

105177
self.log.info('Calling getwalletinfo on a different wallet ("foo"), testing output')
178+
shasum_before = self.wallet_shasum()
179+
timestamp_before = self.wallet_timestamp()
180+
self.log.debug('Wallet file timestamp before calling getwalletinfo: {}'.format(timestamp_before))
106181
out = self.nodes[0].getwalletinfo()
107182
self.stop_node(0)
108183

184+
shasum_after = self.wallet_shasum()
185+
timestamp_after = self.wallet_timestamp()
186+
self.log.debug('Wallet file timestamp after calling getwalletinfo: {}'.format(timestamp_after))
187+
109188
assert_equal(0, out['txcount'])
110189
assert_equal(1000, out['keypoolsize'])
111190
assert_equal(1000, out['keypoolsize_hd_internal'])
112191
assert_equal(True, 'hdseedid' in out)
113192

193+
self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
194+
assert_equal(timestamp_before, timestamp_after)
195+
assert_equal(shasum_after, shasum_before)
196+
self.log.debug('Wallet file shasum unchanged\n')
197+
114198
def run_test(self):
199+
self.wallet_path = os.path.join(self.nodes[0].datadir, 'regtest', 'wallets', 'wallet.dat')
115200
self.test_invalid_tool_commands_and_args()
116201
# Warning: The following tests are order-dependent.
117202
self.test_tool_wallet_info()

0 commit comments

Comments
 (0)