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
Branch
pr14146
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9030
Build 16022: Default Diff Build & Tests
Build 16021: 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
deadalnix requested changes to this revision.Jan 17 2020, 22:09
deadalnix added inline comments.
src/wallet/test/init_test_fixture.cpp
8

Why?

src/wallet/test/init_tests.cpp
5

Why?

18

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

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

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

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.Jan 19 2020, 15:07
This revision is now accepted and ready to land.Jan 19 2020, 15:07