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

Branch

pr14146

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9037
Build 16036: Default Diff Build & Tests
Build 16035: arc lint + arc unit

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