Page MenuHomePhabricator

[Part5] CWallet constructor now has additional parameter chainParams
ClosedPublic

Authored by matra774 on Feb 10 2018, 09:24.

Details

Summary

CWallet constructor (and some of the CWallet's static methods) now take chainParams as parameter. This enabled us to get fix many calls to DecodeAddress, that did not specify chainParams as an explicit parameter.

Wallet_tests. Cpp: wallet was moved from static to local function scope, so that it can be initialized after chainParameters are available. I've preserved the global locking mechanism by adding walletCriticalSection (some state like vCoins is still global)

Refs T170

Test Plan

make check
./test/functional/test_runner.py --extended

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

matra774 created this revision.Feb 10 2018, 09:24
Owners added a reviewer: Restricted Owners Package.Feb 10 2018, 09:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 10 2018, 09:24
jasonbcox requested changes to this revision.Feb 10 2018, 21:54
jasonbcox added inline comments.
src/init.cpp
1756 ↗(On Diff #2835)

Use the chainParams variable you init'd above.

2153 ↗(On Diff #2835)

chainParams here too

src/wallet/test/wallet_tests.cpp
40–42 ↗(On Diff #2835)

If you init a new wallet in every test, is this necessary?

This revision now requires changes to proceed.Feb 10 2018, 21:54
matra774 added inline comments.Feb 11 2018, 09:56
src/wallet/test/wallet_tests.cpp
40–42 ↗(On Diff #2835)

Boost can execute some test in parallel. Previous version locked on wallet.cs_wallet. Since wallet was a global variable, that resulted in global lock. My commit preserves this behavior (global lock). Is it necessary? I am not sure - the code inside a unit test can access some other global resources that requires the lock.

If you take a look at other test cases (for example BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) ), you can see that we have different locking patterns:

  • some of tests first lock on another global variable LOCK(cs_main), and later acquire the lock on local instance of wallet: LOCK(wallet.cs_wallet) (where wallet is a local variable).
  • some of the test lock both critical section at the same time LOCK2(cs_main, wallet.cs_wallet); - line 611, 652
  • other tests create a local instance of CWallet, but does not lock it at all - see lines 488, 652, 581

There could be some other lurking problems regarding test code and locks. For example :

  • test in line 538 (BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)) locks on cs_main and then calls SetMockTime, which sets global Variable
  • test in line 651 (BOOST_AUTO_TEST_CASE(ComputeTimeSmart)) also calls SetMockTime (through AddTx), but does not take any locks

If those two tests are executed at the same time, one of them should probably fail..

As I said, I wanted to preserve existing (global) locking mechanism, which actually serializes test execution - better safe then sorry :-) What do you propose:
a) get rid of LOCK(walletCriticalSection)
b) replace LOCK(walletCriticalSection) with LOCK (cs_main)
c) replace LOCK(walletCriticalSection) with local lock - LOCK(wallet.cs_wallet)
d) something else?

matra774 updated this revision to Diff 2842.Feb 11 2018, 13:59

fixed 2 of 3 review comments. The locking question remains open

matra774 marked 2 inline comments as done.Feb 11 2018, 14:03
jasonbcox accepted this revision.Feb 11 2018, 18:22
jasonbcox added inline comments.
src/wallet/test/wallet_tests.cpp
40–42 ↗(On Diff #2835)

Ok, I wasn't aware this was an issue. Seeing as this preserves behavior, I'm ok with it. However, I've opened up a ticket (T235) to investigate this further, because we (ideally) shouldn't be utilizing different locking styles for different tests.

This revision is now accepted and ready to land.Feb 11 2018, 18:23
matra774 updated this revision to Diff 2918.Feb 18 2018, 10:24

Rebasing onto master to avoid conflicts

Harbormaster failed remote builds in B1887: Diff 2918!
This revision was automatically updated to reflect the committed changes.