Page MenuHomePhabricator

Merge #14146: wallet: Remove trailing separators from -walletdir arg
ClosedPublic

Authored by jasonbcox on Jan 17 2020, 18:44.

Details

Summary

2d471636eb9160ab51b08e491e3f003f57adbc36 wallet: Remove trailing separators from -walletdir arg (Pierre Rochard)
ea3009ee942188750480ca6cc273b2b91cf77ded wallet: Add walletdir arg unit tests (Pierre Rochard)

Pull request description:

If a user passes in a path with a trailing separator as the `walletdir`, multiple BerkeleyEnvironments may be created in the same directory which can lead to data corruption.

Discovered while reviewing https://github.com/bitcoin/bitcoin/pull/12493#issuecomment-417147646

Tree-SHA512: f2bbf1749d904fd3f326b88f2ead58c8386034355910906d7faea155d518642e9cd4ceb3cae272f2d9d8feb61f126523e1c97502799d24e4315bb53e49fd7c09

Backport of Core PR14146
https://github.com/bitcoin/bitcoin/pull/14146/files

Test Plan

ninja check
Autotools build on CI

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

jasonbcox created this revision.Jan 17 2020, 18:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 17 2020, 18:44
deadalnix requested changes to this revision.Jan 17 2020, 22:09
deadalnix added inline comments.
src/wallet/test/init_test_fixture.cpp
8 ↗(On Diff #15616)

Why?

src/wallet/test/init_tests.cpp
5 ↗(On Diff #15616)

Why?

18 ↗(On Diff #15616)

So I added a BOOST_CHECK(false); because this seemed suspicious that this would run. As it turns out, the cmake build indeed doesn't run this - but make does.

This revision now requires changes to proceed.Jan 17 2020, 22:09
jasonbcox added inline comments.Jan 17 2020, 22:33
src/wallet/test/init_tests.cpp
18 ↗(On Diff #15616)

Very good catch. Fixed in D5008

jasonbcox marked 3 inline comments as done.Jan 17 2020, 22:46
jasonbcox added inline comments.
src/wallet/test/init_test_fixture.cpp
8 ↗(On Diff #15616)

Needed for gArgs. I imagine out of order backports elsewhere is the cause, because Core adds this later in PR16278.

src/wallet/test/init_tests.cpp
5 ↗(On Diff #15616)

Original PR has calls like bool result = g_wallet_init_interface.Verify();
In our codebase, we need bool result = g_wallet_init_interface.Verify(Params()); the include is for Params()

jasonbcox updated this revision to Diff 15630.Jan 17 2020, 22:46
jasonbcox marked 2 inline comments as done.

Rebase on D5008

deadalnix accepted this revision.Sun, Jan 19, 15:07
This revision is now accepted and ready to land.Sun, Jan 19, 15:07