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

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

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

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

jasonbcox created this revision.Jan 17 2020, 18:44

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 17 2020, 18:44

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9030: Diff 15616.Jan 17 2020, 19:02

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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

Fabien mentioned this in D5008: [CMAKE] Run wallet tests as part of the check-bitcoin target.Jan 17 2020, 22:35

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()`