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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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
src/wallet/test/init_tests.cpp
18 ↗(On Diff #15616)

Very good catch. Fixed in D5008

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 marked 2 inline comments as done.

Rebase on D5008

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