Page MenuHomePhabricator

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

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

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
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?

tomtomtom7 updated this revision to Diff 1397.Sep 21 2017, 07:24

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

schancel requested changes to this revision.Sep 21 2017, 15:18

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.

sickpig accepted this revision as: sickpig.Sep 22 2017, 14:57
schancel accepted this revision.Sep 27 2017, 17:56
This revision is now accepted and ready to land.Sep 27 2017, 17:56
This revision was automatically updated to reflect the committed changes.
tomtomtom7 marked an inline comment as done.