Ensure backupwallet fails if the target is the same as the source
ClosedPublic
Actions

Authored by tomtomtom7 on Sep 20 2017, 21:08.

Details

Reviewers

schancel
sickpig

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rSTAGINGb0a5cc08e8da: Ensure backupwallet fails if the target is the same as the source
rABCb0a5cc08e8da: Ensure backupwallet fails if the target is the same as the source

Summary

This fixes T105

Test Plan

Includes rpc test

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

tomtomtom7 created this revision.Sep 20 2017, 21:08

Owners added a reviewer: Restricted Owners Package.Sep 20 2017, 21:08

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 20 2017, 21:08

tomtomtom7 retitled this revision from Ensure backupwallet failse if the target is the same as the source to Ensure backupwallet fails if the target is the same as the source.Sep 20 2017, 21:11

schancel added a subscriber: schancel.Sep 20 2017, 21:36

tomtomtom7 updated this revision to Diff 1395.Sep 20 2017, 21:50

I tried this out on my mac:

➜  ~ bitcoin-cli backupwallet ./

No error or anything. It seems to have locked up bitcoind:

➜  ~ bitcoin-cli help
error: Could not locate RPC credentials. No authentication cookie could be found, and no rpcpassword is set in the configuration file (/Users/shammah/Library/Application Support/Bitcoin/bitcoin.conf)
➜  ~ bitcoind
Error: Cannot obtain a lock on data directory /Users/shammah/Library/Application Support/Bitcoin. Bitcoin ABC is probably already running.

OTOH, my coins are still in my wallet after killing everything off and restarting it. :)

Could you investigate a bit more?

Do you still have said conf file with credentials?
What if you pass the explicit path "/Users/shammah/Library/Application Support/Bitcoin/" to backupwallet?

Cleanup test to use assert_raises_jsonrpc

sickpig added a subscriber: sickpig.Sep 21 2017, 07:45

sickpig added inline comments.

qa/rpc-tests/walletbackup.py
207 ↗	(On Diff #1397)	this is linux/unix specific, dunno if we are try to run these tests on win on other OS but I wonder that using `os.path.join` can make no harm

I tested this again today with bitcoind running in the data directory, and got the following (good error):

➜  ~ bitcoin-cli backupwallet ./
error code: -4
error message:
Error: Wallet backup failed!

We may want to disallow relative paths due to the way the cli works with the daemon though. However, that'd be better as another ticket.

Thanks for making these changes!

qa/rpc-tests/walletbackup.py
207 ↗	(On Diff #1397)	Good call.

This revision now requires changes to proceed.Sep 21 2017, 15:18

tomtomtom7 marked 2 inline comments as done.Sep 21 2017, 18:29

tomtomtom7 added inline comments.

qa/rpc-tests/walletbackup.py
207 ↗	(On Diff #1397)	Forward slash works fine on Windows and is currently used everywhere in rpc-tests.

tomtomtom7 marked an inline comment as done.Sep 21 2017, 18:29

I don't think I meant to "done" the comment in code. I just meant to comment on it; have to get used to the interface...

sickpig added inline comments.Sep 22 2017, 14:45

qa/rpc-tests/walletbackup.py
207 ↗	(On Diff #1397)	Didn't know that `/` works on Windows. I erroneously extended the fact that command prompt doesn't accept `/` as path separator to all kind of access. Apparently has been accepted since MS-DOS DOS 2.0 and up. Guess this is the reason why it is used across all the tests then.